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

Injected X-B3-Traceid header may be less then 16 characters long #695

Closed
sullerandras opened this issue Jul 15, 2020 · 1 comment · Fixed by #817
Closed

Injected X-B3-Traceid header may be less then 16 characters long #695

sullerandras opened this issue Jul 15, 2020 · 1 comment · Fixed by #817
Labels
ack bug unintended behavior that has to be fixed

Comments

@sullerandras
Copy link

It seems like the injectTextMap function does not add leading zeroes to the traceid when it's emitted in hexadecimal:

writer.Set(b3TraceIDHeader, strconv.FormatUint(ctx.traceID, 16))
writer.Set(b3SpanIDHeader, strconv.FormatUint(ctx.spanID, 16))

This causes issues in our Ruby app because that assumes the Traceid header will be at least 16 chars long:
https://github.com/twitter/finagle/blob/d2e7485782faeada129d43d7511167c31811df81/finagle-thrift/src/main/ruby/lib/finagle-thrift/trace.rb#L103

Would it be possible to format the traceid in the injected headers to be exactly 16 chars long? E.g. something like fmt.Sprintf("%016x", ctx.traceID)

@knusbaum knusbaum added the ack label Aug 3, 2020
@knusbaum
Copy link
Contributor

knusbaum commented Aug 3, 2020

@sullerandras
I don't think that adding this would be a problem.

It's not made crystal-clear, but I think the B3 specification says that these headers should be 16 (or possibly 32 for traceid) characters anyway, so I think we should change this.

Would you be willing to put up a PR?

@knusbaum knusbaum added the bug unintended behavior that has to be fixed label Sep 16, 2020
knusbaum pushed a commit that referenced this issue Feb 2, 2021
Refactors ctx.traceID and ctx.spanID in injectTextMap() to format injected
headers as exactly 16 characters long.

Fixes #695
dianashevchenko pushed a commit that referenced this issue Feb 8, 2021
Refactors ctx.traceID and ctx.spanID in injectTextMap() to format injected
headers as exactly 16 characters long.

Fixes #695
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ack bug unintended behavior that has to be fixed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants