Modify loggerprovider shutdown, flush to return single Result and handle repeat shutdown calls #1750
Conversation
| Err(LogError::Other(format!("{errs:?}").into())) | ||
| } | ||
| } else { | ||
| Err(LogError::Other("logger provider already shut down".into())) |
There was a problem hiding this comment.
We should make a dedicate LogError variant for repeat shutdown. I image in most use cases users don't care as long as the shutdown has invoked at least once
There was a problem hiding this comment.
Agree to fix it. Not in this PR, as the current structure of placing these errors in the API is not correct, and needs a refactoring. Will cover it as part of #1042
There was a problem hiding this comment.
I'd mark this as releasing blocking and add a todo in code too. It will be very anti-intutive if the users has to parse the error string to understand if the errors is caused by repeated shutdown
There was a problem hiding this comment.
I'd mark this as releasing blocking
yes for stable or even beta release. But not for #1738 which is still alpha for logs.
There was a problem hiding this comment.
Sorry if this is already answered. What is wrong with returning vec of LogResult for shutdown? Is it only for consistency with metrics?
There was a problem hiding this comment.
yes. And also, if any individual sub-component faced issue while shutdown, they should be emitting their own logging. Shutdown() just need to capture the overall state. But I am not very sure we are doing it correctly. The proper solution should be coming once we address #1146 (that also is very much related to #761 as it is hard to solve internal logging without fixing the infinite telemetry/circular dep problem)
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1750 +/- ##
=======================================
+ Coverage 71.0% 71.6% +0.5%
=======================================
Files 135 136 +1
Lines 20751 20845 +94
=======================================
+ Hits 14746 14934 +188
+ Misses 6005 5911 -94 ☔ View full report in Codecov by Sentry. |
This is consistent with Metrics shutdown.
The overall result contains formatted version of combined errors, if any.
Added test to validate that repeated shutdown calls are handled.