Let the sampler vote on traces inherited from a traceparent#75
Merged
Conversation
Previously, when a trace was continued from a W3C traceparent (or an explicit traceId/spanId/sample tuple), the sampling decision was taken directly from the parent and the configured sampler was bypassed. That made DynamicSampler rules like forJob effectively dormant in practice, since jobs almost always run with an inherited traceparent. Trace continuity (the traceId and parent spanId) is now decoupled from the sampling decision. The sampler is always called, and receives the parent's sampling flag as a hint via a new optional parameter. - Sampler::shouldSample gains ?bool \$parentSampled = null - AlwaysSampler and NeverSampler ignore the hint - RateSampler honors the hint when set, otherwise rolls the rate - DynamicSampler evaluates rules first (a match overrides the parent), then falls back to the parent decision, then to base_rate - Tracer::startTrace drops the ?bool \$sample parameter; startFromTraceparent and startFromDefined are inlined Breaking change, acceptable pre-v3.
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
1 participant
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.
Summary
Tracer::startTraceno longer treats a W3Ctraceparent(or an explicittraceId/spanId/sampletuple) as the final word on sampling. Trace continuity (the traceId and parent spanId) is now decoupled from the sampling decision, and the configured sampler is always called.Sampler::shouldSample()gains an optional?bool $parentSampled = nullthat carries the parent's sampled flag (ornullwhen there is no parent).AlwaysSamplerandNeverSamplerignore the hint.RateSamplerhonors it when set.DynamicSamplerlets a matching rule win, falls back to the parent decision, then tobase_rate.?bool $sampleparameter onstartTraceis dropped.startFromTraceparentandstartFromDefinedwere inlined and removed.Why
Previously,
forJobrules inDynamicSamplerwere effectively dormant. Jobs almost always run with an inherited traceparent (written byQueueRecorder::createPayloadUsing), and that traceparent set the sampling decision directly without ever calling the sampler. With this change, rules can opt a job in or out regardless of the dispatching context's decision.Breaking changes
Sampler::shouldSample()signature change. Acceptable pre-v3. Custom samplers can ignore$parentSampledto preserve existing behavior.Tracer::startTrace()drops?bool $sample. The only legitimate user of this parameter was traceparent propagation, which is now handled internally.UPGRADING.mdis updated accordingly.Notes
A latent behavior question surfaced during this change:
JobRecorderrewrites the inherited traceparent tosampled=0when a job matches the ignore list. With rule-overrides-parent semantics, aforJobrule could re-sample a force-unsampled job. Whether the ignore list should be a hard veto or just a default is a separate decision worth following up on.