improve libp2p connected peer metrics #4870
Conversation
| // We have SOCKET_UPDATED messages. This occurs when discovery has a majority of | ||
| // users reporting an external port and our ENR gets updated. | ||
| // Which means we are able to do NAT traversal. | ||
| metrics::set_gauge(&metrics::NAT_OPEN, 1); |
There was a problem hiding this comment.
Just curious, this is only true for the discv5 udp port though right?
There was a problem hiding this comment.
right! Thanks Pawan, had missed this and now understand why there was the check_nat function, but it was using an OR (||) instead of an AND (&&) which also ignored what you have just mentioned.
I updated the metric to count both discovery and libp2p ports
AgeManning
left a comment
AgeManning
left a comment
There was a problem hiding this comment.
Yeah nice. Grouping is good.
Have we tested this on a network yet?
|
@mergify queue |
🛑 The pull request has been removed from the queue
|
|
@Mergifyio requeue |
✅ This pull request will be re-embarked automaticallyDetailsThe followup |
✅ The pull request has been merged automaticallyDetailsThe pull request has been merged automatically at 0c3fef5 |
* improve libp2p connected peer metrics * separate discv5 port from libp2p for NAT open * use metric family for DISCOVERY_BYTES * Merge branch 'unstable' of https://github.com/sigp/lighthouse into improve-metrics
This reverts commit 0c3fef5.
Squashed commit of the following: commit 28fe6be Author: Jimmy Chen <[email protected]> Date: Tue Feb 20 10:04:56 2024 +1100 Revert "improve libp2p connected peer metrics (#4870)" This reverts commit 0c3fef5.
4 participants
Issue Addressed
After speaking with @jimmygchen regarding the transport metrics I noticed that they could be improved in the vein of the #4805 to use of the same metric family for collecting multiple properties.
This PR therefore merges all the connected peer metrics into a single family with the transport and direction labels to differentiate inbound and outbound from tcp and quic.
It also moves the assertion of NAT traversal to the two places where we are sure of it, on
Discv5Event::SocketUpdatedand libp2pFromSwarm::ExternalAddressConfirmed. When we are connected to a libp2p, it may be from a node in the local network.Please confirm everything makes sense. Cc @AgeManning