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.

Sign up for GitHub

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

Jump to bottom

Fix peer count metrics #5404

Merged
merged 5 commits into from
Mar 20, 2024
+75 −68
Merged

Fix peer count metrics #5404

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
b64b7b3
Set the peers_per_client metrics directly, rather than using incremen…
ackintosh Mar 13, 2024
2f83cdd
Move PEERS_CONNECTED related update to the same place
ackintosh Mar 15, 2024
56d6708
Move PEERS_CONNECTED_MULTI related update to the same place
ackintosh Mar 17, 2024
6897ef7
Rename
ackintosh Mar 17, 2024
8ceb702
Remove unused variables
ackintosh Mar 17, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 23 additions & 13 deletions beacon_node/lighthouse_network/src/peer_manager/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,13 +26,16 @@ pub use libp2p::identity::Keypair;
#[allow(clippy::mutable_key_type)] // PeerId in hashmaps are no longer permitted by clippy
pub mod peerdb;

use crate::peer_manager::peerdb::client::ClientKind;
pub use peerdb::peer_info::{
ConnectionDirection, PeerConnectionStatus, PeerConnectionStatus::*, PeerInfo,
};
use peerdb::score::{PeerAction, ReportSource};
pub use peerdb::sync_status::{SyncInfo, SyncStatus};
use std::collections::{hash_map::Entry, HashMap};
use std::net::IpAddr;
use strum::IntoEnumIterator;

pub mod config;
mod network_behaviour;

Expand Down Expand Up @@ -464,19 +467,6 @@ impl<TSpec: EthSpec> PeerManager<TSpec> {
"observed_address" => ?info.observed_addr,
"protocols" => ?info.protocols
);

// update the peer client kind metric if the peer is connected
if matches!(
peer_info.connection_status(),
PeerConnectionStatus::Connected { .. }
| PeerConnectionStatus::Disconnecting { .. }
) {
metrics::inc_gauge_vec(
&metrics::PEERS_PER_CLIENT,
&[peer_info.client().kind.as_ref()],
);
metrics::dec_gauge_vec(&metrics::PEERS_PER_CLIENT, &[previous_kind.as_ref()]);
}
}
} else {
error!(self.log, "Received an Identify response from an unknown peer"; "peer_id" => peer_id.to_string());
Expand Down Expand Up @@ -1267,6 +1257,26 @@ impl<TSpec: EthSpec> PeerManager<TSpec> {
);
}
}

// Update peers per client metrics.
fn update_peers_per_client_metrics(&self) {
let mut clients_per_peer = HashMap::new();

for (_peer, peer_info) in self.network_globals.peers.read().connected_peers() {
*clients_per_peer
.entry(peer_info.client().kind.to_string())
.or_default() += 1;
}

for client_kind in ClientKind::iter() {
let value = clients_per_peer.get(&client_kind.to_string()).unwrap_or(&0);
metrics::set_gauge_vec(
&metrics::PEERS_PER_CLIENT,
&[client_kind.as_ref()],
*value as i64,
);
}
}
}

enum ConnectingType {
Expand Down
4 changes: 4 additions & 0 deletions beacon_node/lighthouse_network/src/peer_manager/network_behaviour.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -272,6 +272,8 @@ impl<TSpec: EthSpec> PeerManager<TSpec> {

metrics::inc_gauge(&metrics::PEERS_CONNECTED);
metrics::inc_counter(&metrics::PEER_CONNECT_EVENT_COUNT);

Copy link
Member

Choose a reason for hiding this comment

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

since we need to revert back into iterating all the connected peers, as @AgeManning suggested let's have all the client metrics updates in the same place, i.e. remove the dec and inc of PEERS_CONNECTED and related wdyt @ackintosh? this way we only don't incur in similar situations as this PR is fixing.

Copy link
Member Author

Choose a reason for hiding this comment

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

@jxs Thanks for your comment, that makes sense. I will make an update.

self.update_peers_per_client_metrics();
}

// Count dialing peers in the limit if the peer dialed us.
Expand Down Expand Up @@ -364,6 +366,8 @@ impl<TSpec: EthSpec> PeerManager<TSpec> {
// Legacy standard metrics.
metrics::dec_gauge(&metrics::PEERS_CONNECTED);
metrics::inc_counter(&metrics::PEER_DISCONNECT_EVENT_COUNT);

self.update_peers_per_client_metrics();
}
}

Expand Down
Loading