Skip to content

Commit

Permalink
POC: Self Referenced Span
Browse files Browse the repository at this point in the history
  • Loading branch information
dm03514 committed Aug 3, 2019
1 parent e895c7a commit bf5f342
Show file tree
Hide file tree
Showing 2 changed files with 47 additions and 33 deletions.
2 changes: 2 additions & 0 deletions span.go
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,8 @@ import (
"github.com/opentracing/opentracing-go/log"
)

const SelfRef opentracing.SpanReferenceType = 99

This comment has been minimized.

Copy link
@dm03514

dm03514 Aug 3, 2019

Author Owner

Where's the best spot for this? What's the best value?

This comment has been minimized.

Copy link
@yurishkuro

yurishkuro Aug 3, 2019

probably constants.go is a better place. 99 seems fine, there's no precedent, to my knowledge

This comment has been minimized.

Copy link
@yurishkuro

yurishkuro Aug 3, 2019

needs documentation in the comments and a section in the README


// Span implements opentracing.Span
type Span struct {
// referenceCounter used to increase the lifetime of
Expand Down
78 changes: 45 additions & 33 deletions tracer.go
Original file line number Diff line number Diff line change
Expand Up @@ -221,7 +221,8 @@ func (t *Tracer) startSpanWithOptions(

var references []Reference
var parent SpanContext
var hasParent bool // need this because `parent` is a value, not reference
var hasParent bool // need this because `parent` is a value, not reference
var selfReference *SpanContext // self references skip the ID generation
for _, ref := range options.References {
ctx, ok := ref.ReferencedContext.(SpanContext)
if !ok {
Expand All @@ -234,6 +235,13 @@ func (t *Tracer) startSpanWithOptions(
continue
}
references = append(references, Reference{Type: ref.Type, Context: ctx})

// if
if ref.Type == SelfRef {
selfReference = &ctx

This comment has been minimized.

Copy link
@yurishkuro

yurishkuro Aug 3, 2019

I would prefer to do without an extra pointer, e.g. store ref.ReferencedContext instead

break

This comment has been minimized.

Copy link
@dm03514

dm03514 Aug 3, 2019

Author Owner

Does it make sense to do reference building if this is a self span? I'm assuming that becomes the responsibility of the client?

This comment has been minimized.

Copy link
@yurishkuro

yurishkuro Aug 3, 2019

yes, it should continue the loop because a span may have multiple happens-before ancestors

}

if !hasParent {
parent = ctx
hasParent = ref.Type == opentracing.ChildOfRef
Expand All @@ -253,42 +261,46 @@ func (t *Tracer) startSpanWithOptions(
var samplerTags []Tag
var ctx SpanContext
newTrace := false
if !hasParent || !parent.IsValid() {
newTrace = true
ctx.traceID.Low = t.randomID()
if t.options.gen128Bit {
ctx.traceID.High = t.options.highTraceIDGenerator()
}
ctx.spanID = SpanID(ctx.traceID.Low)
ctx.parentID = 0
ctx.flags = byte(0)
if hasParent && parent.isDebugIDContainerOnly() && t.isDebugAllowed(operationName) {
ctx.flags |= (flagSampled | flagDebug)
samplerTags = []Tag{{key: JaegerDebugHeader, value: parent.debugID}}
} else if sampled, tags := t.sampler.IsSampled(ctx.traceID, operationName); sampled {
ctx.flags |= flagSampled
samplerTags = tags
}
} else {
ctx.traceID = parent.traceID
if rpcServer && t.options.zipkinSharedRPCSpan {
// Support Zipkin's one-span-per-RPC model
ctx.spanID = parent.spanID
ctx.parentID = parent.parentID
if selfReference == nil {

This comment has been minimized.

Copy link
@dm03514

dm03514 Aug 3, 2019

Author Owner

Only perform ID generation if no self reference is provided

This comment has been minimized.

Copy link
@yurishkuro

yurishkuro Aug 3, 2019

this is hard to review since it's not a diff and I can't seem to find a switch to ignore whitespace changes. I assume below is just a verbatim copy of the original logic indented inside the if statement.

if !hasParent || !parent.IsValid() {
newTrace = true
ctx.traceID.Low = t.randomID()
if t.options.gen128Bit {
ctx.traceID.High = t.options.highTraceIDGenerator()
}
ctx.spanID = SpanID(ctx.traceID.Low)
ctx.parentID = 0
ctx.flags = byte(0)
if hasParent && parent.isDebugIDContainerOnly() && t.isDebugAllowed(operationName) {
ctx.flags |= (flagSampled | flagDebug)
samplerTags = []Tag{{key: JaegerDebugHeader, value: parent.debugID}}
} else if sampled, tags := t.sampler.IsSampled(ctx.traceID, operationName); sampled {
ctx.flags |= flagSampled

This comment has been minimized.

Copy link
@dm03514

dm03514 Aug 3, 2019

Author Owner

Should this be calculated for the Self reference?

This comment has been minimized.

Copy link
@yurishkuro

yurishkuro Aug 3, 2019

good question, I don't know. Given the use case (creating spans based on external, already logged data), it seem s sampling has already happened, so it could be left to the code that creates the SelfRef.

samplerTags = tags
}
} else {
ctx.spanID = SpanID(t.randomID())
ctx.parentID = parent.spanID
ctx.traceID = parent.traceID
if rpcServer && t.options.zipkinSharedRPCSpan {
// Support Zipkin's one-span-per-RPC model
ctx.spanID = parent.spanID
ctx.parentID = parent.parentID
} else {
ctx.spanID = SpanID(t.randomID())
ctx.parentID = parent.spanID
}
ctx.flags = parent.flags
}
ctx.flags = parent.flags
}
if hasParent {
// copy baggage items
if l := len(parent.baggage); l > 0 {
ctx.baggage = make(map[string]string, len(parent.baggage))
for k, v := range parent.baggage {
ctx.baggage[k] = v
if hasParent {
// copy baggage items
if l := len(parent.baggage); l > 0 {
ctx.baggage = make(map[string]string, len(parent.baggage))
for k, v := range parent.baggage {
ctx.baggage[k] = v
}
}
}
} else {
ctx = *selfReference

This comment has been minimized.

Copy link
@dm03514

dm03514 Aug 3, 2019

Author Owner

Self referenced span uses the reference that the client provided

}

sp := t.newSpan()
Expand Down

0 comments on commit bf5f342

Please sign in to comment.