Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

improve libp2p connected peer metrics #5314

Merged
merged 2 commits into from Feb 28, 2024
Merged

Conversation

jxs
Copy link
Member

@jxs jxs commented Feb 26, 2024

Issue Addressed

submits #4870 reworked with #5265 addressed:

  • maintained the libp2p_peers_connected metric as its standard
  • added PEERS_CONNECTED_MULTI which contains the sum of all the peers connected as an IntGaugeVec which means it descriminates per direction and transport
  • removed NETWORK_INBOUND_PEERS and NETWORK_INBOUND_PEERS as they are covered by PEERS_CONNECTED_MULTI and are not standard
  • modified DISCOVERY_BYTES which gathers all the transferred discovery bytes which can be discriminated by outbound and inbound.
  • removed DISCOVERY_SENT_BYTES and DISCOVERY_RECV_BYTES as they are covered by DISCOVERY_BYTES
  • removed update_connected_peer_metrics function, we increment and decrement the number of peers on PeerManager NetworkBehaviour's FromSwarm::ConnectionEstablished and FromSwarm::ConnectionClosed events, and PEERS_PER_CLIENT is already updated when we identify a client.

@jxs jxs changed the base branch from stable to unstable February 26, 2024 19:37
@jxs jxs added the v5.0.1 label Feb 26, 2024
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.

Have we run these changes?

We've switched from setting the gauges for connected to peers to solely relying on increment and decrement. I'm worried there's a thousand edge-cases where peers disconnect/connect and we don't account for them putting these metrics off.

If the peer-count metric is off, it will be something we'll need to fix urgently.

@AgeManning
Copy link
Member

@Mergifyio queue

Copy link

mergify bot commented Feb 28, 2024

queue

✅ The pull request has been merged automatically

The pull request has been merged automatically at a89ff10

mergify bot added a commit that referenced this pull request Feb 28, 2024
@mergify mergify bot merged commit a89ff10 into sigp:unstable Feb 28, 2024
29 checks passed
pawanjay176 pushed a commit to pawanjay176/lighthouse that referenced this pull request Mar 5, 2024
* patch rust-yamux dep

* improve libp2p connected peer metrics
@jxs jxs mentioned this pull request Mar 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants