-
Notifications
You must be signed in to change notification settings - Fork 327
Export trace attributes and annotations properly to Jaeger #622
Conversation
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.
Thank you @rakyll, I have 1 request or rather 1 that changes about 3 things, PTAL.
exporter/jaeger/jaeger_test.go
Outdated
name: "no parent", | ||
data: &trace.SpanData{ | ||
SpanContext: trace.SpanContext{ | ||
TraceID: trace.TraceID{1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 13, 14, 15, 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.
Could you make this a 16 byte TraceID to conform with the usual? This is 15 bytes and it even produces Spans that have 15 byte SpanIDs and TraceIDs.
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.
Aha, you missed 12 in there :)
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.
Whoops, it was a mistake. Done.
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.
That'll change the value of the TraceIDHigh, *Low, SpanID.
Let's also add tests to ensure that they TraceIDHigh is 8 bytes and same for TraceIDLow otherwise we'd never catch such discrepancies.
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 don't think we need a test for this. There might be trailing zeros. The trace ID was 16-bytes with a trailing zero which is still valid.
for k, v := range a.Attributes { | ||
tag := attributeToTag(k, v) | ||
if tag != nil { | ||
fields = append(tags, tag) | ||
fields = append(fields, tag) |
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.
Great catch!
@odeke-em PTAL |
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.
LGTM, thank you @rakyll!
Fixes #621.