Skip to content
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

Use valid Java identifiers for message keys #3863

Merged

Conversation

trask
Copy link
Member

@trask trask commented Aug 17, 2021

This is causing error on IBM MQ:

Error Setting JMS Property io.opentelemetry.javaagent.shaded.instrumentation.spring.integration.ContextAndScope with value ContextAndScope{context=null, scope=INSTANCE}
com.ibm.msg.client.jms.DetailedMessageFormatException: JMSCC0049: The property name 'io.opentelemetry.javaagent.shaded.instrumentation.spring.integration.ContextAndScope' is not a valid Java(tm) identifier.

Probably could rename to "valid" header name, but seems better not to use message header for local propagation.


private final ContextPropagators propagators;
private final Instrumenter<MessageWithChannel, Void> instrumenter;

// TODO (trask): optimize for javaagent by using field-backed context store
private final Cache<MessageChannel, ContextAndScope> sendContextAndScopeHolder =
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't MessageChannel a shared object that is used to send many possibly concurrent messages?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh no 🙁

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the underscore idea doesn't work out, here's another take on this problem that I considered when developing this instrumentation: 7c8d614

@@ -22,12 +23,16 @@
import org.springframework.util.LinkedMultiValueMap;

final class TracingChannelInterceptor implements ExecutorChannelInterceptor {
private static final String CONTEXT_AND_SCOPE_KEY = ContextAndScope.class.getName();
private static final String SCOPE_KEY = TracingChannelInterceptor.class.getName() + ".scope";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From what I understand dot is invalid, underscore would be fine

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 I'll try that. Any idea what a non-serializable message header even means tho?

Copy link
Contributor

@laurit laurit Aug 18, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a very good question. In our test with rabbitmq https://github.com/spring-projects/spring-amqp/blob/bd088731b08ceb7ca90543e18b19cbf70a21c6c8/spring-rabbit/src/main/java/org/springframework/amqp/rabbit/support/DefaultMessagePropertiesConverter.java#L184 converts it to string by calling toString. For actual jms I guess the relevant part is https://github.com/spring-projects/spring-integration/blob/590fb01743d537f3436bccf3a8c445c51b7640d1/spring-integration-jms/src/main/java/org/springframework/integration/jms/DefaultJmsHeaderMapper.java#L133 which seems to ignore properties of unknown type. Though this doesn't explain how there is an exception with IBM MQ, there must be some other place where property is set on jms message. Even if we correct the property name it is likely to still fail as https://docs.oracle.com/javaee/5/api/javax/jms/Message.html#setObjectProperty(java.lang.String,%20java.lang.Object) does not accept arbitrary objects (assuming that this is the method that caused original exception).

@trask trask marked this pull request as draft August 18, 2021 03:08
@trask trask marked this pull request as ready for review August 18, 2021 03:50
@trask trask changed the title Avoid message headers for local propagation Use valid Java identifiers for message keys Aug 18, 2021
@trask
Copy link
Member Author

trask commented Aug 19, 2021

let's try this simple fix, and revisit if needed

@trask trask merged commit 31f22eb into open-telemetry:main Aug 19, 2021
@trask trask deleted the avoid-message-headers-for-local-propagation branch August 19, 2021 23:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants