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 stringer prefix zeroes #532

Closed
lukedirtwalker opened this issue Sep 11, 2020 · 2 comments · Fixed by #533
Closed

TraceID stringer prefix zeroes #532

lukedirtwalker opened this issue Sep 11, 2020 · 2 comments · Fixed by #533

Comments

@lukedirtwalker
Copy link
Contributor

Requirement - what kind of business use case are you trying to solve?

Logging TraceIDs should result in the same TraceIDs that are shown in the JaegerUI.
For example if I see 0c2de5a29f1a2dff in the JaegerUI and if I log the TraceID id with id.String() I will see c2de5a29f1a2dff in the log. This makes it harder than it should be to find a trace ID in the logs.

The change of displaying the 0 prefix in the UI was introduced in Jaeger 1.16 (see https://github.com/jaegertracing/jaeger/releases/tag/v1.16.0)

Problem - what in Jaeger blocks you from solving the requirement?

I could manually address this by writing my own formatting for the logging. The question is whether it wouldn't make sense to change the String method of the TraceID

Proposal - what do you suggest to solve the problem or improve the existing situation?

Change

func (t TraceID) String() string {
if t.High == 0 {
return fmt.Sprintf("%x", t.Low)
}
return fmt.Sprintf("%x%016x", t.High, t.Low)
}
to use always a fixed width hex representation.
Example: https://play.golang.org/p/DyCZUMF33i9

Any open questions to address

If you think it makes sense to change the Stringer in the proposed way, I can send a PR.
If you think it doesn't make sense I would be interested in the reason.

@yurishkuro
Copy link
Member

+1. The wire encoding was already changed in #472 to include leading zeros, and ^^^ function should be consistent with that.

Would you like to create a PR?

lukedirtwalker added a commit to lukedirtwalker/jaeger-client-go that referenced this issue Sep 11, 2020
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>
@lukedirtwalker
Copy link
Contributor Author

Thanks, I created #533 , let me know if you want me to change anything.

yurishkuro pushed a commit that referenced this issue Sep 12, 2020
* TraceID.String prefix with zeroes

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>

* SpanID.String prefix with zeroes

Also test parsing back works.

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

* fix tests?

how do I run this thing locally?

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

* fix tests!

Signed-off-by: Lukas Vogel <vogel@anapaya.net>
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 a pull request may close this issue.

2 participants