tracing: add ExtractTraceSpanID #550
Conversation
Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>
Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>
CI is failing, checking if it has anything to do with this test Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>
This reverts commit ab0f401.
tracing/tracing.go
Outdated
| } | ||
|
|
||
| return sctx.TraceID().String(), true | ||
| return &sctx |
There was a problem hiding this comment.
By returning &sctx, jaeger.SpanContext needs to be allocated on a heap first, and then pointer to it returned, right?
Perhaps we could return just traceID and spanID directly? Or jaeger.SpanContext without the pointer.
There was a problem hiding this comment.
I thought about that , but that (direct return) would be duplicated code. I can definitely do a copy of the jeager.SpanContext thought, if that's preferred?
There was a problem hiding this comment.
By the way , I don't know if the compiler is smart enough to avoid allocation on the heap
There was a problem hiding this comment.
Can we keep ExtractTraceID, and call ExtractTraceSpanID inside, just discard spanID when return result. In that case no need to refectory this extractJaegerContext but leave all logics in ExtractTraceSpanID
There was a problem hiding this comment.
facepalm, of course
although then your doing an extra string formatting call for the spanid
There was a problem hiding this comment.
I think it is a fair comment, current code LGTM, approved.
There was a problem hiding this comment.
it did inspire me to go ahead and look at the assembly :) Got the final result by deferring the formatting to the public function , but only returning the two relevant fields from the helper function. It's the least extra instructions.
Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>
Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>
What this PR does:
Add ExtractTraceSpanID helper function for enhanced logging.
Which issue(s) this PR fixes:
Related to grafana/mimir#8599
Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX]