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 <MrAlias@users.noreply.github.com>

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

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)
+}

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
+
+	handler *storingHandler = &storingHandler{}
 )
 
-type discardHandler struct{}
-
-func (*discardHandler) Handle(_ error) {}
-
 func init() {
 	tid, _ = trace.TraceIDFromHex("01020304050607080102040810203040")
 	sid, _ = trace.SpanIDFromHex("0102040810203040")
 
-	otel.SetErrorHandler(new(discardHandler))
+	otel.SetErrorHandler(handler)
 }
 
 func TestTracerFollowsExpectedAPIBehaviour(t *testing.T) {