Skip to content

fix: ignore deferred spans when delegating sampling decision #3209

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
+38 −6
Open

fix: ignore deferred spans when delegating sampling decision #3209

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
cb7930d
fix: ignore deferred spans when delegating sampling decision
lbeschastny Oct 23, 2025
cf46010
Merge branch 'main' into fix/psrent-based-sampler-with-deferred-trace
lalitb Oct 28, 2025
642d203
Merge branch 'main' into fix/psrent-based-sampler-with-deferred-trace
lbeschastny Nov 6, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions opentelemetry-sdk/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@

## vNext

- **Fix**: `Sampler::ParentBased` now correctly delegates sampling decisions to the `delegate_sampler` when no sampling information was extracted by the context propagator. Previously `SamplingDecision::Drop` was returned in such cases. ([#3209](https://github.com/open-telemetry/opentelemetry-rust/pull/3209)).

## 0.31.0

Released 2025-Sep-25
Expand Down
42 changes: 36 additions & 6 deletions opentelemetry-sdk/src/trace/sampler.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ use opentelemetry::{
trace::{
Link, SamplingDecision, SamplingResult, SpanKind, TraceContextExt, TraceId, TraceState,
},
Context, KeyValue,
Context, KeyValue, TraceFlags,
};

#[cfg(feature = "jaeger_remote_sampler")]
Expand All @@ -13,6 +13,8 @@ pub use jaeger_remote::{JaegerRemoteSampler, JaegerRemoteSamplerBuilder};
#[cfg(feature = "jaeger_remote_sampler")]
use opentelemetry_http::HttpClient;

const TRACE_FLAG_DEFERRED: TraceFlags = TraceFlags::new(0x02);
Copy link
Member

@cijothomas cijothomas Oct 31, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://w3c.github.io/trace-context/#trace-flags
I don't see such a flag definition in the official spec...

Copy link
Author

@lbeschastny lbeschastny Nov 1, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because it's not an official flag. It's a hack used across opentelemetry-rust project to mark spans which lack sampling information to distinguish between unsampled and not-yet-sampled ones.

Here are some examples:

opentelemetry-rust-contrib propagators are using the same approach as well, i.e.:

Copy link
Author

@lbeschastny lbeschastny Nov 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would've been better to provide a way to apply sampling inside a propagator when extracting tracing context.
But that's a separate thing that would require a major release.

Copy link
Author

@lbeschastny lbeschastny Nov 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be best to merge my fix now and then see how this TRACE_FLAG_DEFERRED solution could be refactored for the next major release

Copy link
Contributor

@bantonsson bantonsson Nov 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's unfortunate that the B3 code is using TraceFlags to send over this information between Extractor and Injector. If your application is set up to accept B3 headers and inject both B3 and w3c trace context headers, you will end up with illegal trace flags in the w3c headers. Also, the sampling decision made here will not propagate correctly to the outgoing B3 headers since they look at the deferred flag first, and skip the sampling decision.

To support this use case, I think it would be better to define a separate struct to hold this information (and not B3 specific) in the API or SDK that is added as to the Context and then read by the Sampler. The B3 code would need to be updated as well.

Copy link
Author

@lbeschastny lbeschastny Nov 6, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I testes this patch and it works correctly including context propagation.

Propagation works correctly because existing propagators clear all flags except for TraceFlags::SAMPLED right now, i.e.:
https://github.com/open-telemetry/opentelemetry-rust/blob/main/opentelemetry-sdk/src/propagation/trace_context.rs#L136

I agree that going forward it would be best to store this information separately instead of using trace flags.

Copy link
Author

@lbeschastny lbeschastny Nov 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And it's not only B3. I actually encountered this issue when using xray propagator from a third party opentelemetry_aws crate. It looks like they copy-pasted the hack from zipkin propagator.

Copy link
Author

@lbeschastny lbeschastny Nov 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Incoming flags are also cleared of everything that is not TraceFlags::SAMPLED:
https://github.com/open-telemetry/opentelemetry-rust/blob/main/opentelemetry-sdk/src/propagation/trace_context.rs#L101-L103

So, TRACE_FLAG_DEFERRED can only exist internally and can never leak out. At least not with the current solution.

Copy link
Author

@lbeschastny lbeschastny Nov 6, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, the sampling decision made here will not propagate correctly to the outgoing B3 headers since they look at the deferred flag first, and skip the sampling decision.

Hm... you may be right. I should check that. So far I only verified that w3c headers are generated correctly.

Copy link
Author

@lbeschastny lbeschastny Nov 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, x-b3-sampled is not set correctly


/// The [`ShouldSample`] interface allows implementations to provide samplers
/// which will return a sampling [`SamplingResult`] based on information that
/// is typically available just before the [`Span`] was created.
Expand Down Expand Up @@ -180,6 +182,12 @@ impl ShouldSample for Sampler {
// The parent decision if sampled; otherwise the decision of delegate_sampler
Sampler::ParentBased(delegate_sampler) => parent_context
.filter(|cx| cx.has_active_span())
.map(|cx| cx.span())
.filter(|span| {
let trace_flags = span.span_context().trace_flags();
let is_deferred = trace_flags & TRACE_FLAG_DEFERRED == TRACE_FLAG_DEFERRED;
!is_deferred || trace_flags.is_sampled()
})
.map_or_else(
|| {
delegate_sampler
Expand All @@ -193,10 +201,8 @@ impl ShouldSample for Sampler {
)
.decision
},
|ctx| {
let span = ctx.span();
let parent_span_context = span.span_context();
if parent_span_context.is_sampled() {
|span| {
if span.span_context().is_sampled() {
SamplingDecision::RecordAndSample
} else {
SamplingDecision::Drop
Expand Down Expand Up @@ -249,7 +255,7 @@ pub(crate) fn sample_based_on_probability(prob: &f64, trace_id: TraceId) -> Samp
mod tests {
use super::*;
use crate::testing::trace::TestSpan;
use opentelemetry::trace::{SpanContext, SpanId, TraceFlags};
use opentelemetry::trace::{SpanContext, SpanId};
use rand::random;

#[rustfmt::skip]
Expand Down Expand Up @@ -419,6 +425,30 @@ mod tests {
))),
SamplingDecision::RecordAndSample,
),
(
"should ignore deferred spans",
Sampler::AlwaysOn,
Context::current_with_span(TestSpan(SpanContext::new(
TraceId::from(1),
SpanId::from(1),
TRACE_FLAG_DEFERRED, // deferred
false,
TraceState::default(),
))),
SamplingDecision::RecordAndSample,
),
(
"should prioritize sampled flag over deferred",
Sampler::AlwaysOff,
Context::current_with_span(TestSpan(SpanContext::new(
TraceId::from(1),
SpanId::from(1),
TraceFlags::SAMPLED | TRACE_FLAG_DEFERRED, // sampled and deferred (invalid state)
false,
TraceState::default(),
))),
SamplingDecision::RecordAndSample,
),
];

for (name, delegate, parent_cx, expected) in test_cases {
Expand Down