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.

Sign up for GitHub

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

Jump to bottom

Do not add invalid context to references #521

Merged
merged 2 commits into from Jul 13, 2020
Merged

Do not add invalid context to references #521

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c6b7c94
Do not add invalid context to references
Jul 13, 2020
a09975e
Rename helper
Jul 13, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
8 changes: 6 additions & 2 deletions span_context.go
View file
Open in desktop
Expand Up @@ -212,10 +212,14 @@ func (c SpanContext) SetFirehose() {
}

func (c SpanContext) String() string {
var flags int32
if c.samplingState != nil {
flags = c.samplingState.stateFlags.Load()
}
if c.traceID.High == 0 {
return fmt.Sprintf("%016x:%016x:%016x:%x", c.traceID.Low, uint64(c.spanID), uint64(c.parentID), c.samplingState.stateFlags.Load())
return fmt.Sprintf("%016x:%016x:%016x:%x", c.traceID.Low, uint64(c.spanID), uint64(c.parentID), flags)
}
return fmt.Sprintf("%016x%016x:%016x:%016x:%x", c.traceID.High, c.traceID.Low, uint64(c.spanID), uint64(c.parentID), c.samplingState.stateFlags.Load())
return fmt.Sprintf("%016x%016x:%016x:%016x:%x", c.traceID.High, c.traceID.Low, uint64(c.spanID), uint64(c.parentID), flags)
}

// ContextFromString reconstructs the Context encoded in a string
Expand Down
5 changes: 4 additions & 1 deletion tracer.go
View file
Open in desktop
Expand Up @@ -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 •
edited

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

// we don't want empty context that contains only debug-id or baggage
references = append(references, Reference{Type: ref.Type, Context: ctxRef})
}

if !hasParent {
parent = ctxRef
Expand Down
1 change: 1 addition & 0 deletions tracer_test.go
View file
Open in desktop
Expand Up @@ -414,6 +414,7 @@ func TestThrottling_DebugHeader(t *testing.T) {

sp := tracer.StartSpan("root", opentracing.ChildOf(ctx)).(*Span)
assert.True(t, sp.context.IsDebug())
assert.Len(t, sp.References(), 0)
closer.Close()

tracer, closer = NewTracer("DOOP", NewConstSampler(true), NewNullReporter(),
Expand Down