Network metrics per client #7445
Conversation
| @@ -214,6 +214,19 @@ impl<T: BeaconChainTypes> NetworkBeaconProcessor<T> { | |||
| reprocess_tx: Option<mpsc::Sender<ReprocessQueueMessage>>, | |||
| seen_timestamp: Duration, | |||
| ) { | |||
| let attestation_size = attestation.as_ssz_bytes().len() as u64; | |||
There was a problem hiding this comment.
This calls force us to spend cycles re-serializing a lot of SSZ objects. To track this for free we should do the tracking in the lighthouse-network layer which I'm not sure has access to peer client information. If not possible to track this for free skip tracking bytes per client for now
|
|
||
| metrics::inc_counter_vec( | ||
| &metrics::MESSAGES_RECEIVED_PER_CLIENT, | ||
| &[&client] |
There was a problem hiding this comment.
You should label this metric by [client, object_type] where object type could be
gossip_attestationgossip_blockrange_sync_blockbackfill_sync_block
etc
| @@ -241,6 +254,21 @@ impl<T: BeaconChainTypes> NetworkBeaconProcessor<T> { | |||
| packages: GossipAttestationBatch<T::EthSpec>, | |||
| reprocess_tx: Option<mpsc::Sender<ReprocessQueueMessage>>, | |||
| ) { | |||
| 8000 for package in &packages { | |||
There was a problem hiding this comment.
You can move attestation metrics to process_gossip_attestation_result and only track valid objects
| metrics::inc_counter_vec( | ||
| &metrics::MESSAGES_RECEIVED_PER_CLIENT, | ||
| &[&client] | ||
| ); |
There was a problem hiding this comment.
You could add a helper method
fn track_messages_received_per_client(&self, peer_id: PeerId, object_type: &'static str) {
let client = self.network_globals.client(&peer_id).kind.to_string();
metrics::inc_counter_vec(
&metrics::MESSAGES_RECEIVED_PER_CLIENT,
&[&client, object_type]
);
}to de-duplicate a lot of code
| @@ -169,6 +169,11 @@ impl<E: EthSpec> PeerDB<E> { | |||
| } | |||
| } | |||
|
|
|||
| /// Returns the sync start time of the peer if it exists | |||
| pub fn sync_start_time(&self, peer_id: &PeerId) -> Option<&Instant> { | |||
| self.peers.get(peer_id).and_then(|info| info.sync_start_time()) | |||
There was a problem hiding this comment.
I don't follow the meaning/purpose of sync start time, could you detail it?
There was a problem hiding this comment.
| .peer_info(peer_id) | ||
| .map(|info| info.client().version.clone()) | ||
| .unwrap_or_default() | ||
| } |
There was a problem hiding this comment.
You have a single metric, not one per client type and one for client version. In mainnet is version field set for most clients? Could you test this and report back? If so, to what?
|
FYI @iri031 there are still some unaddressed comment |
Issue Addressed
Which issue # does this PR address?
#7389
Proposed Changes
Metrics for: