Call otel.Handle with non-nil errors (#1384)

* Call otel.Handle with non-nil errors That's what normally happens in other call sites. Those two didn't check it, but passed the "error" to Handle. The default, delegating implementation of ErrorHandler was printing the error unconditionally, which resulted in pointless lines like `2020/12/07 19:51:28 <nil>` in demos, for example. * Add tests Co-authored-by: Tyler Yahn <[email protected]>
open-telemetry · Dec 11, 2020 · af114ba · af114ba
1 parent c3c4273
commit af114ba
Show file tree

Hide file tree

Showing 3 changed files with 60 additions and 9 deletions.
diff --git a/sdk/trace/provider.go b/sdk/trace/provider.go
@@ -144,7 +144,9 @@ func (p *TracerProvider) UnregisterSpanProcessor(s SpanProcessor) {
 	}
 	if stopOnce != nil {
 		stopOnce.state.Do(func() {
-			otel.Handle(s.Shutdown(context.Background()))
+			if err := s.Shutdown(context.Background()); err != nil {
+				otel.Handle(err)
+			}
 		})
 	}
 	if len(new) > 1 {
@@ -192,7 +194,9 @@ func (p *TracerProvider) Shutdown(ctx context.Context) error {
 
 	for _, sps := range spss {
 		sps.state.Do(func() {
-			otel.Handle(sps.sp.Shutdown(ctx))
+			if err := sps.sp.Shutdown(ctx); err != nil {
+				otel.Handle(err)
+			}
 		})
 	}
 	return nil

diff --git a/sdk/trace/provider_test.go b/sdk/trace/provider_test.go
@@ -16,16 +16,20 @@ package trace
 
 import (
 	"context"
+	"errors"
 	"testing"
+
+	"github.com/stretchr/testify/assert"
 )
 
 type basicSpanProcesor struct {
-	running bool
+	running             bool
+	injectShutdownError error
 }
 
 func (t *basicSpanProcesor) Shutdown(context.Context) error {
 	t.running = false
-	return nil
+	return t.injectShutdownError
 }
 
 func (t *basicSpanProcesor) OnStart(parent context.Context, s ReadWriteSpan) {}
@@ -45,3 +49,36 @@ func TestShutdownTraceProvider(t *testing.T) {
 		t.Errorf("Error shutdown basicSpanProcesor\n")
 	}
 }
+
+func TestFailedProcessorShutdown(t *testing.T) {
+	handler.Reset()
+	stp := NewTracerProvider()
+	spErr := errors.New("basic span processor shutdown failure")
+	sp := &basicSpanProcesor{
+		running:             true,
+		injectShutdownError: spErr,
+	}
+	stp.RegisterSpanProcessor(sp)
+
+	_ = stp.Shutdown(context.Background())
+
+	assert.Contains(t, handler.errs, spErr)
+}
+
+func TestFailedProcessorShutdownInUnregister(t *testing.T) {
+	handler.Reset()
+	stp := NewTracerProvider()
+	spErr := errors.New("basic span processor shutdown failure")
+	sp := &basicSpanProcesor{
+		running:             true,
+		injectShutdownError: spErr,
+	}
+	stp.RegisterSpanProcessor(sp)
+	stp.UnregisterSpanProcessor(sp)
+
+	assert.Contains(t, handler.errs, spErr)
+
+	handler.errs = nil
+	_ = stp.Shutdown(context.Background())
+	assert.Empty(t, handler.errs)
+}
diff --git a/sdk/trace/trace_test.go b/sdk/trace/trace_test.go
@@ -41,20 +41,30 @@ import (
 	"go.opentelemetry.io/otel/sdk/resource"
 )
 
+type storingHandler struct {
+	errs []error
+}
+
+func (s *storingHandler) Handle(err error) {
+	s.errs = append(s.errs, err)
+}
+
+func (s *storingHandler) Reset() {
+	s.errs = nil
+}
+
 var (
 	tid trace.TraceID
 	sid trace.SpanID
-)
 
-type discardHandler struct{}
-
-func (*discardHandler) Handle(_ error) {}
+	handler *storingHandler = &storingHandler{}
+)
 
 func init() {
 	tid, _ = trace.TraceIDFromHex("01020304050607080102040810203040")
 	sid, _ = trace.SpanIDFromHex("0102040810203040")
 
-	otel.SetErrorHandler(new(discardHandler))
+	otel.SetErrorHandler(handler)
 }
 
 func TestTracerFollowsExpectedAPIBehaviour(t *testing.T) {