Do not add invalid context to references #521
Conversation
Signed-off-by: Yuri Shkuro <ys@uber.com>
| @@ -245,7 +245,10 @@ func (t *Tracer) startSpanWithOptions( | |||
| continue | |||
| } | |||
|
|
|||
| references = append(references, Reference{Type: ref.Type, Context: ctxRef}) | |||
| if ctxRef.IsValid() { | |||
There was a problem hiding this comment.
Perhaps we should rename isValidReferece on L238 to sth else? The logic is a bit confusing to me
There was a problem hiding this comment.
do you have suggestions? isThereSomethingThere(ref)?
There was a problem hiding this comment.
or couldBeParent(ref)
There was a problem hiding this comment.
or we can simply inline the condition, since it's never reused
There was a problem hiding this comment.
I think inlining and refactoring the logic for the debug container and brining it closer would help.
My confusion was because isValidReference(ctxRef) already evaluates ctxRef.isValid() on L238 and short circuits. Upon first reading I expected L248 to always be valid.
There was a problem hiding this comment.
it's reused so cannot inline it. I renamed it to isEmptyReference
Codecov Report
@@ Coverage Diff @@
## master #521 +/- ##
==========================================
+ Coverage 89.27% 89.28% +0.01%
==========================================
Files 61 61
Lines 3915 3919 +4
==========================================
+ Hits 3495 3499 +4
Misses 294 294
Partials 126 126
Continue to review full report at Codecov.
|
Signed-off-by: Yuri Shkuro <ys@uber.com>
Which problem is this PR solving?
jaeger-debug-idheader, the root span gets an invalid parent reference resulting in a warningShort description of the changes
SpanContext.String()