-
Notifications
You must be signed in to change notification settings - Fork 292
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
[Instrumentation.EventCounters] Avoid race condition and performance trap in EventCountersMetrics #703
[Instrumentation.EventCounters] Avoid race condition and performance trap in EventCountersMetrics #703
Conversation
|
@MihaZupan Thanks for sending this PR! Could you please fix the build errors? |
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #703 +/- ##
==========================================
- Coverage 77.89% 77.66% -0.23%
==========================================
Files 175 175
Lines 5297 5310 +13
==========================================
- Hits 4126 4124 -2
- Misses 1171 1186 +15
|
d3a5c03
to
7c1f538
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM Thanks!
This PR fixes two issues with the current implementation:
EventLevel.Critical
instead ofEventLevel.LogAlways
EventSource
implementor, this change can avoid a lot of overhead (preparing and serializing event arguments, potentially taking more expensive code paths).EventCountersMetrics
instanceOnEventSourceCreated
can run before the constructor is done, you can run into a situation where you fail to enable anEventSource
instance. A simple lock avoids this race.OnEventSourceCreated
and sees options aren't set yetpreInitEventSources
EventSource
intopreInitEventSources
EventSource
instance