From 942713a02da3dddde5d3c1fae56c09087b7bce5b Mon Sep 17 00:00:00 2001 From: Krzesimir Nowak Date: Tue, 4 Feb 2020 17:55:03 +0100 Subject: [PATCH] Replace span relationship with a potentially remote parent context (#451) This PR removes the non-compliant ChildOf and FollowsFrom interfaces and the Relation type, which were inherited from OpenTracing via the initial prototype. Instead allow adding a span context to the go context as a remote span context and use a simple algorithm for figuring out an actual parent of the new span, which was proposed for the OpenTelemetry specification. Also add a way to ignore current span and remote span context in go context, so we can force the tracer to create a new root span - a span with a new trace ID. That required some moderate changes in the opentracing bridge - first reference with ChildOfRef reference type becomes a local parent, the rest become links. This also fixes links handling in the meantime. The downside of the approach proposed here is that we can only set the remote parent when creating a span through the opentracing API. Co-authored-by: Joshua MacDonald --- api/testharness/harness.go | 34 ++++-- api/trace/api.go | 40 ++----- api/trace/{current.go => context.go} | 27 ++++- .../{current_test.go => context_test.go} | 0 .../testtrace/b3_propagator_benchmark_test.go | 5 +- api/trace/testtrace/b3_propagator_test.go | 5 +- ...trace_context_propagator_benchmark_test.go | 4 +- .../trace_context_propagator_test.go | 3 +- api/trace/testtrace/tracer.go | 12 +- api/trace/testtrace/tracer_test.go | 100 ++++++++++++---- bridge/opentracing/bridge.go | 96 ++++++++------- bridge/opentracing/internal/mock.go | 11 +- example/grpc/middleware/tracing/tracing.go | 3 +- example/http-stackdriver/server/server.go | 3 +- example/http/server/server.go | 3 +- internal/trace/mock_tracer.go | 14 ++- internal/trace/parent/parent.go | 41 +++++++ plugin/othttp/handler.go | 7 +- sdk/trace/batch_span_processor_test.go | 3 +- sdk/trace/simple_span_processor_test.go | 3 +- sdk/trace/trace_test.go | 111 ++++++------------ sdk/trace/tracer.go | 26 ++-- 22 files changed, 320 insertions(+), 231 deletions(-) rename api/trace/{current.go => context.go} (52%) rename api/trace/{current_test.go => context_test.go} (100%) create mode 100644 internal/trace/parent/parent.go diff --git a/api/testharness/harness.go b/api/testharness/harness.go index e82c1cc1b13..80e62476621 100644 --- a/api/testharness/harness.go +++ b/api/testharness/harness.go @@ -125,37 +125,55 @@ func (h *Harness) TestTracer(subjectFactory func() trace.Tracer) { e.Expect(csc.SpanID).NotToEqual(psc.SpanID) }) - t.Run("propagates a parent's trace ID through `ChildOf`", func(t *testing.T) { + t.Run("ignores parent's trace ID when new root is requested", func(t *testing.T) { t.Parallel() e := matchers.NewExpecter(t) subject := subjectFactory() - _, parent := subject.Start(context.Background(), "parent") - _, child := subject.Start(context.Background(), "child", trace.ChildOf(parent.SpanContext())) + ctx, parent := subject.Start(context.Background(), "parent") + _, child := subject.Start(ctx, "child", trace.WithNewRoot()) psc := parent.SpanContext() csc := child.SpanContext() - e.Expect(csc.TraceID).ToEqual(psc.TraceID) + e.Expect(csc.TraceID).NotToEqual(psc.TraceID) e.Expect(csc.SpanID).NotToEqual(psc.SpanID) }) - t.Run("propagates a parent's trace ID through `FollowsFrom`", func(t *testing.T) { + t.Run("propagates remote parent's trace ID through the context", func(t *testing.T) { t.Parallel() e := matchers.NewExpecter(t) subject := subjectFactory() - _, parent := subject.Start(context.Background(), "parent") - _, child := subject.Start(context.Background(), "child", trace.FollowsFrom(parent.SpanContext())) + _, remoteParent := subject.Start(context.Background(), "remote parent") + parentCtx := trace.ContextWithRemoteSpanContext(context.Background(), remoteParent.SpanContext()) + _, child := subject.Start(parentCtx, "child") - psc := parent.SpanContext() + psc := remoteParent.SpanContext() csc := child.SpanContext() e.Expect(csc.TraceID).ToEqual(psc.TraceID) e.Expect(csc.SpanID).NotToEqual(psc.SpanID) }) + + t.Run("ignores remote parent's trace ID when new root is requested", func(t *testing.T) { + t.Parallel() + + e := matchers.NewExpecter(t) + subject := subjectFactory() + + _, remoteParent := subject.Start(context.Background(), "remote parent") + parentCtx := trace.ContextWithRemoteSpanContext(context.Background(), remoteParent.SpanContext()) + _, child := subject.Start(parentCtx, "child", trace.WithNewRoot()) + + psc := remoteParent.SpanContext() + csc := child.SpanContext() + + e.Expect(csc.TraceID).NotToEqual(psc.TraceID) + e.Expect(csc.SpanID).NotToEqual(psc.SpanID) + }) }) h.t.Run("#WithSpan", func(t *testing.T) { diff --git a/api/trace/api.go b/api/trace/api.go index 11c7f541427..fc337098c89 100644 --- a/api/trace/api.go +++ b/api/trace/api.go @@ -100,26 +100,11 @@ type StartConfig struct { Attributes []core.KeyValue StartTime time.Time Links []Link - Relation Relation Record bool + NewRoot bool SpanKind SpanKind } -// Relation is used to establish relationship between newly created span and the -// other span. The other span could be related as a parent or linked or any other -// future relationship type. -type Relation struct { - core.SpanContext - RelationshipType -} - -type RelationshipType int - -const ( - ChildOfRelationship RelationshipType = iota - FollowsFromRelationship -) - // Link is used to establish relationship between two spans within the same Trace or // across different Traces. Few examples of Link usage. // 1. Batch Processing: A batch of elements may contain elements associated with one @@ -216,23 +201,14 @@ func WithRecord() StartOption { } } -// ChildOf. TODO: do we need this?. -func ChildOf(sc core.SpanContext) StartOption { - return func(c *StartConfig) { - c.Relation = Relation{ - SpanContext: sc, - RelationshipType: ChildOfRelationship, - } - } -} - -// FollowsFrom. TODO: do we need this?. -func FollowsFrom(sc core.SpanContext) StartOption { +// WithNewRoot specifies that the current span or remote span context +// in context passed to `Start` should be ignored when deciding about +// a parent, which effectively means creating a span with new trace +// ID. The current span and the remote span context may be added as +// links to the span by the implementation. +func WithNewRoot() StartOption { return func(c *StartConfig) { - c.Relation = Relation{ - SpanContext: sc, - RelationshipType: FollowsFromRelationship, - } + c.NewRoot = true } } diff --git a/api/trace/current.go b/api/trace/context.go similarity index 52% rename from api/trace/current.go rename to api/trace/context.go index 55cd2d56528..66b003682fb 100644 --- a/api/trace/current.go +++ b/api/trace/context.go @@ -16,19 +16,42 @@ package trace import ( "context" + + "go.opentelemetry.io/otel/api/core" ) -type currentSpanKeyType struct{} +type traceContextKeyType int -var currentSpanKey = ¤tSpanKeyType{} +const ( + currentSpanKey traceContextKeyType = iota + remoteContextKey +) +// ContextWithSpan creates a new context with a current span set to +// the passed span. func ContextWithSpan(ctx context.Context, span Span) context.Context { return context.WithValue(ctx, currentSpanKey, span) } +// SpanFromContext returns the current span stored in the context. func SpanFromContext(ctx context.Context) Span { if span, has := ctx.Value(currentSpanKey).(Span); has { return span } return NoopSpan{} } + +// ContextWithRemoteSpanContext creates a new context with a remote +// span context set to the passed span context. +func ContextWithRemoteSpanContext(ctx context.Context, sc core.SpanContext) context.Context { + return context.WithValue(ctx, remoteContextKey, sc) +} + +// RemoteSpanContextFromContext returns the remote span context stored +// in the context. +func RemoteSpanContextFromContext(ctx context.Context) core.SpanContext { + if sc, ok := ctx.Value(remoteContextKey).(core.SpanContext); ok { + return sc + } + return core.EmptySpanContext() +} diff --git a/api/trace/current_test.go b/api/trace/context_test.go similarity index 100% rename from api/trace/current_test.go rename to api/trace/context_test.go diff --git a/api/trace/testtrace/b3_propagator_benchmark_test.go b/api/trace/testtrace/b3_propagator_benchmark_test.go index 930b674d70f..b6c7c7117e5 100644 --- a/api/trace/testtrace/b3_propagator_benchmark_test.go +++ b/api/trace/testtrace/b3_propagator_benchmark_test.go @@ -102,10 +102,9 @@ func BenchmarkInjectB3(b *testing.B) { req, _ := http.NewRequest("GET", "http://example.com", nil) ctx := context.Background() if tt.parentSc.IsValid() { - ctx, _ = mockTracer.Start(ctx, "inject", trace.ChildOf(tt.parentSc)) - } else { - ctx, _ = mockTracer.Start(ctx, "inject") + ctx = trace.ContextWithRemoteSpanContext(ctx, tt.parentSc) } + ctx, _ = mockTracer.Start(ctx, "inject") b.ReportAllocs() b.ResetTimer() for i := 0; i < b.N; i++ { diff --git a/api/trace/testtrace/b3_propagator_test.go b/api/trace/testtrace/b3_propagator_test.go index 444dfccdabc..827e1c0ed2d 100644 --- a/api/trace/testtrace/b3_propagator_test.go +++ b/api/trace/testtrace/b3_propagator_test.go @@ -104,10 +104,9 @@ func TestInjectB3(t *testing.T) { req, _ := http.NewRequest("GET", "http://example.com", nil) ctx := context.Background() if tt.parentSc.IsValid() { - ctx, _ = mockTracer.Start(ctx, "inject", trace.ChildOf(tt.parentSc)) - } else { - ctx, _ = mockTracer.Start(ctx, "inject") + ctx = trace.ContextWithRemoteSpanContext(ctx, tt.parentSc) } + ctx, _ = mockTracer.Start(ctx, "inject") propagator.Inject(ctx, req.Header) for h, v := range tt.wantHeaders { diff --git a/api/trace/testtrace/trace_context_propagator_benchmark_test.go b/api/trace/testtrace/trace_context_propagator_benchmark_test.go index ba688930fa8..f4340b695d4 100644 --- a/api/trace/testtrace/trace_context_propagator_benchmark_test.go +++ b/api/trace/testtrace/trace_context_propagator_benchmark_test.go @@ -38,8 +38,8 @@ func injectSubBenchmarks(b *testing.B, fn func(context.Context, *testing.B)) { SpanID: spanID, TraceFlags: core.TraceFlagsSampled, } - ctx := context.Background() - ctx, _ = mockTracer.Start(ctx, "inject", trace.ChildOf(sc)) + ctx := trace.ContextWithRemoteSpanContext(context.Background(), sc) + ctx, _ = mockTracer.Start(ctx, "inject") fn(ctx, b) }) diff --git a/api/trace/testtrace/trace_context_propagator_test.go b/api/trace/testtrace/trace_context_propagator_test.go index ba99c78c8f3..5711c2403f7 100644 --- a/api/trace/testtrace/trace_context_propagator_test.go +++ b/api/trace/testtrace/trace_context_propagator_test.go @@ -273,7 +273,8 @@ func TestInjectTraceContextToHTTPReq(t *testing.T) { req, _ := http.NewRequest("GET", "http://example.com", nil) ctx := context.Background() if tt.sc.IsValid() { - ctx, _ = mockTracer.Start(ctx, "inject", trace.ChildOf(tt.sc)) + ctx = trace.ContextWithRemoteSpanContext(ctx, tt.sc) + ctx, _ = mockTracer.Start(ctx, "inject") } propagator.Inject(ctx, req.Header) diff --git a/api/trace/testtrace/tracer.go b/api/trace/testtrace/tracer.go index deb3221bf04..5295a138ea5 100644 --- a/api/trace/testtrace/tracer.go +++ b/api/trace/testtrace/tracer.go @@ -21,6 +21,8 @@ import ( "go.opentelemetry.io/otel/api/core" "go.opentelemetry.io/otel/api/trace" + + "go.opentelemetry.io/otel/internal/trace/parent" ) var _ trace.Tracer = (*Tracer)(nil) @@ -52,10 +54,9 @@ func (t *Tracer) Start(ctx context.Context, name string, opts ...trace.StartOpti var traceID core.TraceID var parentSpanID core.SpanID - if parentSpanContext := c.Relation.SpanContext; parentSpanContext.IsValid() { - traceID = parentSpanContext.TraceID - parentSpanID = parentSpanContext.SpanID - } else if parentSpanContext := trace.SpanFromContext(ctx).SpanContext(); parentSpanContext.IsValid() { + parentSpanContext, _, links := parent.GetSpanContextAndLinks(ctx, c.NewRoot) + + if parentSpanContext.IsValid() { traceID = parentSpanContext.TraceID parentSpanID = parentSpanContext.SpanID } else { @@ -86,6 +87,9 @@ func (t *Tracer) Start(ctx context.Context, name string, opts ...trace.StartOpti span.SetName(name) span.SetAttributes(c.Attributes...) + for _, link := range links { + span.links[link.SpanContext] = link.Attributes + } for _, link := range c.Links { span.links[link.SpanContext] = link.Attributes } diff --git a/api/trace/testtrace/tracer_test.go b/api/trace/testtrace/tracer_test.go index 3c7907fd8f1..a4daa775450 100644 --- a/api/trace/testtrace/tracer_test.go +++ b/api/trace/testtrace/tracer_test.go @@ -21,6 +21,7 @@ import ( "time" "go.opentelemetry.io/otel/api/core" + "go.opentelemetry.io/otel/api/key" "go.opentelemetry.io/otel/api/testharness" "go.opentelemetry.io/otel/api/trace" "go.opentelemetry.io/otel/api/trace/testtrace" @@ -74,17 +75,17 @@ func TestTracer(t *testing.T) { e.Expect(attributes[attr2.Key]).ToEqual(attr2.Value) }) - t.Run("uses the parent's span context from ChildOf", func(t *testing.T) { + t.Run("uses the current span from context as parent", func(t *testing.T) { t.Parallel() e := matchers.NewExpecter(t) subject := testtrace.NewTracer() - _, parent := subject.Start(context.Background(), "parent") - parentSpanContext := parent.SpanContext() + parent, parentSpan := subject.Start(context.Background(), "parent") + parentSpanContext := parentSpan.SpanContext() - _, span := subject.Start(context.Background(), "child", trace.ChildOf(parentSpanContext)) + _, span := subject.Start(parent, "child") testSpan, ok := span.(*testtrace.Span) e.Expect(ok).ToBeTrue() @@ -95,18 +96,19 @@ func TestTracer(t *testing.T) { e.Expect(testSpan.ParentSpanID()).ToEqual(parentSpanContext.SpanID) }) - t.Run("defers to ChildOf if the provided context also contains a parent span", func(t *testing.T) { + t.Run("uses the current span from context as parent, even if it has remote span context", func(t *testing.T) { t.Parallel() e := matchers.NewExpecter(t) subject := testtrace.NewTracer() - _, parent := subject.Start(context.Background(), "parent") - parentSpanContext := parent.SpanContext() + parent, parentSpan := subject.Start(context.Background(), "parent") + _, remoteParentSpan := subject.Start(context.Background(), "remote not-a-parent") + parent = trace.ContextWithRemoteSpanContext(parent, remoteParentSpan.SpanContext()) + parentSpanContext := parentSpan.SpanContext() - ctx, _ := subject.Start(context.Background(), "should be ignored") - _, span := subject.Start(ctx, "child", trace.ChildOf(parentSpanContext)) + _, span := subject.Start(parent, "child") testSpan, ok := span.(*testtrace.Span) e.Expect(ok).ToBeTrue() @@ -117,47 +119,101 @@ func TestTracer(t *testing.T) { e.Expect(testSpan.ParentSpanID()).ToEqual(parentSpanContext.SpanID) }) - t.Run("uses the parent's span context from FollowsFrom", func(t *testing.T) { + t.Run("uses the remote span context from context as parent, if current span is missing", func(t *testing.T) { t.Parallel() e := matchers.NewExpecter(t) subject := testtrace.NewTracer() - _, parent := subject.Start(context.Background(), "parent") - parentSpanContext := parent.SpanContext() + _, remoteParentSpan := subject.Start(context.Background(), "remote parent") + parent := trace.ContextWithRemoteSpanContext(context.Background(), remoteParentSpan.SpanContext()) + remoteParentSpanContext := remoteParentSpan.SpanContext() - _, span := subject.Start(context.Background(), "child", trace.FollowsFrom(parentSpanContext)) + _, span := subject.Start(parent, "child") testSpan, ok := span.(*testtrace.Span) e.Expect(ok).ToBeTrue() childSpanContext := testSpan.SpanContext() - e.Expect(childSpanContext.TraceID).ToEqual(parentSpanContext.TraceID) + e.Expect(childSpanContext.TraceID).ToEqual(remoteParentSpanContext.TraceID) + e.Expect(childSpanContext.SpanID).NotToEqual(remoteParentSpanContext.SpanID) + e.Expect(testSpan.ParentSpanID()).ToEqual(remoteParentSpanContext.SpanID) + }) + + t.Run("creates new root when both current span and remote span context are missing", func(t *testing.T) { + t.Parallel() + + e := matchers.NewExpecter(t) + + subject := testtrace.NewTracer() + + _, parentSpan := subject.Start(context.Background(), "not-a-parent") + _, remoteParentSpan := subject.Start(context.Background(), "remote not-a-parent") + parentSpanContext := parentSpan.SpanContext() + remoteParentSpanContext := remoteParentSpan.SpanContext() + + _, span := subject.Start(context.Background(), "child") + + testSpan, ok := span.(*testtrace.Span) + e.Expect(ok).ToBeTrue() + + childSpanContext := testSpan.SpanContext() + e.Expect(childSpanContext.TraceID).NotToEqual(parentSpanContext.TraceID) + e.Expect(childSpanContext.TraceID).NotToEqual(remoteParentSpanContext.TraceID) e.Expect(childSpanContext.SpanID).NotToEqual(parentSpanContext.SpanID) - e.Expect(testSpan.ParentSpanID()).ToEqual(parentSpanContext.SpanID) + e.Expect(childSpanContext.SpanID).NotToEqual(remoteParentSpanContext.SpanID) + e.Expect(testSpan.ParentSpanID().IsValid()).ToBeFalse() }) - t.Run("defers to FollowsFrom if the provided context also contains a parent span", func(t *testing.T) { + t.Run("creates new root when requested, even if both current span and remote span context are in context", func(t *testing.T) { t.Parallel() e := matchers.NewExpecter(t) subject := testtrace.NewTracer() - _, parent := subject.Start(context.Background(), "parent") - parentSpanContext := parent.SpanContext() + parentCtx, parentSpan := subject.Start(context.Background(), "not-a-parent") + _, remoteParentSpan := subject.Start(context.Background(), "remote not-a-parent") + parentSpanContext := parentSpan.SpanContext() + remoteParentSpanContext := remoteParentSpan.SpanContext() + parentCtx = trace.ContextWithRemoteSpanContext(parentCtx, remoteParentSpanContext) - ctx, _ := subject.Start(context.Background(), "should be ignored") - _, span := subject.Start(ctx, "child", trace.FollowsFrom(parentSpanContext)) + _, span := subject.Start(parentCtx, "child", trace.WithNewRoot()) testSpan, ok := span.(*testtrace.Span) e.Expect(ok).ToBeTrue() childSpanContext := testSpan.SpanContext() - e.Expect(childSpanContext.TraceID).ToEqual(parentSpanContext.TraceID) + e.Expect(childSpanContext.TraceID).NotToEqual(parentSpanContext.TraceID) + e.Expect(childSpanContext.TraceID).NotToEqual(remoteParentSpanContext.TraceID) e.Expect(childSpanContext.SpanID).NotToEqual(parentSpanContext.SpanID) - e.Expect(testSpan.ParentSpanID()).ToEqual(parentSpanContext.SpanID) + e.Expect(childSpanContext.SpanID).NotToEqual(remoteParentSpanContext.SpanID) + e.Expect(testSpan.ParentSpanID().IsValid()).ToBeFalse() + + expectedLinks := []trace.Link{ + { + SpanContext: parentSpanContext, + Attributes: []core.KeyValue{ + key.String("ignored-on-demand", "current"), + }, + }, + { + SpanContext: remoteParentSpanContext, + Attributes: []core.KeyValue{ + key.String("ignored-on-demand", "remote"), + }, + }, + } + tsLinks := testSpan.Links() + gotLinks := make([]trace.Link, 0, len(tsLinks)) + for sc, attributes := range tsLinks { + gotLinks = append(gotLinks, trace.Link{ + SpanContext: sc, + Attributes: attributes, + }) + } + e.Expect(gotLinks).ToMatchInAnyOrder(expectedLinks) }) t.Run("uses the links provided through LinkedTo", func(t *testing.T) { diff --git a/bridge/opentracing/bridge.go b/bridge/opentracing/bridge.go index 62a0ceb2ec9..7583b604895 100644 --- a/bridge/opentracing/bridge.go +++ b/bridge/opentracing/bridge.go @@ -28,6 +28,7 @@ import ( otlog "github.com/opentracing/opentracing-go/log" otelcore "go.opentelemetry.io/otel/api/core" + otelkey "go.opentelemetry.io/otel/api/key" oteltrace "go.opentelemetry.io/otel/api/trace" "go.opentelemetry.io/otel/bridge/opentracing/migration" @@ -309,15 +310,18 @@ func (t *BridgeTracer) StartSpan(operationName string, opts ...ot.StartSpanOptio for _, opt := range opts { opt.Apply(&sso) } - // TODO: handle links, needs SpanData to be in the API first? - bRelation, _ := otSpanReferencesToBridgeRelationAndLinks(sso.References) + parentBridgeSC, links := otSpanReferencesToParentAndLinks(sso.References) attributes, kind, hadTrueErrorTag := otTagsToOtelAttributesKindAndError(sso.Tags) checkCtx := migration.WithDeferredSetup(context.Background()) + if parentBridgeSC != nil { + checkCtx = oteltrace.ContextWithRemoteSpanContext(checkCtx, parentBridgeSC.otelSpanContext) + } checkCtx2, otelSpan := t.setTracer.tracer().Start(checkCtx, operationName, func(opts *oteltrace.StartConfig) { opts.Attributes = attributes opts.StartTime = sso.StartTime - opts.Relation = bRelation.ToOtelRelation() + opts.Links = links opts.Record = true + opts.NewRoot = false opts.SpanKind = kind }) if checkCtx != checkCtx2 { @@ -328,9 +332,15 @@ func (t *BridgeTracer) StartSpan(operationName string, opts ...ot.StartSpanOptio if hadTrueErrorTag { otelSpan.SetStatus(codes.Unknown) } + // One does not simply pass a concrete pointer to function + // that takes some interface. In case of passing nil concrete + // pointer, we get an interface with non-nil type (because the + // pointer type is known) and a nil value. Which means + // interface is not nil, but calling some interface function + // on it will most likely result in nil pointer dereference. var otSpanContext ot.SpanContext - if bRelation.spanContext != nil { - otSpanContext = bRelation.spanContext + if parentBridgeSC != nil { + otSpanContext = parentBridgeSC } sctx := newBridgeSpanContext(otelSpan.SpanContext(), otSpanContext) span := &bridgeSpan{ @@ -440,53 +450,59 @@ func otTagToOtelCoreKey(k string) otelcore.Key { return otelcore.Key(k) } -type bridgeRelation struct { - spanContext *bridgeSpanContext - relationshipType oteltrace.RelationshipType -} - -func (r bridgeRelation) ToOtelRelation() oteltrace.Relation { - if r.spanContext == nil { - return oteltrace.Relation{} - } - return oteltrace.Relation{ - SpanContext: r.spanContext.otelSpanContext, - RelationshipType: r.relationshipType, +func otSpanReferencesToParentAndLinks(references []ot.SpanReference) (*bridgeSpanContext, []oteltrace.Link) { + var ( + parent *bridgeSpanContext + links []oteltrace.Link + ) + for _, reference := range references { + bridgeSC, ok := reference.ReferencedContext.(*bridgeSpanContext) + if !ok { + // We ignore foreign ot span contexts, + // sorry. We have no way of getting any + // TraceID and SpanID out of it for form a + // otelcore.SpanContext for otelcore.Link. And + // we can't make it a parent - it also needs a + // valid otelcore.SpanContext. + continue + } + if parent != nil { + links = append(links, otSpanReferenceToOtelLink(bridgeSC, reference.Type)) + } else { + if reference.Type == ot.ChildOfRef { + parent = bridgeSC + } else { + links = append(links, otSpanReferenceToOtelLink(bridgeSC, reference.Type)) + } + } } + return parent, links } -func otSpanReferencesToBridgeRelationAndLinks(references []ot.SpanReference) (bridgeRelation, []*bridgeSpanContext) { - if len(references) == 0 { - return bridgeRelation{}, nil - } - first := references[0] - relation := bridgeRelation{ - spanContext: mustGetBridgeSpanContext(first.ReferencedContext), - relationshipType: otSpanReferenceTypeToOtelRelationshipType(first.Type), +func otSpanReferenceToOtelLink(bridgeSC *bridgeSpanContext, refType ot.SpanReferenceType) oteltrace.Link { + return oteltrace.Link{ + SpanContext: bridgeSC.otelSpanContext, + Attributes: otSpanReferenceTypeToOtelLinkAttributes(refType), } - var links []*bridgeSpanContext - for _, reference := range references[1:] { - links = append(links, mustGetBridgeSpanContext(reference.ReferencedContext)) - } - return relation, links } -func mustGetBridgeSpanContext(ctx ot.SpanContext) *bridgeSpanContext { - ourCtx, ok := ctx.(*bridgeSpanContext) - if !ok { - panic("oops, some foreign span context here") +func otSpanReferenceTypeToOtelLinkAttributes(refType ot.SpanReferenceType) []otelcore.KeyValue { + return []otelcore.KeyValue{ + otelkey.String("ot-span-reference-type", otSpanReferenceTypeToString(refType)), } - return ourCtx } -func otSpanReferenceTypeToOtelRelationshipType(srt ot.SpanReferenceType) oteltrace.RelationshipType { - switch srt { +func otSpanReferenceTypeToString(refType ot.SpanReferenceType) string { + switch refType { case ot.ChildOfRef: - return oteltrace.ChildOfRelationship + // "extra", because first child-of reference is used + // as a parent, so this function isn't even called for + // it. + return "extra-child-of" case ot.FollowsFromRef: - return oteltrace.FollowsFromRelationship + return "follows-from-ref" default: - panic("fix yer code, it uses bogus opentracing reference type") + return fmt.Sprintf("unknown-%d", int(refType)) } } diff --git a/bridge/opentracing/internal/mock.go b/bridge/opentracing/internal/mock.go index 53411424806..187e6fc0584 100644 --- a/bridge/opentracing/internal/mock.go +++ b/bridge/opentracing/internal/mock.go @@ -26,6 +26,7 @@ import ( otelcorrelation "go.opentelemetry.io/otel/api/correlation" otelkey "go.opentelemetry.io/otel/api/key" oteltrace "go.opentelemetry.io/otel/api/trace" + otelparent "go.opentelemetry.io/otel/internal/trace/parent" "go.opentelemetry.io/otel/bridge/opentracing/migration" ) @@ -146,14 +147,8 @@ func (t *MockTracer) getParentSpanID(ctx context.Context, spanOpts *oteltrace.St } func (t *MockTracer) getParentSpanContext(ctx context.Context, spanOpts *oteltrace.StartConfig) otelcore.SpanContext { - if spanOpts.Relation.RelationshipType == oteltrace.ChildOfRelationship && - spanOpts.Relation.SpanContext.IsValid() { - return spanOpts.Relation.SpanContext - } - if parentSpanContext := oteltrace.SpanFromContext(ctx).SpanContext(); parentSpanContext.IsValid() { - return parentSpanContext - } - return otelcore.EmptySpanContext() + spanCtx, _, _ := otelparent.GetSpanContextAndLinks(ctx, spanOpts.NewRoot) + return spanCtx } func (t *MockTracer) getSpanID() otelcore.SpanID { diff --git a/example/grpc/middleware/tracing/tracing.go b/example/grpc/middleware/tracing/tracing.go index ce65955d278..b3d4f812133 100644 --- a/example/grpc/middleware/tracing/tracing.go +++ b/example/grpc/middleware/tracing/tracing.go @@ -50,10 +50,9 @@ func UnaryServerInterceptor(ctx context.Context, req interface{}, info *grpc.Una tr := global.TraceProvider().Tracer("example/grpc") ctx, span := tr.Start( - ctx, + trace.ContextWithRemoteSpanContext(ctx, spanCtx), "hello-api-op", trace.WithAttributes(serverSpanAttrs...), - trace.ChildOf(spanCtx), trace.WithSpanKind(trace.SpanKindServer), ) defer span.End() diff --git a/example/http-stackdriver/server/server.go b/example/http-stackdriver/server/server.go index bbda5a30c67..601a2f1535a 100644 --- a/example/http-stackdriver/server/server.go +++ b/example/http-stackdriver/server/server.go @@ -63,10 +63,9 @@ func main() { }))) ctx, span := tr.Start( - req.Context(), + trace.ContextWithRemoteSpanContext(req.Context(), spanCtx), "hello", trace.WithAttributes(attrs...), - trace.ChildOf(spanCtx), ) defer span.End() diff --git a/example/http/server/server.go b/example/http/server/server.go index 94b958f3e2a..3c7f7970270 100644 --- a/example/http/server/server.go +++ b/example/http/server/server.go @@ -57,10 +57,9 @@ func main() { }))) ctx, span := tr.Start( - req.Context(), + trace.ContextWithRemoteSpanContext(req.Context(), spanCtx), "hello", trace.WithAttributes(attrs...), - trace.ChildOf(spanCtx), ) defer span.End() diff --git a/internal/trace/mock_tracer.go b/internal/trace/mock_tracer.go index 98d973f8b5b..0c680ef3c8b 100644 --- a/internal/trace/mock_tracer.go +++ b/internal/trace/mock_tracer.go @@ -22,6 +22,7 @@ import ( "go.opentelemetry.io/otel/api/core" apitrace "go.opentelemetry.io/otel/api/trace" + "go.opentelemetry.io/otel/internal/trace/parent" ) // MockTracer is a simple tracer used for testing purpose only. @@ -45,9 +46,9 @@ func (mt *MockTracer) WithSpan(ctx context.Context, name string, body func(conte return body(ctx) } -// Start starts a MockSpan. It creates a new Span based on Relation SpanContext option. -// TracdID is used from Relation Span Context and SpanID is assigned. -// If Relation SpanContext option is not specified then random TraceID is used. +// Start starts a MockSpan. It creates a new Span based on Parent SpanContext option. +// TracdID is used from Parent Span Context and SpanID is assigned. +// If Parent SpanContext option is not specified then random TraceID is used. // No other options are supported. func (mt *MockTracer) Start(ctx context.Context, name string, o ...apitrace.StartOption) (context.Context, apitrace.Span) { var opts apitrace.StartConfig @@ -56,14 +57,17 @@ func (mt *MockTracer) Start(ctx context.Context, name string, o ...apitrace.Star } var span *MockSpan var sc core.SpanContext - if !opts.Relation.SpanContext.IsValid() { + + parentSpanContext, _, _ := parent.GetSpanContextAndLinks(ctx, opts.NewRoot) + + if !parentSpanContext.IsValid() { sc = core.SpanContext{} _, _ = rand.Read(sc.TraceID[:]) if mt.Sampled { sc.TraceFlags = core.TraceFlagsSampled } } else { - sc = opts.Relation.SpanContext + sc = parentSpanContext } binary.BigEndian.PutUint64(sc.SpanID[:], atomic.AddUint64(mt.StartSpanID, 1)) diff --git a/internal/trace/parent/parent.go b/internal/trace/parent/parent.go new file mode 100644 index 00000000000..bda728699b2 --- /dev/null +++ b/internal/trace/parent/parent.go @@ -0,0 +1,41 @@ +package parent + +import ( + "context" + + "go.opentelemetry.io/otel/api/core" + "go.opentelemetry.io/otel/api/key" + "go.opentelemetry.io/otel/api/trace" +) + +func GetSpanContextAndLinks(ctx context.Context, ignoreContext bool) (core.SpanContext, bool, []trace.Link) { + lsctx := trace.SpanFromContext(ctx).SpanContext() + rsctx := trace.RemoteSpanContextFromContext(ctx) + + if ignoreContext { + links := addLinkIfValid(nil, lsctx, "current") + links = addLinkIfValid(links, rsctx, "remote") + + return core.EmptySpanContext(), false, links + } + if lsctx.IsValid() { + links := addLinkIfValid(nil, rsctx, "remote") + return lsctx, false, links + } + if rsctx.IsValid() { + return rsctx, true, nil + } + return core.EmptySpanContext(), false, nil +} + +func addLinkIfValid(links []trace.Link, sc core.SpanContext, kind string) []trace.Link { + if !sc.IsValid() { + return links + } + return append(links, trace.Link{ + SpanContext: sc, + Attributes: []core.KeyValue{ + key.String("ignored-on-demand", kind), + }, + }) +} diff --git a/plugin/othttp/handler.go b/plugin/othttp/handler.go index a1840e670a6..972809fc7f9 100644 --- a/plugin/othttp/handler.go +++ b/plugin/othttp/handler.go @@ -146,19 +146,20 @@ func (h *Handler) ServeHTTP(w http.ResponseWriter, r *http.Request) { // TODO: do something with the correlation context sc, _ := h.prop.Extract(r.Context(), r.Header) + ctx := r.Context() if sc.IsValid() { // not a valid span context, so no link / parent relationship to establish var opt trace.StartOption if h.public { // If the endpoint is a public endpoint, it should start a new trace // and incoming remote sctx should be added as a link. opt = trace.LinkedTo(sc) + opts = append(opts, opt) } else { // not a private endpoint, so assume child relationship - opt = trace.ChildOf(sc) + ctx = trace.ContextWithRemoteSpanContext(ctx, sc) } - opts = append(opts, opt) } - ctx, span := h.tracer.Start(r.Context(), h.operation, opts...) + ctx, span := h.tracer.Start(ctx, h.operation, opts...) defer span.End() readRecordFunc := func(int64) {} diff --git a/sdk/trace/batch_span_processor_test.go b/sdk/trace/batch_span_processor_test.go index 4c87894d1af..763317023b0 100644 --- a/sdk/trace/batch_span_processor_test.go +++ b/sdk/trace/batch_span_processor_test.go @@ -187,7 +187,8 @@ func generateSpan(t *testing.T, tr apitrace.Tracer, option testOption) { for i := 0; i < option.genNumSpans; i++ { binary.BigEndian.PutUint64(sc.TraceID[0:8], uint64(i+1)) - _, span := tr.Start(context.Background(), option.name, apitrace.ChildOf(sc)) + ctx := apitrace.ContextWithRemoteSpanContext(context.Background(), sc) + _, span := tr.Start(ctx, option.name) span.End() } } diff --git a/sdk/trace/simple_span_processor_test.go b/sdk/trace/simple_span_processor_test.go index 8d57f5cfb90..952c9c494d4 100644 --- a/sdk/trace/simple_span_processor_test.go +++ b/sdk/trace/simple_span_processor_test.go @@ -65,7 +65,8 @@ func TestSimpleSpanProcessorOnEnd(t *testing.T) { SpanID: sid, TraceFlags: 0x1, } - _, span := tr.Start(context.Background(), "OnEnd", apitrace.ChildOf(sc)) + ctx := apitrace.ContextWithRemoteSpanContext(context.Background(), sc) + _, span := tr.Start(ctx, "OnEnd") span.End() wantTraceID := tid diff --git a/sdk/trace/trace_test.go b/sdk/trace/trace_test.go index 9438343a215..1d5151393c5 100644 --- a/sdk/trace/trace_test.go +++ b/sdk/trace/trace_test.go @@ -175,7 +175,7 @@ func TestSampling(t *testing.T) { tr := p.Tracer("test") var sampled int for i := 0; i < total; i++ { - var opts []apitrace.StartOption + ctx := context.Background() if tc.parent { psc := core.SpanContext{ TraceID: idg.NewTraceID(), @@ -184,9 +184,9 @@ func TestSampling(t *testing.T) { if tc.sampledParent { psc.TraceFlags = core.TraceFlagsSampled } - opts = append(opts, apitrace.ChildOf(psc)) + ctx = apitrace.ContextWithRemoteSpanContext(ctx, psc) } - _, span := tr.Start(context.Background(), "test", opts...) + _, span := tr.Start(ctx, "test") if span.SpanContext().IsSampled() { sampled++ } @@ -208,21 +208,22 @@ func TestSampling(t *testing.T) { } } -func TestStartSpanWithChildOf(t *testing.T) { +func TestStartSpanWithParent(t *testing.T) { tp, _ := NewProvider() - tr := tp.Tracer("SpanWith ChildOf") + tr := tp.Tracer("SpanWithParent") + ctx := context.Background() sc1 := core.SpanContext{ TraceID: tid, SpanID: sid, TraceFlags: 0x0, } - _, s1 := tr.Start(context.Background(), "span1-unsampled-parent1", apitrace.ChildOf(sc1)) + _, s1 := tr.Start(apitrace.ContextWithRemoteSpanContext(ctx, sc1), "span1-unsampled-parent1") if err := checkChild(sc1, s1); err != nil { t.Error(err) } - _, s2 := tr.Start(context.Background(), "span2-unsampled-parent1", apitrace.ChildOf(sc1)) + _, s2 := tr.Start(apitrace.ContextWithRemoteSpanContext(ctx, sc1), "span2-unsampled-parent1") if err := checkChild(sc1, s2); err != nil { t.Error(err) } @@ -233,60 +234,18 @@ func TestStartSpanWithChildOf(t *testing.T) { TraceFlags: 0x1, //Tracestate: testTracestate, } - _, s3 := tr.Start(context.Background(), "span3-sampled-parent2", apitrace.ChildOf(sc2)) + _, s3 := tr.Start(apitrace.ContextWithRemoteSpanContext(ctx, sc2), "span3-sampled-parent2") if err := checkChild(sc2, s3); err != nil { t.Error(err) } - ctx, s4 := tr.Start(context.Background(), "span4-sampled-parent2", apitrace.ChildOf(sc2)) + ctx2, s4 := tr.Start(apitrace.ContextWithRemoteSpanContext(ctx, sc2), "span4-sampled-parent2") if err := checkChild(sc2, s4); err != nil { t.Error(err) } s4Sc := s4.SpanContext() - _, s5 := tr.Start(ctx, "span5-implicit-childof-span4") - if err := checkChild(s4Sc, s5); err != nil { - t.Error(err) - } -} - -func TestStartSpanWithFollowsFrom(t *testing.T) { - tp, _ := NewProvider() - tr := tp.Tracer("SpanWith FollowsFrom") - - sc1 := core.SpanContext{ - TraceID: tid, - SpanID: sid, - TraceFlags: 0x0, - } - _, s1 := tr.Start(context.Background(), "span1-unsampled-parent1", apitrace.FollowsFrom(sc1)) - if err := checkChild(sc1, s1); err != nil { - t.Error(err) - } - - _, s2 := tr.Start(context.Background(), "span2-unsampled-parent1", apitrace.FollowsFrom(sc1)) - if err := checkChild(sc1, s2); err != nil { - t.Error(err) - } - - sc2 := core.SpanContext{ - TraceID: tid, - SpanID: sid, - TraceFlags: 0x1, - //Tracestate: testTracestate, - } - _, s3 := tr.Start(context.Background(), "span3-sampled-parent2", apitrace.FollowsFrom(sc2)) - if err := checkChild(sc2, s3); err != nil { - t.Error(err) - } - - ctx, s4 := tr.Start(context.Background(), "span4-sampled-parent2", apitrace.FollowsFrom(sc2)) - if err := checkChild(sc2, s4); err != nil { - t.Error(err) - } - - s4Sc := s4.SpanContext() - _, s5 := tr.Start(ctx, "span5-implicit-childof-span4") + _, s5 := tr.Start(ctx2, "span5-implicit-childof-span4") if err := checkChild(s4Sc, s5); err != nil { t.Error(err) } @@ -574,15 +533,15 @@ func TestLinksOverLimit(t *testing.T) { func TestSetSpanName(t *testing.T) { te := &testExporter{} tp, _ := NewProvider(WithSyncer(te)) + ctx := context.Background() want := "SpanName-1" - _, span := tp.Tracer("SetSpanName").Start(context.Background(), "SpanName-1", - apitrace.ChildOf(core.SpanContext{ - TraceID: tid, - SpanID: sid, - TraceFlags: 1, - }), - ) + ctx = apitrace.ContextWithRemoteSpanContext(ctx, core.SpanContext{ + TraceID: tid, + SpanID: sid, + TraceFlags: 1, + }) + _, span := tp.Tracer("SetSpanName").Start(ctx, "SpanName-1") got, err := endSpan(te, span) if err != nil { t.Fatal(err) @@ -662,13 +621,15 @@ func startSpan(tp *Provider, trName string, args ...apitrace.StartOption) apitra } // startNamed Span is a test utility func that starts a span with a -// passed name and with ChildOf option. remote span context contains -// TraceFlags with sampled bit set. This allows the span to be -// automatically sampled. +// passed name and with remote span context as parent. The remote span +// context contains TraceFlags with sampled bit set. This allows the +// span to be automatically sampled. func startNamedSpan(tp *Provider, trName, name string, args ...apitrace.StartOption) apitrace.Span { - args = append(args, apitrace.ChildOf(remoteSpanContext()), apitrace.WithRecord()) + ctx := context.Background() + ctx = apitrace.ContextWithRemoteSpanContext(ctx, remoteSpanContext()) + args = append(args, apitrace.WithRecord()) _, span := tp.Tracer(trName).Start( - context.Background(), + ctx, name, args..., ) @@ -678,7 +639,7 @@ func startNamedSpan(tp *Provider, trName, name string, args ...apitrace.StartOpt // endSpan is a test utility function that ends the span in the context and // returns the exported export.SpanData. // It requires that span be sampled using one of these methods -// 1. Passing parent span context using ChildOf option +// 1. Passing parent span context in context // 2. Use WithSampler(AlwaysSample()) // 3. Configuring AlwaysSample() as default sampler // @@ -739,9 +700,10 @@ func TestEndSpanTwice(t *testing.T) { func TestStartSpanAfterEnd(t *testing.T) { spans := make(fakeExporter) tp, _ := NewProvider(WithConfig(Config{DefaultSampler: AlwaysSample()}), WithSyncer(spans)) + ctx := context.Background() tr := tp.Tracer("SpanAfterEnd") - ctx, span0 := tr.Start(context.Background(), "parent", apitrace.ChildOf(remoteSpanContext())) + ctx, span0 := tr.Start(apitrace.ContextWithRemoteSpanContext(ctx, remoteSpanContext()), "parent") ctx1, span1 := tr.Start(ctx, "span-1") span1.End() // Start a new span with the context containing span-1 @@ -852,17 +814,18 @@ func TestExecutionTracerTaskEnd(t *testing.T) { tID, _ := core.TraceIDFromHex("0102030405060708090a0b0c0d0e0f") sID, _ := core.SpanIDFromHex("0001020304050607") + ctx := context.Background() + ctx = apitrace.ContextWithRemoteSpanContext(ctx, + core.SpanContext{ + TraceID: tID, + SpanID: sID, + TraceFlags: 0, + }, + ) _, apiSpan = tr.Start( - context.Background(), + ctx, "foo", - apitrace.ChildOf( - core.SpanContext{ - TraceID: tID, - SpanID: sID, - TraceFlags: 0, - }, - ), ) s = apiSpan.(*span) s.executionTracerTaskEnd = executionTracerTaskEnd diff --git a/sdk/trace/tracer.go b/sdk/trace/tracer.go index 0fe7e0fc3b8..18d65078d39 100644 --- a/sdk/trace/tracer.go +++ b/sdk/trace/tracer.go @@ -17,8 +17,8 @@ package trace import ( "context" - "go.opentelemetry.io/otel/api/core" apitrace "go.opentelemetry.io/otel/api/trace" + "go.opentelemetry.io/otel/internal/trace/parent" ) type tracer struct { @@ -30,29 +30,23 @@ var _ apitrace.Tracer = &tracer{} func (tr *tracer) Start(ctx context.Context, name string, o ...apitrace.StartOption) (context.Context, apitrace.Span) { var opts apitrace.StartConfig - var parent core.SpanContext - var remoteParent bool - //TODO [rghetia] : Add new option for parent. If parent is configured then use that parent. for _, op := range o { op(&opts) } - if relation := opts.Relation; relation.SpanContext != core.EmptySpanContext() { - switch relation.RelationshipType { - case apitrace.ChildOfRelationship, apitrace.FollowsFromRelationship: - parent = relation.SpanContext - remoteParent = true - default: - // Future relationship types may have different behavior, - // e.g., adding a `Link` instead of setting the `parent` + parentSpanContext, remoteParent, links := parent.GetSpanContextAndLinks(ctx, opts.NewRoot) + + if p := apitrace.SpanFromContext(ctx); p != nil { + if sdkSpan, ok := p.(*span); ok { + sdkSpan.addChild() } - } else if p, ok := apitrace.SpanFromContext(ctx).(*span); ok { - p.addChild() - parent = p.spanContext } - span := startSpanInternal(tr, name, parent, remoteParent, opts) + span := startSpanInternal(tr, name, parentSpanContext, remoteParent, opts) + for _, l := range links { + span.addLink(l) + } for _, l := range opts.Links { span.addLink(l) }