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

Refactor ErrorHandler #2424

Merged

Conversation

MadVikingGod
Copy link
Contributor

This change was made to enable ALL updates to the ErrorHandler to propagate to every captured GetErrorHandler(). This also was an attempt to make the error handler (and eventually the global logger) simpler. It had the side effect of also making these calls more performant.

Note: When first benchmarking these changes the resetting of the timers made both old and new benchmarks take longer, and occasionally crash. All the benchmark numbers that follow are with the StopTimer() and StartTimer() removed. I can post some before and after stats, but the net effect is additional allocation, because that's what the stop and start guarded, and faster run times, lower ns/op. I don't think the timing is accurate if you pause often within the test.

Note2: I removed one benchmark, because in the new method EVERYTHING is a delegate, so there is no difference between the 1 SetErrorHandler and multiple.

What I'm seeing performance-wise from this change is:

  • A small performance degradation (16 ns) in calling the default logger.
  • A similar performance increase when calling anything but the default.
  • 1 fewer allocations for each SetErrorHandler is called.
  • A large performance increase for GetErrorHandler, this is because it always returns the same location.
  • A net performance increase in the general test.
$ benchstat old new
name                           old time/op    new time/op    delta
ErrorHandler-4                   1.98µs ± 5%    1.86µs ± 4%   -6.29%  (p=0.000 n=9+10)
GetDefaultErrorHandler-4         2.06ns ± 3%    0.78ns ± 8%  -62.03%  (p=0.000 n=9+10)
GetDelegatedErrorHandler-4       2.04ns ± 3%    0.92ns ± 6%  -55.01%  (p=0.000 n=10+9)
DefaultErrorHandlerHandle-4       289ns ± 2%     305ns ± 3%   +5.63%  (p=0.000 n=10+9)
DelegatedErrorHandlerHandle-4     313ns ± 2%     306ns ± 3%   -2.27%  (p=0.003 n=10+9)
SetErrorHandlerDelegation-4       293ns ± 4%     157ns ± 3%  -46.39%  (p=0.000 n=10+9)

name                           old alloc/op   new alloc/op   delta
ErrorHandler-4                     192B ± 0%      144B ± 0%  -25.00%  (p=0.000 n=10+10)
GetDefaultErrorHandler-4          0.00B          0.00B          ~     (all equal)
GetDelegatedErrorHandler-4        0.00B          0.00B          ~     (all equal)
DefaultErrorHandlerHandle-4       48.0B ± 0%     48.0B ± 0%     ~     (all equal)
DelegatedErrorHandlerHandle-4     48.0B ± 0%     48.0B ± 0%     ~     (all equal)
SetErrorHandlerDelegation-4        152B ± 0%      136B ± 0%  -10.53%  (p=0.000 n=10+10)

name                           old allocs/op  new allocs/op  delta
ErrorHandler-4                     9.00 ± 0%      6.00 ± 0%  -33.33%  (p=0.000 n=10+10)
GetDefaultErrorHandler-4           0.00           0.00          ~     (all equal)
GetDelegatedErrorHandler-4         0.00           0.00          ~     (all equal)
DefaultErrorHandlerHandle-4        1.00 ± 0%      1.00 ± 0%     ~     (all equal)
DelegatedErrorHandlerHandle-4      1.00 ± 0%      1.00 ± 0%     ~     (all equal)
SetErrorHandlerDelegation-4        5.00 ± 0%      4.00 ± 0%  -20.00%  (p=0.000 n=10+10)

@codecov
Copy link

codecov bot commented Nov 30, 2021

Codecov Report

Merging #2424 (c351302) into main (a4f183b) will increase coverage by 0.0%.
The diff coverage is 100.0%.

Impacted file tree graph

@@          Coverage Diff          @@
##            main   #2424   +/-   ##
=====================================
  Coverage   75.3%   75.3%           
=====================================
  Files        173     173           
  Lines      11954   11948    -6     
=====================================
- Hits        9007    9003    -4     
+ Misses      2706    2704    -2     
  Partials     241     241           
Impacted Files Coverage Δ
handler.go 100.0% <100.0%> (ø)
sdk/trace/batch_span_processor.go 84.4% <0.0%> (+1.0%) ⬆️

@MadVikingGod MadVikingGod added the Skip Changelog PRs that do not require a CHANGELOG.md entry label Dec 1, 2021
@MadVikingGod
Copy link
Contributor Author

Skipping changelog as this is a non-user facing change.

@MadVikingGod MadVikingGod merged commit 2b7c650 into open-telemetry:main Dec 6, 2021
@MadVikingGod MadVikingGod deleted the refactor-errorhandler branch December 6, 2021 21:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Skip Changelog PRs that do not require a CHANGELOG.md entry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants