-
Notifications
You must be signed in to change notification settings - Fork 10
Wire up composite sampler #410
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
Conversation
src/elasticotel/distro/config.py
Outdated
| logger.debug("Cannot get sampler from tracer provider.") | ||
| return ConfigUpdate() | ||
|
|
||
| # FIXME: this needs to be updated for the consistent probability samplers |
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.
| # FIXME: this needs to be updated for the consistent probability samplers |
| os.environ.setdefault(OTEL_TRACES_SAMPLER, "parentbased_traceidratio") | ||
| os.environ.setdefault(OTEL_TRACES_SAMPLER_ARG, str(DEFAULT_SAMPLING_RATE)) |
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.
We should also remove these from the documentation. Need to check priority of OTEL_TRACES_SAMPLER env var against the configure parameter. An option is to provide our own entry points and just switch to it there?
I consider EDOT to provide an opinionated agent. If customers need better configurability, let them feed that back rather than we try an anticipate needs that might not be requested |
anuraaga
left a comment
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.
@xrmx based on the test logging, it looks like there was already an issue with loading entry points, not just my new one. I guess it's a test infrastructure issue, can you help with it? Note that I run locally using uv run pytest and it works fine, so maybe pip specific.
WARNING opentelemetry.sdk._configuration:__init__.py:390 Using default sampler. Failed to initialize sampler, experimental_composite_parentbased_traceidratio: Requested component 'experimental_composite_parentbased_traceidratio' not found in entry point 'opentelemetry_traces_sampler'
ERROR opentelemetry.sdk.resources:__init__.py:224 Failed to load resource detector 'service_instance', skipping
Traceback (most recent call last):
File "/opt/hostedtoolcache/Python/3.11.13/x64/lib/python3.11/site-packages/opentelemetry/sdk/resources/__init__.py", line 214, in create
next(
StopIteration
ERROR opentelemetry.sdk.resources:__init__.py:224 Failed to load resource detector '_gcp', skipping
Traceback (most recent call last):
File "/opt/hostedtoolcache/Python/3.11.13/x64/lib/python3.11/site-packages/opentelemetry/sdk/resources/__init__.py", line 214, in create
next(
StopIteration
ERROR opentelemetry.sdk.resources:__init__.py:224 Failed to load resource detector 'process_runtime', skipping
Traceback (most recent call last):
File "/opt/hostedtoolcache/Python/3.11.13/x64/lib/python3.11/site-packages/opentelemetry/sdk/resources/__init__.py", line 214, in create
next(
StopIteration
I think I've seen that upstream too when running sdk tests yesterday, I'll take a look. |
|
I think the issue is quite simple, if the package is not installed its entry points are not visible :) So to solve this we can mock the configurator code that loads the sampler entry points or introduce something like nox. noxfile.py: Install |
|
@anuraaga I'm going to setup nox so you can rebase on top |
|
Makes sense. uv also automatically installs the package before uv run so wiring it into CI could be an option too. |
|
Here's the PR for setting up nox: #415 |
…into composite-sampler
…ic-otel-python into composite-sampler
What does this pull request do?
This wires up the new composite sampler to EDOT.
I followed the same approach as EDOT Java which enables the sampler unconditionally I believe
https://github.com/elastic/elastic-otel-java/blob/1254b84e16c5cf6829f2b84bf63c902b71626448/custom/src/main/java/co/elastic/otel/ElasticAutoConfigurationCustomizerProvider.java#L95
Happy to change to registering a OTEL_TRACES_SAMPLER name instead, presumably the behavior should be consistent between the two though - I can update Java if we change it.
/cc @xrmx @AlexanderWert @jackshirazi