Minor Logging tweaks #2895
Minor Logging tweaks #2895
Conversation
Multiple times we've had users wonder why they see `Error handling message from.*; ignoring: Couldn't find channel for update` in their logs and wonder if its related to their channel force-closing. While this does indicate a peer is sending us gossip our of order (and thus misbehaving), its not relevant to channel operation and the logged message and level should indicate that. Thus, here, we move the level to Gossip and add "gossip" between "handling" and "message" (so it reads "Error handling gossip message from.*"). Fixes lightningdevkit#2471
On a high-traffic/channel node, `Channel .* does not qualify for a feerate change.*` is our most common log, and it doesn't provide much useful information. It's logged in two cases - (a) where the estimator feerate is less than the current channel feerate but not by more than half twice and (b) where we'd like to update the channel feerate but the peer is disconnected or channel not available for updates. Because these conditions can persist and we log them once a minute the volume of logs can add up quickly. Here we simply remove the log in case (a), though leave (b) as its anticipated to be somewhat quieter and does indicate a persistent issue that should be addressed (possibly by closing the channel).
On each block, for each `ChannelMonitor`, we log a status statement in `OnChainTx::update_claims_view_from_requests`. This can add up to quite a bit, and is generally not very interesting when we don't actually do anything if there's no claims to bump. Here we drop the log if we have no claims to work with, but retain it if we process any claims.
On each block, for each `ChannelMonitor`, we log two status statements in `OnChainTx::update_claims_view_from_matched_txn`. This can add up to quite a bit, and is generally not very interesting when we don't actually do anything if there's no claims to bump. Here we drop both logs if we have no claims to work with, but retain it if we process any claims.
WalkthroughThe updates across several files in the Lightning project enhance logging mechanisms, refine error handling, and adjust conditions for logging channel feerate changes. These changes aim to improve the clarity and efficiency of log messages, ensuring that important information is logged only when necessary and providing more context in log outputs. Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 5
Configuration used: CodeRabbit UI
Files selected for processing (4)
- lightning/src/chain/onchaintx.rs (5 hunks)
- lightning/src/ln/channelmanager.rs (1 hunks)
- lightning/src/ln/peer_handler.rs (2 hunks)
- lightning/src/routing/gossip.rs (1 hunks)
Files not reviewed due to errors (1)
- (no review received)
Additional comments: 1
lightning/src/chain/onchaintx.rs (1)
- 864-870: The introduction of
maybe_log_introfor conditional logging is a good practice. However, ensure that the logging provided is necessary and informative to avoid cluttering the log files with redundant information.
| return Err(LightningError { | ||
| err: "Couldn't find channel for update".to_owned(), | ||
| action: ErrorAction::IgnoreAndLog(Level::Gossip), | ||
| }); |
There was a problem hiding this comment.
The error message "Couldn't find channel for update" could be more descriptive by including the short channel ID in the message.
- Err(LightningError{err: "Couldn't find channel for update".to_owned(), action: ErrorAction::IgnoreAndLog(Level::Gossip)})
+ Err(LightningError{err: format!("Couldn't find channel for update: {}", msg.short_channel_id), action: ErrorAction::IgnoreAndLog(Level::Gossip)})Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| return Err(LightningError { | |
| err: "Couldn't find channel for update".to_owned(), | |
| action: ErrorAction::IgnoreAndLog(Level::Gossip), | |
| }); | |
| return Err(LightningError { | |
| err: format!("Couldn't find channel for update: {}", msg.short_channel_id), | |
| action: ErrorAction::IgnoreAndLog(Level::Gossip), | |
| }); |
|
|
||
| // If the feerate has decreased by less than half, don't bother | ||
| if new_feerate <= chan.context.get_feerate_sat_per_1000_weight() && new_feerate * 2 > chan.context.get_feerate_sat_per_1000_weight() { | ||
| if new_feerate != chan.context.get_feerate_sat_per_1000_weight() { | ||
| log_trace!(logger, "Channel {} does not qualify for a feerate change from {} to {}.", | ||
| chan_id, chan.context.get_feerate_sat_per_1000_weight(), new_feerate); | ||
| } | ||
| return NotifyOption::SkipPersistNoEvents; | ||
| } | ||
| if !chan.context.is_live() { |
There was a problem hiding this comment.
📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [3-6]
Consider merging the two if conditions into a single compound condition to improve readability and potentially performance by reducing the number of conditional checks.
- if new_feerate <= chan.context.get_feerate_sat_per_1000_weight() && new_feerate * 2 > chan.context.get_feerate_sat_per_1000_weight() {
- return NotifyOption::SkipPersistNoEvents;
- }
- if !chan.context.is_live() {
+ if (new_feerate <= chan.context.get_feerate_sat_per_1000_weight() && new_feerate * 2 > chan.context.get_feerate_sat_per_1000_weight()) || !chan.context.is_live() {
+ return NotifyOption::SkipPersistNoEvents;
+ }| if !requests.is_empty() { | ||
| log_debug!(logger, "Updating claims view at height {} with {} claim requests", cur_height, requests.len()); | ||
| } | ||
|
|
There was a problem hiding this comment.
Consider removing the debug log if it does not provide significant value. Excessive logging, especially at debug level, can clutter log files without offering substantial insights.
| if !self.locktimed_packages.is_empty() { | ||
| log_debug!(logger, | ||
| "Updating claims view at height {} with {} locked packages available for claim", | ||
| cur_height, | ||
| self.locktimed_packages.len()); | ||
| } |
There was a problem hiding this comment.
This debug log could potentially generate a lot of output if there are many locked packages. Evaluate if this level of verbosity is necessary or if it could be condensed or removed.
| if !bump_candidates.is_empty() { | ||
| maybe_log_intro(); | ||
| log_trace!(logger, "Bumping {} candidates", bump_candidates.len()); | ||
| } | ||
|
|
There was a problem hiding this comment.
Similar to previous comments on logging, ensure that this trace log provides value. If the information is not actionable or does not significantly aid in debugging, consider removing or reducing the verbosity.
Codecov ReportAll modified and coverable lines are covered by tests ✅
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #2895 +/- ##
=======================================
Coverage 89.12% 89.13%
=======================================
Files 115 115
Lines 94179 94196 +17
Branches 94179 94196 +17
=======================================
+ Hits 83936 83958 +22
+ Misses 7768 7752 -16
- Partials 2475 2486 +11 ☔ View full report in Codecov by Sentry. |
This removes 4 of our most common log lines (which generally don't impart any information, given they're so common), as well as addresses #2471