Better handling of shutdown in BatchLogProcessor #2581
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2581 +/- ##
=======================================
- Coverage 79.6% 79.5% -0.1%
=======================================
Files 118 118
Lines 22486 22490 +4
=======================================
- Hits 17906 17888 -18
- Misses 4580 4602 +22 ☔ View full report in Codecov by Sentry. |
lalitb
reviewed
Jan 31, 2025
| // If the control message could not be sent, emit a warning. | ||
| otel_debug!( | ||
| name: "BatchLogProcessor.ForceFlush.ControlChannelFull", | ||
| message = "Control message to flush the worker thread could not be sent as the control channel is full. This can occur if user repeatedily calls force_flush without finishing the previous call." |
There was a problem hiding this comment.
I think can also happen if user repeatedly calls the shutdown from different threads, and the first shutdown message is not yet processed, so channel is still intact :)
There was a problem hiding this comment.
that is a good point. Let me modify error message to include that.
There was a problem hiding this comment.
will do in separate PR to keep this merged asap.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Another attempt at #2462
The key change/idea here is
Specifically, in this PR, BatchLogProcessor no longer keeps track of "is_shutdown" in the AtomicBool and check it in every hot path. (1 above).
Instead, it relies on the well known error from the Channel ("failed to send message to channel as no receiver exists") to determine that Shutdown is already performed (2 above).
No change in behavior, just avoid an unnecessary check in hot path. (the perf impact is minimal (couple of ns) but every ns counts)
If this is generally agreed, we need to replicate this idea everywhere.
@scottgerring Unfortunately this touches Log::Shutdown area (these changes were sitting in me for quite a while!!). I hope the changes here will allow you to return better, targeted Result as we agreed. Currently it sticks with LogError for everything. If it turns out to be merge nightmare, I am happy to abandon this one, and re-attempt it after you fix Result handling. (This has no public API change, so can be done later too)