Suppress nested SDK method spans (Azure#21251)

* Suppress nested SDK method spans

The inner SDK span must be omitted.  However, any REST span it creates
must be a child of the outer SDK method span.

* only suppress client and internal spans

Author: jhendrixMSFT

sdk/azcore/CHANGELOG.md

@@ -8,6 +8,8 @@
 
 ### Bugs Fixed
 
+* Suppress creating spans for nested SDK API calls. The HTTP span will be a child of the outer API span.
+
 ### Other Changes
 
 ## 1.8.0-beta.2 (2023-08-14)

sdk/azcore/runtime/policy_http_trace.go

@@ -103,10 +103,23 @@ func StartSpan(ctx context.Context, name string, tracer tracing.Tracer, options
 	if !tracer.Enabled() {
 		return ctx, func(err error) {}
 	}
+	const newSpanKind = tracing.SpanKindInternal
+	if activeSpan := ctx.Value(ctxActiveSpan{}); activeSpan != nil {
+		// per the design guidelines, if a SDK method Foo() calls SDK method Bar(),
+		// then the span for Bar() must be suppressed. however, if Bar() makes a REST
+		// call, then Bar's HTTP span must be a child of Foo's span.
+		// however, there is an exception to this rule. if the SDK method Foo() is a
+		// messaging producer/consumer, and it takes a callback that's a SDK method
+		// Bar(), then the span for Bar() must _not_ be suppressed.
+		if kind := activeSpan.(tracing.SpanKind); kind == tracing.SpanKindClient || kind == tracing.SpanKindInternal {
+			return ctx, func(err error) {}
+		}
+	}
 	ctx, span := tracer.Start(ctx, name, &tracing.SpanOptions{
-		Kind: tracing.SpanKindInternal,
+		Kind: newSpanKind,
 	})
 	ctx = context.WithValue(ctx, shared.CtxWithTracingTracer{}, tracer)
+	ctx = context.WithValue(ctx, ctxActiveSpan{}, newSpanKind)
 	return ctx, func(err error) {
 		if err != nil {
 			errType := strings.Replace(fmt.Sprintf("%T", err), "*exported.", "*azcore.", 1)
@@ -115,3 +128,6 @@ func StartSpan(ctx context.Context, name string, tracer tracing.Tracer, options
 		span.End()
 	}
 }
+
+// ctxActiveSpan is used as a context key for indicating a SDK client span is in progress.
+type ctxActiveSpan struct{}

sdk/azcore/runtime/policy_http_trace_test.go

@@ -161,3 +161,55 @@ func TestStartSpan(t *testing.T) {
 	require.Contains(t, errStr, "*azcore.ResponseError")
 	require.Contains(t, errStr, "ERROR CODE: ErrorItFailed")
 }
+
+func TestStartSpansDontNest(t *testing.T) {
+	srv, close := mock.NewServer()
+	srv.SetResponse() // always return http.StatusOK
+	defer close()
+
+	pl := exported.NewPipeline(srv, newHTTPTracePolicy(nil))
+
+	apiSpanCount := 0
+	httpSpanCount := 0
+	endCalled := 0
+	tr := tracing.NewTracer(func(ctx context.Context, spanName string, options *tracing.SpanOptions) (context.Context, tracing.Span) {
+		if spanName == "HTTP GET" {
+			httpSpanCount++
+		} else if spanName == "FooMethod" {
+			apiSpanCount++
+		} else {
+			t.Fatalf("unexpected span name %s", spanName)
+		}
+		spanImpl := tracing.SpanImpl{
+			End: func() { endCalled++ },
+		}
+		return ctx, tracing.NewSpan(spanImpl)
+	}, nil)
+
+	barMethod := func(ctx context.Context) {
+		ourCtx, endSpan := StartSpan(ctx, "BarMethod", tr, nil)
+		require.Same(t, ctx, ourCtx)
+		defer endSpan(nil)
+		req, err := exported.NewRequest(ourCtx, http.MethodGet, srv.URL()+"/bar")
+		require.NoError(t, err)
+		_, err = pl.Do(req)
+		require.NoError(t, err)
+	}
+
+	fooMethod := func(ctx context.Context) {
+		ctx, endSpan := StartSpan(ctx, "FooMethod", tr, nil)
+		defer endSpan(nil)
+		barMethod(ctx)
+		req, err := exported.NewRequest(ctx, http.MethodGet, srv.URL()+"/foo")
+		require.NoError(t, err)
+		_, err = pl.Do(req)
+		require.NoError(t, err)
+	}
+
+	fooMethod(context.Background())
+
+	// there should be a total of three spans. one for FooMethod, and two HTTP spans
+	require.EqualValues(t, 1, apiSpanCount)
+	require.EqualValues(t, 2, httpSpanCount)
+	require.EqualValues(t, 3, endCalled)
+}