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.

Sign up for GitHub

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

Jump to bottom

fix: left pad short b3 trace identifiers #1021

Merged
merged 2 commits into from
May 5, 2020
Merged

fix: left pad short b3 trace identifiers #1021

merged 2 commits into from
May 5, 2020

Conversation

dyladan
Copy link
Member

@dyladan dyladan commented May 5, 2020

Fixes #1020

@dyladan dyladan added the bug Something isn't working label May 5, 2020
@dyladan dyladan mentioned this pull request May 5, 2020
Closed
@mayurkale22
Copy link
Member

/cc @Aneurysm9

@Aneurysm9
Copy link
Member

Thanks for the quick attention. I think this works but may not be completely correct. The B3 spec says that the trace ID will be either 64-bit or 128-bit, so I would suggest validating that an incoming ID is either 16 or 32 hex digits.

@dyladan
Copy link
Member Author

dyladan commented May 5, 2020

updated

@dyladan dyladan requested a review from Aneurysm9 May 5, 2020 20:15
@mayurkale22 mayurkale22 added Merge:LGTM This PR is ready to be merged by a Maintainer (has enough valid approvals, successful build, etc.) size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels May 5, 2020
Aneurysm9
Aneurysm9 approved these changes May 5, 2020
View reviewed changes
Copy link
Member

@Aneurysm9 Aneurysm9 left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@mayurkale22 mayurkale22 merged commit a5444dc into open-telemetry:master May 5, 2020
@dyladan dyladan deleted the left-pad-b3 branch May 5, 2020 20:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mayurkale22 mayurkale22 mayurkale22 approved these changes

@markwolff markwolff markwolff approved these changes

@vmarchaud vmarchaud vmarchaud approved these changes

@Aneurysm9 Aneurysm9 Aneurysm9 approved these changes

@bg451 bg451 Awaiting requested review from bg451

@obecny obecny Awaiting requested review from obecny

@OlivierAlbertini OlivierAlbertini Awaiting requested review from OlivierAlbertini

@rochdev rochdev Awaiting requested review from rochdev

Assignees
No one assigned
Labels
bug Something isn't working Merge:LGTM This PR is ready to be merged by a Maintainer (has enough valid approvals, successful build, etc.) size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

B3 Propagator should left-pad 64-bit trace IDs with zeros
5 participants
@dyladan @mayurkale22 @Aneurysm9 @vmarchaud @markwolff