-
Notifications
You must be signed in to change notification settings - Fork 435
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
ddtrace/tracer: format injected traceid header with leading zeros #817
Conversation
ddtrace/tracer/textmap_test.go
Outdated
@@ -216,8 +217,7 @@ func TestB3(t *testing.T) { | |||
|
|||
assert := assert.New(t) | |||
assert.Nil(err) | |||
|
|||
assert.Equal(headers[b3TraceIDHeader], strconv.FormatUint(root.TraceID, 16)) | |||
assert.Equal(headers[b3TraceIDHeader], fmt.Sprintf("%016x", strconv.FormatUint(root.TraceID, 16))) |
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.
Hmm, this assert needed improvement already, but we should do that now, as part of this PR.
We are essentially asserting that the header is derived from the TraceID, since we're using the same code to check the value as we are to set the value. (fmt.Sprintf("%016x", strconv.FormatUint(root.TraceID, 16))
). That means we have very little assurance that the header values are actually correct.
In fact, in this case the test will pass (because the code is the same in both places) but it will break b3 headers, because the Sprintf
is incorrect.
strconv.FormatUint
will format the root.TraceID
to a hex string, and then fmt.Sprintf("%016x"
will convert each byte of the string to its hexadecimal representation.
From fmt
documentation:
String and slice of bytes (treated equivalently with these verbs):
%s the uninterpreted bytes of the string or slice
%q a double-quoted string safely escaped with Go syntax
%x base 16, lower-case, two characters per byte
%X base 16, upper-case, two characters per byte
Run the code here to see what's happening: https://play.golang.org/p/68YZ1vVs6Vt
Instead, for this test, we should manually set the TraceID
and SpanID
to known values in root
, and have known strings that the headers should equal. Then we know our string generation code is correct.
We should do this in a table test to test multiple values. (see how we do table tests elsewhere in this project)
Once we have the test testing what we want, it should be easier to change the injectTextMap
code.
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 help, Kyle! Upon further inspection, I think fmt.Sprintf("%016s"
would be correct for adding the leading zeros and not altering the value of the IDs.
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.
I think it would be better to use just Sprintf
and leave out the FormatUint
altogether.
My understanding is that Sprintf("%016x", root.TraceID)
should be sufficient for formating a 16-character hex string.
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.
Ah, understood. Will add to both ids
ddtrace/tracer/textmap.go
Outdated
@@ -306,7 +307,7 @@ func (*propagatorB3) injectTextMap(spanCtx ddtrace.SpanContext, writer TextMapWr | |||
if !ok || ctx.traceID == 0 || ctx.spanID == 0 { | |||
return ErrInvalidSpanContext | |||
} | |||
writer.Set(b3TraceIDHeader, strconv.FormatUint(ctx.traceID, 16)) | |||
writer.Set(b3TraceIDHeader, fmt.Sprintf("%016x", strconv.FormatUint(ctx.traceID, 16))) | |||
writer.Set(b3SpanIDHeader, strconv.FormatUint(ctx.spanID, 16)) |
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.
Don't forget we need to do the same for the SpanID.
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.
This looks great.
Refactors ctx.traceID and ctx.spanID in injectTextMap() to format injected headers as exactly 16 characters long. Fixes #695
Refactors
ctx.traceID
andctx.spanID
ininjectTextMap()
to format injected headers as exactly 16 characters long.Fixes #695