Payment hash and node id logging clean up#4228
Payment hash and node id logging clean up#4228TheBlueMatt merged 3 commits intolightningdevkit:mainfrom
Conversation
|
👋 Thanks for assigning @TheBlueMatt as a reviewer! |
joostjager
force-pushed
the
logging-cleanups
branch
from
36f43e5 to
adeec05
Compare
joostjager
requested review from
valentinewallace
and removed request for
wpaulino
TheBlueMatt
left a comment
TheBlueMatt
left a comment
There was a problem hiding this comment.
Some commit messages and updates to the logger docs to note that these things aren't included like we did channel id would be nice, otherwise lgtm.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4228 +/- ##
==========================================
- Coverage 89.33% 89.32% -0.01%
==========================================
Files 180 180
Lines 138353 138502 +149
Branches 138353 138502 +149
==========================================
+ Hits 123598 123720 +122
- Misses 12132 12157 +25
- Partials 2623 2625 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Now that the default Display implementation of Record also outputs structured log fields, that data can be removed from the log messages. This declutters the log output and also helps with verticality in the code. Note that deduplication is only carried out for log statements that are always called in a log context. For some statements this isn't the case because not every caller sets up the context. Those are left unchanged.
joostjager
force-pushed
the
logging-cleanups
branch
from
adeec05 to
a6812c8
Compare
| "Forwarding HTLC from SCID {} with next hop SCID {} over {}", | ||
| prev_outbound_scid_alias, | ||
| short_chan_id, | ||
| channel_description |
There was a problem hiding this comment.
Would be nice to do the inline thing here and in many places.
| "Forwarding HTLC from SCID {} with next hop SCID {} over {}", | |
| prev_outbound_scid_alias, | |
| short_chan_id, | |
| channel_description | |
| "Forwarding HTLC from SCID {prev_outbound_scid_alias} with next hop SCID {short_chan_id} over {channel_description}", |
| log_debug!(logger, "Peer {} does not support static remote key, disconnecting", log_pubkey!(counterparty_node_id)); | ||
| log_debug!( | ||
| logger, | ||
| "Peer {} does not support static remote key, disconnecting", |
There was a problem hiding this comment.
nit: you can definitely drop the pubkey here as well.
|
👋 The first review has been submitted! Do you think this PR is ready for a second reviewer? If so, click here to assign a second reviewer. |
3 participants
This PR further reduces duplication of data between log messages and structured log fields. The selection of log statements to de-duplicate was determined by running the test suite, tracking all log outputs, and distilling that down to the log lines that for every invocation had the duplication in them.
It is the case that some log statements, depending on the caller, do not always have the right context attached. Those were left unchanged for now and can be reworked incrementally.
Follow up from #4217