Skip to content

[samplers/probability] Implements a traceIdRatio sampler#8714

Closed
yuanyuanzhao3 wants to merge 9 commits into
open-telemetry:mainfrom
yuanyuanzhao3:traceidratio-sampler
Closed

[samplers/probability] Implements a traceIdRatio sampler#8714
yuanyuanzhao3 wants to merge 9 commits into
open-telemetry:mainfrom
yuanyuanzhao3:traceidratio-sampler

Conversation

@yuanyuanzhao3
Copy link
Copy Markdown

@yuanyuanzhao3 yuanyuanzhao3 commented Mar 20, 2026

Description

Implements a TraceIdRatioBased sampler in go.opentelemetry.io/contrib/samplers/probability/traceidratio that conforms to the OpenTelemetry specification's threshold-based sampling algorithm.

Features

  • Threshold-based sampling: Uses the least significant 56 bits of the trace ID (per W3C Trace Context Level 2 Random Trace ID Flag) for deterministic sampling decisions
  • Tracestate th handling: Encodes and propagates the sampling threshold in the W3C ot tracestate vendor key for consistent downstream sampling
  • Random bit support: Integrates with TraceFlags.IsRandom() and WithRandom() (opentelemetry-go#8012) for proper indication of th value for extrapolated metrics support

Dependencies

This module temporarily uses replace directives to pin to the opentelemetry-go commit that includes IsRandom/WithRandom (opentelemetry-go#8012, merge commit 2ffde5a42). These replaces should be removed once opentelemetry-go releases a version that includes those APIs.

Related

Co-Author

Joshua MacDonald jmacd@users.noreply.github.com

…s to the threshold based sampling algorithm.

Made-with: Cursor
@yuanyuanzhao3 yuanyuanzhao3 requested a review from a team as a code owner March 20, 2026 21:29
@codecov
Copy link
Copy Markdown

codecov Bot commented Mar 20, 2026

Codecov Report

❌ Patch coverage is 95.41284% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 82.2%. Comparing base (b24435f) to head (3cd6a7b).
⚠️ Report is 26 commits behind head on main.

Files with missing lines Patch % Lines
samplers/probability/traceidratio/sampler.go 94.6% 2 Missing and 1 partial ⚠️
samplers/probability/traceidratio/version.go 0.0% 2 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@          Coverage Diff           @@
##            main   #8714    +/-   ##
======================================
  Coverage   82.2%   82.2%            
======================================
  Files        183     186     +3     
  Lines      13812   13921   +109     
======================================
+ Hits       11354   11449    +95     
- Misses      2054    2068    +14     
  Partials     404     404            
Files with missing lines Coverage Δ
samplers/probability/traceidratio/tracestate.go 100.0% <100.0%> (ø)
samplers/probability/traceidratio/version.go 0.0% <0.0%> (ø)
samplers/probability/traceidratio/sampler.go 94.6% <94.6%> (ø)

... and 2 files with indirect coverage changes

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

…ndomness (explicit or from traceid with random tracestate flag) exists.
…h` key resulting in an empty otts.

This happens when `th` was the only subkey in `ot` in the incoming tracestate.
However, the incoming traceid or tracestate lacks randomness, so we end up erasing `th`.
@yuanyuanzhao3
Copy link
Copy Markdown
Author

@jmacd as FYI.

Copy link
Copy Markdown
Contributor

@jmacd jmacd left a comment

Choose a reason for hiding this comment

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

Nice! Thank you.

General comments:

  • I think this belongs in the core SDK (eventually), as a core part of the OTel tracing specification
  • With this PR it is definitely time to remove the older probability sampler.
    #6176

Comment thread samplers/probability/traceidratio/sampler.go
if fraction < probabilityZeroThreshold {
return sdktrace.NeverSample()
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@yuanyuanzhao3
Copy link
Copy Markdown
Author

Nice! Thank you.

General comments:

I think so as well. I'm under the impression that this is a temporary home while the sampler is being developed and battle-hardened. Eventually it'll graduate to the core SDK.

@dashpole who suggested this place:

It looks like the TraceIDRatioBased sampler is still development stability, right? In that case we will need to add the sampler in a v0.* module. It might need to live in contrib, similar to the minsev logging processor: https://github.com/open-telemetry/opentelemetry-go-contrib/tree/main/processors/minsev.

yuanyuanzhao3 and others added 3 commits March 27, 2026 11:55
Co-authored-by: Joshua MacDonald <jmacd@users.noreply.github.com>
…ailures in codespell and markdown lint checks

Made-with: Cursor
@dashpole
Copy link
Copy Markdown
Contributor

dashpole commented Mar 30, 2026

If this is going to be part of the sdk/trace module eventually, then we should probably put it in go.opentelemetry.io/otel/sdk/trace/x (new experimental trace module, following our x package pattern).

I would personally prefer to wait until we have a stable replacement to add deprecation warnings to the probability sampler. The deprecation warning isn't very actionable without a stable replacement.

@yuanyuanzhao3
Copy link
Copy Markdown
Author

yuanyuanzhao3 commented Mar 31, 2026

If this is going to be part of the sdk/trace module eventually, then we should probably put it in go.opentelemetry.io/otel/sdk/trace/x (new experimental trace module, following our x package pattern).

Are you suggesting to do this now or wait? Not sure what the "prefer to wait" comment is about.

I would personally prefer to wait until we have a stable replacement to add deprecation warnings to the probability sampler. The deprecation warning isn't very actionable without a stable replacement.

@dashpole
Copy link
Copy Markdown
Contributor

dashpole commented Mar 31, 2026

Are you suggesting to do this now or wait? Not sure what the "prefer to wait" comment is about.

I want to wait to add deprecation warnings to the old probability sampler. You can move this back to core in the new experimental module now.

Edit: I meant wait on the probability sampler deprecation. Not wait on the traceidratio sampler.

@yuanyuanzhao3
Copy link
Copy Markdown
Author

Are you suggesting to do this now or wait? Not sure what the "prefer to wait" comment is about.

I want to wait to add deprecation warnings to the old probability sampler. You can move this back to core in the new experimental module now.

Edit: I meant wait on the probability sampler deprecation. Not wait on the traceidratio sampler.

I can't find the new sdk/trace/x package. Is this under https://github.com/open-telemetry/opentelemetry-go/tree/main/sdk/trace?

@dashpole
Copy link
Copy Markdown
Contributor

We need to add a new module.

@yuanyuanzhao3
Copy link
Copy Markdown
Author

We need to add a new module.

ok. any established patterns for the structure under sdk/<...>/x? Can't find examples in the vicinity.

@dmathieu
Copy link
Copy Markdown
Member

dmathieu commented Apr 1, 2026

See https://github.com/open-telemetry/opentelemetry-go/tree/main/sdk/log/internal/x

@yuanyuanzhao3
Copy link
Copy Markdown
Author

FYI, I created the PR in the core SDK depot under sdk/trace/x: open-telemetry/opentelemetry-go#8123

@dashpole dashpole closed this Apr 2, 2026
@jmacd
Copy link
Copy Markdown
Contributor

jmacd commented Apr 10, 2026

Thanks @yuanyuanzhao3 and @dashpole. The convention sdk/trace/x makes sense.

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.

4 participants