Remove WithSpan method from Tracer interface (#1043)

* Remove WithSpan method from Tracer interface

Also remove implementation in SDK.

* Add panic event reporting to span End

* Update Changelog with changes

* Update CHANGELOG.md

* Update README.md

Fix code tabs

* Refactor span End

* Fix un-ended span from feedback.

CHANGELOG.md

@@ -20,11 +20,15 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm
 
 - The `grpctrace` instrumentation was moved to the `go.opentelemetry.io/contrib` repository and out of this repository.
    This move includes moving the `grpc` example to the `go.opentelemetry.io/contrib` as well. (#1027)
+- The `WithSpan` method of the `Tracer` interface.
+   The functionality this method provided was limited compared to what a user can provide themselves.
+   It was removed with the understanding that if there is sufficient user need it can be added back based on actual user usage. (#1043)
 
 ### Fixed
 
 - The `semconv.HTTPServerMetricAttributesFromHTTPRequest()` function no longer generates the high-cardinality `http.request.content.length` label. (#1031)
 - Correct instrumentation version tag in Jaeger exporter. (#1037)
+- The SDK span will now set an error event if the `End` method is called during a panic (i.e. it was deferred). (#1043)
 
 ## [0.10.0] - 2020-07-29
 

README.md

@@ -50,22 +50,9 @@ func main() {
 	defer pusher.Stop()
 
 	tracer := global.Tracer("ex.com/basic")
-
-	tracer.WithSpan(context.Background(), "foo",
-		func(ctx context.Context) error {
-			tracer.WithSpan(ctx, "bar",
-				func(ctx context.Context) error {
-					tracer.WithSpan(ctx, "baz",
-						func(ctx context.Context) error {
-							return nil
-						},
-					)
-					return nil
-				},
-			)
-			return nil
-		},
-	)
+	ctx, span := tracer.Start(context.Background(), "main")
+	defer span.End()
+	/* … */
 }
 ```
 

api/global/internal/trace.go

@@ -114,15 +114,6 @@ func (t *tracer) setDelegate(provider trace.Provider) {
 	t.once.Do(func() { t.delegate = provider.Tracer(t.name, t.opts...) })
 }
 
-// WithSpan implements trace.Tracer by forwarding the call to t.delegate if
-// set, otherwise it forwards the call to a NoopTracer.
-func (t *tracer) WithSpan(ctx context.Context, name string, body func(context.Context) error, opts ...trace.StartOption) error {
-	if t.delegate != nil {
-		return t.delegate.WithSpan(ctx, name, body, opts...)
-	}
-	return trace.NoopTracer{}.WithSpan(ctx, name, body, opts...)
-}
-
 // Start implements trace.Tracer by forwarding the call to t.delegate if
 // set, otherwise it forwards the call to a NoopTracer.
 func (t *tracer) Start(ctx context.Context, name string, opts ...trace.StartOption) (context.Context, trace.Span) {

api/global/internal/trace_test.go

@@ -31,13 +31,9 @@ func TestTraceWithSDK(t *testing.T) {
 	ctx := context.Background()
 	gtp := global.TraceProvider()
 	tracer1 := gtp.Tracer("pre")
+	// This is started before an SDK was registered and should be dropped.
 	_, span1 := tracer1.Start(ctx, "span1")
 
-	// This should be dropped.
-	if err := tracer1.WithSpan(ctx, "withSpan1", func(context.Context) error { return nil }); err != nil {
-		t.Errorf("failed to wrap function with span prior to initialization: %v", err)
-	}
-
 	sr := new(testtrace.StandardSpanRecorder)
 	tp := testtrace.NewProvider(testtrace.WithSpanRecorder(sr))
 	global.SetTraceProvider(tp)
@@ -48,17 +44,11 @@ func TestTraceWithSDK(t *testing.T) {
 	// The existing Tracer should have been configured to now use the configured SDK.
 	_, span2 := tracer1.Start(ctx, "span2")
 	span2.End()
-	if err := tracer1.WithSpan(ctx, "withSpan2", func(context.Context) error { return nil }); err != nil {
-		t.Errorf("failed to wrap function with span post initialization: %v", err)
-	}
 
 	// The global trace Provider should now create Tracers that also use the newly configured SDK.
 	tracer2 := gtp.Tracer("post")
 	_, span3 := tracer2.Start(ctx, "span3")
 	span3.End()
-	if err := tracer2.WithSpan(ctx, "withSpan3", func(context.Context) error { return nil }); err != nil {
-		t.Errorf("failed to wrap function with span post initialization with new tracer: %v", err)
-	}
 
 	filterNames := func(spans []*testtrace.Span) []string {
 		names := make([]string, len(spans))
@@ -67,7 +57,7 @@ func TestTraceWithSDK(t *testing.T) {
 		}
 		return names
 	}
-	expected := []string{"span2", "withSpan2", "span3", "withSpan3"}
+	expected := []string{"span2", "span3"}
 	assert.ElementsMatch(t, expected, filterNames(sr.Started()))
 	assert.ElementsMatch(t, expected, filterNames(sr.Completed()))
 }

api/testharness/harness.go

@@ -16,7 +16,6 @@ package testharness
 
 import (
 	"context"
-	"errors"
 	"sync"
 	"testing"
 	"time"
@@ -177,138 +176,6 @@ func (h *Harness) TestTracer(subjectFactory func() trace.Tracer) {
 		})
 	})
 
-	h.t.Run("#WithSpan", func(t *testing.T) {
-		t.Run("returns nil if the body does not return an error", func(t *testing.T) {
-			t.Parallel()
-
-			e := matchers.NewExpecter(t)
-			subject := subjectFactory()
-
-			err := subject.WithSpan(context.Background(), "test", func(ctx context.Context) error {
-				return nil
-			})
-
-			e.Expect(err).ToBeNil()
-		})
-
-		t.Run("propagates the error from the body if the body errors", func(t *testing.T) {
-			t.Parallel()
-
-			e := matchers.NewExpecter(t)
-			subject := subjectFactory()
-
-			expectedErr := errors.New("test error")
-
-			err := subject.WithSpan(context.Background(), "test", func(ctx context.Context) error {
-				return expectedErr
-			})
-
-			e.Expect(err).ToMatchError(expectedErr)
-		})
-
-		t.Run("propagates the original context to the body", func(t *testing.T) {
-			t.Parallel()
-
-			e := matchers.NewExpecter(t)
-			subject := subjectFactory()
-
-			ctxKey := testCtxKey{}
-			ctxValue := "ctx value"
-			ctx := context.WithValue(context.Background(), ctxKey, ctxValue)
-
-			var actualCtx context.Context
-
-			err := subject.WithSpan(ctx, "test", func(ctx context.Context) error {
-				actualCtx = ctx
-
-				return nil
-			})
-
-			e.Expect(err).ToBeNil()
-
-			e.Expect(actualCtx.Value(ctxKey)).ToEqual(ctxValue)
-		})
-
-		t.Run("stores a span containing the expected properties on the context provided to the body function", func(t *testing.T) {
-			t.Parallel()
-
-			e := matchers.NewExpecter(t)
-			subject := subjectFactory()
-
-			var span trace.Span
-
-			err := subject.WithSpan(context.Background(), "test", func(ctx context.Context) error {
-				span = trace.SpanFromContext(ctx)
-
-				return nil
-			})
-
-			e.Expect(err).ToBeNil()
-
-			e.Expect(span).NotToBeNil()
-
-			e.Expect(span.Tracer()).ToEqual(subject)
-			e.Expect(span.SpanContext().IsValid()).ToBeTrue()
-		})
-
-		t.Run("starts spans with unique trace and span IDs", func(t *testing.T) {
-			t.Parallel()
-
-			e := matchers.NewExpecter(t)
-			subject := subjectFactory()
-
-			var span1 trace.Span
-			var span2 trace.Span
-
-			err := subject.WithSpan(context.Background(), "span1", func(ctx context.Context) error {
-				span1 = trace.SpanFromContext(ctx)
-
-				return nil
-			})
-
-			e.Expect(err).ToBeNil()
-
-			err = subject.WithSpan(context.Background(), "span2", func(ctx context.Context) error {
-				span2 = trace.SpanFromContext(ctx)
-
-				return nil
-			})
-
-			e.Expect(err).ToBeNil()
-
-			sc1 := span1.SpanContext()
-			sc2 := span2.SpanContext()
-
-			e.Expect(sc1.TraceID).NotToEqual(sc2.TraceID)
-			e.Expect(sc1.SpanID).NotToEqual(sc2.SpanID)
-		})
-
-		t.Run("propagates a parent's trace ID through the context", func(t *testing.T) {
-			t.Parallel()
-
-			e := matchers.NewExpecter(t)
-			subject := subjectFactory()
-
-			ctx, parent := subject.Start(context.Background(), "parent")
-
-			var child trace.Span
-
-			err := subject.WithSpan(ctx, "child", func(ctx context.Context) error {
-				child = trace.SpanFromContext(ctx)
-
-				return nil
-			})
-
-			e.Expect(err).ToBeNil()
-
-			psc := parent.SpanContext()
-			csc := child.SpanContext()
-
-			e.Expect(csc.TraceID).ToEqual(psc.TraceID)
-			e.Expect(csc.SpanID).NotToEqual(psc.SpanID)
-		})
-	})
-
 	h.testSpan(subjectFactory)
 }
 
@@ -340,19 +207,6 @@ func (h *Harness) testSpan(tracerFactory func() trace.Tracer) {
 
 			return subject
 		},
-		"Span created via Tracer#WithSpan": func() trace.Span {
-			tracer := tracerFactory()
-
-			var actualCtx context.Context
-
-			_ = tracer.WithSpan(context.Background(), "test", func(ctx context.Context) error {
-				actualCtx = ctx
-
-				return nil
-			})
-
-			return trace.SpanFromContext(actualCtx)
-		},
 	}
 
 	for mechanismName, mechanism := range mechanisms {

api/trace/api.go

@@ -54,16 +54,6 @@ func WithInstrumentationVersion(version string) TracerOption {
 type Tracer interface {
 	// Start a span.
 	Start(ctx context.Context, spanName string, opts ...StartOption) (context.Context, Span)
-
-	// WithSpan wraps the execution of the fn function with a span.
-	// It starts a new span, sets it as an active span in the context,
-	// executes the fn function and closes the span before returning the result of fn.
-	WithSpan(
-		ctx context.Context,
-		spanName string,
-		fn func(ctx context.Context) error,
-		opts ...StartOption,
-	) error
 }
 
 // EndConfig provides options to set properties of span at the time of ending

api/trace/noop_trace.go

@@ -22,11 +22,6 @@ type NoopTracer struct{}
 
 var _ Tracer = NoopTracer{}
 
-// WithSpan wraps around execution of func with noop span.
-func (t NoopTracer) WithSpan(ctx context.Context, name string, body func(context.Context) error, opts ...StartOption) error {
-	return body(ctx)
-}
-
 // Start starts a noop span.
 func (NoopTracer) Start(ctx context.Context, name string, opts ...StartOption) (context.Context, Span) {
 	span := NoopSpan{}

api/trace/testtrace/tracer.go

@@ -86,10 +86,3 @@ func (t *Tracer) Start(ctx context.Context, name string, opts ...trace.StartOpti
 	}
 	return trace.ContextWithSpan(ctx, span), span
 }
-
-func (t *Tracer) WithSpan(ctx context.Context, name string, body func(ctx context.Context) error, opts ...trace.StartOption) error {
-	ctx, span := t.Start(ctx, name, opts...)
-	defer span.End()
-
-	return body(ctx)
-}

api/trace/testtrace/tracer_test.go

@@ -256,46 +256,6 @@ func TestTracer(t *testing.T) {
 			e.Expect(links[link2.SpanContext]).ToEqual(link2.Attributes)
 		})
 	})
-
-	t.Run("#WithSpan", func(t *testing.T) {
-		testTracedSpan(t, func(tracer trace.Tracer, name string) (trace.Span, error) {
-			var span trace.Span
-
-			err := tracer.WithSpan(context.Background(), name, func(ctx context.Context) error {
-				span = trace.SpanFromContext(ctx)
-
-				return nil
-			})
-
-			return span, err
-		})
-
-		t.Run("honors StartOptions", func(t *testing.T) {
-			t.Parallel()
-
-			e := matchers.NewExpecter(t)
-
-			attr1 := kv.String("a", "1")
-			attr2 := kv.String("b", "2")
-
-			subject := tp.Tracer(t.Name())
-			var span trace.Span
-			err := subject.WithSpan(context.Background(), "test", func(ctx context.Context) error {
-				span = trace.SpanFromContext(ctx)
-
-				return nil
-			}, trace.WithAttributes(attr1, attr2))
-			e.Expect(err).ToBeNil()
-
-			testSpan, ok := span.(*testtrace.Span)
-			e.Expect(ok).ToBeTrue()
-
-			attributes := testSpan.Attributes()
-			e.Expect(attributes[attr1.Key]).ToEqual(attr1.Value)
-			e.Expect(attributes[attr2.Key]).ToEqual(attr2.Value)
-		})
-
-	})
 }
 
 func testTracedSpan(t *testing.T, fn func(tracer trace.Tracer, name string) (trace.Span, error)) {

bridge/opentracing/internal/mock.go

@@ -71,12 +71,6 @@ func NewMockTracer() *MockTracer {
 	}
 }
 
-func (t *MockTracer) WithSpan(ctx context.Context, name string, body func(context.Context) error, opts ...oteltrace.StartOption) error {
-	ctx, span := t.Start(ctx, name, opts...)
-	defer span.End()
-	return body(ctx)
-}
-
 func (t *MockTracer) Start(ctx context.Context, name string, opts ...oteltrace.StartOption) (context.Context, oteltrace.Span) {
 	spanOpts := oteltrace.StartConfig{}
 	for _, opt := range opts {

bridge/opentracing/wrapper.go

@@ -71,20 +71,6 @@ func (t *WrapperTracer) otelTracer() oteltrace.Tracer {
 	return t.tracer
 }
 
-// WithSpan forwards the call to the wrapped tracer with a modified
-// body callback, which sets the active OpenTracing span before
-// calling the original callback.
-func (t *WrapperTracer) WithSpan(ctx context.Context, name string, body func(context.Context) error, opts ...oteltrace.StartOption) error {
-	return t.otelTracer().WithSpan(ctx, name, func(ctx context.Context) error {
-		span := oteltrace.SpanFromContext(ctx)
-		if spanWithExtension, ok := span.(migration.OverrideTracerSpanExtension); ok {
-			spanWithExtension.OverrideTracer(t)
-		}
-		ctx = t.bridge.ContextWithBridgeSpan(ctx, span)
-		return body(ctx)
-	}, opts...)
-}
-
 // Start forwards the call to the wrapped tracer. It also tries to
 // override the tracer of the returned span if the span implements the
 // OverrideTracerSpanExtension interface.