fix: make shutdown_with_timeout required on LogProcessor
#3133
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3133 +/- ##
=====================================
Coverage 80.6% 80.6%
=====================================
Files 126 126
Lines 22195 22218 +23
=====================================
+ Hits 17898 17919 +21
- Misses 4297 4299 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
davidhewitt
changed the title
shutdown_with_timeout required on LogProcessorshutdown_with_timeout required on LogProcessor
| fn shutdown_with_timeout(&self, _timeout: Duration) -> OTelSdkResult { | ||
| Ok(()) | ||
| } | ||
| fn shutdown_with_timeout(&self, _timeout: Duration) -> OTelSdkResult; |
There was a problem hiding this comment.
we cannot make this change, as this is breaking.
There was a problem hiding this comment.
I documented it in the changelog as breaking, am fully aware.
I defend it as a good change because as evidenced here there are existing bugs in production where implementations got that wrong.
The span processor trait does not have this implementation, so removing it improves consistency too.
But if you prefer I can close the PR.
There was a problem hiding this comment.
Logs (and Metrics) SDK were declared stable, so we should not make any breaking change.
(trace/span sdk is beta, so we can fix anything there.)
there are existing bugs in production where implementations got that wrong.
Given we can't make breaking change, only option is to better document this so custom processor authors' won't make the mistake!
The existing bug (in this repo) is in an experimental component only right?
But if you prefer I can close the PR.
Let's fix the bug by implementing the right shutdown method in log_processor - that should be a good fix and no breaking change issues.
Agree it's not great. IIRC, we did have some discussions about providing default implementations before declaring stable, and ended up with a less-than-ideal choice :(
There was a problem hiding this comment.
Understood. I'll aim to come back to this PR probably on Monday.
The existing bug (in this repo) is in an experimental component only right?
Unfortunately when running this change I found it also affects SimpleLogProcessor.
Agree it's not great. IIRC, we did have some discussions about providing default implementations before declaring stable, and ended up with a less-than-ideal choice :(
Ouch.
I guess we can document that shutdown usually should not be implemented and shutdown_with_timeout should be? Can we change the default implementation of shutdown_with_timeout to log a warning? Or is that still too breaking?
There was a problem hiding this comment.
adding a log is fine. It is not common for users to write processors that require implementing any of these.. Most use cases are around enrichment (adding more attributes), and they don't need to worry about shutdown method at all.
There was a problem hiding this comment.
I pushed a commit which re-adds this default implementation with an internal warning log.
Fixes #3132
Changes
Removes the default implementation of
shutdown_with_timeoutfromLogProcessor, to matchSpanProcessorwhich also requires this method.This fixes two non-test
LogProcessorimplementations which had implementedshutdownbut notshutdown_with_timeout, meaning that they were doing a no-op shutdown as identified in #3132:SimpleLogProcessorBatchLogProcessorMerge requirement checklist
CHANGELOG.mdfiles updated for non-trivial, user-facing changes