From 6defcfdf457ac642bc01c77e91aa8079de3b6682 Mon Sep 17 00:00:00 2001 From: Tyler Yahn Date: Thu, 25 Mar 2021 14:36:39 +0000 Subject: [PATCH] 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 | 6 ++++ bridge/opentracing/bridge.go | 3 +- bridge/opentracing/internal/mock.go | 6 ++-- internal/trace/parent/parent.go | 33 +++-------------- oteltest/tracer.go | 14 -------- oteltest/tracer_test.go | 17 --------- sdk/trace/span.go | 55 +++++++++++++++-------------- sdk/trace/tracer.go | 9 ++--- 8 files changed, 45 insertions(+), 98 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2831a693525..1b38561314c 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/bridge/opentracing/bridge.go b/bridge/opentracing/bridge.go index 36df70fbd75..68c7f0d3852 100644 --- a/bridge/opentracing/bridge.go +++ b/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 diff --git a/bridge/opentracing/internal/mock.go b/bridge/opentracing/internal/mock.go index 54d0b5bfe03..de74de7673e 100644 --- a/bridge/opentracing/internal/mock.go +++ b/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 { diff --git a/internal/trace/parent/parent.go b/internal/trace/parent/parent.go index 232961fb161..c82167ba0f6 100644 --- a/internal/trace/parent/parent.go +++ b/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 - } - 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 +func SpanContext(ctx context.Context) trace.SpanContext { + if p := trace.SpanContextFromContext(ctx); p.IsValid() { + return p } - return append(links, trace.Link{ - SpanContext: sc, - Attributes: []attribute.KeyValue{ - attribute.String("ignored-on-demand", kind), - }, - }) + return trace.RemoteSpanContextFromContext(ctx) } diff --git a/oteltest/tracer.go b/oteltest/tracer.go index f9302650e13..487a9814713 100644 --- a/oteltest/tracer.go +++ b/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() { diff --git a/oteltest/tracer_test.go b/oteltest/tracer_test.go index dc9e2d232f9..679a4fcec5d 100644 --- a/oteltest/tracer_test.go +++ b/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) { diff --git a/sdk/trace/span.go b/sdk/trace/span.go index 2b61a88c677..1f84f94b397 100644 --- a/sdk/trace/span.go +++ b/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,30 +517,30 @@ 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 + // 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) + } + + // 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 hasEmptySpanContext(parent) { - // Generate both TraceID and SpanID + if !psc.TraceID().IsValid() { tid, sid = provider.idGenerator.NewIDs(ctx) } else { - // TraceID already exists, just generate a SpanID - tid = parent.TraceID() + tid = psc.TraceID() sid = provider.idGenerator.NewSpanID(ctx, tid) } - span.spanContext = trace.NewSpanContext(trace.SpanContextConfig{ - TraceID: tid, - SpanID: sid, - TraceFlags: parent.TraceFlags(), - TraceState: parent.TraceState(), - }) - spanLimits := provider.spanLimits span.attributes = newAttributesMap(spanLimits.AttributeCountLimit) span.messageEvents = newEvictedQueue(spanLimits.EventCountLimit) @@ -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, }) + + scc := trace.SpanContextConfig{ + TraceID: tid, + SpanID: sid, + TraceState: samplingResult.Tracestate, + } if isSampled(samplingResult) { - span.spanContext = span.spanContext.WithTraceFlags(span.spanContext.TraceFlags() | trace.FlagsSampled) + scc.TraceFlags = psc.TraceFlags() | trace.FlagsSampled } else { - span.spanContext = span.spanContext.WithTraceFlags(span.spanContext.TraceFlags() &^ trace.FlagsSampled) + scc.TraceFlags = psc.TraceFlags() &^ trace.FlagsSampled } - span.spanContext = span.spanContext.WithTraceState(samplingResult.Tracestate) + 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 } diff --git a/sdk/trace/tracer.go b/sdk/trace/tracer.go index 8bc5109d977..b7869eddd2e 100644 --- a/sdk/trace/tracer.go +++ b/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) }