Repository navigation
Conversation
The B3 debug flag arrives either as the sampling state "d" in the single header or as x-b3-flags: 1 in the multi header form. Both were collapsed into the SAMPLED trace flag and the debug state itself was discarded, so nothing downstream could tell a debug trace from an ordinary sampled one. Keep the collapse into SAMPLED and additionally record the debug state under a context key, which the inject side can then read. The two forms are kept apart: a sampling state of "d" means debug only in the single header, since x-b3-sampled is not a carrier for it, even though this propagator has always accepted "d" there as a request to sample.
Inject only ever emitted a sampling decision of 1 or 0, so a debug trace arriving at a service left it as an ordinary sampled one and the debug state was lost for the rest of the trace. Read the debug state recorded by extract and re-emit it: the sampling state "d" in the single header form, and x-b3-flags: 1 in the multi header form. Debug implies an accept decision, so x-b3-sampled is set to 1 alongside it, which is what the Java implementation emits as well. x-b3-flags joins the fields of B3MultiFormat, since inject can now set it and a reused carrier has to have it cleared.
extract takes an optional context to extend, and it only ever set the debug flag, never cleared it. Extracting a carrier that carries no debug flag into a context that already held one therefore returned a context still marked for debug, and injecting it put the debug flag on the wire for a trace that was never marked for it. Record the debug state of the carrier being extracted, whether or not it is set, so the state the inject side reads is the state the carrier actually carried.
Two paths the suite did not reach. Instrumentation activates an extracted context by attaching it and then calls inject with no context argument, and every test so far passed the context to inject explicitly. test_flags_and_sampling named a combination of x-b3-flags and x-b3-sampled that it never built, leaving it byte for byte the same carrier as test_flags, so the case where the two headers disagree had no test at all.
test_flags and test_flags_and_sampling used to prove that x-b3-flags: 1 sets TraceFlags.SAMPLED on the extracted span context. Rewriting them to assert the debug flag replaced that with an assertion the inject side satisfies from the debug state alone, so deleting the flags clause from extract left the whole suite green. Assert the trace flag on the extracted parent as well as the debug flag on the injected carrier.
The B3 specification allows a sampling state to be sent on its own, so "b3: d" and "x-b3-flags: 1" are both complete carriers that mark a trace for debug without naming one. extract returned early on the missing ids and threw the flag away, and the service then started a root trace and injected an ordinary sampled decision. Store the flag on that path, and only when no ids were supplied at all, so a carrier whose ids are present but unparsable still leaves the context alone.
Every test that reached inject used a carrier with ids, so the store on the early-return path was only ever asserted through the context and never through what goes on the wire.
Nothing failed when either comparison was relaxed to a presence check, so an x-b3-flags of "0" read as debug and a single header accept state of "1" read as debug, and the suite stayed green both times.
Owner
Author
|
Superseded by the upstream pull request open-telemetry#5870 |
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.
Description
The B3 propagator accepted a received debug flag and then threw it away.
On extract, a sampling state of
din the single header form, andX-B3-Flags: 1in the multi header form, were both collapsed into theSAMPLEDtrace flag, and the debug state itself was not stored anywhere. On inject, the sampling decision was always re-emitted as1or0. A debug trace therefore stopped being a debug trace at the first service that used this propagator, and the operator who set the flag at the edge lost it for the rest of the request chain.The B3 extract requirements in
specification/context/api-propagators.mdsay "MUST preserve a debug trace flag, if received, and propagate it with subsequent requests. Additionally, an OpenTelemetry implementation MUST set the sampled trace flag when the debug flag is set." The second sentence was already satisfied. The first was not.extractnow records the debug state of the carrier under a context key, which is whereio.opentelemetry.extension.trace.propagation.B3Propagatorkeeps it, andinjectreads it back:B3SingleFormatemits the sampling stated.B3MultiFormatemitsX-B3-Flags: 1, together withX-B3-Sampled: 1.X-B3-FlagsjoinsB3MultiFormat.fields, because inject can now set it and a reused carrier has to have it cleared.Three decisions in this change are worth a reviewer's attention.
X-B3-Sampledis still sent for a debug trace. https://github.com/openzipkin/b3-propagation carries the note "Debug implies an accept decision, so don't also send theX-B3-Sampledheader", so emitting both is a deliberate deviation from it.B3PropagatorInjectorMultipleHeadersin opentelemetry-java setsX-B3-Sampledto1alongsideX-B3-Flags, so this keeps the two implementations in agreement, and it keeps the accept decision readable by a receiver that handlesX-B3-Sampledand ignoresX-B3-Flags. Omitting it is a one-line change if the project prefers the note.The two header forms are kept apart on extract. A sampling state of
dmeans debug only in the single header, becauseX-B3-Sampledis not a carrier for the debug flag. This propagator has always accepteddinX-B3-Sampledas a request to sample, and that behaviour is unchanged.A debug flag that arrives with no identifiers is kept. The B3 specification allows a sampling state to be sent on its own, so
b3: dandX-B3-Flags: 1are both complete carriers that mark a trace for debug without naming one.extractreturns early when the identifiers are missing, which threw that flag away too, and the service then started a root trace and injected an ordinary sampled decision. The flag is now stored on that path, and only when no identifiers were supplied at all, so a carrier whose identifiers are present but unparsable still leaves the context untouched.B3PropagatorExtractorSingleHeaderandB3PropagatorExtractorMultipleHeadersin opentelemetry-java both return before their debug check here, so this part goes further than that implementation. Under the SDK default sampler the root span that follows is recorded, so the injected decision and the local one agree.One consequence of keeping the state on the
Contextrather than on theSpanContextis worth knowing.Tracer.use_spanattachesset_value(_SPAN_KEY, span)against the current context, so a caller that passes an extracted context only asstart_as_current_span(context=...), without attaching it, loses every non-span entry of that context, and the debug flag is then not re-emitted. This is not specific to this change: W3C baggage is lost by the same call in the same way. Instrumentation that attaches the extracted context, which is what_start_internal_or_server_spanin opentelemetry-python-contrib does, re-emits the flag correctly, and there is a test for that path.Type of change
tox -e py313-test-opentelemetry-propagator-b3passes 82 tests,tox -e lint-opentelemetry-propagator-b3scores 10.00/10, and everytox -e precommithook passes.tox -e typecheckreports two errors, both in the Python 3.14 standard librarystring/templatelib.py, and both are present on the base commit without these changes;[tool.pyright].includeinpyproject.tomldoes not list this package, so the run does not examine the changed file.New tests cover extraction of the debug flag from both header forms, extraction from a sampling state sent with no identifiers in both forms, the absence of a debug flag on an ordinary sampled trace,
X-B3-Flags: 0and a single header sampling state of1not being read as debug,X-B3-Sampled: dnot being read as debug, a debug flag left in a context by an earlier carrier being cleared by a later one, re-propagation on inject from both forms, re-propagation onto the first local span after a debug flag received with no identifiers, re-propagation after the extracted context is attached, a received debug flag forcing an accept decision for an unsampled span, andX-B3-Flags: 1winning overX-B3-Sampled: 0.Checked by mutation: deleting
or flags == "1"from the sampling decision, dropping the debug branch from either inject, removingX-B3-Flagsfromfields, makingextractstop clearing a stored flag, or relaxing either debug comparison from an exact value to a presence check each fails at least one test.