-
Notifications
You must be signed in to change notification settings - Fork 437
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
Global error handler cleanup - MeterProvider #2237
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2237 +/- ##
=======================================
- Coverage 79.5% 79.5% -0.1%
=======================================
Files 121 121
Lines 21113 21117 +4
=======================================
+ Hits 16803 16805 +2
- Misses 4310 4312 +2 ☔ View full report in Codecov by Sentry. |
message = "Meter provider was already shut down; drop will not attempt shutdown again." | ||
); | ||
} else if let Err(err) = self.shutdown() { | ||
otel_debug!( |
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 one is a better fit for otel_error
, I guess? It's helpful to know if the metrics were not flushed out during the app/meter provider shutdown.
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.
Yes, deciding between otel_error
and otel_debug
is challenging in this case—since not all failure errors may be actionable by the user. We can make it more granular later to capture different types of errors and log them separately based on the error type. For now, it’s best to use otel_error
, as it's a one-time error during shutdown, and no harm in user knowing even if they can't do anything.
if self.is_shutdown.load(Ordering::Relaxed) { | ||
otel_debug!( | ||
name: "MeterProvider.AlreadyShutdown", | ||
message = "Meter provider was already shut down; drop will not attempt shutdown again." |
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.
nit: change semicolon to comma?
Fixes #
Design discussion issue (if applicable) #
Changes
Please provide a brief description of the changes here.
Merge requirement checklist
CHANGELOG.md
files updated for non-trivial, user-facing changes