Skip to content
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

Do not add invalid context to references #521

Conversation

yurishkuro
Copy link
Member

Which problem is this PR solving?

  • when sending requests with jaeger-debug-id header, the root span gets an invalid parent reference resulting in a warning

image

Short description of the changes

  • do not add span context to references if it does not contain valid trace ID
  • fix null pointer error in SpanContext.String()

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() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps we should rename isValidReferece on L238 to sth else? The logic is a bit confusing to me

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do you have suggestions? isThereSomethingThere(ref)?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

or couldBeParent(ref)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

or we can simply inline the condition, since it's never reused

Copy link
Contributor

@vprithvi vprithvi Jul 13, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's reused so cannot inline it. I renamed it to isEmptyReference

@codecov
Copy link

codecov bot commented Jul 13, 2020

Codecov Report

Merging #521 into master will increase coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            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              
Impacted Files Coverage Δ
span_context.go 93.58% <100.00%> (+0.12%) ⬆️
tracer.go 95.91% <100.00%> (+0.01%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 704ce28...a09975e. Read the comment docs.

Signed-off-by: Yuri Shkuro <ys@uber.com>
@yurishkuro yurishkuro merged commit 59fdc96 into jaegertracing:master Jul 13, 2020
@yurishkuro yurishkuro deleted the do-not-add-invalid-context-to-references branch July 13, 2020 20:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants