Samplers never receive the parent's tracestate, and the composite sampler erases it - #5579
Open
dwin-gharibi wants to merge 2 commits into
Open
Conversation
Sampler.should_sample declares a trace_state parameter and the consistent probability sampling design reads the parent threshold out of it. Assert that a sampler driven through Tracer.start_span actually receives it, that ParentBased forwards it to its delegate, and that a root span correctly gets None. Also assert the composite sampler preserves vendor tracestate entries rather than replacing the whole tracestate, and that SamplingIntent.update_trace_state is applied to root spans as well as children. These tests fail against the current implementation.
Tracer.start_span never passed the trace_state argument, and ParentBased accepted it without forwarding it to its delegate, so no sampler ever received a tracestate despite the Sampler ABC declaring the parameter. This left the consistent probability sampling design inert: _ComposableParentThreshold reads the parent threshold out of tracestate and always saw None, falling back to the sampled flag with threshold_reliable false. Worse, _CompositeSampler rebuilds the outgoing tracestate from that parameter, so given None it emitted a fresh tracestate containing only `ot` and discarded every vendor entry from the incoming request. Pass it from start_span, guarding for root spans, and forward it through ParentBased. Also apply SamplingIntent.update_trace_state before the emptiness check in _update_trace_state, so a composable sampler that stamps tracestate is no longer skipped for root spans.
Pull request dashboard statusWaiting on reviewers · refreshed 2026-08-23 17:14 UTC Review the latest changes. Status above doesn't look right?
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #5578.
Description
Two independent omissions on the same path.
Tracer.start_spancallsshould_sample(context, trace_id, name, kind, attributes, links)and never passes the seventh parameter,trace_state. Separately,ParentBased.should_sampleacceptstrace_statebut does not forward it to its delegate. Between them, no sampler ever receives a tracestate, even though theSamplerABC declares the parameter and the comment at the call site says the sampler may modify it.A third, related bug:
_update_trace_statereturns early when the incoming tracestate is empty, soSamplingIntent.update_trace_stateis silently skipped for every root span.Root cause
opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py:1171- the argument is simply not passed.opentelemetry-sdk/src/opentelemetry/sdk/trace/sampling.py:356-363-ParentBased.should_samplebuilds the delegate call withouttrace_state.opentelemetry-sdk/src/opentelemetry/sdk/trace/_sampling_experimental/_sampler.py:71-_update_trace_statereturns beforeintent.update_trace_stateis ever called when the incoming tracestate is falsy.Approach
Pass
parent_span_context.trace_statefromTracer.start_span, guarding for root spans where there is no parent; forwardtrace_statethroughParentBased; and in_update_trace_state, applyintent.update_trace_stateto an emptyTraceState()before the emptiness check rather than after it.All three are one-line changes at the point where the value was already available.
Files changed
opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.pyopentelemetry-sdk/src/opentelemetry/sdk/trace/sampling.pyopentelemetry-sdk/src/opentelemetry/sdk/trace/_sampling_experimental/_sampler.pyopentelemetry-sdk/tests/trace/test_sampler_tracestate.py.changelog/5567.fixedTesting
A spy sampler records what it is handed when driven through a real
TracerProviderwith a remote parent, covering both the bare sampler and theParentBasedcomposition. A third test asserts a root span still receivesNone, so "always pass something" cannot be mistaken for the fix.End-to-end coverage extracts a remote context carrying
vendora=alpha,ot=th:8, starts a child through the composite sampler and re-injects it, asserting the vendor entry survives. The default sampler is exercised as a control, since it already behaved correctly - that contrast is what makes the composite sampler's behaviour identifiable as a bug rather than a propagation problem.Two further tests cover
SamplingIntent.update_trace_statefor both the absent and present incoming-tracestate cases.Four of the seven fail before the change.
Result: 862 passed in
opentelemetry-sdk(855 baseline plus 7 new).Risk / compatibility
Samplers now receive a parameter they always declared, so a custom sampler that ignores it is unaffected and one that reads it starts working.
SamplingResult.trace_statemay now be non-None for root spans where a composable sampler asked for it - which is the intended behaviour. No public signatures change.