feat(sampling): Support for OpenTelemetry consistent tracestate sampling - #12397
MilanGarnier wants to merge 21 commits into
Conversation
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
This comment was marked as resolved.
This comment was marked as resolved.
db321c0 to
1c02400
Compare
3c81345 to
629892d
Compare
d5a6721 to
eb4c156
Compare
34ef4a2 to
d324e16
Compare
There was a problem hiding this comment.
The change can keep an inherited OpenTelemetry threshold that conflicts with a new local sampling decision. It also sends threshold values with extra trailing zeroes.
🤖 Datadog Autotest · Commit a51831b · What is Autotest? · @DataDog review to ask questions · Any feedback? Reach out in #autotest
duplicate dd and probably more things + regression test
| int samplingPriority, | ||
| CharSequence rate, | ||
| double sampleRate, | ||
| boolean probabilitySamplingResult, |
There was a problem hiding this comment.
Adding this parameter is the cause of much of the changes in this PR.
I would have thought you'd have this information already by combining samplingPriority and samplingMechanism - can you list the situations which aren't covered by the existing parameters?
There was a problem hiding this comment.
This is a design decision that was already questioned in other implementations and I don't really know if there is a nicer solution to it... Essentially the issue comes from the rate limiter.
The two cases that are indistinguishable are:
- sampling rule rejects
- sampling rule accepts but limiter demotes (from a locally configured rate)
Both have a priority USER_DROP with mechanism LOCAL_USER_RULE
However, the first case falls in the probabilistic sampling category and therefore needs ot=... being emitted, whereas the second one isn't and requires not emitting a threshold for it
There was a problem hiding this comment.
TBH I would consider whether internally we could encode the fact that the original mechanism has been overridden by the limiter. For example by setting the upper-bit of the mechanism parameter*. We could make sure to filter that bit out before encoding, but it would allow us to pass additional context internally that two mechanisms were involved without the internal codebase churn.
Especially since it wasn't immediately obvious to me what probabilitySamplingResult was meant to convey.
(*) this is just one idea - other encodings are possible, and we'd still have to treat a mechanism of -1 specially and not just clear its top bit :)
There was a problem hiding this comment.
I'm not sure I see the advantage of encoding things in the bits of the sampling mechanism. It moves the churn to its handling everywhere in the tracer.
I have some encodings in mind, but they introduce churn elsewhere, and another requirement is that it needs to be unambiguous
There was a problem hiding this comment.
Effectively the information you're trying to pass in is the original mechanism plus the fact that original mechanism was overridden/supplanted by a different mechanism (rate limiter)
The mechanism parameter is an int with enough space to capture both of those pieces of information. It saves the overhead of maintaining the extra parameter.
There was a problem hiding this comment.
I wonder if this could be achieved by only changing the internals of SamplingState, inferring it in its method calls. The sampling mechanism and the otel tracestate did not live together in every previous implrmentation, but since I already had to put them together here, this may remove the need of carrying the samplingResult altogether. I'll have a deeper look at this when I'm back on Tuesday
There was a problem hiding this comment.
Thanks, that would be appreciated - I'm not saying we can't change this internal API, but if we could avoid it then that might help simplify the overall PR.
There was a problem hiding this comment.
I've looked into it and it doesn't work as well as I thought by only changing the internals of SamplingState, so I went for your approach : af7e307
This adds a RATE_LIMITER_REJECTED flag (keeping the sign of the value) for the rate limiter path.
The thing I don't like about it is that it can very easily be misused (for example doing a switch case on the value without thinking about the flag possibly being present), but here the number of places where the flag is used is very limited.
There was a problem hiding this comment.
Is it just in RuleBasedTraceSampler where we need to make the distinction?
mcculls
left a comment
There was a problem hiding this comment.
The addition of a new parameter to setSamplingPriority has a large a ripple effect across the codebase - on the face of it, this new parameter looks derivable from existing sampling parameters.
Can you look at whether you really need to add this new parameter?
@mcculls I've answered in this comment Essentially this is the same (valid though, I wish it was simpler) question than Zach asked in DataDog/dd-trace-dotnet#8983 (comment) . In the final implementation it ended up changing the constructor for sampling decisions... |
c36a915 to
88f374c
Compare
Code review (Claude)Stale/unreconciled OTel In The inherited Concretely: a trace extracted from a pure-OTel upstream (tracestate has 🤖 Generated with Claude Code |
Code review (Claude)Divergent
For Caveat: currently latent. I checked every call site of 🤖 Generated with Claude Code |
|
Note: this comment is from Claude (AI-assisted review), not a human reviewer. Finding:
|
|
Note: this comment is from Claude (AI-assisted review), not a human reviewer. Suggested fix for the
|
… default method + delegation to minimize impact
What Does This Do
Implements OpenTelemetry consistent probability sampling with the
ottracestate member.
rvandthfor agent-rateand rule-based sampling (and forwards them is they were extracted and valid).
probability state.
otfields, rebuilding W3C tracestatewithin its member and size limits while preserving unmanaged vendor members.
together so W3C injection and OTLP JSON/protobuf export use one decision.
Adds coverage for parsing and normalization, W3C extraction and
reinjection, sampling decisions and overrides, Org Guard, and OTLP export.
Motivation
OpenTelemetry consistent probability sampling lets tracers make compatible
sampling decisions using a shared random value and threshold. The Java tracer
needs to retain compatible inbound state and publish locally made probability
decisions without conflicting with Datadog sampling behavior.
Additional Notes
ottracestate parser added in refactor(propagation): Parse OpenTelemetry tracestate member #12405, which has merged.DataDog/system-tests#7649.
Contributor Checklist
type:andcomp:labels.Jira ticket: APMAPI-2171