Eth peer logging improvements #6200

macfarla · 2023-11-22T04:33:10Z

Moved the full "mismatched network id" message to TRACE and have a brief version at DEBUG
moved "Disconnect - Outbound" brief message to INFO
moved the EthPeers {} collection logging to TRACE

Signed-off-by: Sally MacFarlane <macfarla.github@gmail.com>

…frequently Signed-off-by: Sally MacFarlane <macfarla.github@gmail.com>

Signed-off-by: Sally MacFarlane <macfarla.github@gmail.com>

github-actions · 2023-11-22T04:34:29Z

I thought about documentation and added the doc-change-required label to this PR if updates are required.
I thought about the changelog and included a changelog update if required.
If my PR includes database changes (e.g. KeyValueSegmentIdentifier) I have thought about compatibility and performed forwards and backwards compatibility tests

fab-10

There is an opportunity for small optimization converting, trace and debug, log statement to their lazy version, since some methods used there have a little overhead.
And would like to have a view of the impact of the info logs on the UX

fab-10 · 2023-11-22T10:03:55Z

ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/EthPeer.java

@@ -257,14 +257,14 @@ public RequestManager.ResponseStream send(
      throws PeerNotConnected {
    if (connectionToUse.getAgreedCapabilities().stream()
        .noneMatch(capability -> capability.getName().equalsIgnoreCase(protocolName))) {
-      LOG.debug("Protocol {} unavailable for this peer {}", protocolName, this.getShortNodeId());
+      LOG.debug("Protocol {} unavailable for this peer {}...", protocolName, this.getShortNodeId());


small opt

Suggested change

LOG.debug("Protocol {} unavailable for this peer {}...", protocolName, this.getShortNodeId());

LOG.atDebug().setMessage("Protocol {} unavailable for this peer {}...").addArgument(protocolName).addArgument(this::getShortNodeId).log();

fab-10 · 2023-11-22T10:05:46Z

ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/EthProtocolManager.java

-          ethPeers.peerCount(),
-          ethPeers);
+          ethPeers.peerCount());
+      LOG.trace(ethPeers.toString());


small opt

Suggested change

LOG.trace(ethPeers.toString());

LOG.trace(ethPeers);

I had to do this to get it to compile - is this better?

LOG.atTrace().addArgument(ethPeers).log();

Ok is see the problem with my initial suggestion, it should be LOG.trace("{}", ethPeers);

fab-10 · 2023-11-22T10:06:32Z

ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/EthProtocolManager.java

@@ -394,13 +394,13 @@ public void handleDisconnect(
      final DisconnectReason reason,
      final boolean initiatedByPeer) {
    if (ethPeers.registerDisconnect(connection)) {
-      LOG.debug(
-          "Disconnect - {} - {} - {}... - {} peers left\n{}",
+      LOG.info(


how frequent is this one? asking to understand the impact on log UX

changed back to debug

fab-10 · 2023-11-22T10:10:06Z

ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/EthProtocolManager.java

+        LOG.debug(
+            "Mismatched network id: {}, EthPeer {}...", status.networkId(), peer.getShortNodeId());
+        LOG.trace("Mismatched network id: {}, EthPeer {}", status.networkId(), peer);


Suggested change

LOG.debug(

"Mismatched network id: {}, EthPeer {}...", status.networkId(), peer.getShortNodeId());

LOG.trace("Mismatched network id: {}, EthPeer {}", status.networkId(), peer);

LOG.atDebug().setMessage(

"Mismatched network id: {}, EthPeer {}...").addArgument(status::networkId).addArgument(peer::getShortNodeId).log();

LOG.atTrace().setMessage("Mismatched network id: {}, EthPeer {}") .addArgument(status::networkId).addArgument(peer).log();

...main/java/org/hyperledger/besu/ethereum/eth/manager/task/AbstractGetHeadersFromPeerTask.java

...c/main/java/org/hyperledger/besu/ethereum/eth/manager/task/GetHeadersFromPeerByHashTask.java

...src/main/java/org/hyperledger/besu/ethereum/p2p/rlpx/connections/AbstractPeerConnection.java

Signed-off-by: Sally MacFarlane <macfarla.github@gmail.com>

macfarla · 2023-11-28T07:20:24Z

thanks for your review @fab-10 - have made some changes, if you can re-review - thanks!

fab-10

suggested a minor change, then LGTM

ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/EthProtocolManager.java

Co-authored-by: Fabio Di Fabio <fabio.difabio@consensys.net> Signed-off-by: Sally MacFarlane <macfarla.github@gmail.com>

* log list of peers at TRACE level Co-authored-by: Fabio Di Fabio <fabio.difabio@consensys.net> Signed-off-by: Sally MacFarlane <macfarla.github@gmail.com> --------- Signed-off-by: Sally MacFarlane <macfarla.github@gmail.com> Co-authored-by: Fabio Di Fabio <fabio.difabio@consensys.net> Signed-off-by: Justin Florentine <justin+github@florentine.us>

* log list of peers at TRACE level Co-authored-by: Fabio Di Fabio <fabio.difabio@consensys.net> Signed-off-by: Sally MacFarlane <macfarla.github@gmail.com> --------- Signed-off-by: Sally MacFarlane <macfarla.github@gmail.com> Co-authored-by: Fabio Di Fabio <fabio.difabio@consensys.net>

* log list of peers at TRACE level Co-authored-by: Fabio Di Fabio <fabio.difabio@consensys.net> Signed-off-by: Sally MacFarlane <macfarla.github@gmail.com> --------- Signed-off-by: Sally MacFarlane <macfarla.github@gmail.com> Co-authored-by: Fabio Di Fabio <fabio.difabio@consensys.net> Signed-off-by: Justin Florentine <justin+github@florentine.us>

* log list of peers at TRACE level Co-authored-by: Fabio Di Fabio <fabio.difabio@consensys.net> Signed-off-by: Sally MacFarlane <macfarla.github@gmail.com> --------- Signed-off-by: Sally MacFarlane <macfarla.github@gmail.com> Co-authored-by: Fabio Di Fabio <fabio.difabio@consensys.net> Signed-off-by: Gabriel Fukushima <gabrielfukushima@gmail.com>

macfarla added 6 commits November 22, 2023 13:17

move handleDisconnect to TRACE

8acae86

Signed-off-by: Sally MacFarlane <macfarla.github@gmail.com>

move Disconnect - Outbound message to INFO - this doesn't happen too …

bf91fce

…frequently Signed-off-by: Sally MacFarlane <macfarla.github@gmail.com>

log list of peers at TRACE level

6bb1e3b

Signed-off-by: Sally MacFarlane <macfarla.github@gmail.com>

added more ... and moved mismatched network id to TRACE

116694d

Signed-off-by: Sally MacFarlane <macfarla.github@gmail.com>

brief version at debug

4ef3bf3

Signed-off-by: Sally MacFarlane <macfarla.github@gmail.com>

Merge branch 'main' of github.com:hyperledger/besu into eth-peer-logging

790db29

macfarla added logging peering labels Nov 22, 2023

fab-10 reviewed Nov 22, 2023

View reviewed changes

macfarla added 7 commits November 23, 2023 14:01

Merge branch 'main' of github.com:hyperledger/besu into eth-peer-logging

28e2a40

Merge branch 'main' of github.com:hyperledger/besu into eth-peer-logging

a0087b2

use atDebug()

8bd26bb

Signed-off-by: Sally MacFarlane <macfarla.github@gmail.com>

changed back to debug

a34e9b7

Signed-off-by: Sally MacFarlane <macfarla.github@gmail.com>

use hashCode() method to match the rest of the logging

cf32ba9

Signed-off-by: Sally MacFarlane <macfarla.github@gmail.com>

shortNodeId

3791f44

Signed-off-by: Sally MacFarlane <macfarla.github@gmail.com>

add count to log

5f18826

Signed-off-by: Sally MacFarlane <macfarla.github@gmail.com>

fab-10 approved these changes Nov 28, 2023

View reviewed changes

ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/EthProtocolManager.java Outdated Show resolved Hide resolved

small opt

7ba3f4d

Co-authored-by: Fabio Di Fabio <fabio.difabio@consensys.net> Signed-off-by: Sally MacFarlane <macfarla.github@gmail.com>

macfarla enabled auto-merge (squash) November 28, 2023 10:25

macfarla merged commit 8f5a444 into hyperledger:main Nov 28, 2023

macfarla deleted the eth-peer-logging branch December 1, 2023 03:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Eth peer logging improvements #6200

Eth peer logging improvements #6200

macfarla commented Nov 22, 2023

github-actions bot commented Nov 22, 2023

fab-10 left a comment

fab-10 Nov 22, 2023

fab-10 Nov 22, 2023

macfarla Nov 28, 2023

fab-10 Nov 28, 2023

fab-10 Nov 22, 2023

macfarla Nov 28, 2023

macfarla Nov 28, 2023

fab-10 Nov 22, 2023

macfarla commented Nov 28, 2023

fab-10 left a comment

	LOG.debug("Protocol {} unavailable for this peer {}...", protocolName, this.getShortNodeId());
	LOG.atDebug().setMessage("Protocol {} unavailable for this peer {}...").addArgument(protocolName).addArgument(this::getShortNodeId).log();

Eth peer logging improvements #6200

Eth peer logging improvements #6200

Conversation

macfarla commented Nov 22, 2023

github-actions bot commented Nov 22, 2023

fab-10 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

macfarla commented Nov 28, 2023

fab-10 left a comment

Choose a reason for hiding this comment