Refactor some public surface in azcore/tracing

eng/pipelines/templates/jobs/archetype-sdk-client.yml

@@ -71,7 +71,7 @@ parameters:
   - name: IncludeRelease
     type: boolean
     default: false
-  - name: ExcludeGoNMinus3
+  - name: ExcludeGoNMinus2
     type: boolean
     default: false
 
@@ -85,12 +85,12 @@ stages:
         - template: /eng/pipelines/templates/variables/globals.yml
       strategy:
         matrix:
-          ${{ if eq(parameters.ExcludeGoNMinus3, false) }}:
+          ${{ if eq(parameters.ExcludeGoNMinus2, false) }}:
             Linux_Go118:
               pool.name: azsdk-pool-mms-ubuntu-2004-general
               image.name: MMSUbuntu20.04
               go.version: '1.18.10'
-          ${{ if eq(parameters.ExcludeGoNMinus3, false) }}:
+          ${{ if eq(parameters.ExcludeGoNMinus2, false) }}:
             Windows_Go118:
               pool.name: azsdk-pool-mms-win-2022-general
               image.name: MMS2022

sdk/azcore/CHANGELOG.md

@@ -5,6 +5,11 @@
 ### Features Added
 
 ### Breaking Changes
+> These changes affect only code written against beta version v1.7.0-beta.1
+* Method `SpanFromContext()` on type `tracing.Tracer` had the `bool` return value removed.
+  * This includes the field `SpanFromContext` in supporting type `tracing.TracerOptions`.
+* Method `AddError()` has been removed from type `tracing.Span`.
+* Method `Span.End()` now requires an argument of type `*tracing.SpanEndOptions`.
 
 ### Bugs Fixed
 

sdk/azcore/arm/runtime/policy_trace_namespace.go

@@ -22,9 +22,8 @@ func httpTraceNamespacePolicy(req *policy.Request) (resp *http.Response, err err
 		rt, err := resource.ParseResourceType(req.Raw().URL.Path)
 		if err == nil {
 			// add the namespace attribute to the current span
-			if span, ok := tracer.SpanFromContext(req.Raw().Context()); ok {
-				span.SetAttributes(tracing.Attribute{Key: "az.namespace", Value: rt.Namespace})
-			}
+			span := tracer.SpanFromContext(req.Raw().Context())
+			span.SetAttributes(tracing.Attribute{Key: "az.namespace", Value: rt.Namespace})
 		}
 	}
 	return req.Next()

sdk/azcore/arm/runtime/policy_trace_namespace_test.go

