Skip to content

Conversation

@anuraaga
Copy link
Contributor

This updates to the proposal for sampler behavior for EDOT.

/cc @trentm

@anuraaga anuraaga requested a review from jackshirazi October 17, 2025 05:19
@anuraaga anuraaga requested a review from a team as a code owner October 17, 2025 05:19
@jackshirazi
Copy link
Contributor

Central config handling needs to output a warning when sampling rate is changed and the sampler isn't the dynamic handling one

@anuraaga
Copy link
Contributor Author

Thanks @jackshirazi - since the SDK isn't exposed in a way for something like instanceof, I used a somewhat indirect way of recording it was initialized. Does it make sense?

@jackshirazi
Copy link
Contributor

That's too fragile, any touch from future maintenance to the class would result in the wrong state.

We have ConfigLoggingAgentListener which is triggered after the agent is fully configured. You can test there, eg add

if (autoConfiguredOpenTelemetrySdk
                .getOpenTelemetrySdk()
                .getSdkTracerProvider()
                .getSampler() instanceof ...)
something = true

@anuraaga
Copy link
Contributor Author

Ah good call, that's more clear. I went with checking in the existing ConfigLoggingAgentListener. Another idea is to have CentralConfig itself also be an AgentListener to keep it more composed but left it since it would be a bigger change.

public static final String LOG_THE_CONFIG =
"elastic.otel.java.experimental.configuration.logging.enabled";

public static volatile boolean enableDynamicSamplingRate = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

this should be private, with a public getter

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call, fixed

Copy link
Contributor

@jackshirazi jackshirazi left a comment

Choose a reason for hiding this comment

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

Thank you for this improvement!

@jackshirazi jackshirazi merged commit a0ec06b into elastic:main Oct 24, 2025
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants