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

JMS instrumentation #584

Closed
codefromthecrypt opened this issue Jan 21, 2018 · 21 comments
Closed

JMS instrumentation #584

codefromthecrypt opened this issue Jan 21, 2018 · 21 comments

Comments

@codefromthecrypt
Copy link
Member

codefromthecrypt commented Jan 21, 2018

There's an indirect requirement for JMS instrumentation through spring-cloud-sleuth. Besides the multiple versions of JMS apis, there's an important concern with propagation due to naming constraints which exist in JMS, but not necessarily in the underlying transport.

JMS requires that message header names follow java naming conventions. Notably this excludes hyphens and dots. For example, in Camel, there's a naming policy to map to and from these constraints.

For example, many simply downcase B3 headers to x-b3-traceid for use in non-http transports (which might not be case insensitive). In JMS, this wouldn't work, even if rabbitmq is used underneath which has no such constraint. Ex, if using camel JMS, this header would map into x_HYPHEN_b3_HYPHEN_traceid.

In some cases, JMS headers are replaced based on constants or other patterns, like globally replacing hyphens with underscores. Interestingly opentracing decided on a different pattern _$dash$_, though they have no pattern for dots: For example, if you were using the OT library and camel on the other, you'd get x_HYPHEN_b3_HYPHEN_traceid on one side and x_$dash$_b3_$dash$_traceid on the other, mutually unreadable.

The universal implication of using JMS is that it implies global coordination of these naming patterns, or there will be propagation incompatibility (ex traces will restart). This will also break anything else propagated that isn't a trace ID. This type of problem has already been noticed in early users of message tracing.

Importantly, even if we were to have static replacements for B3, we don't know if users will introduce propagated headers with dots or hyphens in them. In other words, I don't think a solution to this problem can be constant in nature, though constants could help.

Challenge here is to introduce support for JMS propagation, but without assuming we are using B3 format, and ideally inheriting a naming convention as opposed to introducing another dimension of naming convention complexity for others. Coming up with a sane default might be particularly tricky.

Calling for advice from @davsclaus @garyrussell @Xylus @jkowall @christian-schwarzbauer @CodingFabian (others feel free to contribute too!)

See also discussion on w3c trace context https://github.com/w3c/distributed-tracing/issues/35

@codefromthecrypt
Copy link
Member Author

One thing I discussed with @marcingrzejszczak about this issue, is that it is virulent: at the point we start propagating different field names, we have the concern of whether the other side can read it or not. For this reason, I'm very in-favor of JMS specific notions of this. A lot of messaging providers are unaffected by this problem and can safely pass headers from http directly into messaging headers. Even if it feels tempting to make a "messaging" header name, we have to keep in mind that many are already using header names with hyphens. If we start propagating underscores universally, we introduce complexity to existing non-JMS consumers.

Luckily, JMS header mapping is a problem sites using it already know. It is mechanical for people to rewrite headers from hyphens to something else, even if annoying. It is above why I'm biased towards containing this mapping problem as close as practical to JMS and not spreading the infection beyond it, potentially into innocent frameworks with no such constraints. If we choose to do the infectious route, it will imply folks who aren't using JMS will have to start adding what might feel like spaghetti code.

/me ends commentary

@codefromthecrypt
Copy link
Member Author

to fix this longer, we've rallied to have the trace headers not have hyphens in the w3c spec https://github.com/w3c/distributed-tracing/tree/master/trace_context

@codefromthecrypt
Copy link
Member Author

openzipkin/b3-propagation#21 potentially solves this

@chsi13
Copy link

chsi13 commented Aug 20, 2018

Any planning for jms instrumentation in brave ? Interested via spring sleuth usage. +1 for this.

@codefromthecrypt
Copy link
Member Author

codefromthecrypt commented Aug 20, 2018 via email

@codefromthecrypt
Copy link
Member Author

this needs to merge prior to working on JMS #763

@codefromthecrypt
Copy link
Member Author

folks who want JMS to happen should emoji or reply on this comment. Until the format is sorted, we are blocked openzipkin/b3-propagation#21 (comment)

@codefromthecrypt
Copy link
Member Author

Can folks here offer how they are using JMS at the moment? via spring-jms, camel, MDBs, or something else? I want to make sure the README is relevant

@jorgheymans
Copy link
Contributor

spring-jms talking to weblogic 12, Oracle Service Bus

@codefromthecrypt
Copy link
Member Author

I'll make a separate module for spring-jms.

Can anyone review this from a user-friendliness and/or implementation perspective? #764

@zeagord
Copy link
Member

zeagord commented Aug 27, 2018

Some are using spring-jms to ActiveMQ. And some services using ActiveMQ component with JMS listener with Camel Routes.

Sample configuration:

spring:
  activemq:
    broker-url: tcp:https://0.0.0.0:61616
    in-memory: false
    pool:
      block-if-full: true
      block-if-full-timeout: -1
      create-connection-on-startup: true
      enabled: false
      expiry-timeout: 0
      idle-timeout: 30s
      max-connections: 5
      maximum-active-session-per-connection: 500
      reconnect-on-exception: true
      time-between-expiration-check: -1
      use-anonymous-producers: true
  jms:
    pub-sub-domain: false
    listener:
      acknowledge-mode: auto
      auto-startup: true
      concurrency: 20
      max-concurrency: 30
    template:
      default-destination:
      delivery-mode: non_persistent
      priority: 100
      qos-enabled: true
      receive-timeout: 1000
      time-to-live: 36000

@chsi13
Copy link

chsi13 commented Aug 27, 2018 via email

@codefromthecrypt
Copy link
Member Author

@jeqo asked about tracing the new JMS* apis in JMS 2.0. I will add discussion about that to #764

@codefromthecrypt
Copy link
Member Author

updated #764 with JMS 2.0 apis (entrypoint of TracingJMSContext). please give feedback on this, if you use JMSContext.

@codefromthecrypt
Copy link
Member Author

On our snapshot repo (http:https://oss.jfrog.org/oss-snapshot-local) 5.3.0-SNAPSHOT includes brave-instrumentation-jms

@codefromthecrypt
Copy link
Member Author

Added #778 to track spring-jms as right now message listeners that are handled by spring aren't invoked in the context of a parent producer span

@vijaykunapareddy
Copy link

Do you have any example regarding, how to integrate brave with Apache ActiveMQ ?

@codefromthecrypt
Copy link
Member Author

codefromthecrypt commented Dec 4, 2018 via email

@Serkan80
Copy link

Im still facing this problem with Spring, Camel and Jms.

Is there already any fix available for this problem ?

@chsi13
Copy link

chsi13 commented Nov 28, 2019

Yes. Just Upgrade to last version.

@Serkan80
Copy link

Oops wrong post, I am using Jaeger not Zipkin. I guess the update wont work for me.

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

No branches or pull requests

6 participants