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 auto-configuration for OTLP span exporter #34508

Closed
wants to merge 1 commit into from

Conversation

jonatan-ivanov
Copy link
Member

@jonatan-ivanov jonatan-ivanov commented Mar 7, 2023

With these changes an OTLP HTTP/protobuf exporter is auto-configured if opentelemetry-exporter-otlp is on the classpath.

Closes gh-34390

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Mar 7, 2023
@jonatan-ivanov jonatan-ivanov marked this pull request as ready for review March 9, 2023 01:28
@jonatan-ivanov jonatan-ivanov marked this pull request as draft March 9, 2023 08:32
@jonatan-ivanov
Copy link
Member Author

I put this back to draft since support for (auth) headers should be also added.

@mhalbritter mhalbritter added the theme: observability Issues related to observability label Mar 9, 2023
@jonatan-ivanov jonatan-ivanov marked this pull request as ready for review March 9, 2023 20:10
* @author Jonatan Ivanov
* @since 3.1.0
*/
@ConfigurationProperties("management.otlp.tracing")
Copy link
Contributor

Choose a reason for hiding this comment

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

Hi @jonatan-ivanov , I see that spring is diverging from the configuration properties proposed by the otel-sdk. For instance, the properties managed by this class maps to otel-sdk "otel.exporter.otlp.endpoint", "otel.exporter.otlp.compression" and so on...

Are there any plans to converge the config property names to the same used by otel-sdk ?

If no, isnt there another way to make them at least seem easier to map. E.g, spring version would only add the management.otlp prefix ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Here's the rationale behind the current structure: #30381 This PR follows that pattern.

There are at least two tricky things here:

  1. There are at least two components that can export OTLP right now in Boot, the Micrometer Tracing OTel bridge (in this PR) and Micrometer's OTLP Meter Registry. I think users need to be able to configure them separately and otel.exporter.otlp.endpoint does not really make that distinction.
  2. Under the hood, shipping OTLP does not need to use the OTel SDK at all, e.g.: Micrometer's OTLP registry does not use it, if for example Brave will add OTLP support, I guess that won't either. Check out the Zipkin properties and auto-configuration. Boot does not use OTel properties there and it needs to auto-configure either OTel or Brave based on those properties.

I think right now this is the easiest thing you can do to set these:

otel.exporter.otlp.xyz=...
management.otlp.metrics.export.xyz=${otel.exporter.otlp.xyz}
management.otlp.tracing.xyz=${otel.exporter.otlp.xyz} // maybe I should add export here?

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 think we should figure out what would be the best and your feedback is apprechiated, right now this is what we have:

  • OtlpProperties for metrics
    • has a different prefix: management.otlp.metrics.export (this PR has management.otlp.tracing)
    • has a different property url (this PR calls it endpoint)
  • ZipkinProperties for tracing (Brave/OTel)
    • has similar prefix: management.zipkin.tracing (this PR follows that convention)
  • OTel SDK Properties
    • has the prefix: otel.exporter.otlp
    • has similar properties

Copy link
Contributor

Choose a reason for hiding this comment

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

  1. There are at least two components that can export OTLP right now in Boot, the Micrometer Tracing OTel bridge (in this PR) and Micrometer's OTLP Meter Registry. I think users need to be able to configure them separately and otel.exporter.otlp.endpoint does not really make that distinction.

Fair enough, makes sense as users could possibly use 2 different otel-compatible backends: one for metrics and other for tracing

management.otlp.metrics.export.xyz=${otel.exporter.otlp.xyz}

Im doing the opposite: trying to support the SDK props as they are more widely known, so end-users would be able to configure it based on SDK docs, but it is really hard to map all the spring props to the sdk props. If it was just a matter of prefixes, life would be easier. But I understand the complexity and the other scenarios that you mention. There's even a more complex one such as management.tracing.sampling.probability=${otel.traces.sampler.arg} - the otel.traces.sampler.arg (urgh!) is a beast on its own as it includes inner properties on it depending on the sampler implementation. Perhaps somewhere in the docs we could have a section mapping the spring->otel-sdk properties.

  • has a different prefix: management.otlp.metrics.export (this PR has management.otlp.tracing)

  • has a different property url (this PR calls it endpoint)

