-
Notifications
You must be signed in to change notification settings - Fork 202
Fix bug when traceparent is too small #2047
Conversation
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here with What to do if you already signed the CLAIndividual signers
Corporate signers
ℹ️ Googlers: Go here for more info. |
@googlebot I signed it! |
CLAs look good, thanks! ℹ️ Googlers: Go here for more info. |
Codecov Report
@@ Coverage Diff @@
## master #2047 +/- ##
============================================
- Coverage 83.06% 83.04% -0.03%
+ Complexity 2251 2249 -2
============================================
Files 319 319
Lines 10529 10528 -1
Branches 1048 1047 -1
============================================
- Hits 8746 8743 -3
- Misses 1446 1447 +1
- Partials 337 338 +1
Continue to review full report at Codecov.
|
@nilebox Can you review these changes? This seems to be a higher priority during the opencensus transition to Opentelemetry. |
@@ -123,8 +123,7 @@ | |||
// TODO(bdrutu): Do we need to verify that version is hex and that for the version | |||
// the length is the expected one? | |||
checkArgument( | |||
traceparent.charAt(TRACE_OPTION_OFFSET - 1) == TRACEPARENT_DELIMITER |
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.
Could you add a unit test reproducing the issue #2038 and confirming it's fixed by this change?
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.
I haven't find a recurrence here, but anyway, there is something wrong with the logic of this code, right?
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.
Unfortunately, I'm not familiar with this code.
I do agree that the expression seems to have a redundant duplicate condition, though removing the first check may lead to some other issue since when it's "false" the remaining conditions won't get executed.
So I suggest to add a test reproducing the issue this change is supposed to fix.
for #2038