Remove links on NewRoot spans (#1726)

* Remove links on NewRoot spans

To ensure forwards compatibility, remove the unspecified links currently
set on `NewRoot` spans.

Resolves #461

* Remove links from oteltest tracer to match

CHANGELOG.md

@@ -14,6 +14,12 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm
 - Migrate from using internally built and maintained version of the OTLP to the one hosted at `go.opentelemetry.io/proto/otlp`. (#1713)
 - Migrate from using `github.com/gogo/protobuf` to `google.golang.org/protobuf` to match `go.opentelemetry.io/proto/otlp`. (#1713)
 
+### Removed
+
+- No longer set the links for a `Span` in `go.opentelemetry.io/otel/sdk/trace` that is configured to be a new root.
+  This is unspecified behavior that the OpenTelemetry community plans to standardize in the future.
+  To prevent backwards incompatible changes when it is specified, these links are removed. (#1726)
+
 ## [0.19.0] - 2021-03-18
 
 ### Added

bridge/opentracing/bridge.go

@@ -655,10 +655,9 @@ func (t *BridgeTracer) Extract(format interface{}, carrier interface{}) (ot.Span
 	header := http.Header(hhcarrier)
 	ctx := t.getPropagator().Extract(context.Background(), propagation.HeaderCarrier(header))
 	baggage := baggage.MapFromContext(ctx)
-	otelSC, _, _ := otelparent.GetSpanContextAndLinks(ctx, false)
 	bridgeSC := &bridgeSpanContext{
 		baggageItems:    baggage,
-		otelSpanContext: otelSC,
+		otelSpanContext: otelparent.SpanContext(ctx),
 	}
 	if !bridgeSC.otelSpanContext.IsValid() {
 		return nil, ot.ErrSpanContextNotFound

bridge/opentracing/internal/mock.go

@@ -137,8 +137,10 @@ func (t *MockTracer) getParentSpanID(ctx context.Context, config *trace.SpanConf
 }
 
 func (t *MockTracer) getParentSpanContext(ctx context.Context, config *trace.SpanConfig) trace.SpanContext {
-	spanCtx, _, _ := otelparent.GetSpanContextAndLinks(ctx, config.NewRoot)
-	return spanCtx
+	if !config.NewRoot {
+		return otelparent.SpanContext(ctx)
+	}
+	return trace.SpanContext{}
 }
 
 func (t *MockTracer) getSpanID() trace.SpanID {

internal/trace/parent/parent.go

@@ -17,37 +17,12 @@ package parent
 import (
 	"context"
 
-	"go.opentelemetry.io/otel/attribute"
 	"go.opentelemetry.io/otel/trace"
 )
 
-func GetSpanContextAndLinks(ctx context.Context, ignoreContext bool) (trace.SpanContext, bool, []trace.Link) {
-	lsctx := trace.SpanContextFromContext(ctx)
-	rsctx := trace.RemoteSpanContextFromContext(ctx)
-
-	if ignoreContext {
-		links := addLinkIfValid(nil, lsctx, "current")
-		links = addLinkIfValid(links, rsctx, "remote")
-
-		return trace.SpanContext{}, false, links
+func SpanContext(ctx context.Context) trace.SpanContext {
+	if p := trace.SpanContextFromContext(ctx); p.IsValid() {
+		return p
 	}
-	if lsctx.IsValid() {
-		return lsctx, false, nil
-	}
-	if rsctx.IsValid() {
-		return rsctx, true, nil
-	}
-	return trace.SpanContext{}, false, nil
-}
-
-func addLinkIfValid(links []trace.Link, sc trace.SpanContext, kind string) []trace.Link {
-	if !sc.IsValid() {
-		return links
-	}
-	return append(links, trace.Link{
-		SpanContext: sc,
-		Attributes: []attribute.KeyValue{
-			attribute.String("ignored-on-demand", kind),
-		},
-	})
+	return trace.RemoteSpanContextFromContext(ctx)
 }

oteltest/tracer.go

@@ -53,20 +53,6 @@ func (t *Tracer) Start(ctx context.Context, name string, opts ...trace.SpanOptio
 
 	if c.NewRoot {
 		span.spanContext = trace.SpanContext{}
-
-		iodKey := attribute.Key("ignored-on-demand")
-		if lsc := trace.SpanContextFromContext(ctx); lsc.IsValid() {
-			span.links = append(span.links, trace.Link{
-				SpanContext: lsc,
-				Attributes:  []attribute.KeyValue{iodKey.String("current")},
-			})
-		}
-		if rsc := trace.RemoteSpanContextFromContext(ctx); rsc.IsValid() {
-			span.links = append(span.links, trace.Link{
-				SpanContext: rsc,
-				Attributes:  []attribute.KeyValue{iodKey.String("remote")},
-			})
-		}
 	} else {
 		span.spanContext = t.config.SpanContextFunc(ctx)
 		if lsc := trace.SpanContextFromContext(ctx); lsc.IsValid() {

oteltest/tracer_test.go

@@ -198,23 +198,6 @@ func TestTracer(t *testing.T) {
 			e.Expect(childSpanContext.SpanID()).NotToEqual(parentSpanContext.SpanID())
 			e.Expect(childSpanContext.SpanID()).NotToEqual(remoteParentSpanContext.SpanID())
 			e.Expect(testSpan.ParentSpanID().IsValid()).ToBeFalse()
-
-			expectedLinks := []trace.Link{
-				{
-					SpanContext: parentSpanContext,
-					Attributes: []attribute.KeyValue{
-						attribute.String("ignored-on-demand", "current"),
-					},
-				},
-				{
-					SpanContext: remoteParentSpanContext,
-					Attributes: []attribute.KeyValue{
-						attribute.String("ignored-on-demand", "remote"),
-					},
-				},
-			}
-			gotLinks := testSpan.Links()
-			e.Expect(gotLinks).ToMatchInAnyOrder(expectedLinks)
 		})
 
 		t.Run("uses the links provided through WithLinks", func(t *testing.T) {

sdk/trace/span.go

@@ -24,6 +24,7 @@ import (
 
 	"go.opentelemetry.io/otel/attribute"
 	"go.opentelemetry.io/otel/codes"
+	"go.opentelemetry.io/otel/internal/trace/parent"
 	"go.opentelemetry.io/otel/trace"
 
 	export "go.opentelemetry.io/otel/sdk/export/trace"
@@ -77,8 +78,6 @@ type ReadWriteSpan interface {
 	ReadOnlySpan
 }
 
-var emptySpanContext = trace.SpanContext{}
-
 // span is an implementation of the OpenTelemetry Span API representing the
 // individual component of a trace.
 type span struct {
@@ -518,29 +517,29 @@ func (s *span) addDroppedAttributeCount(delta int) {
 
 func (*span) private() {}
 
-func startSpanInternal(ctx context.Context, tr *tracer, name string, parent trace.SpanContext, remoteParent bool, o *trace.SpanConfig) *span {
+func startSpanInternal(ctx context.Context, tr *tracer, name string, o *trace.SpanConfig) *span {
 	span := &span{}
 
 	provider := tr.provider
 
-	var tid trace.TraceID
-	var sid trace.SpanID
-
-	if hasEmptySpanContext(parent) {
-		// Generate both TraceID and SpanID
-		tid, sid = provider.idGenerator.NewIDs(ctx)
-	} else {
-		// TraceID already exists, just generate a SpanID
-		tid = parent.TraceID()
-		sid = provider.idGenerator.NewSpanID(ctx, tid)
+	// If told explicitly to make this a new root use a zero value SpanContext
+	// as a parent which contains an invalid trace ID and is not remote.
+	var psc trace.SpanContext
+	if !o.NewRoot {
+		psc = parent.SpanContext(ctx)
 	}
 
-	span.spanContext = trace.NewSpanContext(trace.SpanContextConfig{
-		TraceID:    tid,
-		SpanID:     sid,
-		TraceFlags: parent.TraceFlags(),
-		TraceState: parent.TraceState(),
-	})
+	// If there is a valid parent trace ID, use it to ensure the continuity of
+	// the trace. Always generate a new span ID so other components can rely
+	// on a unique span ID, even if the Span is non-recording.
+	var tid trace.TraceID
+	var sid trace.SpanID
+	if !psc.TraceID().IsValid() {
+		tid, sid = provider.idGenerator.NewIDs(ctx)
+	} else {
+		tid = psc.TraceID()
+		sid = provider.idGenerator.NewSpanID(ctx, tid)
+	}
 
 	spanLimits := provider.spanLimits
 	span.attributes = newAttributesMap(spanLimits.AttributeCountLimit)
@@ -549,20 +548,26 @@ func startSpanInternal(ctx context.Context, tr *tracer, name string, parent trac
 	span.spanLimits = spanLimits
 
 	samplingResult := provider.sampler.ShouldSample(SamplingParameters{
-		ParentContext:   parent,
-		TraceID:         span.spanContext.TraceID(),
+		ParentContext:   psc,
+		TraceID:         tid,
 		Name:            name,
-		HasRemoteParent: remoteParent,
+		HasRemoteParent: psc.IsRemote(),
 		Kind:            o.SpanKind,
 		Attributes:      o.Attributes,
 		Links:           o.Links,
 	})
-	if isSampled(samplingResult) {
-		span.spanContext = span.spanContext.WithTraceFlags(span.spanContext.TraceFlags() | trace.FlagsSampled)
-	} else {
-		span.spanContext = span.spanContext.WithTraceFlags(span.spanContext.TraceFlags() &^ trace.FlagsSampled)
+
+	scc := trace.SpanContextConfig{
+		TraceID:    tid,
+		SpanID:     sid,
+		TraceState: samplingResult.Tracestate,
 	}
-	span.spanContext = span.spanContext.WithTraceState(samplingResult.Tracestate)
+	if isSampled(samplingResult) {
+		scc.TraceFlags = psc.TraceFlags() | trace.FlagsSampled
+	} else {
+		scc.TraceFlags = psc.TraceFlags() &^ trace.FlagsSampled
+	}
+	span.spanContext = trace.NewSpanContext(scc)
 
 	if !isRecording(samplingResult) {
 		return span
@@ -576,21 +581,17 @@ func startSpanInternal(ctx context.Context, tr *tracer, name string, parent trac
 
 	span.spanKind = trace.ValidateSpanKind(o.SpanKind)
 	span.name = name
-	span.hasRemoteParent = remoteParent
+	span.hasRemoteParent = psc.IsRemote()
 	span.resource = provider.resource
 	span.instrumentationLibrary = tr.instrumentationLibrary
 
 	span.SetAttributes(samplingResult.Attributes...)
 
-	span.parent = parent
+	span.parent = psc
 
 	return span
 }
 
-func hasEmptySpanContext(parent trace.SpanContext) bool {
-	return parent.Equal(emptySpanContext)
-}
-
 func isRecording(s SamplingResult) bool {
 	return s.Decision == RecordOnly || s.Decision == RecordAndSample
 }

sdk/trace/tracer.go

@@ -18,7 +18,6 @@ import (
 	"context"
 	rt "runtime/trace"
 
-	"go.opentelemetry.io/otel/internal/trace/parent"
 	"go.opentelemetry.io/otel/trace"
 
 	"go.opentelemetry.io/otel/sdk/instrumentation"
@@ -40,18 +39,14 @@ var _ trace.Tracer = &tracer{}
 func (tr *tracer) Start(ctx context.Context, name string, options ...trace.SpanOption) (context.Context, trace.Span) {
 	config := trace.NewSpanConfig(options...)
 
-	parentSpanContext, remoteParent, links := parent.GetSpanContextAndLinks(ctx, config.NewRoot)
-
+	// For local spans created by this SDK, track child span count.
 	if p := trace.SpanFromContext(ctx); p != nil {
 		if sdkSpan, ok := p.(*span); ok {
 			sdkSpan.addChild()
 		}
 	}
 
-	span := startSpanInternal(ctx, tr, name, parentSpanContext, remoteParent, config)
-	for _, l := range links {
-		span.addLink(l)
-	}
+	span := startSpanInternal(ctx, tr, name, config)
 	for _, l := range config.Links {
 		span.addLink(l)
 	}