-
Notifications
You must be signed in to change notification settings - Fork 960
listnetworkevents: keep a log of all connect/disconnect/failed-connect and pings #8558
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
listnetworkevents: keep a log of all connect/disconnect/failed-connect and pings #8558
Conversation
6c0e9ed to
4eb19d5
Compare
|
Note: failed-connect should note if it's unreachable because there are no addresses using a protocol we support! |
4eb19d5 to
b14d27e
Compare
3039eb8 to
fe4ada6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there's a problem with one of the sql insert statements. But otherwise this looks great. I think it will be really useful to automatically log all recent connections.
Do you think it would be worth timing out the ping and logging a lack of pong response, or would that be more problematic than it's worth? I'll try to test this out tomorrow.
connectd/connectd.c
Outdated
| * in progress right now) */ | ||
| if (connect->conn) | ||
| io_set_finish(connect->conn, NULL, NULL); | ||
| connect_reason = tal_steal(tmpctx, connect->reason); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So an inbound connection when we were also wanting to connect to the peer will still be attributed with our connection request reason?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed. If incoming, we should say "incoming connection while we were trying to connect" instead.
connectd/connectd.c
Outdated
| connect_time_nsec = time_to_nsec(timemono_since(connect->start)); | ||
| tal_free(connect); | ||
| } else { | ||
| connect_reason = ""; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be helpful to explicitly label an "unrequested inbound connection" here?
…ults. Signed-off-by: Rusty Russell <[email protected]>
Signed-off-by: Rusty Russell <[email protected]>
Signed-off-by: Rusty Russell <[email protected]>
… *try* to connect to. This is important: if it's tor-only and we don't have a proxy, we will fail to connect, but it's no indication that the node is unreachable. Same with IPv6. Signed-off-by: Rusty Russell <[email protected]>
Changelog-Added: JSON-RPC: `wait` now has `networkevents` subsystem. Signed-off-by: Rusty Russell <[email protected]>
Signed-off-by: Rusty Russell <[email protected]>
Signed-off-by: Rusty Russell <[email protected]>
Signed-off-by: Rusty Russell <[email protected]>
Signed-off-by: Rusty Russell <[email protected]> Changelog-Added: JSON-RPC: `sql` now supports the `networkevents` table.
Changelog-Added: JSON-RPC: `delnetworkevent` to delete from listnetworkevents. Signed-off-by: Rusty Russell <[email protected]>
We also document this in the listnetworkevents command itself. The test_autoclean_once was getting repetitive, so I cleaned that up too. Signed-off-by: Rusty Russell <[email protected]> Changelog-Added: `autoclean` will remove networkevents after 30 days by default.
fe4ada6 to
ac80f2f
Compare
Combined with gossipd connecting to random peers, this gives us an indication of peer latency. This might be useful to someone.