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

codecov Bot commented Jun 3, 2026

Copy link
Copy Markdown

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.

@zeitlinger zeitlinger left a comment

Copy link
Copy Markdown
Member

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.

@jack-berg

Copy link
Copy Markdown
Member

@PeterF778 this looks good but for the sampling SIG - OTEPs are just the original proposal but stop being the source of truth once they are codified in spec text. In this case, the source of truth is now: https://github.com/open-telemetry/opentelemetry-specification/blob/v1.57.0/specification/trace/sdk.md#getsamplingintent

Can someone from the sampling SIG go and update https://github.com/open-telemetry/opentelemetry-specification/blame/efceef79720efc238645666139a7cee1b0145df1/specification/trace/sdk.md#L661 to reflect your update here? Thanks.

* (reciprocal of sampling probability, used by Span-to-Metrics estimation) reliably, because a
* non-consistent-probability sampling decision might have affected the threshold value.
*
* @return true iff the threshold can be reliably used for adjusted count calculation

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
* @return true iff the threshold can be reliably used for adjusted count calculation
* @return true if the threshold can be reliably used for adjusted count calculation

@PeterF778

Copy link
Copy Markdown
Contributor Author

Can someone from the sampling SIG go and update https://github.com/open-telemetry/opentelemetry-specification/blame/efceef79720efc238645666139a7cee1b0145df1/specification/trace/sdk.md#L661 to reflect your update here? Thanks.

@jack-berg Does this change require an Issue?

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.

3 participants