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.
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.
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.
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.
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.
Don't forget we need to do the same for the SpanID.
Refactors ctx.traceID and ctx.spanID in injectTextMap() to format injected headers as exactly 16 characters long. Fixes #695
Refactors
ctx.traceIDandctx.spanIDininjectTextMap()to format injected headers as exactly 16 characters long.Fixes #695