Add req/resp metrics from additional PeerDAS list#6430
Add req/resp metrics from additional PeerDAS list#6430KatyaRyazantseva wants to merge 8 commits intosigp:unstablefrom
Conversation
| }, | ||
| ))) | ||
| } | ||
| SupportedProtocol::PingV1 => Ok(Some(InboundRequest::Ping(Ping { |
There was a problem hiding this comment.
Do I need to count Ping in TOTAL_RPC_REQUESTS_BYTES_RECEIVED metric?
| decoded_buffer, | ||
| )?))) | ||
| } | ||
| SupportedProtocol::GoodbyeV1 => Ok(Some(InboundRequest::Goodbye( |
There was a problem hiding this comment.
Do I need to count Goodbye in TOTAL_RPC_REQUESTS_BYTES_RECEIVED metric?
chong-he
added
ready-for-review
| &metrics::TOTAL_RPC_REQUESTS_BYTES_SENT, | ||
| &[item.into()], | ||
| dst.len() as u64, | ||
| ); |
There was a problem hiding this comment.
Feels wrong to place a metric inside an encode function in a code struct, may be best to place this on the consumer of this function?
| )?, | ||
| }, | ||
| ))), | ||
| SupportedProtocol::DataColumnsByRangeV1 => { |
There was a problem hiding this comment.
There's a lot of repetition in this match, you can turn SupportedProtocol into a str and have a single register metric call
| ) | ||
| }); | ||
| pub static TOTAL_RPC_REQUESTS: LazyLock<Result<IntCounterVec>> = LazyLock::new(|| { | ||
| try_create_int_counter_vec("libp2p_rpc_requests_total", "RPC requests total", &["type"]) |
There was a problem hiding this comment.
We generally avoid renaming metrics as it is a breaking change for users, but I do agree the current name is not just unclear but also a bit misleading, so I think this is a good change. I'll marked this PR as breaking change so we remember to mention it in our release notes.
This dashboard needs to be updated too:
https://github.com/sigp/lighthouse-metrics/blob/9c654c48d108180cfec48398c9410ae80839047e/dashboards/Network.json#L2620
jimmygchen
added
waiting-on-author
|
This pull request has merge conflicts. Could you please resolve them @KatyaRyazantseva? 🙏 |
|
Closing this PR due to long period of inactivity - some of these metric changes are likely outdated and conflicts with the current code. Feel free to re-open this if you'd like to continue work on this. |
4 participants
Issue Addressed
This PR addresses issue #6018
Proposed Changes
The following list of metrics is implemented: