Skip to content

improve libp2p connected peer metrics#4870

Merged
mergify[bot] merged 4 commits intosigp:unstablefrom
jxs:improve-metrics
Feb 8, 2024
Merged

improve libp2p connected peer metrics#4870
mergify[bot] merged 4 commits intosigp:unstablefrom
jxs:improve-metrics

Conversation

@jxs
Copy link
Member

@jxs jxs commented Oct 21, 2023

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::SocketUpdated and libp2p FromSwarm::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

@jimmygchen jimmygchen added Networking ready-for-review The code is ready for review labels Oct 23, 2023
pawanjay176
pawanjay176 approved these changes Oct 27, 2023
View reviewed changes
Copy link
Member

@pawanjay176 pawanjay176 left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, relevant metrics on running a node look right.

Also, think the libp2p_inbound_bytes metric should be renamed to libp2p_bytes

beacon_node/lighthouse_network/src/discovery/mod.rs Outdated
// 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);
Copy link
Member

@pawanjay176 pawanjay176 Oct 27, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just curious, this is only true for the discv5 udp port though right?

Copy link
Member Author

@jxs jxs Oct 30, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
AgeManning approved these changes Nov 6, 2023
View reviewed changes
Copy link
Member

@AgeManning AgeManning left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah nice. Grouping is good.
Have we tested this on a network yet?

@jxs
Copy link
Member Author

jxs commented Feb 6, 2024

@mergify queue

@mergify
Copy link

mergify bot commented Feb 6, 2024 •
edited
Loading

queue

🛑 The pull request has been removed from the queue default

Details

The queue conditions cannot be satisfied due to failing checks.

You can take a look at Queue: Embarked in merge queue check runs for more details.

In case of a failure due to a flaky test, you should first retrigger the CI.
Then, re-embark the pull request into the merge queue by posting the comment
@mergifyio refresh on the pull request.

mergify bot added a commit that referenced this pull request Feb 6, 2024
@mergify
Merge of #4870
8c48bf6
@mergify mergify bot mentioned this pull request Feb 6, 2024
Closed
5 tasks
@AgeManning
Copy link
Member

AgeManning commented Feb 8, 2024

@Mergifyio requeue

@mergify
Copy link

mergify bot commented Feb 8, 2024

requeue

✅ This pull request will be re-embarked automatically

Details

The followup queue command will be automatically executed to re-embark the pull request

@mergify
Copy link

mergify bot commented Feb 8, 2024 •
edited
Loading

queue

✅ The pull request has been merged automatically

Details

The pull request has been merged automatically at 0c3fef5

mergify bot added a commit that referenced this pull request Feb 8, 2024
@mergify
Merge of #4870
a7f51e7
This was referenced Feb 8, 2024
Closed
Merged
Merged
Merged
@mergify mergify bot merged commit 0c3fef5 into sigp:unstable Feb 8, 2024
danielrachi1 pushed a commit to danielrachi1/lighthouse that referenced this pull request Feb 14, 2024
@jxs @danielrachi1
improve libp2p connected peer metrics (sigp#4870)
a639f93 
* 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
jimmygchen added a commit to jimmygchen/lighthouse that referenced this pull request Feb 19, 2024
@jimmygchen
Revert "improve libp2p connected peer metrics (sigp#4870)"
28fe6be 
This reverts commit 0c3fef5.
@jimmygchen jimmygchen mentioned this pull request Feb 19, 2024
Merged
paulhauner added a commit that referenced this pull request Feb 19, 2024
@paulhauner
Revert libp2p metrics (#4870) (#5265)
df2a886 
Squashed commit of the following:

commit 28fe6be
Author: Jimmy Chen <jchen.tc@gmail.com>
Date:   Tue Feb 20 10:04:56 2024 +1100

    Revert "improve libp2p connected peer metrics (#4870)"

    This reverts commit 0c3fef5.
mergify bot pushed a commit that referenced this pull request Feb 20, 2024
@jimmygchen
Revert libp2p metrics (#4870) (#5265)
50c423a 
* Revert "improve libp2p connected peer metrics (#4870)"

This reverts commit 0c3fef5.
@jxs jxs mentioned this pull request Feb 26, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@AgeManning AgeManning AgeManning approved these changes

@pawanjay176 pawanjay176 pawanjay176 approved these changes

Assignees

No one assigned

Labels

Networking ready-for-review The code is ready for review

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@jxs @AgeManning @pawanjay176 @jimmygchen