Skip to content

Restore compliance between Composite Samplers code and specs#8450

Open
PeterF778 wants to merge 1 commit into
open-telemetry:mainfrom
PeterF778:Restore_compliance_between_Composite_Samplers_code_and_specs
Open

Restore compliance between Composite Samplers code and specs#8450
PeterF778 wants to merge 1 commit into
open-telemetry:mainfrom
PeterF778:Restore_compliance_between_Composite_Samplers_code_and_specs

Conversation

@PeterF778
Copy link
Copy Markdown
Contributor

The Composite Samplers based on Consistent Probability Sampling are described at https://github.com/open-telemetry/opentelemetry-specification/blob/v1.57.0/oteps/trace/0250-Composite_Samplers.md

@PeterF778 PeterF778 requested a review from a team as a code owner June 3, 2026 20:58
@codecov
Copy link
Copy Markdown

codecov Bot commented Jun 3, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 91.02%. Comparing base (79608eb) to head (b837e4c).

Additional details and impacted files
@@            Coverage Diff            @@
##               main    #8450   +/-   ##
=========================================
  Coverage     91.02%   91.02%           
  Complexity     7817     7817           
=========================================
  Files           893      893           
  Lines         23719    23719           
  Branches       2364     2364           
=========================================
  Hits          21590    21590           
  Misses         1408     1408           
  Partials        721      721           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Copy Markdown
Member

@zeitlinger zeitlinger left a comment

Choose a reason for hiding this comment

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

I agree with removing traceId from GetSamplingIntent and with the adjusted-count naming, but the OTEP still lists TraceId as a required Predicate argument. Is that part of the OTEP stale/inconsistent? If so, could you link the spec issue/PR or clarify the intended interpretation here?

@PeterF778
Copy link
Copy Markdown
Contributor Author

the OTEP still lists TraceId as a required Predicate argument. Is that part of the OTEP stale/inconsistent? If so, could you link the spec issue/PR or clarify the intended interpretation here?

Good catch! Yes, the OTEP (https://github.com/open-telemetry/opentelemetry-specification/blob/v1.57.0/oteps/trace/0250-Composite_Samplers.md#predicate) lists TraceId as a parameter, but this is a mistake. Predicates and GetSamplingIntent must not depend on trace id for the same reasons.
The prototype implementation (https://github.com/open-telemetry/opentelemetry-java-contrib/blob/main/consistent-sampling/src/main/java/io/opentelemetry/contrib/sampler/consistent56/Predicate.java) does not have this parameter.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants