From af114baf6b5cad9c71adc01d277d173bc71e6d44 Mon Sep 17 00:00:00 2001
From: Krzesimir Nowak <krzesimir@kinvolk.io>
Date: Fri, 11 Dec 2020 06:28:41 +0100
Subject: [PATCH] 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      |  8 ++++++--
 sdk/trace/provider_test.go | 41 ++++++++++++++++++++++++++++++++++++--
 sdk/trace/trace_test.go    | 20 ++++++++++++++-----
 3 files changed, 60 insertions(+), 9 deletions(-)

diff --git a/sdk/trace/provider.go b/sdk/trace/provider.go
index 4c6e6e2ecf0..901e8d4a76d 100644
--- 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
index ea0a2f870d5..32bec2fb75c 100644
--- 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
index 462978f8181..22ff6b20fb9 100644
--- 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) {