Allow global ErrorHandler to be set multiple times (#2160)

* Add benchmarks for error handler

* All multiple global ErrorHandler sets

* Use ioutil instead of io for Discard

* Add changes to changelog

* Add unit tests for delegation

CHANGELOG.md

@@ -22,7 +22,8 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm
 
 ### Fixed
 
-The `fromEnv` detector no longer throws an error when `OTEL_RESOURCE_ATTRIBUTES` environment variable is not set or empty. (#2138)
+- The `fromEnv` detector no longer throws an error when `OTEL_RESOURCE_ATTRIBUTES` environment variable is not set or empty. (#2138)
+- Setting the global `ErrorHandler` with `"go.opentelemetry.io/otel".SetErrorHandler` multiple times is now supported. (#2160, #2140)
 
 ### Security
 

handler.go

@@ -26,36 +26,42 @@ var (
 	// throughout an OpenTelemetry instrumented project. When a user
 	// specified ErrorHandler is registered (`SetErrorHandler`) all calls to
 	// `Handle` and will be delegated to the registered ErrorHandler.
-	globalErrorHandler = &loggingErrorHandler{
+	globalErrorHandler = defaultErrorHandler()
-		l: log.New(os.Stderr, "", log.LstdFlags),
-	}
 
 	// delegateErrorHandlerOnce ensures that a user provided ErrorHandler is
 	// only ever registered once.
 	delegateErrorHandlerOnce sync.Once
 
-	// Comiple time check that loggingErrorHandler implements ErrorHandler.
+	// Compile-time check that delegator implements ErrorHandler.
-	_ ErrorHandler = (*loggingErrorHandler)(nil)
+	_ ErrorHandler = (*delegator)(nil)
 )
 
-// loggingErrorHandler logs all errors to STDERR.
+type holder struct {
-type loggingErrorHandler struct {
+	eh ErrorHandler
+}
+
+func defaultErrorHandler() *atomic.Value {
+	v := &atomic.Value{}
+	v.Store(holder{eh: &delegator{l: log.New(os.Stderr, "", log.LstdFlags)}})
+	return v
+}
+
+// delegator logs errors if no delegate is set, otherwise they are delegated.
+type delegator struct {
 	delegate atomic.Value
 
 	l *log.Logger
 }
 
-// setDelegate sets the ErrorHandler delegate if one is not already set.
+// setDelegate sets the ErrorHandler delegate.
-func (h *loggingErrorHandler) setDelegate(d ErrorHandler) {
+func (h *delegator) setDelegate(d ErrorHandler) {
-	if h.delegate.Load() != nil {
+	// It is critical this is guarded with delegateErrorHandlerOnce, if it is
-		// Delegate already registered
+	// called again with a different concrete type it will panic.
-		return
-	}
 	h.delegate.Store(d)
 }
 
-// Handle implements ErrorHandler.
+// Handle logs err if no delegate is set, otherwise it is delegated.
-func (h *loggingErrorHandler) Handle(err error) {
+func (h *delegator) Handle(err error) {
 	if d := h.delegate.Load(); d != nil {
 		d.(ErrorHandler).Handle(err)
 		return
@@ -63,27 +69,39 @@ func (h *loggingErrorHandler) Handle(err error) {
 	h.l.Print(err)
 }
 
-// GetErrorHandler returns the global ErrorHandler instance. If no ErrorHandler
+// GetErrorHandler returns the global ErrorHandler instance.
-// instance has been set (`SetErrorHandler`), the default ErrorHandler which
+//
-// logs errors to STDERR is returned.
+// The default ErrorHandler instance returned will log all errors to STDERR
+// until an override ErrorHandler is set with SetErrorHandler. All
+// ErrorHandler returned prior to this will automatically forward errors to
+// the set instance instead of logging.
+//
+// Subsequent calls to SetErrorHandler after the first will not forward errors
+// to the new ErrorHandler for prior returned instances.
 func GetErrorHandler() ErrorHandler {
-	return globalErrorHandler
+	return globalErrorHandler.Load().(holder).eh
 }
 
-// SetErrorHandler sets the global ErrorHandler to be h.
+// SetErrorHandler sets the global ErrorHandler to h.
+//
+// The first time this is called all ErrorHandler previously returned from
+// GetErrorHandler will send errors to h instead of the default logging
+// ErrorHandler. Subsequent calls will set the global ErrorHandler, but not
+// delegate errors to h.
 func SetErrorHandler(h ErrorHandler) {
 	delegateErrorHandlerOnce.Do(func() {
 		current := GetErrorHandler()
 		if current == h {
 			return
 		}
-		if internalHandler, ok := current.(*loggingErrorHandler); ok {
+		if internalHandler, ok := current.(*delegator); ok {
 			internalHandler.setDelegate(h)
 		}
 	})
+	globalErrorHandler.Store(holder{eh: h})
 }
 
-// Handle is a convience function for ErrorHandler().Handle(err)
+// Handle is a convenience function for ErrorHandler().Handle(err)
 func Handle(err error) {
 	GetErrorHandler().Handle(err)
 }

handler_test.go

@@ -17,7 +17,10 @@ package otel
 import (
 	"bytes"
 	"errors"
+	"io/ioutil"
 	"log"
+	"sync"
+	"sync/atomic"
 	"testing"
 
 	"github.com/stretchr/testify/suite"
@@ -39,29 +42,46 @@ func (l *errLogger) Got() []string {
 	return []string(*l)
 }
 
+type logger struct {
+	l *log.Logger
+}
+
+func (l *logger) Handle(err error) {
+	l.l.Print(err)
+}
+
+func causeErr(text string) {
+	Handle(errors.New(text))
+}
+
 type HandlerTestSuite struct {
 	suite.Suite
 
-	origHandler *loggingErrorHandler
+	origHandler ErrorHandler
 	errLogger   *errLogger
 }
 
 func (s *HandlerTestSuite) SetupSuite() {
 	s.errLogger = new(errLogger)
-	s.origHandler = globalErrorHandler
+	s.origHandler = globalErrorHandler.Load().(holder).eh
-	globalErrorHandler = &loggingErrorHandler{
+
-		l: log.New(s.errLogger, "", 0),
+	globalErrorHandler.Store(holder{eh: &delegator{l: log.New(s.errLogger, "", 0)}})
-	}
 }
 
 func (s *HandlerTestSuite) TearDownSuite() {
-	globalErrorHandler = s.origHandler
+	globalErrorHandler.Store(holder{eh: s.origHandler})
+	delegateErrorHandlerOnce = sync.Once{}
 }
 
 func (s *HandlerTestSuite) SetupTest() {
 	s.errLogger.Reset()
 }
 
+func (s *HandlerTestSuite) TearDownTest() {
+	globalErrorHandler.Store(holder{eh: &delegator{l: log.New(s.errLogger, "", 0)}})
+	delegateErrorHandlerOnce = sync.Once{}
+}
+
 func (s *HandlerTestSuite) TestGlobalHandler() {
 	errs := []string{"one", "two"}
 	GetErrorHandler().Handle(errors.New(errs[0]))
@@ -69,29 +89,178 @@ func (s *HandlerTestSuite) TestGlobalHandler() {
 	s.Assert().Equal(errs, s.errLogger.Got())
 }
 
-func (s *HandlerTestSuite) TestNoDropsOnDelegate() {
+func (s *HandlerTestSuite) TestDelegatedHandler() {
-	causeErr := func() func() {
+	eh := GetErrorHandler()
-		err := errors.New("")
-		return func() {
-			Handle(err)
-		}
-	}()
 
-	causeErr()
+	newErrLogger := new(errLogger)
+	SetErrorHandler(&logger{l: log.New(newErrLogger, "", 0)})
+
+	errs := []string{"TestDelegatedHandler"}
+	eh.Handle(errors.New(errs[0]))
+	s.Assert().Equal(errs, newErrLogger.Got())
+}
+
+func (s *HandlerTestSuite) TestSettingDefaultIsANoOp() {
+	SetErrorHandler(GetErrorHandler())
+	d := globalErrorHandler.Load().(holder).eh.(*delegator)
+	s.Assert().Nil(d.delegate.Load())
+}
+
+func (s *HandlerTestSuite) TestNoDropsOnDelegate() {
+	causeErr("")
 	s.Require().Len(s.errLogger.Got(), 1)
 
 	// Change to another Handler. We are testing this is loss-less.
 	newErrLogger := new(errLogger)
-	secondary := &loggingErrorHandler{
+	secondary := &logger{
 		l: log.New(newErrLogger, "", 0),
 	}
 	SetErrorHandler(secondary)
 
-	causeErr()
+	causeErr("")
 	s.Assert().Len(s.errLogger.Got(), 1, "original Handler used after delegation")
 	s.Assert().Len(newErrLogger.Got(), 1, "new Handler not used after delegation")
 }
 
+func (s *HandlerTestSuite) TestAllowMultipleSets() {
+	notUsed := new(errLogger)
+
+	secondary := &logger{l: log.New(notUsed, "", 0)}
+	SetErrorHandler(secondary)
+	s.Require().Same(GetErrorHandler(), secondary, "new Handler not set")
+
+	tertiary := &logger{l: log.New(notUsed, "", 0)}
+	SetErrorHandler(tertiary)
+	s.Assert().Same(GetErrorHandler(), tertiary, "user Handler not overridden")
+}
+
 func TestHandlerTestSuite(t *testing.T) {
 	suite.Run(t, new(HandlerTestSuite))
 }
+
+func BenchmarkErrorHandler(b *testing.B) {
+	primary := &delegator{l: log.New(ioutil.Discard, "", 0)}
+	secondary := &logger{l: log.New(ioutil.Discard, "", 0)}
+	tertiary := &logger{l: log.New(ioutil.Discard, "", 0)}
+
+	globalErrorHandler.Store(holder{eh: primary})
+
+	err := errors.New("BenchmarkErrorHandler")
+
+	b.ReportAllocs()
+	b.ResetTimer()
+	for i := 0; i < b.N; i++ {
+		GetErrorHandler().Handle(err)
+		Handle(err)
+
+		SetErrorHandler(secondary)
+		GetErrorHandler().Handle(err)
+		Handle(err)
+
+		SetErrorHandler(tertiary)
+		GetErrorHandler().Handle(err)
+		Handle(err)
+
+		b.StopTimer()
+		primary.delegate = atomic.Value{}
+		globalErrorHandler.Store(holder{eh: primary})
+		delegateErrorHandlerOnce = sync.Once{}
+		b.StartTimer()
+	}
+
+	b.StopTimer()
+	reset()
+}
+
+var eh ErrorHandler
+
+func BenchmarkGetDefaultErrorHandler(b *testing.B) {
+	b.ReportAllocs()
+	for i := 0; i < b.N; i++ {
+		eh = GetErrorHandler()
+	}
+}
+
+func BenchmarkGetDelegatedErrorHandler(b *testing.B) {
+	SetErrorHandler(&logger{l: log.New(ioutil.Discard, "", 0)})
+
+	b.ReportAllocs()
+	b.ResetTimer()
+	for i := 0; i < b.N; i++ {
+		eh = GetErrorHandler()
+	}
+
+	b.StopTimer()
+	reset()
+}
+
+func BenchmarkDefaultErrorHandlerHandle(b *testing.B) {
+	globalErrorHandler.Store(holder{
+		eh: &delegator{l: log.New(ioutil.Discard, "", 0)},
+	})
+
+	eh := GetErrorHandler()
+	err := errors.New("BenchmarkDefaultErrorHandlerHandle")
+
+	b.ReportAllocs()
+	b.ResetTimer()
+	for i := 0; i < b.N; i++ {
+		eh.Handle(err)
+	}
+
+	b.StopTimer()
+	reset()
+}
+
+func BenchmarkDelegatedErrorHandlerHandle(b *testing.B) {
+	eh := GetErrorHandler()
+	SetErrorHandler(&logger{l: log.New(ioutil.Discard, "", 0)})
+	err := errors.New("BenchmarkDelegatedErrorHandlerHandle")
+
+	b.ReportAllocs()
+	b.ResetTimer()
+	for i := 0; i < b.N; i++ {
+		eh.Handle(err)
+	}
+
+	b.StopTimer()
+	reset()
+}
+
+func BenchmarkSetErrorHandlerDelegation(b *testing.B) {
+	alt := &logger{l: log.New(ioutil.Discard, "", 0)}
+
+	b.ReportAllocs()
+	b.ResetTimer()
+	for i := 0; i < b.N; i++ {
+		SetErrorHandler(alt)
+
+		b.StopTimer()
+		reset()
+		b.StartTimer()
+	}
+}
+
+func BenchmarkSetErrorHandlerNoDelegation(b *testing.B) {
+	eh := []ErrorHandler{
+		&logger{l: log.New(ioutil.Discard, "", 0)},
+		&logger{l: log.New(ioutil.Discard, "", 0)},
+	}
+	mod := len(eh)
+	// Do not measure delegation.
+	SetErrorHandler(eh[1])
+
+	b.ReportAllocs()
+	b.ResetTimer()
+	for i := 0; i < b.N; i++ {
+		SetErrorHandler(eh[i%mod])
+	}
+
+	b.StopTimer()
+	reset()
+}
+
+func reset() {
+	globalErrorHandler = defaultErrorHandler()
+	delegateErrorHandlerOnce = sync.Once{}
+}