-
Notifications
You must be signed in to change notification settings - Fork 855
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.
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
1911 b3 propagator debug flag #2038
1911 b3 propagator debug flag #2038
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2038 +/- ##
============================================
+ Coverage 84.37% 85.37% +0.99%
- Complexity 1879 2029 +150
============================================
Files 223 231 +8
Lines 7585 7856 +271
Branches 808 832 +24
============================================
+ Hits 6400 6707 +307
+ Misses 874 840 -34
+ Partials 311 309 -2 Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the help
...ace-propagators/src/main/java/io/opentelemetry/extension/trace/propagation/B3Propagator.java
Outdated
Show resolved
Hide resolved
.../java/io/opentelemetry/extension/trace/propagation/B3PropagatorExtractorMultipleHeaders.java
Outdated
Show resolved
Hide resolved
.../java/io/opentelemetry/extension/trace/propagation/B3PropagatorExtractorMultipleHeaders.java
Outdated
Show resolved
Hide resolved
...propagators/src/test/java/io/opentelemetry/extension/trace/propagation/B3PropagatorTest.java
Show resolved
Hide resolved
…tension/trace/propagation/B3Propagator.java Co-authored-by: Anuraag Agrawal <anuraaga@gmail.com>
...ain/java/io/opentelemetry/extension/trace/propagation/B3PropagatorExtractorSingleHeader.java
Outdated
Show resolved
Hide resolved
.../java/io/opentelemetry/extension/trace/propagation/B3PropagatorExtractorMultipleHeaders.java
Outdated
Show resolved
Hide resolved
@@ -26,6 +26,12 @@ | |||
|
|||
String sampled = spanContext.isSampled() ? Common.TRUE_INT : Common.FALSE_INT; | |||
|
|||
String debug = context.get(B3Propagator.DEBUG_CONTEXT_KEY); | |||
if (!StringUtils.isNullOrEmpty(debug) && debug.contentEquals(Common.TRUE_INT)) { | |||
setter.set(carrier, B3Propagator.DEBUG_HEADER, Common.TRUE_INT); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we use the new constant here, MULTI_HEADER_DEBUG
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeh, have changed it/tidied it up.
@@ -26,6 +26,12 @@ | |||
|
|||
String sampled = spanContext.isSampled() ? Common.TRUE_INT : Common.FALSE_INT; | |||
|
|||
String debug = context.get(B3Propagator.DEBUG_CONTEXT_KEY); | |||
if (!StringUtils.isNullOrEmpty(debug) && debug.contentEquals(Common.TRUE_INT)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Common.TRUE_INT.equals(debug)
for this and next file I think
@@ -26,6 +26,11 @@ | |||
|
|||
String sampled = spanContext.isSampled() ? Common.TRUE_INT : Common.FALSE_INT; | |||
|
|||
if (B3Propagator.MULTI_HEADER_DEBUG.equals(context.get(B3Propagator.DEBUG_CONTEXT_KEY))) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm. In this case, aren't we putting Common.TRUE_INT
into the context, not B3Propagator.MULTI_HEADER_DEBUG
? They might happen to have the same value at the moment, but I don't think we should confuse the constant from the wire protocol with the constant that we're using internally for propagation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah good point - whether the b3 debug was enabled or not is sort of independent of how it was actually modeled in the header, and it might be converted from single to multiple, or vice versa. @jarebudev How about just using a Boolean
context value for this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeh makes sense. I've changed it to set a boolean, hopefully this sorts it!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
resolves #1911