Fix another JMS readonly exception #1730
Conversation
| span.withName("JMS SEND to "); | ||
| addDestinationDetails(null, destination, destinationName, span); | ||
| if (isDestinationNameComputed) { | ||
| message.setStringProperty(JMS_DESTINATION_NAME_PROPERTY, destinationName); |
There was a problem hiding this comment.
the offending writing call was right here.
| logger.debug("Failed to set JMS message property {} due to read-only message", headerName, e); | ||
| } catch (JMSException e) { | ||
| logger.warn("Failed to set JMS message property. Distributed tracing cannot work without that."); | ||
| logger.warn("Failed to set JMS message property {}.", headerName); |
There was a problem hiding this comment.
here the name of the property will make it obvious to know the case and if it's related to distributed tracing or not.
Now this is also used to add the destination name for the receiver side.
There was a problem hiding this comment.
It's not for us, it's for the user who doesn't necessarily knows this property is related to distributed tracing. In addition, this message is still valid - if we can't set a property, distributed tracing will not work. We can say "Distributed tracing may not work..." instead.
| | ||
| public class JmsInstrumentationHelper { | ||
| | ||
| private static final JmsMessagePropertyAccessor MSG_ACCESSOR = JmsMessagePropertyAccessor.instance(); |
There was a problem hiding this comment.
[minor] It's already a singleton, I don't think it makes sense to hold a reference to it
| logger.debug("Failed to set JMS message property {} due to read-only message", headerName, e); | ||
| } catch (JMSException e) { | ||
| logger.warn("Failed to set JMS message property. Distributed tracing cannot work without that."); | ||
| logger.warn("Failed to set JMS message property {}.", headerName); |
There was a problem hiding this comment.
It's not for us, it's for the user who doesn't necessarily knows this property is related to distributed tracing. In addition, this message is still valid - if we can't set a property, distributed tracing will not work. We can say "Distributed tracing may not work..." instead.
💚 Build Succeeded
Expand to view the summary
Build stats
Test stats 🧪
Trends 🧪💚 Flaky test reportTests succeeded. Expand to view the summary
Test stats 🧪
|
2 participants
What does this PR do?
Improves previous fix #1715 as there was another direct writing to JMS message not covered.
Checklist