tracing: disallow children of sterile span with different Tracer

Before this patch, creating a "child" of a sterile span with a different
Tracer than the one used to create the sterile span was tolerated - on
the argument that sterile spans don't actually get children (the
would-be child span is created as a root), so the arguments for not
allowing a children to be created with different tracers don't apply.
At the same time, creating a child of a noop span with a different
Tracer than the noop span's Tracer was documented to not be allowed. In
practice, it was, because the code was confused [1].

This patch disallows creating children of sterile spans with a different
tracer, for consistency with all the other spans. The patch also makes
it a panic for the children of noop spans to be created with a different
Tracer.

This is all meant as a cleanup / code simplification.

[1] WithParent(sp) meant to treat sterile spans differently than noop
spans but it was using sp.IsSterile(), which returns true for both. As
such, it was unintentionally returning an empty parent option.
startSpanGeneric() meant to check the tracer of parent noop spans, but
it was failing to actually do so because it was going through the
opts.Parent.empty().

Release note: None
Epic: None

pkg/util/tracing/span_options.go

@@ -203,13 +203,16 @@ func WithParent(sp *Span) SpanOption {
 	// below.
 	_ = sp.detectUseAfterFinish()
 
-	// Sterile spans don't get children. Noop spans also don't, but that case is
-	// handled in StartSpan, not here, because we want to assert that the parent's
-	// Tracer is the same as the child's. In contrast, in the IsSterile() case, it
-	// is allowed for the "child" to be created with a different Tracer than the
-	// parent.
-	if sp.IsSterile() {
-		return (parentOption)(spanRef{})
+	// Noop spans and sterile spans don't get children; a span constructed with
+	// WithParent(sterile-or-noop-span) will be a root span. We could return a
+	// zero parentOption here, but instead we return a parentOption referencing
+	// the tracer's noopSpan. This is so that, when constructing the "child" span
+	// using the returned option we can assert that the Tracer used to construct
+	// the child is the same as sp's Tracer.
+	if sp.IsNoop() || sp.IsSterile() {
+		return parentOption{
+			Span: sp.Tracer().noopSpan,
+		}
 	}
 
 	ref, _ /* ok */ := tryMakeSpanRef(sp)

pkg/util/tracing/tracer.go

@@ -1096,14 +1096,17 @@ func (t *Tracer) startSpanGeneric(
 	}
 
 	if !opts.Parent.empty() {
-		// If we don't panic, opts.Parent will be moved into the child, and this
-		// release() will be a no-op.
-		defer opts.Parent.release()
+		if !opts.Parent.IsNoop() {
+			// If we don't panic, opts.Parent will be moved into the child, and this
+			// release() will be a no-op.
+			// Note that we can't call release() on a no-op span.
+			defer opts.Parent.release()
+		}
 
 		if !opts.RemoteParent.Empty() {
 			panic("can't specify both Parent and RemoteParent")
 		}
-		if opts.Parent.IsSterile() {
+		if opts.Parent.i.sterile {
 			// A sterile parent should have been optimized away by
 			// WithParent.
 			panic("invalid sterile parent")
@@ -1126,7 +1129,6 @@ child operation: %s, tracer created at:
 		}
 		if opts.Parent.IsNoop() {
 			// If the parent is a no-op, we'll create a root span.
-			opts.Parent.decRef()
 			opts.Parent = spanRef{}
 		}
 	}

pkg/util/tracing/tracer_test.go

@@ -737,25 +737,43 @@ span: test
 	require.Equal(t, rec1, rec2)
 }
 
+// Check that it is illegal to create a child with a different Tracer than the
+// parent.
 func TestChildNeedsSameTracerAsParent(t *testing.T) {
-	// Check that it is illegal to create a child with a different Tracer than the
-	// parent.
-	tr1 := NewTracer()
-	tr2 := NewTracer()
-	parent := tr1.StartSpan("parent")
-	defer parent.Finish()
-	require.Panics(t, func() {
-		tr2.StartSpan("child", WithParent(parent))
-	})
-
-	// Sterile spans can have children created with a different Tracer (because
-	// they are not really children).
-	parent = tr1.StartSpan("parent", WithSterile())
-	defer parent.Finish()
-	require.NotPanics(t, func() {
-		sp := tr2.StartSpan("child", WithParent(parent))
-		sp.Finish()
-	})
+	type testCase struct {
+		sterileParent bool
+		noopParent    bool
+	}
+	for _, tc := range []testCase{
+		{},
+		{sterileParent: true},
+		{noopParent: true},
+	} {
+		t.Run("", func(t *testing.T) {
+			tr1 := NewTracerWithOpt(context.Background(), WithTracingMode(TracingModeOnDemand))
+			tr2 := NewTracer()
+
+			var opt []SpanOption
+			if tc.sterileParent {
+				opt = append(opt, WithSterile())
+			}
+			if !tc.noopParent {
+				opt = append(opt, WithForceRealSpan())
+			}
+			parent := tr1.StartSpan("parent", opt...)
+			defer parent.Finish()
+			if tc.noopParent {
+				require.True(t, parent.IsNoop())
+			}
+			if tc.sterileParent {
+				require.True(t, parent.IsSterile())
+			}
+
+			require.Panics(t, func() {
+				tr2.StartSpan("child", WithParent(parent))
+			})
+		})
+	}
 }
 
 // TestSpanReuse checks that spans are reused through the Tracer's pool, instead