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

TraceID.String prefix with zeroes #533

Merged
merged 4 commits into from Sep 12, 2020

Conversation

lukedirtwalker
Copy link
Contributor

The wire encoding of the TraceID uses zero prefixes (#472).
The JaegerUI also uses zero prefixes (since 1.16).
So it makes sense that the Stringer of the TraceID also does this.

Resolves #532

Signed-off-by: Lukas Vogel vogel@anapaya.net

The wire encoding of the TraceID uses zero prefixes (jaegertracing#472).
The JaegerUI also uses zero prefixes (since 1.16).
So it makes sense that the Stringer of the TraceID also does this.

Resolves jaegertracing#532

Signed-off-by: Lukas Vogel <vogel@anapaya.net>
@@ -344,9 +344,9 @@ func (c *SpanContext) isDebugIDContainerOnly() bool {

func (t TraceID) String() string {
if t.High == 0 {
return fmt.Sprintf("%x", t.Low)
return fmt.Sprintf("%016x", t.Low)
Copy link
Member

Choose a reason for hiding this comment

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

I would also do it in L382 for span ID

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

Should I squash? Or will you do that on pre-merge?

Copy link
Member

Choose a reason for hiding this comment

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

we squash on merge

Also test parsing back works.

Signed-off-by: Lukas Vogel <vogel@anapaya.net>
Copy link
Member

@yurishkuro yurishkuro left a comment

Choose a reason for hiding this comment

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

Thanks!

@yurishkuro
Copy link
Member

please run all the tests, there are CI failures.

how do I run this thing locally?

Signed-off-by: Lukas Vogel <vogel@anapaya.net>
@lukedirtwalker
Copy link
Contributor Author

Sorry somehow couldn't get this to run locally. a go.mod would really make this easier :P
I'll have to take a look at this again tomorrow or early next week, just wasting CI cycles isn't optimal :/

Signed-off-by: Lukas Vogel <vogel@anapaya.net>
@lukedirtwalker
Copy link
Contributor Author

Alright, seems to pass CI now. I just had to check the project out in the correct local folder, then I could get it to work locally. Thanks.

@codecov
Copy link

codecov bot commented Sep 12, 2020

Codecov Report

Merging #533 into master will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #533   +/-   ##
=======================================
  Coverage   89.28%   89.28%           
=======================================
  Files          61       61           
  Lines        3919     3919           
=======================================
  Hits         3499     3499           
  Misses        294      294           
  Partials      126      126           
Impacted Files Coverage Δ
span_context.go 93.58% <100.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 4b20232...b57daf2. Read the comment docs.

@yurishkuro yurishkuro merged commit b1fe7fe into jaegertracing:master Sep 12, 2020
@lukedirtwalker lukedirtwalker deleted the traceIDStringer branch September 12, 2020 07:01
nabowler added a commit to nabowler/jaeger-client-go that referenced this pull request Oct 6, 2021
…gator

Based on the openzipkin/b3-propagation specification, the SpanID and
ParentSpanID are to be encoded as 16 lower-hex characters.

jaegertracing#533 fixed this
for TraceID, and fixed the SpanID.String() method, but did not update
the Zipkin Propagator SpanID encoding.

This fix uses SpanID.String() to ensure proper encoding of the SpanID and
ParentSpandID within the Zipkin Propagator.

Signed-off-by: Nathan Bowler <nzb301@psu.edu>
yurishkuro pushed a commit that referenced this pull request Oct 6, 2021
Based on the openzipkin/b3-propagation specification, the SpanID and
ParentSpanID are to be encoded as 16 lower-hex characters.

#533 fixed this
for TraceID, and fixed the SpanID.String() method, but did not update
the Zipkin Propagator SpanID encoding.

This fix uses SpanID.String() to ensure proper encoding of the SpanID and
ParentSpandID within the Zipkin Propagator.

Signed-off-by: Nathan Bowler <nzb301@psu.edu>
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.

TraceID stringer prefix zeroes
2 participants