Add ability to customize some aspects of the otel sdk creation #1586
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,65 @@ | ||
| /* | ||
| * Copyright The OpenTelemetry Authors | ||
| * SPDX-License-Identifier: Apache-2.0 | ||
| */ | ||
|
|
||
| package io.opentelemetry.android.agent.dsl | ||
|
|
||
| import io.opentelemetry.sdk.logs.SdkLoggerProviderBuilder | ||
| import io.opentelemetry.sdk.metrics.SdkMeterProviderBuilder | ||
| import io.opentelemetry.sdk.trace.SdkTracerProviderBuilder | ||
|
|
||
| @OpenTelemetryDslMarker | ||
| class OtelSdkCustomizationsSpec internal constructor() { | ||
|
Member
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.
Contributor
Author
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.
Member
There was a problem hiding this comment. The Incubating annotation in |
||
| internal val tracerProviderCustomizers: MutableList<TracerProviderCustomizer> = mutableListOf() | ||
| internal val loggerProviderCustomizers: MutableList<LoggerProviderCustomizer> = mutableListOf() | ||
| internal val meterProviderCustomizers: MutableList<MeterProviderCustomizer> = mutableListOf() | ||
|
|
||
| /** | ||
| * Modify the creation of the OpenTelemetry TracerProvider by providing | ||
| * your own customizer here. The customizer is a function | ||
| * that receives an instance of [SdkTracerProviderBuilder] and returns an | ||
| * instance of [SdkTracerProviderBuilder], which should almost always be | ||
| * the same instance. If a new instance is returned, the operation can be | ||
| * destructive. | ||
| */ | ||
| fun customizeTracerProvider(customizer: TracerProviderCustomizer) { | ||
| tracerProviderCustomizers.add(customizer) | ||
| } | ||
|
|
||
| /** | ||
| * Modify the creation of the OpenTelemetry LoggerProvider by providing | ||
| * your own customizer here. The customizer is a function | ||
| * that receives an instance of [SdkLoggerProviderBuilder] and returns an | ||
| * instance of [SdkLoggerProviderBuilder], which should almost always be | ||
| * the same instance. If a new instance is returned, the operation can be | ||
| * destructive. | ||
| */ | ||
| fun customizeLoggerProvider(customizer: LoggerProviderCustomizer) { | ||
| loggerProviderCustomizers.add(customizer) | ||
| } | ||
|
|
||
| /** | ||
| * Modify the creation of the OpenTelemetry MeterProvider by providing | ||
| * your own customizer here. The customizer is a function | ||
| * that receives an instance of [SdkMeterProviderBuilder] and returns an | ||
| * instance of [SdkMeterProviderBuilder], which should almost always be | ||
| * the same instance. If a new instance is returned, the operation can be | ||
| * destructive. | ||
| */ | ||
| fun customizeMeterProvider(customizer: MeterProviderCustomizer) { | ||
| meterProviderCustomizers.add(customizer) | ||
| } | ||
| } | ||
|
|
||
| fun interface TracerProviderCustomizer { | ||
| fun customize(builder: SdkTracerProviderBuilder): SdkTracerProviderBuilder | ||
|
Member
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?
Contributor
Author
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....
Member
There was a problem hiding this comment. I think the benefits outweigh the downside here tbh. |
||
| } | ||
|
|
||
| fun interface LoggerProviderCustomizer { | ||
| fun customize(builder: SdkLoggerProviderBuilder): SdkLoggerProviderBuilder | ||
| } | ||
|
|
||
| fun interface MeterProviderCustomizer { | ||
| fun customize(builder: SdkMeterProviderBuilder): SdkMeterProviderBuilder | ||
| } | ||
|
Contributor
Author
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?
Contributor
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.
Member
There was a problem hiding this comment.
Contributor
Author
There was a problem hiding this comment.
Yes! Thank you. I think the
Contributor
Author
There was a problem hiding this comment.
Good question. The ones that I've heard, and what was discussed in #1577, is the ability to add custom 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.
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).
Contributor
There was a problem hiding this comment. Got it, thanks for the details.
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?
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.
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. |
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,100 @@ | ||
| /* | ||
| * Copyright The OpenTelemetry Authors | ||
| * SPDX-License-Identifier: Apache-2.0 | ||
| */ | ||
|
|
||
| package io.opentelemetry.android.agent.dsl | ||
|
|
||
| import androidx.test.ext.junit.runners.AndroidJUnit4 | ||
| import io.opentelemetry.android.agent.OpenTelemetryRumInitializer | ||
| import io.opentelemetry.sdk.trace.IdGenerator | ||
| import io.opentelemetry.sdk.trace.SdkTracerProvider | ||
| import org.assertj.core.api.Assertions.assertThat | ||
| import org.junit.Test | ||
| import org.junit.runner.RunWith | ||
| import org.robolectric.RuntimeEnvironment | ||
| import java.util.concurrent.atomic.AtomicBoolean | ||
|
|
||
| @RunWith(AndroidJUnit4::class) | ||
| class OtelSdkCustomizationsSpecTest { | ||
| @Test | ||
| fun `can configure tracer provider`() { | ||
| val traceId = "b9a654847cb3514b6e5bd80cb168ed1c" | ||
| val spanId = "0666666666666666" | ||
| val idGen = | ||
| object : IdGenerator { | ||
| override fun generateSpanId(): String? = spanId | ||
|
|
||
| override fun generateTraceId(): String? = traceId | ||
| } | ||
| val agent = | ||
| OpenTelemetryRumInitializer.initialize( | ||
| context = RuntimeEnvironment.getApplication(), | ||
| configuration = { | ||
| otelSdkCustomizations { | ||
| customizeTracerProvider { _ -> | ||
| SdkTracerProvider.builder().setIdGenerator(idGen) | ||
| } | ||
| } | ||
| }, | ||
| ) | ||
| val tracer = | ||
| agent.openTelemetry.tracerProvider | ||
| .tracerBuilder("test") | ||
| .build() | ||
| val span = tracer.spanBuilder("test").startSpan() | ||
| assertThat(span.spanContext.spanId).isEqualTo(spanId) | ||
| } | ||
|
|
||
| @Test | ||
| fun `can configure logger provider`() { | ||
| val seen = AtomicBoolean(false) | ||
| val agent = | ||
| OpenTelemetryRumInitializer.initialize( | ||
| context = RuntimeEnvironment.getApplication(), | ||
| configuration = { | ||
| otelSdkCustomizations { | ||
| customizeLoggerProvider { builder -> | ||
| builder.addLogRecordProcessor { _, _ -> | ||
| run { | ||
| seen.set(true) | ||
| } | ||
| } | ||
| } | ||
| } | ||
| }, | ||
| ) | ||
| val logger = | ||
| agent.openTelemetry.logsBridge | ||
| .loggerBuilder("test") | ||
| .build() | ||
| logger.logRecordBuilder().setBody("howdy").emit() | ||
| assertThat(seen.get()).isTrue | ||
| } | ||
|
|
||
| @Test | ||
| fun `can configure meter provider`() { | ||
| val seen = AtomicBoolean(false) | ||
| val agent = | ||
| OpenTelemetryRumInitializer.initialize( | ||
| context = RuntimeEnvironment.getApplication(), | ||
| configuration = { | ||
| otelSdkCustomizations { | ||
| customizeMeterProvider { builder -> | ||
| run { | ||
| seen.set(true) | ||
| builder | ||
| } | ||
| } | ||
| } | ||
| }, | ||
| ) | ||
| val counter = | ||
| agent.openTelemetry.meterProvider | ||
| .get("test") | ||
| .counterBuilder("test") | ||
| .build() | ||
| counter.add(1) | ||
| assertThat(seen.get()).isTrue | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.