-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Add manual instrumentation to emailservice #158
Add manual instrumentation to emailservice #158
Conversation
@ahayworth also added you as a reviewer. |
LGTM |
@julianocosta89 @cartersocha Yes I will take a look this afternoon - I am busy this morning, but I have a suggestion or two and I'll write those up in a bit. 😄 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for adding this! After thinking about it, I would want to request a few changes before we merge this in. They're a little bit nit-picky, but since this is a reference application I felt maybe it was appropriate. 😄
(Of course, these are "strong opinions, weakly held" and I won't be upset if you choose not to take them. We also really appreciate the contribution! ❤️ )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM - I agree with @ahayworth's comments as well
Thank you very much for the review @ahayworth! Regarding the tracer with an empty name I want to bring @puckpuck to the thread, because i believe it would be better if we follow the same approach in all services. |
@ahayworth this one should be good to go now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thanks so much! 😄
@cartersocha this one should be good to go 🤩 |
* Add manual instrumentation to emailservice * Apply name suggestion * Apply suggestions Co-authored-by: Carter Socha <43380952+cartersocha@users.noreply.github.com>
* Add manual instrumentation to emailservice * Apply name suggestion * Apply suggestions Co-authored-by: Carter Socha <43380952+cartersocha@users.noreply.github.com>
Towards #55 .
Changes
Adds attributes to auto instrumented spans, and creates a new span with attributes.
If sending the email fails (what doesn't happen at the moment),
span.recordException()
is called from the auto-instrumented span, which ultimately creates a Span event.