Fix B3 propagator and add tests #882
Merged
Conversation
This file contains 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
MrAlias
added
pkg:testing
MrAlias
requested review from
Aneurysm9,
evantorrie,
jmacd,
lizthegrey and
paivagustavo
as code owners
evantorrie
reviewed
Jul 1, 2020
Add a new "not sampled" mask to complement the existing "sampled" one. Rename `FlagsUnused` to `FlagsUnset`. Add documentation for each of the flags to help understand their purpose.
The B3 specification states "Debug is encoded as `X-B3-Flags: 1`. Absent or any other values can be ignored", so testing of other values should not result in an error.
Remove test cases that would fail if the fallback header format was expected to not be used.
Add the B3Encoding and valid HTTP based values. Change the B3 propagator to use these bitmask fields to specify the inject encoding it will propagate.
Add a FlagsDebug and FlagsDeferred to track the B3 trace state. Add helper methods to the SpanContext to check the debug and deferred bit of the trace flags. Update SpanContext.IsSampled to return if the sampling decision is to sample rather than if the sample bit is set. This means that if the debug bit is also set it will return true.
This check makes sample only headers not propagate.
Use the passed SpanContext and check directly the span ID.
Run update checked SpanID to match sent. Add tests to validate sample only transmissions and debug flag support.
This is no longer the parent.
Aneurysm9
reviewed
Jul 3, 2020
evantorrie
reviewed
Jul 4, 2020
evantorrie
reviewed
Jul 4, 2020
evantorrie
approved these changes
Jul 4, 2020
jmacd
approved these changes
Jul 7, 2020
The B3SingleHeader name will conflict with the upcoming change to prefix the SingleHeader encoding with "B3". There are a few options to address this conflict, but in the end we do not need to be exporting these values. They are duplicates of the OpenZipkin package and users should use those.
Include a `B3` prefix to scope the encoding names. Move the related support method to the B3Encoding itself, instead of the B3 propagator. Add tests to provide a sanity check for encoding bitmasks.
Update test name to better describe how unused bits have no affect on the sampling decision. Include the inverse of this test as well: not sampled but has unused bits.
Aneurysm9
approved these changes
Jul 7, 2020
Co-authored-by: Anthony Mirabella <a9@aneurysm9.com>
Merged
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.
The existing implementation of the B3 propagator does not fully implement the HTTP B3 specification. This changes the propagator to closer match the specification. The remaining non-compliance issue is the lack of injecting
ParentSpanIDto outgoing requests.Fixed
b3: 0,b3: 1, orb3: d) for a Single B3 Header.X-B3-Samplingheader.X-B3-Samplingheader when injecting.Added
B3Encodingtype to represent the B3 encoding(s) the B3 propagator can inject. A value for HTTP supported encodings (Multiple Header:MultipleHeader, Single Header:SingleHeader) are included.FlagsDeferredtrace flag to indicate if the trace sampling decision has been deferred.FlagsDebugtrace flag to indicate if the trace is a debug trace.Removed
FlagsUnusedtrace flag is removed. The purpose of this flag was to act as the inverse ofFlagsSampled, the inverse ofFlagsSampledis used instead.Changed
SingleHeaderfield has been replaced withInjectEncoding. This new field can be set to combinations of theB3Encodingbitmasks and will inject trace information in these encodings. If no encoding is set, the propagator will default toMultipleHeaderencoding.References: