feat(sampling): Support for OpenTelemetry consistent tracestate sampling - #12397
MilanGarnier wants to merge 9 commits into
Conversation
MilanGarnier
added
type: feature
MilanGarnier
changed the title
This comment has been minimized.
This comment has been minimized.
🟢 Java Benchmark SLOs — All performance SLOs passed
PR vs. master results
Commit: Load and DaCapo benchmarks can be triggered manually in the GitLab pipeline. Results will appear in the Benchmarking Platform UI after completion. |
3643548 to
2f90ba6
Compare
a168627 to
9e2422a
Compare
7a1ddea to
a0176c9
Compare
|
@codex review |
![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: a0176c99c0
ℹ️ 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".
db321c0 to
1c02400
Compare
3c81345 to
629892d
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 629892d492
ℹ️ 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".
![datadog-datadog-prod-us1-2[bot]](https://avatars.githubusercontent.com/u/365230?s=60&v=4){kind=small}
There was a problem hiding this comment.
Compound extraction can emit a W3C sampled flag that conflicts with the propagated OTel probability threshold when propagation styles have different sampling decisions.
🤖 Datadog Autotest · Commit 3c81345 · What is Autotest? · @DataDog review to ask questions · Any feedback? Reach out in #autotest
d5a6721 to
eb4c156
Compare
3a2ad40 to
7772c8c
Compare
| boolean sampled = sampler.sample(span); | ||
| int samplingPriority = sampled ? PrioritySampling.SAMPLER_KEEP : PrioritySampling.SAMPLER_DROP; | ||
|
|
||
| Boolean probabilitySamplingResult = rates.hasAgentRates() ? sampled : null; |
There was a problem hiding this comment.
probabilitySamplingResult is gated on rates.hasAgentRates() — a flag for the whole RateSamplersByEnvAndService snapshot — rather than whether this span's sampling decision actually came from an agent-provided rate.
hasAgentRates is set true as soon as any entry in the agent's rate update is non-null (line ~121), for the entire snapshot across all env/service combinations. But rates.getSampler(env, serviceName) can still fall through to fallbackSampler (the hardcoded DEFAULT_SAMPLER, not agent-derived) for any env/service combination the agent didn't send a rate for. In that case sampled comes from the default sampler, yet probabilitySamplingResult is still computed as non-null (since hasAgentRates is true snapshot-wide) — so DDSpan.setSamplingPriority(..., probabilitySamplingResult) injects an OTel consistent-sampling probability decision (rv/th) into the span's tracestate that misrepresents an unrelated default-sampler decision as agent-rate-based probability sampling.
Compare with RuleBasedTraceSampler elsewhere in this PR, which correctly ties probabilitySamplingResult to the matched rule for that specific span rather than a class-wide flag. This one should probably check whether sampler (the one actually used for this span) came from an agent rate vs. the fallback, not whether the snapshot contains any agent rate at all.
| original.startsWith(DATADOG_MEMBER_KEY, memberStart) | ||
| || original.startsWith(OTEL_MEMBER_KEY, memberStart); | ||
| if (!managedMember) { | ||
| boolean datadogMember = original.startsWith(DATADOG_MEMBER_KEY, memberStart); |
There was a problem hiding this comment.
appendOtelAndVendorMembers, when called with preserveDatadogMember=true (the new OTLP-export path via getW3CTracestate(int)), copies every member matching original.startsWith(DATADOG_MEMBER_KEY, memberStart) verbatim into the output. Contrast with fromHeaderValue's extraction parser (line 73), which guards the same check with ddMemberIndex == -1 so only the first dd= member is ever recognized/kept.
If an inbound W3C tracestate somehow contains more than one dd= member (malformed, but stored verbatim as tracestate at extraction since nothing here validates uniqueness on the way in), this loop will re-emit all of them into the OTLP-exported traceState, producing a tracestate with duplicate list-member keys — invalid per the W3C tracestate spec (https://www.w3.org/TR/trace-context/#tracestate-header-field-values, "duplicated with the same key MUST be discarded"). Worth applying the same first-occurrence guard here that extraction already uses.
|
|
||
| String tracestate = propagationTags.getW3CTracestate(); | ||
| int samplingPriority = span.samplingPriority(); | ||
| // TODO Cache the effective tracestate once per trace. |
There was a problem hiding this comment.
getW3CTracestate(samplingPriority) bypasses headerCache entirely and unconditionally calls W3CPTagsCodec.updateOtelTraceState(this, resolved), which fully re-parses the original tracestate and rebuilds it with a fresh StringBuilder — even though OtelSamplingDecision.resolve() already caches the resolved decision per trace. For an N-span trace exported via OTLP (this call runs once per span, same TODO duplicated in OtlpTraceProto.java:95), that's O(N) redundant identical string rebuilds instead of O(1).
Given this is on the OTLP export hot path and scales with span count, this is worth fixing rather than leaving as a TODO — every tracer feature that adds a per-span cost here compounds for every other product exporting via OTLP. A per-trace cache keyed on the resolved OtelTraceState/OtelSamplingDecision (mirroring how headerCache already works for headerValue()) would turn this back into O(1) per trace.
|
I got more ideas today on how to have a cleaner design so I'll refactor this before re-asking for review |
7772c8c to
cd47a1a
Compare
cd47a1a to
e9d42f7
Compare
|
Since the PR is a bit big (~1100 lines of feature, 1000 of tests), I've rewritten the history into a set of commits that should each be independently reviewable and testable. |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e9d42f767a
ℹ️ 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".
fdb0a3a to
34ef4a2
Compare
Reuse the immutable empty sampling state across propagation tags. Cache effective W3C tracestate by immutable sampling state identity. Size W3C encoding buffers from the captured sampling state.
34ef4a2 to
d324e16
Compare
There was a problem hiding this comment.
A matching traceparent and Datadog priority can keep an OTel threshold that gives the opposite sampling decision. Reinjection then sends conflicting sampling data.
🤖 Datadog Autotest · Commit d324e16 · What is Autotest? · @DataDog review to ask questions · Any feedback? Reach out in #autotest
| SamplingState currentState = samplingState; | ||
| this.samplingState = | ||
| newSamplingState( | ||
| currentState.getSamplingPriority(), | ||
| tracestate, | ||
| otelTraceState, |
There was a problem hiding this comment.
A downstream OTel sampler can drop a trace that traceparent marks as sampled.
Assertion details
- Input: Use a sampled
traceparentwithtracestate: dd=s:1,ot=rv:00000000000000;th:8. - Expected:
Remove the threshold when the parsed OTel state conflicts with the final sampling priority. - Actual: Extraction keeps
th:8. Injection then sends a sampledtraceparentwith anotstate that means drop.
| SamplingState currentState = samplingState; | |
| this.samplingState = | |
| newSamplingState( | |
| currentState.getSamplingPriority(), | |
| tracestate, | |
| otelTraceState, | |
| SamplingState currentState = samplingState; | |
| int samplingPriority = currentState.getSamplingPriority(); | |
| if (otelTraceState != null | |
| && samplingPriority != PrioritySampling.UNSET | |
| && !otelTraceState.isConsistentWith(samplingPriority > 0)) { | |
| otelTraceState = otelTraceState.withoutThreshold(); | |
| } | |
| this.samplingState = | |
| newSamplingState( | |
| samplingPriority, | |
| tracestate, | |
| otelTraceState, |
Was this helpful? React 👍 or 👎
🤖 Datadog Autotest · What is Autotest? · @DataDog review to ask questions · Any feedback? Reach out in #autotest · Open Bits AI session
3 participants
What Does This Do
Adds OpenTelemetry consistent probability-sampling behavior on top of the
ottracestate parsing introduced by #12405.rvandth.Motivation
Make Java tracer sampling decisions interoperable with OpenTelemetry
consistent probability sampling and provide the state needed for downstream
tracestate propagation.
Additional Notes
depends on refactor(propagation): Parse OpenTelemetry tracestate member #12405 which introduced the OtelTracestate class.
system-tests will be enabled in test(java): enable ot.th/ot.rv tracestate sampling scenarios [java@milan.garnier/ot.th] system-tests#7649
OTLP export optimizations (which should already work with this PR, but it can be optimized) will follow in a separate change.
Contributor Checklist
type:andcomp:labels.Jira ticket: APMAPI-2171