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

Add exemplars support for Micrometer Tracing #805

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jonatan-ivanov
Copy link
Contributor

This PR adds exemplars support for Micrometer Tracing.
Micrometer Tracing is the successor of Spring Cloud Sleuth in terms of providing an abstraction layer over tracing libraries (i.e.: Brave and OTel) and it provides distributed tracing support for the Spring portfolio (Spring Boot 3.x, Spring Framework 6.x, etc).

There is one thing missing from this PR where I would like to ask for some guidance from the maintainers: there is no static configuration. Since Micrometer Tracing needs a Tracer instance and it supports both Brave + OTel (Brave does not have static config) this might be problematic since users can use their own Tracer instance that might be different than the Prometheus client is aware of.

@fstab @tomwilkie

@jonatan-ivanov jonatan-ivanov force-pushed the exemplars-support-for-micrometer-tracing branch from 68cdf76 to 1d5e4c7 Compare August 10, 2022 01:51
@jonatan-ivanov
Copy link
Contributor Author

Also, do you happen to know why the javadoc goal is failing? The error message does not seem to be very helpful.

Comment on lines +23 to +26
ExemplarSampler exemplarSampler = new DefaultExemplarSampler(new MicrometerTracingSpanContextSupplier(tracer));
ExemplarConfig.setCounterExemplarSampler(exemplarSampler);
ExemplarConfig.setHistogramExemplarSampler(exemplarSampler);
ExemplarConfig.enableExemplars();
Copy link
Member

@fstab fstab Sep 8, 2022

Choose a reason for hiding this comment

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

I just read your comment above again. Yes, the current implementation in Tracer.findSpanContextSupplier() assumes that it can get a tracer from a static variable. In that case client_java will use that tracer automatically, and users don't need any explicit code for that.

However, if the tracer is not available via a static variable, you need to set it explicitly as in your example above. I don't see any other way how to do it.

Copy link
Member

@fstab fstab left a comment

Choose a reason for hiding this comment

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

Sorry again for dropping the ball. Thanks a lot for adding Micrometer tracer support for Examplars.

I will take another look at the initialization, but I think if there's no static variable to get the tracer, users will need to set the tracer explicitly as in your example.

@fstab
Copy link
Member

fstab commented Sep 9, 2022

Hello again, I had another look.

  • Regarding javadoc: The directory with the source code is literally called micrometer.tracing and not micrometer/tracing.
  • Regarding static initialization: Is there any chance to create a class that would be detected by Spring or CDI so that we can get the Micrometer tracer injected? I guess if there's neither a static variable that we can use nor a way to hook into Spring or CDI initialization then users will need to configure the Examplar sampler programmatically.

@fstab
Copy link
Member

fstab commented Sep 11, 2022

Heads-up: I renamed master to main.

@jonatan-ivanov
Copy link
Contributor Author

jonatan-ivanov commented Oct 12, 2022

javadoc: 🤦🏼 🤣 I'm sorry.

static initialization
I'm not 100% sure I understand this:

Is there any chance to create a class that would be detected by Spring or CDI so that we can get the Micrometer tracer injected?

Spring and other CDI solutions will initialize their context after class loading (by creating non-static instances) so even if they inject something to a static context, that will happen (or can easily happen without control) after the java client's static initializers were executed. Also, since they can (and will) inject the registry, the global/static components are less useful in those environments (in fact they can cause trouble and make testing hard/impossible).

This is my honest feedback, I don't want to offend anyone or step on any toes (also, I'm biased): I think the Prometheus Java Client should focus on letting CDI/users configure things in non-static ways (providing builders with defaults and factory methods are completely ok). I think it could also use a ServiceLoader for static access. I assume that this ship has sailed (and might not be the scope of this PR) so I guess the options are:

  1. Keep this as-is saying that users will not get static initializer support for Micrometer Tracing they should do this manually or their framework should do this for them.
    (Right now, this is enough for me from Micrometer/Micrometer Tracing/Spring side.)

  2. Only support OTel: this means handling two cases: 1. only OTel, 2. Micrometer Tracing (with OTel bridge) + OTel 3. Neither.

  3. Support everything (this is basically reimplementing Spring Boot auto-configuration or other auto-configuration in other frameworks), the cases are:

    1. OTel only
    2. Micrometer Tracing (with OTel bridge) + OTel
    3. Micrometer Tracing (with Brave bridge) + Brave
    4. None of the above
    5. Lots of invalid cases like Micrometer Tracing with OTel bridge + Brave/something else/nothing (or Micrometer Tracing with OTel bridge and Brave bridge together)

If Micrometer Tracing adds support for another Tracing library or Prometheus Client adds support for another Tracing library (i.e.: Brave) or both, this will increase quickly

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.

2 participants