Merge #62703

62703: tracing: fix OpenTracing traces across nodes r=tbg a=andreimatei

"Shadow traces" (OpenTracing) were broken at node boundaries because the
server shadow span was not using the remote span context.
I believe this was broken in #56109.

Fixes #62702

Release note (bug fix): OpenTracing traces work correctly again across
nodes. The client and server spans for RPCs are once again part of the
same trace instead of the server being a root span erroneously.

Co-authored-by: Andrei Matei <[email protected]>
Co-authored-by: Tobias Grieger <[email protected]>

pkg/util/tracing/span_options.go

@@ -66,6 +66,16 @@ func (opts *spanOptions) shadowTrTyp() (string, bool) {
 	return "", false
 }
 
+func (opts *spanOptions) shadowContext() opentracing.SpanContext {
+	if opts.Parent != nil && opts.Parent.i.ot.shadowSpan != nil {
+		return opts.Parent.i.ot.shadowSpan.Context()
+	}
+	if opts.RemoteParent != nil && opts.RemoteParent.shadowCtx != nil {
+		return opts.RemoteParent.shadowCtx
+	}
+	return nil
+}
+
 // SpanOption is the interface satisfied by options to `Tracer.StartSpan`.
 // A synopsis of the options follows. For details, see their comments.
 //

pkg/util/tracing/tracer.go

@@ -325,11 +325,7 @@ func (t *Tracer) startSpanGeneric(
 		// for the new span to avoid compat issues between the
 		// two underlying tracers.
 		if ok2 && (!ok1 || typ1 == typ2) {
-			var shadowCtx opentracing.SpanContext
-			if opts.Parent != nil && opts.Parent.i.ot.shadowSpan != nil {
-				shadowCtx = opts.Parent.i.ot.shadowSpan.Context()
-			}
-			ot = makeShadowSpan(shadowTr, shadowCtx, opts.RefType, opName, startTime)
+			ot = makeShadowSpan(shadowTr, opts.shadowContext(), opts.RefType, opName, startTime)
 			// If LogTags are given, pass them as tags to the shadow span.
 			// Regular tags are populated later, via the top-level Span.
 			if opts.LogTags != nil {

pkg/util/tracing/tracer_test.go

@@ -391,6 +391,17 @@ func TestShadowTracer(t *testing.T) {
 			const testBaggageKey = "test-baggage"
 			const testBaggageVal = "test-val"
 			s.SetBaggageItem(testBaggageKey, testBaggageVal)
+			// Also add a baggage item that is exclusive to the shadow span.
+			// This wouldn't typically happen in practice, but it serves as
+			// a regression test for #62702. Losing the Span context directly
+			// is hard to verify via baggage items since the top-level baggage
+			// is transported separately and re-inserted into the shadow context
+			// on the remote side, i.e. the test-baggage item above shows up
+			// regardless of whether #62702 is fixed. But if we're losing the
+			// shadowCtx, the only-in-shadow item does get lost as well, so if
+			// it does not then we know for sure that the shadowContext was
+			// propagated properly.
+			s.i.ot.shadowSpan.SetBaggageItem("only-in-shadow", "foo")
 
 			carrier := metadataCarrier{metadata.MD{}}
 			if err := tr.InjectMetaInto(s.Meta(), carrier); err != nil {
@@ -413,11 +424,13 @@ func TestShadowTracer(t *testing.T) {
 				shadowBaggage[k] = v
 				return true
 			})
-			exp := map[string]string{
+			require.Equal(t, map[string]string{
 				testBaggageKey: testBaggageVal,
-			}
-			require.Equal(t, exp, s2.Meta().Baggage)
-			require.Equal(t, exp, shadowBaggage)
+			}, s2.Meta().Baggage)
+			require.Equal(t, map[string]string{
+				testBaggageKey:   testBaggageVal,
+				"only-in-shadow": "foo",
+			}, shadowBaggage)
 		})
 	}