util/tracing: fix an edge case for active span registration

Before this patch, a span with a no-op parent (as opposed to a span
with no parent), would not be present in the active spans registry. The
code was confused: the span didn't qualify as a "local root" because it
had a local parent, but of course it also wasn't really recorded by the
local parent cause a no-op span can't record anything. As such, the span
in question was missing from the registry.
This patch makes a span with a no-op parent behave like a root span.

Release note: None

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)
 }
 

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

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())