Extending tracing scope for @RabbitListener #1287
Conversation
It seems TracingRabbitListenerAdvice (the proxy in this class) only wraps the invocation of the listener not the invocation AND the error handling. This change modifies the target of the proxy so that the advice wraps the invocation and the error handling (see the referenced issue). Please notice that this also changes the order of measurements of micrometer and brave. Before the change micrometer's instrumentation included brave's instrumentation. With this change the order will be inverted, brave's instrumentation wraps micrometer's instrumentation.
jonatan-ivanov
changed the title
@RabbitListener
I need to think some more, but I am not sure we can make such a change in a patch release, we probably need an option to opt-in to the new behavior. It might also break existing user advices. I agree this makes more sense, but the existing behavior has been there for 10+ years (before my time even). |
|
See my comment here: spring-cloud/spring-cloud-sleuth#1663 (comment) |
|
See one more my comment: spring-cloud/spring-cloud-sleuth#1663 (comment). It looks like all the We probably still may consider this fix but as a common (or similar) solution for all the support Does it make sense or what ma I missing? Thanks |
|
@garyrussell @artembilan Yepp, I agree both of you, this is totally why I was seeking for feedback. :) I haven't really looked into the details into the other, possibly related issues, but seemingly they share the same characteristics and probably the root cause too (error handling is not included in the tracing scope), let me list them here:
Could somebody confirm this: Based on this, no matter what common or not common solution will we came up with to fix this, it still would be a breaking change since the fix is including the error handling in the scope, right? |
|
Sorry, I didn't realize there were multiple issues; my comments on the the JMS issue are for RabbitMQ. |
|
Workaround suggestion here: spring-cloud/spring-cloud-sleuth#1660 (comment) We can consider moving the scope of the proxy in 2.4. |
- expand scope to include error handler: spring-projects#1287
- expand scope to include error handler: spring-projects#1287
- expand scope to include error handler: spring-projects#1287 - tracing can't be used with a batch listener (multiple messages in listener call)
- expand scope to include error handler: spring-projects#1287 - tracing can't be used with a batch listener (multiple messages in listener call)
- expand scope to include error handler: spring-projects#1287 - tracing can't be used with a batch listener (multiple messages in listener call)
* GH-1444: Listener Observability Initial Commit - expand scope to include error handler: #1287 - tracing can't be used with a batch listener (multiple messages in listener call) * Rename Sender/Receiver contexts and other PR review comments. * Rename contexts to Rabbit...; supply default KeyValues via the conventions. * Javadoc polishing. * Don't add default KV to high-card KVs. * Fix previous commit. * Fix contextual name (receiver side). * Fix checkstyle. * Polish previous commit. * Fix contextual name (sender side) * Remove contextual names from observations. * Fix checkstyle. * Remove customization of KeyValues from conventions. * Add `getDefaultConvention()` to observations. * Fix since 3.0. * Support wider convention customization. * Convention type safety. * Fix Test - not sure why PR build succeeded. * Add Meters to ObservationTests. * Fix checkstyle. * Make INSTANCE final. * Add integration test. * Test all available integrations. * Remove redundant test code. * Move getContextualName to conventions. * Add docs. * Fix doc link. Co-authored-by: Artem Bilan <abilan@vmware.com> * Remove unnecessary method overrides; make tag names more meaningful. * Move getName() from contexts to conventions. * Fix Race in Test * Fix Race in Test. Co-authored-by: Artem Bilan <abilan@vmware.com>
|
Superessed via #1500 |
Fixes spring-cloud/spring-cloud-sleuth#1660
It seems TracingRabbitListenerAdvice (the proxy in this class) only wraps the invocation of the listener not the invocation AND the error handling. This change modifies the target of the proxy so that the advice wraps the invocation and the error handling (see the referenced issue).
Please notice that this also changes the order of measurements of micrometer and brave. Before the change micrometer's instrumentation included brave's instrumentation. With this change the order will be inverted, brave's instrumentation wraps micrometer's instrumentation.