I personally like management.otlp.tracing.export.http.endpoint more.
I also liked that you exposed all the OtlpHttpSpanExporterBuilder configurations, different from the OpenTelemetryAutoConfiguration.otelSpanProcessor which is using the BatchSpanProcessor internally and not offering the possibility to configure its propertyes such as batch/queue size, timeouts and so on...

@AndersClausen
Copy link

@jonatan-ivanov Really looking forward to this feature being made available, as it is really the last thing that is preventing us from migrating to Spring Boot 3.x. Can you tell me which release you're aiming for? Cheers

@wilkinsona wilkinsona added the type: enhancement A general enhancement label Apr 14, 2023
@wilkinsona wilkinsona added this to the 3.1.x milestone Apr 14, 2023
@jonatan-ivanov
Copy link
Member Author

jonatan-ivanov commented Apr 14, 2023

@AndersClausen See Andy setting the milestone on this PR (formerly, the connected issue had the same milestone).
If you want to, you can migrate and try this out, with the latest 3.x, you "just" need to create beans, see what this PR does. It's not optimal and should be temporary but it should be simple and lets you try things out.

With these changes an OTLP HTTP/protobuf exporter is auto-configured
if opentelemetry-exporter-otlp is on the classpath.

See spring-projectsgh-34508
@wilkinsona wilkinsona removed the status: waiting-for-triage An issue we've not yet triaged label Apr 19, 2023
@wilkinsona wilkinsona modified the milestones: 3.1.x, 3.1.0-RC1 Apr 19, 2023
wilkinsona pushed a commit that referenced this pull request Apr 19, 2023
With these changes an OTLP HTTP/protobuf exporter is auto-configured
if opentelemetry-exporter-otlp is on the classpath.

See gh-34508
wilkinsona added a commit that referenced this pull request Apr 19, 2023
@jonatan-ivanov jonatan-ivanov deleted the otlp branch April 19, 2023 16:06
@chicobento
Copy link
Contributor

Hi @jonatan-ivanov , we are facing some issues integrating this into our project. The scenario is the following:

  • we have a custom class that implements SpanExporter interface and internally wraps our own mutable OtlpHttpSpanExporter
  • with this, we need to prevent the OtlpAutoConfiguration from creating the OtlpHttpSpanExporter Bean. However, the @ConditionalOnMissingBean is based only on OtlpHttpSpanExporter and OtlpGrpcSpanExporter beans, which is not our case as we implement SpanExporter interface and cannnot descend from the OtlpHttpSpanExporter since its final

Some possible solutions to my problem that I tried:

  • Remove OtlpAutoConfiguration: this behavior is added by a internal library, so I cannot easily remove the OtlpAutoConfiguration without impacting the applications that uses our lib
  • Replace the SpanProcessor: as the OpenTelemetryAutoConfiguration.otelSpanProcessor method does not have a @ConditionalOnMissingBean. I cannot easily replace the SpanProcessor with a custom one that would ignore the OtlpHttpSpanExporter Bean provided by OtlpAutoConfiguration
  • Replace the OpenTelemetryAutoConfiguration.otelSdkTracerProvider: this would not work because the SpanProcessor.otelSpanProcessor will always instantiate a BatchSpanProcessor and start its thread. I'd have to shutdown it

Do you have any other suggestion on how can I overcome this ?

I was wondering if isn't the current OtlpAutoConfiguration.otlpHttpSpanExporter factory method too intrusive as it always provide a SpanExporter Bean even when the application has already defined its own SpanExporter ? Can we change this to a simple @ConditionalOnMissingBean(SpanExporter.class) ?

@jonatan-ivanov
Copy link
Member Author

@chicobento I created a separate issue to discuss this, could you please check my answer there and reply to my questions? #35596

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
theme: observability Issues related to observability type: enhancement A general enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add auto-configuration for OTLP span exporter
7 participants