feat(sampling): Support for OpenTelemetry consistent tracestate sampling - #12397
feat(sampling): Support for OpenTelemetry consistent tracestate sampling#12397MilanGarnier wants to merge 15 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".
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".
| if (setSamplingPriorityWithRate(samplingPriority, rate, sampleRate, samplingMechanism) | ||
| && probabilitySamplingResult != null) { | ||
| context | ||
| .getPropagationTags() | ||
| .updateOtelTraceState( |
There was a problem hiding this comment.
When sibling spans inject concurrently on a trace whose priority is initially unset, setSamplingPriorityWithRate publishes the non-UNSET priority before this separate OTel-state update. Another injector can therefore skip sampling and serialize a W3C header without rv/th; if it finishes caching that header after this thread clears the cache, the stale header can remain cached for every later injection on the trace. Update the priority and probability state coherently, or prevent header construction from observing and caching this intermediate state.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
I'm unsure whether this is a real issue or not, and if it is real, what is the best way to mitigate it
There was a problem hiding this comment.
Confirmed this is real, not just theoretical — walking through the exact sequence:
DDSpan.setSamplingPriority(...)callscontext.setSamplingPriority(...)first, publishing the new non-UNSETpriority. Only afterward does it callupdateOtelTraceState(...)to updateotelTraceState/otelSamplingDecision.- In the window between those two steps, a sibling span's injector can observe the now-published priority and take the
headerCachemiss path inPTagsFactory.PTags.headerValue(HeaderType)(PTagsFactory.java~445-464). It computes a header viaresolveOtelTraceState(headerType, samplingPriority)using the new priority but the still-staleotelTraceState/otelSamplingDecision— producing a header withoutrv/th— and caches it viasetCachedHeader(plain field write, no version check). - If that cache write lands after step 1's later call to
clearCachedHeader(W3C)(triggered bysetOtelTraceState/setOtelSamplingDecisiononce the real update happens), the incomplete header is now permanently cached — nothing clears it again — and every subsequent injection on the trace serializes a W3C header missing the OTel probability-sampling fields.
So this needs to be fixed, not just accepted as a known edge case. The cleanest mitigation is probably to make the priority publish and the OTel-state update atomic from the perspective of headerValue() — e.g. compute/set both under the same guard that setSamplingPriority uses, so no reader can observe the new priority before the OTel state that goes with it.
![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
eb4c156 to
7716a38
Compare
7716a38 to
3a2ad40
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.
| } else if (hasKey(raw, start, end, separator, 't', 'h')) { | ||
| boolean validThresholdLength = fieldValueStart < end && end - fieldValueStart <= HEX_DIGITS; | ||
| long parsedThreshold = | ||
| validThresholdLength ? parseLowercaseHex(raw, fieldValueStart, end) : NO_VALUE; |
There was a problem hiding this comment.
th omits trailing zeros, so this needs to be right-padded to 14 digits before conversion. For example, th:8 represents 0x80000000000000, but this currently parses it as numeric 8.
| } | ||
|
|
||
| @Override | ||
| public String getW3CTracestate(int samplingPriority) { |
There was a problem hiding this comment.
OtelTraceState.parse() may normalize the ot member while tracestate remains raw. Is returning the raw value here intentional? It preserves the previous OTLP behavior, but means invalid nested rv/th fields are still exported whenever resolution leaves the state unchanged.
There was a problem hiding this comment.
I was not intending to put the OTLP trace export in this PR but rather later in the stack
|
I got more ideas today on how to have a cleaner design so I'll refactor this before re-asking for review |
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