Improve router logging and update documentation #2436
Conversation
|
Just to carry over some of the context here, the benches from #2417 with |
Codecov ReportPatch coverage:
❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more. Additional details and impacted files@@ Coverage Diff @@
## main #2436 +/- ##
==========================================
+ Coverage 90.24% 90.96% +0.71%
==========================================
Files 106 106
Lines 55774 62618 +6844
Branches 55774 62618 +6844
==========================================
+ Hits 50335 56959 +6624
- Misses 5439 5659 +220
☔ View full report in Codecov by Sentry. |
| if !contributes_sufficient_value || exceeds_max_path_length || | ||
| exceeds_cltv_delta_limit || payment_failed_on_this_channel { | ||
| // Path isn't useful, ignore it and move on. | ||
| if !contributes_sufficient_value { | ||
| if should_log_candidate { | ||
| log_trace!(logger, "Ignoring {} due to insufficient value contribution.", LoggedCandidateHop(&$candidate)); | ||
| } | ||
| num_ignored_value_contribution += 1; | ||
| } else if exceeds_max_path_length { | ||
| if should_log_candidate { | ||
| log_trace!(logger, "Ignoring {} due to exceeding max. path length limit.", LoggedCandidateHop(&$candidate)); | ||
| } | ||
| num_ignored_path_length_limit += 1; | ||
| } else if exceeds_cltv_delta_limit { | ||
| if should_log_candidate { | ||
| log_trace!(logger, "Ignoring {} due to exceeding CLTV delta limit.", LoggedCandidateHop(&$candidate)); | ||
| } | ||
| num_ignored_cltv_delta_limit += 1; | ||
| } else if payment_failed_on_this_channel { | ||
| if should_log_candidate { | ||
| log_trace!(logger, "Ignoring {} due to a failed previous payment attempt.", LoggedCandidateHop(&$candidate)); | ||
| } | ||
| num_ignored_previously_failed += 1; |
There was a problem hiding this comment.
Are these actually mutually exclusive?
There was a problem hiding this comment.
True, might be best to just have one long log that includes all the variables, which would remove all the branching as well
There was a problem hiding this comment.
Are these actually mutually exclusive?
Not necessarily, but we log and count the first reason why a candidate would be excluded. Yes, some should count towards multiple categories, but it's probably not worth to check for all combinations.
True, might be best to just have one long log that includes all the variables
Mh, I think even if some candidates might fall under multiple categories, it's much more readable to have individual logs?
There was a problem hiding this comment.
I think its useful to have individual reasons why a particular candidate was ignored.
As `RouteParameters` are not included anymore in `Event::PaymentPathFailed` since 0.0.115, and we don't give value/payee as immediate arguments to `find_route` anymore.
Previously, we barely gave any hints why we excluded certain hops during pathfinding. Here, we introduce more verbose logging by a) accounting how much candidates we ignored for which reasons and b) logging any first/last/blinded hops we end up ignoring. Fixes lightningdevkit#1646.
tnull
force-pushed
the
2023-07-improve-router-logging
branch
from
fd94350 to
1db53a9
Compare
Squashed without further changes. |
6 participants
Fixes #1646
As #2417 will slip to a future release, this PR just includes the improved logging and documentation fixups of of our router.
Previously, we barely gave any hints why we excluded certain hops during pathfinding. Here, we introduce more verbose logging by a) accounting how much candidates we ignored for which reasons and b) logging any first/last/blinded hops we end up ignoring.
We also improve documentation as
RouteParametersare not included inEvent::PaymentPathFailedsince 0.0.115, and we don't give value/payee as immediate arguments tofind_routeanymore.