Affects: 6.1.0-RC2
In the release candidates micrometer tracing for JMS messages was added. I am very excited about this, because it means we can get rid of some janky code that was written to do this in our library.
But I found one issue I'm not sure if it is intentional or if I am understanding something wrong. I figured the best place to move our exception logging code to is to the errorHandler of the ListenerContainer but the micrometer span only has the invokeListener method in scope. So any logging happening in the errorHandler no longer has the traceId in the logging.
I've made a demonstration repo here, run mvn test to view the logging.
Is there a possibility to get the span to also be present in the errorHandler? Or is there a better place for us to do exception logging for failed @JmsListener methods? I've found this previous issue which seems to be the same which was closed with a reference to the issue to add observability support for JMS, but it isn't clear to me if there is something that prevents the errorHandler to have the tracing context.
Comment From: bclozel
I guess it's a tradeoff between extending the observation scope and recording errors.
Right now, the scope is limited to the actual invocation (doesn't include error handling, nor the session management parts) but records all exception thrown from the invocation.
We could instead observe the entire thing (invocation, error handling and session management) but we would not record any error anymore, because AbstractMessageListenerContainer#invokeErrorHandler swallows all errors unless rethrown by the error handler.
I'm tempted to slightly move the instrumentation to only record exceptions if they've not been handled by the error handler. Would this be the expected behavior from a JMS perspective in your opinion?
Comment From: BobLuursema
I am not sure I understand it. What do you mean with "recording errors", I believe you mean something else than logging them like the errorHandler function does when it is not configured?
In general I expected to see the traceId in the log when the 'errorHandler` logged it, so that I know which exception belongs to which message.
Comment From: bclozel
What I mean by recording errors is storing the exception class name as a key in the recorded observation (metric or trace), see here: https://docs.spring.io/spring-framework/reference/6.1/integration/observability.html#observability.jms
Comment From: BobLuursema
Ah I see. We currently don't send our traces anywhere, we just use the log output with Splunk to "manually" correlate across our services. So for our use case recording the exception in the observation isn't interesting.
I think it would be very good to have the traceId with the exception. You suggested you could record the exception if not handled by the error handler? That means that users can do stuff with the exception in the error handler, and if they rethrow it then it will still be added in the observation and if they don't then it is assumed to be handled and the exception will not be recorded. That is what you mean then correct? That sounds quite sensible I think.
Comment From: bclozel
Thanks @BobLuursema for your insights, I've adapted the instrumentation to align the behavior here with other instrumentations in Spring Framework. The error handling phase is now fully part of the observation: you should be able to get the current observation in ErrorHandler implementations and the tracing information should be available.
Comment From: BobLuursema
Thank you @bclozel for the quick help! I've tested the snapshot build with my demo repo and it works perfectly!
Comment From: bclozel
Thanks for testing both our RCs and SNAPSHOT @BobLuursema , this helps a lot!