Repository navigation
feat(sampling): OTel consistent-probability rv/th derivation (APMAPI-2181) - #2276
gh-worker-dd-mergequeue-cf854d[bot] merged 14 commits into
Conversation
🎉 All green!🧪 All tests passed 🎯 Code Coverage (details) 🔗 Commit SHA: fa97744 | Docs | Datadog PR Page | Give us feedback! |
BenchmarksComparisonBenchmark execution time: 2026-08-11 15:48:59 Comparing candidate commit fa97744 in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 48 metrics, 10 unstable metrics.
|
Artifact Size Benchmark Reportaarch64-alpine-linux-musl
aarch64-unknown-linux-gnu
libdatadog-x64-windows
libdatadog-x86-windows
x86_64-alpine-linux-musl
x86_64-unknown-linux-gnu
|
Compute th as round(2^56*(1-rate)) via 2^56 - round(2^56*rate) (exact, avoids the single-shot IEEE-754 rounding the RFC warns against), and return None from otel_consistent_sampling when a probability keep was overturned by the trace rate limiter (rl_effective_rate set).
Use exact integer arithmetic over the canonical 6-decimal rate (the form format_sampling_rate / dd= already use) so th reproduces the RFC imprecision-appendix's decimal table (0.2 -> cccccccccccccd, 0.99 -> 028f5c28f5c28f), which an f64 pipeline cannot represent.
…consistent_sampling
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cce3b2ac43
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 99ec5bae75
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
…consistent_sampling
…nsistent-sampling
99ec5ba to
984c9f7
Compare
bantonsson
left a comment
bantonsson
left a comment
There was a problem hiding this comment.
The PR description is messy and confusing. It says:
th = round(2^56 * (1 - rate_micros / 1_000_000)) (56-bit), on the rate rounded to 6 decimals so it
matches the RFC appendix's imprecision table rather than a float
(1.0 - rate) * 2^56
But the code does exactly (1.0 - rate) * 2^56, or am I missing something?
Also, the test code in DataDog/system-tests#7372 does truncation, ie int(...) but this code does .round().
Please clean up the description and change the code.
| fn derive_th(rate: f64) -> u64 { | ||
| const U56_MOD: u64 = 1u64 << 56; | ||
| const U56_MAX: u64 = U56_MOD - 1; | ||
| (((1.0 - rate) * U56_MOD as f64).round() as u64).clamp(0, U56_MAX) |
There was a problem hiding this comment.
Are you sure this is a good idea?
f64 have 53 bits of precision, and you are doing 56 bits arithmetic. So when you cast back to u64, if 1.0 - rate is bigger than 1/8th you will collapse the lower bits of your result together
There was a problem hiding this comment.
I think that the system tests also suffer from this issue. This is the code that the OTel spec for this points to as an example https://github.com/open-telemetry/opentelemetry-collector-contrib/blob/main/pkg/sampling/probability.go
There was a problem hiding this comment.
The spec we used as a basis (and also for system tests) is https://opentelemetry.io/docs/specs/otel/trace/tracestate-probability-sampling/
The code you linked also uses round and then the part about adding precision I'm not exactly sure to understand why we would need this.
About system-tests, although the comment says int(), the values are calculated with round() and the go tracer opted for round() as well https://github.com/DataDog/dd-trace-go/pull/5060/changes
There was a problem hiding this comment.
About doing 1.0 - rate, I understand it would be better to do the operations in 56 bit arithmetic (converting rate to a 56 bit int before), this is just a different implem from what we went for with the go tracer and system tests already @paullegranddc
There was a problem hiding this comment.
An alternative would be to do
fn derive_th(rate: f64) -> u64 {
const U56_MOD: u64 = 1u64 << 56;
const U56_MAX: u64 = U56_MOD - 1;
U56_MAX - ((U56_MOD as f64 * rate) as u64).clamp(0, U56_MAX)
}This would not pass the current system tests in their current form
WDYT ?
There was a problem hiding this comment.
The thing I'm most confused about is why the code and system tests don't do what the spec says:
OpenTelemetry SDKs are recommended to use 4 digits of precision by default.
Why do we compare the full hex representation and not only to 4 digits of precision?
Also, the comments in system tests must be very explicit (and correct) about how the values are computed and what precision is expected of the tracers.
There was a problem hiding this comment.
Also, the comments in system tests must be very explicit (and correct) about how the values are computed and what precision is expected of the tracers.
I'm changing the comment in system-tests: DataDog/system-tests#7482
This is only one recommendation we are not following but it is technically respecting the spec.
If it's needed to make small gains on the tracestate size one day, it will still be possible to implement it but since it's already implemented (and merged) this way in dd-trace-go, I prefer consistency
There was a problem hiding this comment.
I'm changing the comment in system-tests: DataDog/system-tests#7482
Thanks
This is only one recommendation we are not following but it is technically respecting the spec.
Sure, it's optional.
4a4cfad to
532f4fb
Compare
# Release proposal for libdd-sampling and its dependencies This PR contains version bumps based on public API changes and commits since last release. ## libdd-common **Next version:** `5.2.0` **Semver bump:** `minor` **Tag:** `libdd-common-v5.2.0` ### Commits - feat(remote-config)!: agentless RC fetcher (#2112) - feat(common): allocation-free tag validation and parsing (#2329) ## libdd-trace-utils **Next version:** `10.1.0` **Semver bump:** `minor` **Tag:** `libdd-trace-utils-v10.1.0` ### Commits - feat(data-pipeline): adds new attributes to OTLP trace metrics export (#2316) - feat: add v1 isolated bricks for trace exporter (#2280) ## libdd-sampling **Next version:** `6.0.0` **Semver bump:** `major` **Tag:** `libdd-sampling-v6.0.0` ###⚠️ major bump forced due to: - `libdd-trace-utils`: ^9.0.0 → ^10.1.0 ### Commits - feat(sampling): OTel consistent-probability rv/th derivation (APMAPI-2181) (#2276) - fix(sampling): record rate limiter's effective rate on allow, not just drop (#2288) - refactor: migrate to workspace dependencies, phase 4 (#2296) - chore: moving to workspace-level dependencies, phase 2 (#2270) - refactor: consolidate core dependencies at workspace level (phase 1) (#2253) --------- Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: hoolioh <107922352+hoolioh@users.noreply.github.com>
3 participants
Adds a public API to emit OpenTelemetry consistent-probability-sampling tracestate (
ot.th/ot.rv). https://opentelemetry.io/docs/specs/otel/trace/tracestate-probability-sampling/Part of APMAPI-2170 (OTel consistent probability sampling across dd-trace-*).
Changes
SamplingMechanism::is_probability()— classifies rate-driven mechanismsOtelConsistentSampling { rv, th }(threshold, and rv as source of randomness derived from the trace id)TraceRootSamplingInfo::otel_consistent_sampling(trace_id)returnsSome(OtelConsistentSampling)for a probability decisionTests
Tests from system-tests #7372
cargo test -p libdd-sampling