Clean up RLPX handshake logging#5568
Conversation
Also ensure that RLPX log messages have remote pubkey and endpoint information
Change RLPXHandshake timer from boost deadline timer to boost steady timer so we can use std::chrono. Also use constexpr where possible.
Add an error logger and use it when error conditions occur (e.g. received invalid packet type) and convert all cnet* log calls to use RLPXHandshake loggers. Remove "received" / "sending" from log messages.
Also only log boost error code in "handshake failed" message if handshake failed because of a boost error.
Add connection direction information to some error messages and overload the << operator for std::ostream so that abridged NodeIDs are written.
halfalicious
requested review from
chfast and
gumb0
and removed request for
gumb0
halfalicious
changed the title
Codecov Report
@@ Coverage Diff @@
## master #5568 +/- ##
=========================================
- Coverage 62.13% 62.1% -0.04%
=========================================
Files 347 347
Lines 28982 29041 +59
Branches 3284 3287 +3
=========================================
+ Hits 18009 18036 +27
- Misses 9796 9832 +36
+ Partials 1177 1173 -4 |
halfalicious
changed the title
halfalicious
force-pushed
the
p2p-logging
branch
from
d20b35f to
713f1a4
Compare
halfalicious
force-pushed
the
p2p-logging
branch
from
713f1a4 to
f6179b5
Compare
|
Squashed some commits |
halfalicious
force-pushed
the
p2p-logging
branch
from
21096b8 to
9949dd4
Compare
|
I'm occasionally seeing some messages being logged twice: I don't think this is due to my changes since I didn't modify any logic. I'll file a new issue once this PR has been merged. |
There was a problem hiding this comment.
-
That's a lot of messages, I'm for leaving it all at Trace level.
-
An idea to make logging code less verbose: you could make a logger that adds
connectionDirectionString()to each message.
Something like this inRLPXHandshakeconstructor should be enough:
m_logger.add_attribute("Context", boost::log::attrs::constant<std::string>(connectionDirectionString()));
Log formatter will output this Context attribute if it's not empty as a prefix to each message (it's currently used in capability/sync messages)
Also maybe m_remote and m_socket->remoteEndpoint(); belong to this context, too.
Change position of const keyword, make some variables const, remove unnecessary "p2p.connect", change log level to trace
halfalicious
force-pushed
the
p2p-logging
branch
from
b6f79b7 to
5b85610
Compare
Thanks for bringing this up 😄 Where is it used in capability / sync messages? I did a search for add_attribute but didn't find anything? I've added an attribute for the connection direction string but the string is only being included in some log messages: Rather than spend more time on this, I'd like to take care of this in a future PR. I also want to add attributes for the remote node ID (pubkey) and endpoint (when the socket is connected) but I think that's a little more complicated because I'd need to do some custom formatting to keep the messages readable. Also, the remote endpoint wouldn't be a constant so I'd have to probably define a new attribute type. I'll file a new issue to track this work once this PR has been merged. |
It's used in a different way - with
Adding new log attributes probably is not a good idea, because current formatter (defined in
It's also possible to set |
3 participants
Clean up the RLPX handshake logging. I've done the following:
There are still opportunities to clean up the output even more - for example, I think the "Handshake Disconnect" messages can be too verbose (in some cases there are something like 3 messages are logged on a disconnect). However, these changes improve things significantly and further output cleanup isn't critical so I think it can be taken care of in a future PR.
Here's a log output snippet before the changes:
And here's the log output now: