opentracing / opentracing-java

OpenTracing API for Java. 🛑 This library is DEPRECATED! https://github.com/opentracing/specification/issues/163
http://opentracing.io
Apache License 2.0
1.68k stars 344 forks source link

JMS support and general interoperability #307

Closed fgiloux closed 5 years ago

fgiloux commented 5 years ago

Hi

I am struggling with opentracing and interoperability, which is I believe a main focus of the project. I stumbled on a specific case with 2 messaging/JMS implementations. The issue originates from the fact that "-" dashes are not supported in names of JMS headers and used in opentracing. I would first be interested in getting the rational for using non-alphanumerics as it may require special handling depending on the communication channel. The next thing is that I have not been able to see how encoding should be addressed in a standard way. Jaeger provides URLencoding: https://github.com/jaegertracing/jaeger-client-java/blob/master/jaeger-core/src/main/java/io/jaegertracing/internal/propagation/TextMapCodec.java but nothing further, JMS in my case. Using 2 different libraries java-jms and camel-opentracing, one does not propagate the traces of the other. java-jms does dash-encoding: https://github.com/opentracing-contrib/java-jms/blob/master/opentracing-jms-common/src/main/java/io/opentracing/contrib/jms/common/JmsTextMapInjectAdapter.java Camel not, using: https://github.com/jaegertracing/jaeger-client-java/blob/master/jaeger-core/src/main/java/io/jaegertracing/internal/propagation/TextMapCodec.java It is easy to fix in Camel and I have a trial working but my second question is: Is java-jms the reference and what should be used by Camel and any other framework using the JMS API for communication? I was not able to see any information on that.

Thanks,

Frédéric

objectiser commented 5 years ago

Hi Frédéric

I would first be interested in getting the rational for using non-alphanumerics as it may require special handling depending on the communication channel.

OpenTracing doesn't define the header names, so this would be more of a question for the tracer implementations.

Regarding the interoperability issue - as the encoding is specific to Java, and a particular Java API, I don't think it belongs in the OpenTracing spec - so I think the ot-contrib projects could be a good place to define language/api specific criteria - so in this case java-jms should be considered the reference. Would be worth creating an issue/PR on that repo to clearly define the header encoding.

fgiloux commented 5 years ago

Hi Gary

Thanks for the clarification. Sorry for me missing some project background but why are the header names not in scope of OpenTracing? Isn't it necessary for having one implementation being able to communicate with another? I can see the use case where you may have a component in the middle provided by a third party, where you don't control the implementation, understanding that supporting OpenTracing is already a call to be made by third party. As per your recommendation I will pursue this topic with java-jms and Camel projects.

yurishkuro commented 5 years ago

@fgiloux data formats are not in scope because of separation of concerns principle: they don't have to be. If you are using OpenTracing implementation from the same vendor/oss-project in different applications, they will interop without you needing to know anything about data formats, your only concern is programming API that the OpenTracing provides. If you use different implementations, then yes, you need to worry about the interop, but then it's the implementation's issue, not the API's. There's no reason why a project designing the APIs need to be coupled with another project (e.g. W3C TraceContext) that standardizes on data formats.

fgiloux commented 5 years ago

@yurishkuro it makes perfectly sense. Thank you for pointing me to W3C Distributed Tracing Working Group. I was happy to read there: "In order to increase interoperability across multiple protocols and encourage successful integration by default it is recommended to keep the header name lower case. Header name is a single word without any delimiters like hyphen (-)."

Serkan80 commented 4 years ago

Im still facing this problem with Jaeger, Camel and Spring. Is there already any fix available ?

whiskeysierra commented 4 years ago

If text map doesn't work over JMS, why don't we use binary?

https://opentracing.io/docs/overview/inject-extract/#required-inject-extract-carrier-formats

pavolloffay commented 4 years ago

@Serkan80 @objectiser maintains camel instrumentation, it is supposed to work. Could you share a reproducer?

coder-x-esb commented 4 years ago

@pavolloffay My flow looks like the following:

WS call -> Route A -> JMS Queue -> Route B -> WS client call (and then back)

Route A sends uber-trace-id 1234, B receives uber-HYPHEN-trace-hyphen-id in the JMSMessage properties and creates a new traceId 5678 in the Camel Exchange Header.

Here is my pom.xml: `

org.apache.camel
        <artifactId>camel-opentracing</artifactId>
        <version>${version.camel}</version> <!-- 2.24.1-->
    </dependency>
    <dependency>
        <groupId>io.jaegertracing</groupId>
        <artifactId>jaeger-client</artifactId>
        <version>0.34.0</version>  
    </dependency>

`

When I use jaeger-client versien >0.34.0, then no any traceId is created in the Camel Exchange Header, it is just empty.

And this is how I initialize Jaeger in my Spring & Camel application:

JaegerTracingConfig.java: `@Bean public Tracer getTracer() throws FrameworkException {

    return new Configuration("Service A")
        .withSampler(new SamplerConfiguration().withType("const").withParam(1))
        .withReporter(new ReporterConfiguration().withSender(SenderConfiguration.fromEnv().withEndpoint(jeagerUrl)))
//        .withCodec(new Configuration.CodecConfiguration()
//            .withCodec(Format.Builtin.HTTP_HEADERS, new TextMapCodec(false))
//            .withCodec(Format.Builtin.TEXT_MAP, new TextMapCodec(false)))
        .getTracer();
}

`

CamelContextStartupListener.java:

` @Slf4j @Configuration public class CamelContextStartupListener extends EventNotifierSupport implements ApplicationContextAware {

private Tracer tracer;

@Override
public void notify(EventObject event) {
    if (event instanceof CamelContextStartedEvent) {
        log.info("Initializing Tracer...");
        if (this.tracer != null) {
            CamelContextStartedEvent cce = (CamelContextStartedEvent) event;
            OpenTracingTracer ott = new OpenTracingTracer();
            ott.setTracer(tracer);
            //ott.setEncoding(false);
            ott.init(cce.getContext());
            log.info("Tracer initialized...");
        } else {
            log.warn("No tracer found. Did you add the correct dependencies ?");
        }
    }
}

@Override
public boolean isEnabled(EventObject event) {
    return event instanceof CamelContextStartedEvent;
}

@Override
public void setApplicationContext(ApplicationContext applicationContext) throws BeansException {
    tracer = applicationContext.getBean(Tracer.class);
}

} `

I've tried disabling encoding (see the commented code) but it didn't work. And also updating my jaeger-client dependency didn't solve the problem. I've tried version 1.0.0 till 0.34.0 and only the latter adds the traceId in the Exchange header.

Hope someone can help me.

Thx in advance.

objectiser commented 4 years ago

@coder-x-esb Would it be possible to setup a github repo with the example, and a readme to describe how to build and run the example? It would make it faster to investigate.

Serkan80 commented 4 years ago

@objectiser I can't do that since the code uses my company's code and libraries.

But I've added breakpoints to CamelMessagingHeadersExtractAdapter.java, CamelHeadersExtractAdapter.java and CamelHeadersInjectAdapter.java, and I notice that they are never getting called.

And the decode method in these classes try to replace '$dash$', but I get HYPHEN, which will not get replaced.

Is there an easy way for registering these adapters when I initialize the Tracer ?

objectiser commented 4 years ago

@coder-x-esb Versions after 0.34.0 are not compatible with opentracing 0.31.0 used by camel 2.24.1 - this is probably why you see no headers with more recent versions of Jaeger. I will try to reproduce the issue.

Serkan80 commented 4 years ago

@objectiser I think I got the root cause of the problem.

I see somewhere in our code that we are defining JmsComponents via Spring with a bean name, like the following:

` @Bean(name = "core-jms") public JmsComponent createJmsComponentBean( @Qualifier(value = "core-jmsConnectionFactory") ConnectionFactory connectionFactory) throws JMSException { JmsComponent jmsComponent = new JmsComponent(); JmsConfiguration jmsConfiguration = new JmsConfiguration(connectionFactory); jmsConfiguration.setJmsMessageType(JmsMessageType.Text); jmsConfiguration.setUseMessageIDAsCorrelationID(true); jmsComponent.setConfiguration(jmsConfiguration);

    return jmsComponent;
}

`

And this component's ("protocol") name changes into core-jms:// in Camel in stead of using the default jms://.

And this also causes that no any existing SpanDecorator will match with this component name, thus no correct extraction and injection will occur.

objectiser commented 4 years ago

@Serkan80 yes that would cause it. You should create an issue on the camel jira, to get core-jms handled.

objectiser commented 4 years ago

@Serkan80 Sorry didn't read your comment closely enough - so your bean name is being used instead - thought it was another common protocol used for jms. Might be worth still creating an issue on the camel jira though to explore options.

Serkan80 commented 4 years ago

@objectiser How can I do that ?

Btw I've created my own SpanDecorators for core-jms and core-jms-tx, and I see that these get loaded up, see below:

`public class CoreJmsSpanDecorator extends JmsSpanDecorator {

@Override
public String getComponent() {
    return "core-jms";
}

@Override
public TextMap getExtractAdapter(final Map<String, Object> map, final boolean jmsEncoding) {
    return new CamelMessagingHeadersExtractAdapter(map, true);
}

@Override
public TextMap getInjectAdapter(final Map<String, Object> map, final boolean jmsEncoding) {
    return new CamelMessagingHeadersInjectAdapter(map, true);
}

}`

And it is still not working.

What I see is that the injection with encoding works from Service A, but when the message gets picked up by Service B, the extraction isn't called and a new traceId is generated again.

objectiser commented 4 years ago

@Serkan80 As this is more of a camel issue, could you create a ticket here: https://issues.apache.org/jira/projects/CAMEL and we can investigate further. Thanks.