-
Notifications
You must be signed in to change notification settings - Fork 623
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
Add proper length zero padding to hex strings sent on the wire #908
Conversation
|
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.
once CI is fixed and a specific regression test is added, LGTM!
Also can you add entries to the appropriate changelogs?
@@ -97,10 +97,10 @@ def test_constructor_explicit(self): | |||
def test_export(self): | |||
|
|||
span_names = ("test1", "test2", "test3", "test4") | |||
trace_id = 0x6E0C63257DE34C926F9EFCD03927272E | |||
span_id = 0x34BF92DEEFC58C92 | |||
trace_id = 0x0E6C63257DE34C926F9EFCD03927272E |
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.
IMO it would be better if we added a specific test to ensure that zero-padding works as intended. The current test makes no indicator that the zero-padded traceid is intentional to catch bugs.
A good test with a docstring per regression to educate and ensure future refactors are aware of the rationale for the test, is great.
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 review, will do your suggestions tomorrow.
I hope I can get the CLA signed by my company though.
It's weird, I had the approved CLA for CNCF last month but this got switched to EasyCLA which requires a new signature.
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.
Thank you! I believe if you signed at least one of the CLAs, that's ok for us to merge. But I'll let the maintainers weigh in there.
5baee80
to
5c2a704
Compare
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.
LGTM, thanks!
1b4fbcb
to
d42fa63
Compare
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.
Please update the CHANGELOG
c0f7044
to
cc5df61
Compare
cc5df61
to
fc58032
Compare
…entId sent on the wire For compatibility with jaeger-collector Fixes open-telemetry#907
…emetry#910) Co-authored-by: Daniel Dyla <dyladan@users.noreply.github.com>
traceId length 32, parentId and spanId length 16
For compatibility with jaeger-collector
Fixes #907