Skip to content
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

Composite Head Samplers #4321

Merged
merged 30 commits into from
Mar 12, 2025
Merged

Conversation

PeterF778
Copy link
Contributor

@PeterF778 PeterF778 commented Dec 4, 2024

Changes

Migrating from open-telemetry/oteps#250.

This is another approach for introducing Composite (Head) Samplers. The previous one (open-telemetry/oteps#240) proved too large, with some controversial elements.
This OTEP is a split-off from that one, focusing just on one area - new Composite Samplers.

Two prototypes exist for this functionality. We are seeking a third prototype.

In the Java-contrib repo: https://github.com/open-telemetry/opentelemetry-java-contrib/blob/main/consistent-sampling/README.md

In @jmacd's https://github.com/jmacd/go-sampler/blob/main/README.md.

@carlosalberto carlosalberto added the OTEP OpenTelemetry Enhancement Proposal (OTEP) label Dec 5, 2024
Copy link

This PR was marked stale due to lack of activity. It will be closed in 7 days.

@github-actions github-actions bot added the Stale label Dec 12, 2024
@PeterF778 PeterF778 marked this pull request as draft December 14, 2024 01:14
@PeterF778
Copy link
Contributor Author

Added IsAdjustedCountReliable to SamplingIntent, as discussed by the Sampling SIG.

@github-actions github-actions bot removed the Stale label Dec 14, 2024
Copy link

This PR was marked stale due to lack of activity. It will be closed in 7 days.

Copy link

github-actions bot commented Jan 4, 2025

Closed as inactive. Feel free to reopen if this PR is still being worked on.

@jmacd jmacd requested a review from oertl January 23, 2025 16:06
@PeterF778 PeterF778 marked this pull request as ready for review January 23, 2025 16:21
@carlosalberto
Copy link
Contributor

Overall LGTM, just left a few questions. Also, is the plan that languages implement all of these samplers? There's no requirement anywhere but I'm curious on the expectation, e.g. what if at least consistent and consistentparentbased should be supported?

@PeterF778
Copy link
Contributor Author

Also, is the plan that languages implement all of these samplers? There's no requirement anywhere but I'm curious on the expectation, e.g. what if at least consistent and consistentparentbased should be supported?

Good question, this has not been discussed yet, but if we assume that the example is realistic, we probably need all of them. I think we should say that an OTel SDK SHOULD implement them all.

@carlosalberto
Copy link
Contributor

Forgot to mention all the sampling OTEPs are under oteps/tracing, so maybe consider moving it there?

@carlosalberto
Copy link
Contributor

I think we can go ahead and merge this @PeterF778 . Probably only consider moving this OTEP under trace/?

@carlosalberto carlosalberto merged commit 30a21da into open-telemetry:main Mar 12, 2025
6 checks passed
carlosalberto added a commit that referenced this pull request Mar 14, 2025
@jmacd
Copy link
Contributor

jmacd commented Mar 18, 2025

FYI A new prototype has been written in OTel-Rust:
https://github.com/jmacd/rust-sampler

The portion of this OTEP that my two prototypes (Go and Rust) do not include, thus far, is a rate-limited sampler. I will suggest we (sampling SIG) prototype the rate-limited sampler as we work to propose updates to the spec to reflect this OTEP.

Thank you @PeterF778!

@arminru arminru mentioned this pull request Mar 18, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog.opentelemetry.io OTEP OpenTelemetry Enhancement Proposal (OTEP)
Projects
None yet
Development

Successfully merging this pull request may close these issues.