MeterProvider to return specific MetricError #2461
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2461 +/- ##
=======================================
- Coverage 76.8% 76.8% -0.1%
=======================================
Files 122 122
Lines 22217 22231 +14
=======================================
+ Hits 17082 17086 +4
- Misses 5135 5145 +10 ☔ View full report in Codecov by Sentry. |
| Err(MetricError::Other( | ||
| "Cannot perform flush as MeterProvider shutdown already invoked.".into(), | ||
| )) | ||
| Err(MetricError::AlreadyShutdown) |
There was a problem hiding this comment.
Just to be clear, is this approach different from the one proposed in #2381, where we were using separate enum types for Shutdown, and possibly ForceFlush as well? In this case, the proposal is to use the same MetricError enum for the entire user-facing interface, correct?
There was a problem hiding this comment.
I believe it is a best practice to have error-per-call when there is a clear disunity of potential errors, like there is between shutdown and export (but probably not between export and forceFlush).
As it stands, shutdown() can return two errors - AlreadyShutdown and ShutdownTimeout (plus the common boxed one for PoisonError).
By adding these types to MetricError we're promising the caller that shutdown() may also return any of Config, ExportErr, InvalidInstrumentConfiguration errors. This is simply not true of the interface, and places extra burden on the caller to handle, leaving the caller with these options:
- 1/ Treat all errors as unrecoverable - simply match error
- 2/ Extensively dig through our code, work out that
AlreadyShutdownandShutdownTimeoutare two that they care about handling, and_ =>to ignore the rest - 3/ Exhaustively match all the arms without realising that a bunch of them will never be invoked
There was a problem hiding this comment.
By adding these types to MetricError we're promising the caller that shutdown() may also return any of Config, ExportErr, InvalidInstrumentConfiguration errors.
Agree this PR is the wrong approach. Thanks for the feedback. Need to rework.
There was a problem hiding this comment.
Hey @cijothomas I think I overindexed on the single error type - sorry about that. I don't think this needs a dramatic rework!
What you have done with the named-logical-errors makes sense to me! I stole it and lifted it into the other PR for metrics/traces :D
Related to #2381
Trying to play with better Error handling, happy to get feedback on the approach..