-
Notifications
You must be signed in to change notification settings - Fork 791
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
JMS instrumentation: do not create spans if no messages are received #2091
Conversation
…ssages are received Signed-off-by: Sergei Malafeev <sergei@malafeev.org>
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.
This looks good to me. Thanks for picking this up! 🎉
destination = MessageDestination.UNKNOWN; | ||
} | ||
// Do not create span when no message is received | ||
return; | ||
} else { |
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.
Not a huge deal, but my preference is to remove the redundant else
when there is a return above it.
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.
fixed
Signed-off-by: Sergei Malafeev <sergei@malafeev.org>
…-instrumentation into jms-no-message
@Advice.Enter long startTime, | ||
@Advice.Return Message message, | ||
@Advice.Thrown Throwable throwable) { | ||
MessageDestination destination; | ||
if (message == null) { | ||
destination = |
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.
JmsSessionInstrumentation
can be removed now, right?
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.
removed
Signed-off-by: Sergei Malafeev <sergei@malafeev.org>
looks like ready to merge. |
fix #1989