Repository navigation
fix: finish shutdown after a provider error - #158
bruno-espino wants to merge 3 commits into
Conversation
Exercise explicit guard shutdown and guard drop after a trace provider error. Check the metric export, log export and runtime shutdown signal.
`Logfire::shutdown()` returns on the first provider error. This skips the remaining providers and the background runtime shutdown signal. Attempt all provider shutdowns and send the runtime signal before returning the first error. The explicit shutdown and guard drop regressions fail before the fix and pass after it. Each checks a counter, a log and the runtime signal. Workspace tests, Clippy and repository hooks pass. This change does not add a shutdown deadline or handle provider panics.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
| } | ||
|
|
||
| #[cfg(test)] | ||
| mod shutdown_failure_regression { |
There was a problem hiding this comment.
I don't really know we need this change tested to such a degree. Let's omit the tests but leave the main change
| for result in results { | ||
| result?; | ||
| } | ||
|
|
||
| Ok(()) |
There was a problem hiding this comment.
I wonder if we should change ShutdownError to be
struct ShutdownError {
tracing: Optional<OTelSdkError>,
metrics: Optional<OTelSdkError>,
logging: Optional<OTelSdkError>,
}so that we can capture all the errors, if multiple failed. Or we could just have a Vec<OTelSdkError> and not distinguish from which source the error came.
@cetra3 would be interested in your opinion whether we need to bother capturing all of the errors.
There was a problem hiding this comment.
Happy to add the variant here if you and @cetra3 want it, otherwise it can be a follow up
| if let Ok(mut sender_guard) = self.0.shutdown_sender.lock() | ||
| && let Some(sender) = sender_guard.take() | ||
| { | ||
| let _ = sender.send(()); |
There was a problem hiding this comment.
In principle the background runtime thread could fail to shutdown, I wonder whether we want to try to wait for that shutdown and capture any shutdown error.
Logfire::shutdown()returns on the first provider error. This skips the remaining providers and the background runtime shutdown signal. Attempt all provider shutdowns and send the runtime signal before returning the first error.