fix: ignore deferred spans when delegating sampling decision

[lbeschastny]

Propagators could leave spans in a deferred state. For example, the following valid set of b3-multi headers does eactly that:

X-B3-TraceId: 68f8e1942794b9a8072bfc5f4f2c368f
X-B3-SpanId: a4295738dcb1aac4

but ShouldSample implementation for opentelemetry_sdk::trace::sampler::Sampler only checks that the parent span is active.

The correct behaviour would be to fall back to delegate sampler when the parent spans has deferred flag set by a propagator.

Changes

Sampler::ParentBased branch in the should_sample method now checks is the parent span has TRACE_FLAG_DEFERRED set and filters all deferred spans.

Merge requirement checklist

• CONTRIBUTING guidelines followed
• Unit tests added/updated (if applicable)
• Appropriate CHANGELOG.md files updated for non-trivial, user-facing changes
• Changes in public API reviewed (if applicable)

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: lbeschastny / name: Leonid Beschasny (642d203)

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 80.8%. Comparing base (cff5728) to head (642d203).

Additional details and impacted files

@@          Coverage Diff          @@
##            main   #3209   +/-   ##
=====================================
  Coverage   80.8%   80.8%           
=====================================
  Files        128     128           
  Lines      23288   23316   +28     
=====================================
+ Hits       18821   18849   +28     
  Misses      4467    4467           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[cijothomas]

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

[lbeschastny]

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_zipkin::propagator sets TRACE_FLAG_DEFERRED in several places:

  opentelemetry-rust/opentelemetry-zipkin/src/propagator/mod.rs

  Line 34 in b54fa84

  const TRACE_FLAG_DEFERRED: TraceFlags = TraceFlags::new(0x02);

• opentelemetry_jaeger_propagator::propagator checks it here:

  opentelemetry-rust/opentelemetry-jaeger-propagator/src/propagator.rs

  Line 149 in b54fa84

  if flag & 0x02 == 0x02 {

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

• opentelemetry_aws::trace::xray_propagator: https://github.com/open-telemetry/opentelemetry-rust-contrib/blob/ef1a3669459a9e3bdd1c2cec3d6593662ae7d584/opentelemetry-aws/src/trace/xray_propagator.rs#L61
• opentelemetry_datadog: https://github.com/open-telemetry/opentelemetry-rust-contrib/blob/ef1a3669459a9e3bdd1c2cec3d6593662ae7d584/opentelemetry-datadog/src/lib.rs#L172

[lbeschastny]

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.

[lbeschastny]

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

[bantonsson]

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.

[lbeschastny]

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.

[lbeschastny]

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.

[lbeschastny]

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.

[lbeschastny]

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.

[lbeschastny]

Yep, x-b3-sampled is not set correctly