diff --git a/CHANGELOG.md b/CHANGELOG.md index c8a0e8fd41d..037f9c0b056 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/handler.go b/handler.go index 27e1caa30d9..238f9de12fb 100644 --- a/handler.go +++ b/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{ - l: log.New(os.Stderr, "", log.LstdFlags), - } + globalErrorHandler = defaultErrorHandler() // delegateErrorHandlerOnce ensures that a user provided ErrorHandler is // only ever registered once. delegateErrorHandlerOnce sync.Once - // Comiple time check that loggingErrorHandler implements ErrorHandler. - _ ErrorHandler = (*loggingErrorHandler)(nil) + // Compile-time check that delegator implements ErrorHandler. + _ ErrorHandler = (*delegator)(nil) ) -// loggingErrorHandler logs all errors to STDERR. -type loggingErrorHandler struct { +type holder 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. -func (h *loggingErrorHandler) setDelegate(d ErrorHandler) { - if h.delegate.Load() != nil { - // Delegate already registered - return - } +// setDelegate sets the ErrorHandler delegate. +func (h *delegator) setDelegate(d ErrorHandler) { + // It is critical this is guarded with delegateErrorHandlerOnce, if it is + // called again with a different concrete type it will panic. h.delegate.Store(d) } -// Handle implements ErrorHandler. -func (h *loggingErrorHandler) Handle(err error) { +// Handle logs err if no delegate is set, otherwise it is delegated. +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 -// instance has been set (`SetErrorHandler`), the default ErrorHandler which -// logs errors to STDERR is returned. +// GetErrorHandler returns the global ErrorHandler instance. +// +// 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) } diff --git a/handler_test.go b/handler_test.go index 5268ac6aa90..531ec64c195 100644 --- a/handler_test.go +++ b/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 - globalErrorHandler = &loggingErrorHandler{ - l: log.New(s.errLogger, "", 0), - } + s.origHandler = globalErrorHandler.Load().(holder).eh + + 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) TestDelegatedHandler() { + eh := GetErrorHandler() + + 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 := func() func() { - err := errors.New("") - return func() { - Handle(err) - } - }() - - causeErr() + 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{} +}