Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow global ErrorHandler to be set multiple times #2160

Merged
merged 8 commits into from
Aug 6, 2021

Conversation

MrAlias
Copy link
Contributor

@MrAlias MrAlias commented Aug 3, 2021

Resolves #2140

These changes allow a user to set the global ErrorHandler multiple times. This is done by storing the state in an atomic.Value.

This also adds a benchmark to track performance and overhead of these changes. The increase in memory usage and allocations appears related to the setting operation. Prior to the changes setting a global ErrorHandler had no memory overhead and with these changes it requires an allocation. This seems acceptable given this code correctly allows multiple setting of an ErrorHandler and this operation is expected to be called a limited amount of times during initialization of user code.

Benchmark Before

$ go test -timeout 300s -bench=.
goos: linux
goarch: amd64
pkg: go.opentelemetry.io/otel
cpu: Intel(R) Core(TM) i7-8550U CPU @ 1.80GHz
BenchmarkErrorHandler-8                  	  542686	      2256 ns/op	     144 B/op	       6 allocs/op
BenchmarkGetDefaultErrorHandler-8        	1000000000	         0.7989 ns/op	       0 B/op	       0 allocs/op
BenchmarkGetDelegatedErrorHandler-8      	1000000000	         0.9686 ns/op	       0 B/op	       0 allocs/op
BenchmarkDefaultErrorHandlerHandle-8     	 2890158	       415.4 ns/op	      48 B/op	       1 allocs/op
BenchmarkDelegatedErrorHandlerHandle-8   	 4011504	       303.8 ns/op	      48 B/op	       1 allocs/op
BenchmarkSetErrorHandlerDelegation-8     	 7772974	       158.2 ns/op	       0 B/op	       0 allocs/op
BenchmarkSetErrorHandlerNoDelegation-8   	344574063	         3.454 ns/op	       0 B/op	       0 allocs/op

Benchmark After

$ go test -timeout=300s -bench=.
goos: linux
goarch: amd64
pkg: go.opentelemetry.io/otel
cpu: Intel(R) Core(TM) i7-8550U CPU @ 1.80GHz
BenchmarkErrorHandler-8                  	  480949	      2306 ns/op	     176 B/op	       8 allocs/op
BenchmarkGetDefaultErrorHandler-8        	660670377	         1.887 ns/op	       0 B/op	       0 allocs/op
BenchmarkGetDelegatedErrorHandler-8      	627903168	         1.903 ns/op	       0 B/op	       0 allocs/op
BenchmarkDefaultErrorHandlerHandle-8     	 4298862	       288.5 ns/op	      48 B/op	       1 allocs/op
BenchmarkDelegatedErrorHandlerHandle-8   	 3892299	       301.8 ns/op	      48 B/op	       1 allocs/op
BenchmarkSetErrorHandlerDelegation-8     	 3984223	       342.1 ns/op	      16 B/op	       1 allocs/op
BenchmarkSetErrorHandlerNoDelegation-8   	25017502	        46.58 ns/op	      16 B/op	       1 allocs/op

@codecov
Copy link

codecov bot commented Aug 4, 2021

Codecov Report

Merging #2160 (4ba96a6) into main (d18c135) will increase coverage by 0.0%.
The diff coverage is 100.0%.

Impacted file tree graph

@@          Coverage Diff          @@
##            main   #2160   +/-   ##
=====================================
  Coverage   72.2%   72.2%           
=====================================
  Files        176     176           
  Lines      12088   12091    +3     
=====================================
+ Hits        8731    8741   +10     
+ Misses      3115    3110    -5     
+ Partials     242     240    -2     
Impacted Files Coverage Δ
handler.go 100.0% <100.0%> (+28.0%) ⬆️

handler.go Show resolved Hide resolved
@MrAlias MrAlias merged commit 56c743b into open-telemetry:main Aug 6, 2021
@MrAlias MrAlias deleted the global-err branch August 6, 2021 17:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Inconsistent global set behavior
4 participants