Add ability to customize some aspects of the otel sdk creation#1586
Add ability to customize some aspects of the otel sdk creation#1586breedx-splk wants to merge 4 commits intoopen-telemetry:mainfrom
Conversation
| fun interface TracerProviderCustomizer { | ||
| fun customize(builder: SdkTracerProviderBuilder): SdkTracerProviderBuilder | ||
| } | ||
|
|
||
| fun interface LoggerProviderCustomizer { | ||
| fun customize(builder: SdkLoggerProviderBuilder): SdkLoggerProviderBuilder | ||
| } | ||
|
|
||
| fun interface MeterProviderCustomizer { | ||
| fun customize(builder: SdkMeterProviderBuilder): SdkMeterProviderBuilder | ||
| } |
There was a problem hiding this comment.
So these types aren't completely necessary -- but I thought that it read a bit cleaner than using the underlying functional methods from the SDK. If we're worried about the increase in surface area, then we could probably just remove them and refer to the functions...but it's a bit verbose.
What do you think?
There was a problem hiding this comment.
What's the use case you have in mind for these customizers in the initializer? They seem to be providing a huge hammer to me. If I'm not mistaken, they would allow users to override our processors and our exporters, so they might get into trouble by doing so and then seeing that sessions, disk buffering, and export parameters don't work as configured via the DSL options.
There was a problem hiding this comment.
typealias might be an option here if you want a degree of separation but without creating an extra interface
There was a problem hiding this comment.
typealiasmight be an option here if you want a degree of separation but without creating an extra interface
Yes! Thank you. I think the typealias is considerably cleaner here.
There was a problem hiding this comment.
What's the use case you have in mind for these customizers in the initializer?
Good question. The ones that I've heard, and what was discussed in #1577, is the ability to add custom SpanProcessors into the sdk. Without some help from the initializer, the developer would need to use the OpenTelemetryRumBuilder directly, missing out on the benefits/convenience of the Initializer.
Another use case that has been brought up more than once is the ability to expire/swap HTTP exporter headers/tokens...so basically customizing the exporters. That's not covered here, but I think we will need to address it before too long.
They seem to be providing a huge hammer to me. If I'm not mistaken, they would allow users to override our processors and our exporters, so they might get into trouble by doing so and then seeing that sessions, disk buffering, and export parameters don't work as configured via the DSL options.
You're completely correct -- this is a big hammer! You're also right, that I think this allows users to break things that the agent tries to help with. I think this is already true of the upstream autoconfigure as well -- it's possible to ab/use the autoconfigure callbacks to undo some of the defaults (like breaking the BSP which is responsible for exporting, for example).
There was a problem hiding this comment.
Got it, thanks for the details.
The ones that I've heard, and what was discussed in #1577, is the ability to add custom SpanProcessors into the sdk.
This is fair enough. I believe we should be more specific with the api surface additions to make its evolution more iterative, though. I see the benefits of adding a big hammer like this one in terms of making it future-proof, but I'm afraid it can cause strange and difficult to address isssues down the line. Based on that, why don't we provide an API to add processors specifically instead?
Another use case that has been brought up more than once is the ability to expire/swap HTTP exporter headers/tokens...so basically customizing the exporters.
This should probably be a separate API that would allow to customize the exporters that we create in the agent. Not only to avoid providing a big hammer, but also because of how the Java SDK works. By adding a [Signal]ProviderCustomizer as proposed in this PR, I don't think that would help users to customize our exporters, as they can't access those via the Java SDK builders. So a specific exporter customizer seems to be the only feasible approach.
You're completely correct -- this is a big hammer! You're also right, that I think this allows users to break things that the agent tries to help with. I think this is already true of the upstream autoconfigure as well -- it's possible to ab/use the autoconfigure callbacks to undo some of the defaults (like breaking the BSP which is responsible for exporting, for example).
Don't get me wrong, I do not to like the idea of having to be some sort of police for our users and to tell them what they should and shouldn't do with their code. The thing is that experienced users can already do whatever they want to by using the rum builder, while being conscious that they might break things, which is why I believe that whoever decides to use the agent is expecting not only simplicity but also reliability, and it will become difficult to guarantee the reliability if we make it too flexible. Because of that, and to being able to provide better support for agent users when they run into trouble, I think it's best to increase the API surface to support concrete use cases in mind, such as allowing for custom processors in this case, and thus prevent other accidental changes.
| fun interface TracerProviderCustomizer { | ||
| fun customize(builder: SdkTracerProviderBuilder): SdkTracerProviderBuilder | ||
| } | ||
|
|
||
| fun interface LoggerProviderCustomizer { | ||
| fun customize(builder: SdkLoggerProviderBuilder): SdkLoggerProviderBuilder | ||
| } | ||
|
|
||
| fun interface MeterProviderCustomizer { | ||
| fun customize(builder: SdkMeterProviderBuilder): SdkMeterProviderBuilder | ||
| } |
There was a problem hiding this comment.
typealias might be an option here if you want a degree of separation but without creating an extra interface
| import io.opentelemetry.sdk.trace.SdkTracerProviderBuilder | ||
|
|
||
| @OpenTelemetryDslMarker | ||
| class OtelSdkCustomizationsSpec internal constructor() { |
There was a problem hiding this comment.
Is it worth considering marking these new APIs as 'experimental' or 'incubating'? I suppose this is a more general question on what stability guarantees we want to provide for new APIs in modules that are otherwise stable?
My personal opinion is that marking a new API as experimental for a couple of versions is fairly normal on Android. Would be curious to hear others thoughts.
There was a problem hiding this comment.
Seems like a reasonably good idea to me. Open to ideas on how to actually do this in practice.
There was a problem hiding this comment.
The Incubating annotation in android-agent is one way of achieving this. It's probably worth chatting about this at the SIG as it's a broader discussion of how we approach adding new APIs to otherwise stable modules
| } | ||
|
|
||
| fun interface TracerProviderCustomizer { | ||
| fun customize(builder: SdkTracerProviderBuilder): SdkTracerProviderBuilder |
There was a problem hiding this comment.
One potential downside is that this does tie our API further to symbols declared in opentelemetry-java's SDK module. If we ever do decide to use opentelemetry-kotlin then this takes us further from that goal - but perhaps that's ok for now?
There was a problem hiding this comment.
Right, this did occur to me as I was hacking on this, and I respect that you're especially sensitive to this (with KSP compat being an end goal). I'm not sure how to do things like this without some amount of dependency on java core for now....
There was a problem hiding this comment.
I think the benefits outweigh the downside here tbh.
| * You can use this to customize additional nonstandard aspects of the OpenTelemetry SDK. | ||
| * Most users should not need to provide any customizations here. | ||
| */ | ||
| fun otelSdkCustomizations(action: OtelSdkCustomizationsSpec.() -> Unit) { |
There was a problem hiding this comment.
I think we should add some sort of guarantee about configuration order here. For example, this could be the final action after all the other configuration (such as OTLP exporters) has been setup on the builder object.
There was a problem hiding this comment.
I agree this is important to consider. I think right now that the implementation does these last, but I'm not sure how to bake that into this contract.
|
I agree with the goal of allowing users to configure the underlying OpenTelemetry instance more. I think this changeset opens up some interesting questions about the tradeoffs of how best to achieve that. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1586 +/- ##
==========================================
+ Coverage 65.86% 66.36% +0.50%
==========================================
Files 157 158 +1
Lines 3132 3146 +14
Branches 318 319 +1
==========================================
+ Hits 2063 2088 +25
+ Misses 972 966 -6
+ Partials 97 92 -5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Thanks for a good discussion, everyone. After the SIG meeting today, I think we concluded that the specific use case of adding geo attributes would be better handled by an existing API which we overlooked previously -- namely the With that, I'm going to convert o draft for now, in case we want to continue discussing the tradeoffs...but I definitely agree that the approach provided in this PR is pretty broad and opens up some specific problems (big footgun, sdk in api footprint). |
Related to #1577
This adds some additional customization capabilities to the initialization DSL. We've had users ask for similar things quite a few times now...and rather than always having to resort to using the
OpenTelemetryRumBuilderI figured we could try and find some middle ground -- something that allows users to get the benefits of the initializer while still being able to do niche/bespoke customizations when needed.This does change the api by adding functionality, but is backwards compatible and thus non-breaking.