Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Payment service simplification refactor #169

Merged
merged 20 commits into from
Jul 8, 2022
Merged

Payment service simplification refactor #169

merged 20 commits into from
Jul 8, 2022

Conversation

mic-max
Copy link
Contributor

@mic-max mic-max commented Jun 24, 2022

Changes

  • Add .dockerignore to improve build times and image size
  • Simplify away the basic error classes
  • Merge server.js into index.js
  • Simplify code, remove classes
  • Update and set dependencies to fixed versions
  • Working health check added

Showing healthy service
image

Note: for the above command to show healthy it needs the grpc_health_probe executable to be included in the image which is done by this PR

Output when starting payment service
image

Gracefully exit when using ctrl + c
image
With this code missing and we ctrl + c the docker compose the result of the running the command shows exited with non-zero value
image

Resources

mic-max added 10 commits June 1, 2022 11:04
- copy package-lock.json too
- npm ci only production dependencies (not any dev ones rn but could be added in the future.)
- NODE_ENV=production
- only copy demo.proto and use npm module for the health check
- move server.js contents into index.js
- document imports
- organize common things into blocks of code separated with comments (setup, functions, main, etc.)
- handle exits gracefully
@mic-max mic-max requested a review from a team June 24, 2022 02:06
@mic-max mic-max marked this pull request as draft June 27, 2022 15:54
@mic-max mic-max marked this pull request as ready for review June 28, 2022 18:11
@github-actions
Copy link

github-actions bot commented Jul 6, 2022

This PR was marked stale due to lack of activity. It will be closed in 7 days.

@github-actions github-actions bot added the Stale label Jul 6, 2022
Copy link
Member

@julianocosta89 julianocosta89 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @mic-max sorry for not taking a look at this one before.

I've tried it locally and it seems we have some issues.
The 1st thing is that I can't build the container without the package-lock.json file.
I had to generate it locally and run the docker compose build again.

I think in this case, to make the users' life easier, we could simply commit the package-lock.json. Just not sure how that would affect different OS.


Once the container was built, whenever running the sample I got the following error in the UI:

rpc error: code = Internal desc = failed to charge card: could not charge the card: rpc error: code = Unavailable desc = connection error: desc = "transport: Error while dialing dial tcp: lookup paymentservice on 127.0.0.11:53: no such host"
failed to complete the order
main.(*frontendServer).placeOrderHandler
	/usr/src/app/handlers.go:393
main.instrumentHandler.func1
	/usr/src/app/middleware.go:120
net/http.HandlerFunc.ServeHTTP
	/usr/local/go/src/net/http/server.go:2047
go.opentelemetry.io/contrib/instrumentation/github.com/gorilla/mux/otelmux.traceware.ServeHTTP
	/go/pkg/mod/go.opentelemetry.io/contrib/instrumentation/github.com/gorilla/mux/[email protected]/mux.go:145
github.com/gorilla/mux.(*Router).ServeHTTP
	/go/pkg/mod/github.com/gorilla/[email protected]/mux.go:210
main.(*logHandler).ServeHTTP
	/usr/src/app/middleware.go:94
main.ensureSessionID.func1
	/usr/src/app/middleware.go:148
net/http.HandlerFunc.ServeHTTP
	/usr/local/go/src/net/http/server.go:2047
net/http.serverHandler.ServeHTTP
	/usr/local/go/src/net/http/server.go:2879
net/http.(*conn).serve
	/usr/local/go/src/net/http/server.go:1930
runtime.goexit
	/usr/local/go/src/runtime/asm_amd64.s:1581

Looks like the proto files were not loaded properly.

@julianocosta89
Copy link
Member

I've tested after your last commit and now we are getting the following error on the paymentservice>

{"level":40,"time":1657173084895,"pid":1,"hostname":"4e4174c9376f","err":{"type":"TypeError","message":"Cannot read properties of undefined (reading 'setAttributes')","stack":"TypeError: Cannot read properties of undefined (reading 'setAttributes')\n    at Object.chargeServiceHandler [as charge] (/usr/src/app/index.js:31:10)\n    at handleUnary (/usr/src/app/node_modules/@grpc/grpc-js/build/src/server.js:688:13)\n    at processTicksAndRejections (node:internal/process/task_queues:96:5)"},"msg":"Cannot read properties of undefined (reading 'setAttributes')"}
/usr/src/app/index.js:43
    span.recordException(err)
         ^

TypeError: Cannot read properties of undefined (reading 'recordException')
    at Object.chargeServiceHandler [as charge] (/usr/src/app/index.js:43:10)
    at handleUnary (/usr/src/app/node_modules/@grpc/grpc-js/build/src/server.js:688:13)
    at processTicksAndRejections (node:internal/process/task_queues:96:5)

And the following error in the UI:

rpc error: code = Internal desc = failed to charge card: could not charge the card: rpc error: code = Unavailable desc = connection error: desc = "transport: Error while dialing dial tcp: lookup paymentservice on 127.0.0.11:53: server misbehaving"
failed to complete the order
main.(*frontendServer).placeOrderHandler
	/usr/src/app/handlers.go:393
main.instrumentHandler.func1
	/usr/src/app/middleware.go:120
net/http.HandlerFunc.ServeHTTP
	/usr/local/go/src/net/http/server.go:2047
go.opentelemetry.io/contrib/instrumentation/github.com/gorilla/mux/otelmux.traceware.ServeHTTP
	/go/pkg/mod/go.opentelemetry.io/contrib/instrumentation/github.com/gorilla/mux/[email protected]/mux.go:145
github.com/gorilla/mux.(*Router).ServeHTTP
	/go/pkg/mod/github.com/gorilla/[email protected]/mux.go:210
main.(*logHandler).ServeHTTP
	/usr/src/app/middleware.go:94
main.ensureSessionID.func1
	/usr/src/app/middleware.go:148
net/http.HandlerFunc.ServeHTTP
	/usr/local/go/src/net/http/server.go:2047
net/http.serverHandler.ServeHTTP
	/usr/local/go/src/net/http/server.go:2879
net/http.(*conn).serve
	/usr/local/go/src/net/http/server.go:1930
runtime.goexit
	/usr/local/go/src/runtime/asm_amd64.s:1581

@mic-max
Copy link
Contributor Author

mic-max commented Jul 7, 2022

@julianocosta89 Thanks for testing it, I should have converted back to draft because I knew that there were still some issue I needed to address after including package-lock.json. I'll look into that failing line which is caused by the line

const span = opentelemetry.trace.getSpan(opentelemetry.context.active())

which I didn't change from @puckpuck 's PR - maybe the context in which it is called is different though and that's the source of the bug


Edit: yes it was due to the reordering of the require cli option, oops. All fixed now and working

image

Copy link
Member

@julianocosta89 julianocosta89 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome @mic-max!
Tested here and everything is looking good!
We should be good to go! 🚀

@cartersocha cartersocha merged commit fa796f8 into open-telemetry:main Jul 8, 2022
@mic-max mic-max deleted the payment-simple branch July 8, 2022 19:35
jmichalak9 pushed a commit to jmichalak9/opentelemetry-demo that referenced this pull request Mar 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants