Log active peer count and peer list #5575
Conversation
halfalicious
requested review from
chfast and
gumb0
and removed request for
chfast
halfalicious
force-pushed
the
log-active-peer-info
branch
from
17690bf to
a270d31
Compare
There was a problem hiding this comment.
What do you think about logging peer count to INFO verbosity instead?
It would help to show in the default logging that the client is doing something even if sync is not progressing. And during sync it's also interesting to see once in a while how many peers we have at the moment.
Also in case we have 0 peers Looking for peers... could be more user-friendly message than Active peers: 0
| @@ -267,6 +267,16 @@ inline std::ostream& operator<<(std::ostream& _strm, NodeID const& _id) | |||
| return _strm; | |||
| } | |||
|
|
|||
| inline boost::log::formatting_ostream& operator<<( | |||
| boost::log::formatting_ostream& _strm, PeerSessionInfo const& _peerSessionInfo) | |||
There was a problem hiding this comment.
Better to add a version for non-const PeerSessionInfo&, too
There was a problem hiding this comment.
Better to add a version for non-const
PeerSessionInfo&, too
Why do we need this? I thought that non-const PeerInfo would be converted to const when calling the operator, which is acceptable since we don't need to modify the PeerInfo when logging it?
There was a problem hiding this comment.
Apparently generic template<class T> operator<<(formatting_ostream& _strm, T& _value) is a better fit during overload resolution than implicitly converting to const reference
Lines 136 to 138 in f5db298
I found no better solution for this than defining second versions for non-const reference.
Maybe check that this is the case for PeerSessionInfo.
I like the idea of logging the peer count with info verbosity. I agree that "looking for peers..." is more user-friendly, but I think that only logging it when there are 0 peers could be confusing since we're looking for peers regardless of whether or not we already have active peers. How about I log both the peer count and "looking for peers"? |
Codecov Report
@@ Coverage Diff @@
## master #5575 +/- ##
=========================================
- Coverage 62.11% 62.1% -0.02%
=========================================
Files 347 347
Lines 29101 29057 -44
Branches 3299 3290 -9
=========================================
- Hits 18077 18045 -32
+ Misses 9843 9830 -13
- Partials 1181 1182 +1 |
halfalicious
force-pushed
the
log-active-peer-info
branch
from
8be220c to
3cbbc85
Compare
Move logging to separate host function, add "looking for peers" message, and use Host helpers to retrieve peer count and peer session info vector. Also rename Host::peerSessionInfo to Host::peerSessionInfos so it more accurately reflects what the function is returning (vector of peerSessionInfo) Also use const& when iterating over capabilities and update changelog so that new "changed" entry is grouped with other "changed" entries.
halfalicious
force-pushed
the
log-active-peer-info
branch
from
3cbbc85 to
2d2d072
Compare
libp2p/Host.cpp
Outdated
| if (m_netConfig.discovery) | ||
| LOG(m_infoLogger) << "Looking for peers..."; | ||
|
|
||
| LOG(m_detailsLogger) << peerSessionInfos(); |
|
@gumb0 : I looked at the log output for multiple peers and the << overload for vector logs each entry on the same line, which makes the output a little difficult to read:
I think it's better to log each peer info on its own line - I think I'll create a private function in Host called something like logPeerInfos which logs each peer info to its own line (and prepends the info with "Peer: "). Any objections? |
|
@halfalicious If that's necessary, I'd better make it return the string with newline-delimited peer infos, and then with we'll make sure we don't do any iteration/locking/copying if filters disable these logs. Another option could be to try to overload I personally wouldn't bother I think, it's in |
Fix #5151
Log the active peer count and peer list to the "net" channel every 30 seconds. The peer count is logged with debug verbosity whereas the peer list is logged with trace verbosity.