feat(sampling): OTel consistent-probability rv/th derivation (APMAPI-2181) - #2276
feat(sampling): OTel consistent-probability rv/th derivation (APMAPI-2181)#2276MilanGarnier wants to merge 13 commits into
Conversation
📚 Documentation Check Results📦
|
🔒 Cargo Deny Results📦
|
|
BenchmarksComparisonBenchmark execution time: 2026-08-06 15:36:41 Comparing candidate commit 532f4fb in PR branch Found 6 performance improvements and 1 performance regressions! Performance is the same for 41 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.
4a4cfad to
532f4fb
Compare
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 }+TraceRootSamplingInfo::otel_consistent_sampling(trace_id)returnsSome(OtelConsistentSampling)for a probability decisionTests
Tests from system-tests #7372
cargo test -p libdd-sampling