feat(rpc): record call metrics per method #5338
Conversation
There was a problem hiding this comment.
I think we could precompute all labels for known methods during build and put them into a map
reth/crates/rpc/rpc-builder/src/lib.rs
Line 1361 in e6ea251
so we need to find a way to pass the TransportRpcModuleConfig
here
reth/crates/rpc/rpc-builder/src/lib.rs
Line 1756 in e6ea251
| @@ -2148,7 +2148,17 @@ impl<TX: DbTxMut + DbTx> BlockWriter for DatabaseProvider<TX> { | |||
|
|
|||
| let start = Instant::now(); | |||
| self.tx.put::<tables::Transactions>(next_tx_num, transaction.into())?; | |||
| transactions_elapsed += start.elapsed(); | |||
| let elapsed = start.elapsed(); | |||
| if elapsed > Duration::from_millis(100) { | |||
There was a problem hiding this comment.
this might be too short and could potentially spam output?
There was a problem hiding this comment.
agree, increased to 1s
| /// Connection metrics per transport type | ||
| connection_metrics: ConnectionMetrics, | ||
|
|
||
| /// Call metrics per RPC method | ||
| call_metrics: HashMap<&'static str, RpcServerCallMetrics>, |
There was a problem hiding this comment.
can we Arc the internals?
because atleast for the ipc server this is cloned.
and I believe this is also the case for HTTP and ws
There was a problem hiding this comment.
connection_metrics should be fine, as it already has all metrics as Arc under the hood. Agree for call_metrics to avoid the cloning of hashmap, fixed.
shekhirin
force-pushed
the
alexey/rpc-metrics-per-method
branch
from
9703b58 to
f546950
Compare
shekhirin
added
C-enhancement
| @@ -1452,15 +1470,20 @@ impl RpcServerConfig { | |||
| /// This consumes the builder and returns a server. | |||
| /// | |||
| /// Note: The server ist not started and does nothing unless polled, See also [RpcServer::start] | |||
| pub async fn build(mut self) -> Result<RpcServer, RpcError> { | |||
| pub async fn build(mut self, modules: &TransportRpcModules) -> Result<RpcServer, RpcError> { | |||
There was a problem hiding this comment.
this is a bit awkward now, but I'm okay with this because this should only be called when an instance of TransportRpcModules exists.
| /// Connection metrics per transport type | ||
| connection_metrics: ConnectionMetrics, | ||
| /// Call metrics per RPC method | ||
| call_metrics: Arc<HashMap<&'static str, RpcServerCallMetrics>>, |
There was a problem hiding this comment.
can we group both in an inner type that we arc in a single field here? so a clone here is a single Arc::clone?
Currently, we don't have RPC metrics per method, but it can be extremely useful for debugging of long-running requests. This PR fixes it, and also changes the layout of RPC server metrics to make use of labels.