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

Remove dashes from key identifiers, as those are forbidden by the JMS spec #1

Conversation

jpkrohling
Copy link

In the JMS 2.0 spec, section 3.5.1:

Property names must obey the rules for a message selector identifier. See
Section 3.8“ Message selection” for more information

and then in section 3.8:

An identifier is an unlimited -length character sequence that must
begin with a Java identifier start character; all following characters
must be Java identifier part characters. An identifier start character
is any character for which the method Character.isJavaIdentifierStart
returns true. This includes '_' and '$'. An identifier part character is any character
for which the method Character.isJavaIdentifierPart returns true.

and

Identifiers are either header field references or property references.
The type of a property value in a message selector corresponds to
the type used to set the property. If a property that does not exist in
a message is referenced, its value is NULL.

One specific example of where this is causing problems is when having the OT-contrib Servlet and OT-contrib JMS as part of the same trace: in some tracers, all HTTP Headers are passed down. Other situation can be seen on the example application swarm-jms, which fails with the following exception when using Jaeger:

2017-05-04 11:43:42,777 INFO  [io.opentracing.example.swarm.jms.rest.HelloWorldEndpoint] (default task-1) Creating JMS message with content: text
2017-05-04 11:43:42,837 ERROR [org.jboss.as.ejb3.invocation] (default task-1) WFLYEJB0034: EJB Invocation failed on component HelloWorldEndpoint for method public javax.ws.rs.core.Response io.opentracing.example.swarm.jms.rest.HelloWorldEndpoint.doGet(javax.servlet.http.HttpServletRequest) throws javax.jms.JMSException: javax.ejb.EJBException: javax.jms.JMSRuntimeException: AMQ129012: The property name 'uber-trace-id' is not a valid java identifier.
...
...
Caused by: javax.jms.JMSRuntimeException: AMQ129012: The property name 'uber-trace-id' is not a valid java identifier.
	at org.apache.activemq.artemis.jms.client.ActiveMQMessage.checkProperty(ActiveMQMessage.java:862)
	at org.apache.activemq.artemis.jms.client.ActiveMQMessage.setStringProperty(ActiveMQMessage.java:606)
	at io.opentracing.contrib.jms.common.JmsTextMapInjectAdapter.put(JmsTextMapInjectAdapter.java:28)

A similar exception happens when using Hawkular APM.

With this patch applied, the linked example shows this on the logs when doing a call to http://localhost:8080/hello:

2017-05-04 11:40:12,805 INFO  [io.opentracing.example.swarm.jms.rest.HelloWorldEndpoint] (default task-1) Creating JMS message with content: text
2017-05-04 11:40:12,867 INFO  [io.opentracing.example.swarm.jms.rest.HelloWorldEndpoint] (default task-1) JMS message submitted
2017-05-04 11:40:12,881 INFO  [io.opentracing.example.swarm.jms.rest.HelloWorldEndpoint] (Thread-0 (ActiveMQ-client-global-threads-806501318)) Got the following message: text

@pavolloffay
Copy link

Note that injector/extractor adapters are in common and used for both JMS 1 and 2, therefore allowed characters (e.g. _$ )might differ.

@jpkrohling
Copy link
Author

Also, this PR is intended provide a quick and dirty fix, so that I can keep moving on the samples and to start a discussion on what's appropriate on such situations. Would we want to skip adding those properties, or would we try to clean up the key? Should we try to revert the cleanup on the extract adapter? If we change the key, the data stored within the JMS span might not match with the data stored within the servlet span.

@jpkrohling
Copy link
Author

therefore allowed characters (e.g. _$ )might differ.

I think JMS 1.0 and 2.0 are quite similar on this: https://docs.oracle.com/cd/E19957-01/816-5904-10/816-5904-10.pdf

@objectiser
Copy link
Contributor

Whatever processing is performed in the inject needs to be reversed in the extract - the sending and receiving services should be unaffected by the fact that such a transformation has been required to satisfy the JMS spec restrictions.

@malafeev
Copy link

malafeev commented May 4, 2017

Isn't better don't add properties with dashes (simply skip)?
Modified properties in producer can be not expected in consumer.
And we cannot modify already added properties (if we want clean up in extractor).

@malafeev
Copy link

malafeev commented May 4, 2017

I think the best approach would be to encode dash in injector and decode in extractor.

@malafeev
Copy link

malafeev commented May 5, 2017

fix in 0.0.2 version

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.

4 participants