diff --git a/pkg/util/tracing/span_options.go b/pkg/util/tracing/span_options.go index a8296ea5b28a..3428fecde0af 100644 --- a/pkg/util/tracing/span_options.go +++ b/pkg/util/tracing/span_options.go @@ -121,7 +121,8 @@ type parentAndAutoCollectionOption Span // from a parent Span. // // WithParentAndAutoCollection can be called with a nil `sp`, in which case -// it'll be a no-op. +// it'll be a no-op. It can also be called with a "no-op span", in which case +// the option will also be a no-op (i.e. the upcoming span will be a root). // // The child inherits the parent's log tags. The data collected in the // child trace will be retrieved automatically when the parent's data is @@ -143,6 +144,12 @@ type parentAndAutoCollectionOption Span // WithParentAndManualCollection should be used, which incurs an // obligation to manually propagate the trace data to the parent Span. func WithParentAndAutoCollection(sp *Span) SpanOption { + if sp == nil { + return (*parentAndAutoCollectionOption)(nil) + } + if sp.IsNoop() { + return (*parentAndAutoCollectionOption)(nil) + } return (*parentAndAutoCollectionOption)(sp) } diff --git a/pkg/util/tracing/tracer.go b/pkg/util/tracing/tracer.go index cad2efc90d06..c1d737ea5d7a 100644 --- a/pkg/util/tracing/tracer.go +++ b/pkg/util/tracing/tracer.go @@ -431,6 +431,12 @@ func (t *Tracer) startSpanGeneric( if !opts.RemoteParent.Empty() { panic("can't specify both Parent and RemoteParent") } + if opts.Parent.IsNoop() { + // This method relies on the parent, if any, not being a no-op. A no-op + // parent should have been optimized away by the + // WithParentAndAutoCollection option. + panic("invalid no-op parent") + } } // Are we tracing everything, or have a parent, or want a real span? Then @@ -443,7 +449,7 @@ func (t *Tracer) startSpanGeneric( opts.LogTags = logtags.FromContext(ctx) } - if opts.LogTags == nil && opts.Parent != nil && !opts.Parent.IsNoop() { + if opts.LogTags == nil && opts.Parent != nil { // If no log tags are specified in the options, use the parent // span's, if any. This behavior is the reason logTags are // fundamentally different from tags, which are strictly per span, @@ -565,15 +571,15 @@ func (t *Tracer) startSpanGeneric( // spans contained in Span. // // NB: this could be optimized. - if opts.Parent != nil { - if !opts.Parent.IsNoop() { - opts.Parent.i.crdb.mu.Lock() - m := opts.Parent.i.crdb.mu.baggage - opts.Parent.i.crdb.mu.Unlock() - - for k, v := range m { - s.SetBaggageItem(k, v) - } + // NB: (opts.Parent != nil && opts.Parent.i.crdb == nil) is not possible at + // the moment, but let's not rely on that. + if opts.Parent != nil && opts.Parent.i.crdb != nil { + opts.Parent.i.crdb.mu.Lock() + m := opts.Parent.i.crdb.mu.baggage + opts.Parent.i.crdb.mu.Unlock() + + for k, v := range m { + s.SetBaggageItem(k, v) } } else { // Local root span - put it into the registry of active local root diff --git a/pkg/util/tracing/tracer_test.go b/pkg/util/tracing/tracer_test.go index 73021faf2f59..5f9ccfe51d6e 100644 --- a/pkg/util/tracing/tracer_test.go +++ b/pkg/util/tracing/tracer_test.go @@ -573,6 +573,21 @@ func TestNoopSpanFinish(t *testing.T) { require.EqualValues(t, 1, tr.noopSpan.numFinishCalled) } +// Test that a span constructed with a no-op span behaves like a root span - it +// is present in the active spans registry. +func TestSpanWithNoopParentIsInActiveSpans(t *testing.T) { + tr := NewTracer() + noop := tr.StartSpan("noop") + require.True(t, noop.IsNoop()) + root := tr.StartSpan("foo", WithParentAndAutoCollection(noop), WithForceRealSpan()) + require.Len(t, tr.activeSpans.m, 1) + visitor := func(sp *Span) error { + require.Equal(t, root, sp) + return nil + } + require.NoError(t, tr.VisitSpans(visitor)) +} + func TestConcurrentChildAndRecording(t *testing.T) { tr := NewTracer() rootSp := tr.StartSpan("root", WithForceRealSpan())