-
Couldn't load subscription status.
- Fork 22
Add experimental composite samplers #777
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
|
I think for now we depend on knowing what are the upstream implementation choices for this, so I would suggest that we make this a To me there are a few questions that we need to answer before merging this:
|
|
Thanks @SylvainJuge - I think one general point is deciding whether we support experimenting on new features before upstream is clear on them in EDOT, and if so how we do it. I think it can be hard to provide feedback upstream without having a system in place for it, but at the same time it can cause some confusion for users so needs a general product decision I guess. @AlexanderWert for any thoughts.
There isn't any configuration being discussed yet - as far as I know, the desire is to transparently swap the existing implementations with the new ones. In reality, there may be pushback and eventually configuration options are provided but I expect this to take a few months to settle down.
I have a PR to implement it in SDK which will allow the upstream agent to adopt it, but given the idea of transparently swapping I don't think they would add it until that is decided. open-telemetry/opentelemetry-java#7626
While the spec suggests returning different instances for 0%, it's awkward for the user so currently during implementation we are leaning towards not taking that approach. But either way, it would be on the implementation side rather than configuration side to take care of it since it wouldn't be specific to env var instantantiation. open-telemetry/opentelemetry-js#5839 (comment) I'd lean towards not changing this PR to draft, I think we can decide on whether we should close it or not instead based on the approach to take
I think either of these is fine and just depends on the goals - except for SDK vs contrib timing decision, it would apply to all the languages, not just Java. |
|
We don't need all these samplers. We want specifically the ConsistentSampler.parentBased(ConsistentSampler.updateableProbabilityBased) from consistent56, which we'll make our default sampler. I don't see the need for any of the others until they've been moved into the core. The approach looks fine, can you adjust so that we just have those two? |
.../co/elastic/otel/compositesampling/CompositeParentBasedTraceIdRatioBasedSamplerProvider.java
Show resolved
Hide resolved
…nto contrib-compositesamplers
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.
Thanks, I tested it hooked up to the central config and it works fine. Thank you for the contribution and your patience!
This adds the contrib composite samplers to EDOT. There is interest from @AlexanderWert and others in being able to check sampling + representative count behavior end-to-end with EDOT, so sending this PR. One issue is there aren't any official env vars for it yet, so I prefaced with
experimental_composite_to be able to use them initially. In the long term, the SDK will probably switch the samplers transparently to the new ones at which point they can be deprecated / removed.Let me know what you think of the approach - whatever we decide on we would want to apply to other languages in the same way as the sampler implementations become available.