Skip to content
This repository has been archived by the owner on Dec 23, 2023. It is now read-only.

Fix bug when traceparent is too small #2047

Closed

Conversation

dengliming
Copy link

for #2038

@googlebot
Copy link

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 @googlebot I signed it! and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

@dengliming
Copy link
Author

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 @googlebot I signed it! and we'll verify it.

What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

@googlebot I signed it!

@googlebot
Copy link

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

@codecov-commenter
Copy link

codecov-commenter commented Jul 26, 2020

Codecov Report

Merging #2047 into master will decrease coverage by 0.02%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             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     
Impacted Files Coverage Δ Complexity Δ
...implcore/trace/propagation/TraceContextFormat.java 94.02% <100.00%> (-0.09%) 15.00 <0.00> (-1.00)
...pencensus/implcore/trace/RecordEventsSpanImpl.java 92.34% <0.00%> (-1.03%) 49.00% <0.00%> (-1.00%)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5be7044...18d4938. Read the comment docs.

@dengliming
Copy link
Author

@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
Copy link
Contributor

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?

Copy link
Author

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?

Copy link
Contributor

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.

@punya punya closed this Jan 25, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants