Skip to content
This repository has been archived by the owner on Jun 20, 2024. It is now read-only.

always include the remote address in connection log entries #560

Merged

Conversation

rade
Copy link
Member

@rade rade commented Apr 14, 2015

in addition to the peer name/nick

This is handy for debugging, especially in situations when there are multiple connections in flight to the same peer.

in addition to the peer name

This is handy for debugging, especially in situations when there are
multiple connections in flight to the same peer.
@rade
Copy link
Member Author

rade commented Apr 14, 2015

I settled on the format after experimenting with a few variants.

weave 2015/04/14 21:18:45.116227 ->[172.17.0.156:33230] connection accepted
weave 2015/04/14 21:18:45.116806 ->[172.17.0.156:33230|7a:28:ff:08:ef:f7(weave2)]: completed handshake
weave 2015/04/14 21:18:45.116845 ->[172.17.0.156:33230|7a:28:ff:08:ef:f7(weave2)]: connection added (new peer)
weave 2015/04/14 21:18:45.118688 ->[172.17.0.156:33230|7a:28:ff:08:ef:f7(weave2)]: connection fully established
weave 2015/04/14 21:18:45.119004 EMSGSIZE on send, expecting PMTU update (IP packet was 60082 bytes, payload was 60074 bytes)
weave 2015/04/14 21:18:45.119020 ->[172.17.0.156:33230|7a:28:ff:08:ef:f7(weave2)]: Effective PMTU set to 1420
weave 2015/04/14 21:18:45.119687 ->[172.17.0.156:33230|7a:28:ff:08:ef:f7(weave2)]: Effective PMTU verified at 1420
weave 2015/04/14 21:18:45.345509 ->[172.17.0.157:6783] attempting connection
weave 2015/04/14 21:18:45.346475 ->[172.17.0.157:6783|7a:cb:5c:5a:a7:08(weave3)]: completed handshake
weave 2015/04/14 21:18:45.346697 ->[172.17.0.157:6783|7a:cb:5c:5a:a7:08(weave3)]: connection added (new peer)
weave 2015/04/14 21:18:45.346911 ->[172.17.0.157:60027] connection accepted
weave 2015/04/14 21:18:45.348980 ->[172.17.0.157:60027|7a:cb:5c:5a:a7:08(weave3)]: completed handshake
weave 2015/04/14 21:18:45.349043 ->[172.17.0.157:60027|7a:cb:5c:5a:a7:08(weave3)]: connection shutting down due to error: Multiple connections to 7a:cb:5c:5a:a7:08(weave3) added to 7a:c4:08:a4:b6:ce(weave1)
weave 2015/04/14 21:18:45.349343 ->[172.17.0.157:6783|7a:cb:5c:5a:a7:08(weave3)]: connection fully established

The remote address comes first since it is all we have to begin with. The vertical bar is a separator that is quite easy to spot, yet doesn't dominate. And it goes well with the brackets, strength wise.

The question is whether we should change the weave status output to match. In the 'peers' and 'reconnects' sections. The latter is a no-brainer, imo. The former is debatable. The main argument against it would be that the 'peers' section is primarily denoting a connectivity graph of peers, with the connection details being a secondary concern.

@bboreham what do you reckon? You have probably stared at weave logs and weave status output more than anybody else.

@bboreham
Copy link
Contributor

Consider a snippet of 'Peers' status like this:

Peer 7a:f4:69:81:0a:d4(bbvlx1) (v4) (UID 7933436855282280889)
   -> 7a:35:27:51:ff:e7(host1) [192.168.48.11:6783]
   -> 7a:aa:a4:ac:7d:ab(host2) [192.168.48.12:6783]

The latter two lines already have similar information, albeit in a different order [1]. But the IP address is from the perspective of the peer on the first line; it is not (and should not be, IMO) the IP address from our perspective. So, in that context, I would vote against adding the IP address from our perspective on that first line, because the whole thing then gets very context-dependent.

Historically, when staring at weave status output, I would repeatedly go through a process of scanning the 'Peers' lines to map a PeerName to an IP address. I suggested 'nicknames' to alleviate this pain, and given that I don't think I really need further info in weave status.

It probably would be a good idea to put ourself first in the Peers section, because then you have an unambiguous list of "who am I connected to, and at what IP address" immediately following.

[1] Changing that order is probably a good idea, but will disadvantage anybody who has parsed the text. (They should probably use the JSON alternative)

@rade
Copy link
Member Author

rade commented Apr 15, 2015

Ah yes, good point re address perspective.

@rade rade assigned rade and unassigned bboreham Apr 15, 2015
rade added a commit that referenced this pull request Apr 15, 2015
…n_logging

always include the remote address in connection log entries
tidy up / declutter some log and status output
@rade rade merged commit 9bd2afc into weaveworks:master Apr 15, 2015
@rade rade modified the milestone: 0.10.0 Apr 18, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants