-
Notifications
You must be signed in to change notification settings - Fork 599
InternalLog fixes for GlobalMeterProvider #2333
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
InternalLog fixes for GlobalMeterProvider #2333
Conversation
| otel_info!(name: "MeterProvider.GlobalSet", message = "Global meter provider is set. Meters can now be created using global::meter() or global::meter_with_scope()."); | ||
| } else { | ||
| otel_error!(name: "MeterProvider.GlobalSetFailed", message = "Global meter provider is not set due to lock poison. Meters created using global::meter() or global::meter_with_scope() will not function."); | ||
| otel_error!(name: "MeterProvider.GlobalSetFailed", message = "Setting global meter provider failed. Meters created using global::meter() or global::meter_with_scope() will not function. Report this issue in OpenTelemetry repo."); |
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.
in a future PR, i'll try to make this consistent, by providing an attribute "suggested_action" where the value can inform users about actions they can do. In this case there are 2
- Report it at OpenTelemetry repro.
- Use meters from other meter provider instance, if they have access to.
This requires some time to come up with a consistent way across entire repo to ensure every warn/err logs have a clear action for end users.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2333 +/- ##
=====================================
Coverage 79.5% 79.5%
=====================================
Files 123 123
Lines 21363 21365 +2
=====================================
+ Hits 17002 17004 +2
Misses 4361 4361 ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
| provider.clone() | ||
| } else { | ||
| otel_error!(name: "MeterProvider.GlobalGetFailed", message = "Getting global meter provider failed. Meters created using global::meter() or global::meter_with_scope() will not function. Report this issue in OpenTelemetry repo."); | ||
| Arc::new(crate::metrics::noop::NoopMeterProvider::new()) |
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.
This does great, to maintain continuity for application. Additionally, we can also consider trying to recover the poisoned provider, check if it's still intact (since corruption could happen if another thread panicked while holding the lock), and return it if everything is good. Of course, this would need to be handled on a case-by-case basis, and only where recovery makes sense. Something, to be tried separate of this PR, I will have a look if it is possible.
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.
agree. we absolutely cannot panic here so this is the minimal change to meet that goal. lets see if we can recover nicely.
lalitb
left a comment
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.
Thanks for the fix.
Addresses #2331 (comment)
Adds log when Get fails due to posion.