@@ -53,7 +53,7 @@ func TestHTTPTraceNamespacePolicy(t *testing.T) {
 	tr = tracing.NewTracer(func(ctx context.Context, spanName string, options *tracing.SpanOptions) (context.Context, tracing.Span) {
 		return ctx, tracing.Span{}
 	}, &tracing.TracerOptions{
-		SpanFromContext: func(ctx context.Context) (tracing.Span, bool) {
+		SpanFromContext: func(ctx context.Context) tracing.Span {
 			spanImpl := tracing.SpanImpl{
 				SetAttributes: func(a ...tracing.Attribute) {
 					require.Len(t, a, 1)
@@ -62,7 +62,7 @@ func TestHTTPTraceNamespacePolicy(t *testing.T) {
 					attrString = a[0].Key + ":" + v
 				},
 			}
-			return tracing.NewSpan(spanImpl), true
+			return tracing.NewSpan(spanImpl)
 		},
 	})
 	req, err = exported.NewRequest(context.WithValue(context.Background(), shared.CtxWithTracingTracer{}, tr), http.MethodGet, srv.URL())
@@ -76,7 +76,7 @@ func TestHTTPTraceNamespacePolicy(t *testing.T) {
 	tr = tracing.NewTracer(func(ctx context.Context, spanName string, options *tracing.SpanOptions) (context.Context, tracing.Span) {
 		return ctx, tracing.Span{}
 	}, &tracing.TracerOptions{
-		SpanFromContext: func(ctx context.Context) (tracing.Span, bool) {
+		SpanFromContext: func(ctx context.Context) tracing.Span {
 			spanImpl := tracing.SpanImpl{
 				SetAttributes: func(a ...tracing.Attribute) {
 					require.Len(t, a, 1)
@@ -85,7 +85,7 @@ func TestHTTPTraceNamespacePolicy(t *testing.T) {
 					attrString = a[0].Key + ":" + v
 				},
 			}
-			return tracing.NewSpan(spanImpl), true
+			return tracing.NewSpan(spanImpl)
 		},
 	})
 	req, err = exported.NewRequest(context.WithValue(context.Background(), shared.CtxWithTracingTracer{}, tr), http.MethodGet, srv.URL()+requestEndpoint)

sdk/azcore/runtime/policy_http_trace.go

@@ -78,7 +78,7 @@ func (h *httpTracePolicy) Do(req *policy.Request) (resp *http.Response, err erro
 				// so instead of attempting to sanitize the output, we simply output the error type.
 				span.SetStatus(tracing.SpanStatusError, fmt.Sprintf("%T", err))
 			}
-			span.End()
+			span.End(nil)
 		}()
 
 		req = req.WithContext(ctx)
@@ -112,6 +112,6 @@ func StartSpan(ctx context.Context, name string, tracer tracing.Tracer, options
 			errType := strings.Replace(fmt.Sprintf("%T", err), "*exported.", "*azcore.", 1)
 			span.SetStatus(tracing.SpanStatusError, fmt.Sprintf("%s:\n%s", errType, err.Error()))
 		}
-		span.End()
+		span.End(nil)
 	}
 }

sdk/azcore/tracing/tracing.go

@@ -46,7 +46,7 @@ func (p Provider) NewTracer(name, version string) (tracer Tracer) {
 // TracerOptions contains the optional values when creating a Tracer.
 type TracerOptions struct {
 	// SpanFromContext contains the implementation for the Tracer.SpanFromContext method.
-	SpanFromContext func(context.Context) (Span, bool)
+	SpanFromContext func(context.Context) Span
 }
 
 // NewTracer creates a Tracer with the specified values.
@@ -66,7 +66,7 @@ func NewTracer(newSpanFn func(ctx context.Context, spanName string, options *Spa
 type Tracer struct {
 	attrs             []Attribute
 	newSpanFn         func(ctx context.Context, spanName string, options *SpanOptions) (context.Context, Span)
-	spanFromContextFn func(ctx context.Context) (Span, bool)
+	spanFromContextFn func(ctx context.Context) Span
 }
 
 // Start creates a new span and a context.Context that contains it.
@@ -99,11 +99,11 @@ func (t Tracer) Enabled() bool {
 
 // SpanFromContext returns the Span associated with the current context.
 // If the provided context has no Span, false is returned.
-func (t Tracer) SpanFromContext(ctx context.Context) (Span, bool) {
+func (t Tracer) SpanFromContext(ctx context.Context) Span {
 	if t.spanFromContextFn != nil {
 		return t.spanFromContextFn(ctx)
 	}
-	return Span{}, false
+	return Span{}
 }
 
 // SpanOptions contains optional settings for creating a span.
@@ -130,9 +130,6 @@ type SpanImpl struct {
 	// AddEvent contains the implementation for the Span.AddEvent method.
 	AddEvent func(string, ...Attribute)
 
-	// AddError contains the implementation for the Span.AddError method.
-	AddError func(err error)
-
 	// SetStatus contains the implementation for the Span.SetStatus method.
 	SetStatus func(SpanStatus, string)
 }
@@ -152,7 +149,7 @@ type Span struct {
 
 // End terminates the span and MUST be called before the span leaves scope.
 // Any further updates to the span will be ignored after End is called.
-func (s Span) End() {
+func (s Span) End(opts *SpanEndOptions) {
 	if s.impl.End != nil {
 		s.impl.End()
 	}
@@ -173,20 +170,18 @@ func (s Span) AddEvent(name string, attrs ...Attribute) {
 	}
 }
 
-// AddError adds the specified error event to the span.
-func (s Span) AddError(err error) {
-	if s.impl.AddError != nil {
-		s.impl.AddError(err)
-	}
-}
-
 // SetStatus sets the status on the span along with a description.
 func (s Span) SetStatus(code SpanStatus, desc string) {
 	if s.impl.SetStatus != nil {
 		s.impl.SetStatus(code, desc)
 	}
 }
 
+// SpanEndOptions contains the optional values for the Span.End() method.
+type SpanEndOptions struct {
+	// for future expansion
+}
+
 /////////////////////////////////////////////////////////////////////////////////////////////////////////////
 
 // Attribute is a key-value pair.

sdk/azcore/tracing/tracing_test.go

@@ -22,18 +22,15 @@ func TestProviderZeroValues(t *testing.T) {
 	ctx, sp := tr.Start(context.Background(), "spanName", nil)
 	require.Equal(t, context.Background(), ctx)
 	require.Zero(t, sp)
-	sp.AddError(nil)
 	sp.AddEvent("event")
-	sp.End()
+	sp.End(nil)
 	sp.SetAttributes(Attribute{})
 	sp.SetStatus(SpanStatusError, "boom")
-	sp, ok := tr.SpanFromContext(ctx)
-	require.False(t, ok)
-	require.Zero(t, sp)
+	spCtx := tr.SpanFromContext(ctx)
+	require.Zero(t, spCtx)
 }
 
 func TestProvider(t *testing.T) {
-	var addErrorCalled bool
 	var addEventCalled bool
 	var endCalled bool
 	var setAttributesCalled bool
@@ -43,24 +40,22 @@ func TestProvider(t *testing.T) {
 	pr := NewProvider(func(name, version string) Tracer {
 		return NewTracer(func(context.Context, string, *SpanOptions) (context.Context, Span) {
 			return nil, NewSpan(SpanImpl{
-				AddError:      func(error) { addErrorCalled = true },
 				AddEvent:      func(string, ...Attribute) { addEventCalled = true },
 				End:           func() { endCalled = true },
 				SetAttributes: func(...Attribute) { setAttributesCalled = true },
 				SetStatus:     func(SpanStatus, string) { setStatusCalled = true },
 			})
 		}, &TracerOptions{
-			SpanFromContext: func(context.Context) (Span, bool) {
+			SpanFromContext: func(context.Context) Span {
 				spanFromContextCalled = true
-				return Span{}, true
+				return Span{}
 			},
 		})
 	}, nil)
 	tr := pr.NewTracer("name", "version")
 	require.NotZero(t, tr)
 	require.True(t, tr.Enabled())
-	sp, ok := tr.SpanFromContext(context.Background())
-	require.True(t, ok)
+	sp := tr.SpanFromContext(context.Background())
 	require.Zero(t, sp)
 	tr.SetAttributes(Attribute{Key: "some", Value: "attribute"})
 	require.Len(t, tr.attrs, 1)
@@ -71,12 +66,10 @@ func TestProvider(t *testing.T) {
 	require.NotEqual(t, context.Background(), ctx)
 	require.NotZero(t, sp)
 
-	sp.AddError(nil)
 	sp.AddEvent("event")
-	sp.End()
+	sp.End(nil)
 	sp.SetAttributes()
 	sp.SetStatus(SpanStatusError, "desc")
-	require.True(t, addErrorCalled)
 	require.True(t, addEventCalled)
 	require.True(t, endCalled)
 	require.True(t, setAttributesCalled)

sdk/tracing/azotel/ci.yml

@@ -25,4 +25,4 @@ stages:
 - template: /eng/pipelines/templates/jobs/archetype-sdk-client.yml
   parameters:
     ServiceDirectory: tracing/azotel
-    ExcludeGoNMinus3: true
+    ExcludeGoNMinus2: true

sdk/tracing/azotel/go.mod

@@ -31,3 +31,5 @@ require (
 	golang.org/x/text v0.8.0 // indirect
 	gopkg.in/yaml.v3 v3.0.1 // indirect
 )
+
+replace github.com/Azure/azure-sdk-for-go/sdk/azcore => ../../azcore

sdk/tracing/azotel/otel.go

@@ -40,8 +40,8 @@ func NewTracingProvider(tracerProvider *otelsdk.TracerProvider, opts *TracingPro
 			ctx, span := tracer.Start(ctx, spanName, trace.WithSpanKind(convertSpanKind(kind)), trace.WithAttributes(attrs...))
 			return ctx, convertSpan(span)
 		}, &tracing.TracerOptions{
-			SpanFromContext: func(ctx context.Context) (tracing.Span, bool) {
-				return convertSpan(trace.SpanFromContext(ctx)), true
+			SpanFromContext: func(ctx context.Context) tracing.Span {
+				return convertSpan(trace.SpanFromContext(ctx))
 			},
 		})
 
@@ -59,9 +59,6 @@ func convertSpan(traceSpan trace.Span) tracing.Span {
 		AddEvent: func(name string, attrs ...tracing.Attribute) {
 			traceSpan.AddEvent(name, trace.WithAttributes(convertAttributes(attrs)...))
 		},
-		AddError: func(err error) {
-			traceSpan.RecordError(err)
-		},
 		SetStatus: func(code tracing.SpanStatus, desc string) {
 			traceSpan.SetStatus(convertStatus(code), desc)
 		},

sdk/tracing/azotel/otel_test.go

@@ -8,7 +8,6 @@ package azotel
 
 import (
 	"context"
-	"io"
 	"net/http"
 	"testing"
 
@@ -37,17 +36,15 @@ func TestNewTracingProvider(t *testing.T) {
 	require.NoError(t, err)
 
 	// returns a no-op span as there is no span yet
-	emptySpan, ok := client.Tracer().SpanFromContext(context.Background())
-	assert.True(t, ok)
+	emptySpan := client.Tracer().SpanFromContext(context.Background())
 	emptySpan.AddEvent("noop_event")
 
 	ctx, endSpan := azruntime.StartSpan(context.Background(), "test_span", client.Tracer(), nil)
 
 	req, err := azruntime.NewRequest(ctx, http.MethodGet, "https://www.microsoft.com/")
 	require.NoError(t, err)
 
-	startedSpan, ok := client.Tracer().SpanFromContext(req.Raw().Context())
-	assert.True(t, ok)
+	startedSpan := client.Tracer().SpanFromContext(req.Raw().Context())
 	startedSpan.AddEvent("post_event")
 
 	_, err = client.Pipeline().Do(req)
@@ -65,9 +62,6 @@ func TestConvertSpan(t *testing.T) {
 	ts := testSpan{t: t}
 	span := convertSpan(&ts)
 
-	span.AddError(io.EOF)
-	assert.Equal(t, io.EOF, ts.recordedError)
-
 	const eventName = "event"
 	eventAttr := tracing.Attribute{
 		Key:   "key",
@@ -77,7 +71,7 @@ func TestConvertSpan(t *testing.T) {
 	assert.EqualValues(t, eventName, ts.eventName)
 	require.Len(t, ts.eventOptions, 1)
 
-	span.End()
+	span.End(nil)
 	assert.True(t, ts.endCalled)
 
 	attr := tracing.Attribute{
@@ -206,14 +200,13 @@ func (c *testExporter) Shutdown(ctx context.Context) error {
 }
 
 type testSpan struct {
-	t             *testing.T
-	attributes    []attribute.KeyValue
-	eventName     string
-	eventOptions  []trace.EventOption
-	endCalled     bool
-	recordedError error
-	statusCode    codes.Code
-	statusDesc    string
+	t            *testing.T
+	attributes   []attribute.KeyValue
+	eventName    string
+	eventOptions []trace.EventOption
+	endCalled    bool
+	statusCode   codes.Code
+	statusDesc   string
 }
 
 func (ts *testSpan) End(options ...trace.SpanEndOption) {
@@ -231,7 +224,7 @@ func (ts *testSpan) IsRecording() bool {
 }
 
 func (ts *testSpan) RecordError(err error, options ...trace.EventOption) {
-	ts.recordedError = err
+	ts.t.Fatal("RecordError not required")
 }
 
 func (ts *testSpan) SpanContext() trace.SpanContext {