[BUG] DEBUG-trace is not always reported

[tgeens]

Assumption: even using NEVER_SAMPLE, a debug trace should always be sampled.

From the B3-Propagation spec:

When Debug is set, the trace should be reported to the tracing system and also override any collection-tier sampling policy. Debug implies an accept sampling decision.

There is a bug under the following conditions:

• service tracer has sampler that is not ALWAYS_SAMPLE
• X-B3-Flags: 1
• X-B3-Sampled is not set
• X-B3-TraceId/SpanId/ParentSpanId is correctly set

Actual result: the X-B3-Flags: 1 trace is NOT sampled.
Expected result: the X-B3-Flags: 1 trace IS sampled.

According to the B3-Propagation spec, this set of propagation headers are expected in a downstream service:

Debug is encoded as X-B3-Flags: 1. Debug implies an accept decision: don't also send X-B3-Sampled: 1.

In the method B3Propagation#extract(C carrier) the parameters are extracted from the carrier and a TraceContextOrSamplingFlags is returned. This object is created with TraceContextOrSamplingFlags.create(result.sampled(sampledV).debug(debug).build()) which is a TraceContext with field flags = 8.

This results in traceContext.debug() returning true and traceContext.sampled() returning null. Strictly speaking, this is correct: the extractor only found a debug flag and not a sampling decision. Please note that this is a bit of an anomaly: for SamplingFlags.DEBUG both the fields sampled and debug are set to true.

But let's assume that a TraceContext with debug enabled, without a sampling decision set, is correct:

In Tracer#joinSpan(TraceContext context) the context is queried to find out a sampling decision has been made or not:

// If we are joining a trace, we are sharing IDs with the caller
// If the sampled flag was left unset, we need to make the decision here
if (context.sampled() == null) { // then the caller didn't contribute data
      context = context.toBuilder().sampled(sampler.isSampled(context.traceId())).build();
}

The sampler is asked to make a decision on wether this trace should be sampled or not. Note that .debug() is never checked. If the sampler makes the decision to not sample this trace, the new context has the flags: sample=false and debug=true.

At that point, Tracer#toSpan(TraceContext context) is called with the newly constructed context. The test Boolean.TRUE.equals(decorated.sampled()) returns false and a NoopSpan.create(decorated) is returned.

Result: A NoopSpan is returned and the DEBUG-trace is NOT sampled/recorded/reported.

So which part is wrong ?

1. The B3Propagation extractor by correctly extracting the debug flag, but not setting sampled=true ?
2. The Tracer asking the sampler for a sampling decision, without checking the context.debug() state first ?

I can provide a failing unit test for both root-causes, but probably only one is valid ?!

[tgeens]

@adriancole comment from openzipkin/openzipkin.github.io#31 (comment) says:

the sampled() method includes the debug flag in its decision

That is currently no longer the case and I think that would resolve this issue!
I will try to create a failing unit test that points this out!

[codefromthecrypt]

If I understand you correctly.. you have set `X-B3-Flags: 1` and not `X-B3-Sampled: 1` You are right that according to how below was written it would seem this should work https://github.com/openzipkin/b3-propagation However, that is more likely just an accident of how it is written. Are you unable to set `X-B3-Sampled: 1`? Is there any open source library that allows you to leave out `X-B3-Sampled: 1` when setting `X-B3-Flags: 1`? I don't mind changing this impl, just concerned you may end up needing to request this in every library..

[tgeens]

Is there any open source library that allows you to leave out X-B3-Sampled: 1 when setting X-B3-Flags: 1?

Yes, brave instrumentation does exactly that ? :-)

Both in the B3-Propagation spec & the implementation, the sampled flag is not injected in the carrier if debug is enabled.

See

brave/brave/src/main/java/brave/propagation/B3Propagation.java

Line 92 in 686379f

if (traceContext.debug()) {

[codefromthecrypt]

anyway I agree! let's get this fixed

[codefromthecrypt]

release 4.18.2 is on the way. Thank you so much!

[tgeens]

Thank you very much for fixing this so fast!

I had some feedback on the PR ... but okay ;-)