feat: add additional message type metrics to EthRequestHandlerMetrics #8319
Conversation
We don't have good ways to test metrics, but luckily most of the time they are supposed to be very simple, and easy to observe once we have grafana dashboards for them. Not having tests for these metrics should be fine |
There was a problem hiding this comment.
these would be my naming suggestions - my rule of thumb for naming is:
- field names should be terse, and
- documentation should be descriptive and thorough
as field / variable names that are too long (even if descriptive) can be confusing, since they are not supposed to be read as sentences
crates/net/network/src/metrics.rs
Outdated
| /// Number of GetReceipts requests received | ||
| pub(crate) eth_requests_get_receipts_received_total: Counter, | ||
|
|
||
| /// Number of GetBlockBodies requests received | ||
| pub(crate) eth_requests_get_block_bodies_received_total: Counter, | ||
|
|
||
| /// Number of GetNodeData requests received | ||
| pub(crate) eth_requests_get_node_data_received_total: Counter, |
There was a problem hiding this comment.
| /// Number of GetReceipts requests received | |
| pub(crate) eth_requests_get_receipts_received_total: Counter, | |
| /// Number of GetBlockBodies requests received | |
| pub(crate) eth_requests_get_block_bodies_received_total: Counter, | |
| /// Number of GetNodeData requests received | |
| pub(crate) eth_requests_get_node_data_received_total: Counter, | |
| /// Number of GetReceipts requests received | |
| pub(crate) eth_receipt_requests_received: Counter, | |
| /// Number of GetBlockBodies requests received | |
| pub(crate) eth_bodies_requests_received: Counter, | |
| /// Number of GetNodeData requests received | |
| pub(crate) eth_node_data_requests_received: Counter, |
There was a problem hiding this comment.
Sounds good with respect to the name changes. I have mostly accepted these changes in 9e9abdd with slight modification:
- added
_totalto all these metrics to align with issue Counter metric name should adhere to*_totalpattern #8150 for consistency across the codebase eth_receipt_requests_received->eth_receipts_requests_received_totalto align casing with the other metrics
let me know if this is good and thanks for the comments 👍
crates/net/network/src/metrics.rs
Outdated
| /// Number of received headers requests | ||
| pub(crate) received_headers_requests: Counter, | ||
| /// Number of GetBlockHeaders requests received | ||
| pub(crate) eth_requests_get_block_headers_received_total: Counter, |
There was a problem hiding this comment.
| pub(crate) eth_requests_get_block_headers_received_total: Counter, | |
| pub(crate) eth_headers_requests_received: Counter, |
…paradigmxyz#8319) Co-authored-by: Victor Shih <victor@Basement-PC>
Resolves #8245
Adds additional metrics for when
GetNodeDataandGetReceiptseth requests are received. Also normalizes theCountermetric names to better follow convention as mentioned in #8150. Let me know if these metric names make sense or if there are any further suggestions.I tried to look for ways to test this, but I didn't see any tests for metrics handling in this file. Would love to add some if it is helpful and if they are tested some other way in the codebase that I can use for guidance.