Skip to content
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

fix(propagator-ot-trace): read sampled flag correctly from span context #1077

Conversation

basti1302
Copy link
Contributor

@basti1302 basti1302 commented Jul 4, 2022

Which problem is this PR solving?

Fixes the way the sampled flag is read from the span context. The way it
is currently read will break when more flags are introduced. This is even
mentioned explicitly in the
specification.

Short description of the changes

  • check the flag bit independently instead of comparing the whole flags byte value with the SAMPLED_FLAG constant.
  • add a unit test that show cases the issue (and breaks without the change in production code)

Checklist

  • Ran npm run test for the edited package(s) on the latest commit if applicable.

@basti1302 basti1302 requested a review from a team July 4, 2022 14:38
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Jul 4, 2022

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: basti1302 / name: Bastian Krol (76fa970)

Copy link
Member

@rauno56 rauno56 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch! Thanks.

@basti1302
Copy link
Contributor Author

For the record, I am currently working with the CCLA managers at my org to get my CLA in place.

@codecov
Copy link

codecov bot commented Jul 5, 2022

Codecov Report

Merging #1077 (1e8558a) into main (57cedc5) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main    #1077   +/-   ##
=======================================
  Coverage   95.91%   95.91%           
=======================================
  Files          13       13           
  Lines         856      857    +1     
  Branches      178      179    +1     
=======================================
+ Hits          821      822    +1     
  Misses         35       35           
Impacted Files Coverage Δ
...metry-propagator-ot-trace/src/OTTracePropagator.ts 95.91% <100.00%> (+0.08%) ⬆️

@basti1302
Copy link
Contributor Author

I have the CLA in place now. It shows up okay on #1081, but not here. If I go through the EasyCLA process again for this PR (clicking on the links in the EasyCLA bot message in this PR), it ends with this

image

... but after that, the check still shows up as failed.

I guess EasyCLA does not handle multiple simultaneous PRs from a first time contributor all that well? Should I just close this PR and open it again? Or use the support ticket link? ("For further assistance with EasyCLA, please submit a support request ticket.

@Flarna
Copy link
Member

Flarna commented Jul 8, 2022

@basti1302 Try to push a commit, maybe this retriggers the check here.

@basti1302 basti1302 force-pushed the fix-ot-trace-propagator-sampled-flag-handling branch from fedc75f to 76fa970 Compare July 8, 2022 11:52
@basti1302
Copy link
Contributor Author

@basti1302 Try to push a commit, maybe this retriggers the check here.

It did! Thanks 👍 CLA shows up as "covered" now.

@basti1302 basti1302 force-pushed the fix-ot-trace-propagator-sampled-flag-handling branch from 76fa970 to 4c65220 Compare July 26, 2022 16:23
@basti1302 basti1302 force-pushed the fix-ot-trace-propagator-sampled-flag-handling branch 2 times, most recently from 7add428 to 8eda5a8 Compare August 9, 2022 10:21
@basti1302
Copy link
Contributor Author

I think this change is ready to be merged.

@Flarna
Copy link
Member

Flarna commented Aug 9, 2022

@basti1302 it requires a rebase/merge from main.
I have no rights to do this on your fork, most likely because its owned by an org. In that case the "allow maintainer edits" doesn't work.

The way the sampled flag is currently read from the span context will
break when more flags are introduced. This is mentioned explicitly in
the [specification](https://www.w3.org/TR/trace-context/#trace-flags).
@basti1302 basti1302 force-pushed the fix-ot-trace-propagator-sampled-flag-handling branch from 8eda5a8 to 1e8558a Compare August 9, 2022 12:43
@basti1302
Copy link
Contributor Author

@basti1302 it requires a rebase/merge from main. I have no rights to do this on your fork, most likely because its owned by an org. In that case the "allow maintainer edits" doesn't work.

I had rebased this branch earlier today, but of course after the merge of #1078 it now needed yet another rebase, which I have performed just now.

@Flarna Flarna merged commit 69740ab into open-telemetry:main Aug 9, 2022
@dyladan dyladan mentioned this pull request Aug 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants