cumulus/metrics: Measure the time of including a tx in a backed block#8902
cumulus/metrics: Measure the time of including a tx in a backed block#8902
Conversation
Signed-off-by: Alexandru Vasile <[email protected]>
Signed-off-by: Alexandru Vasile <[email protected]>
Signed-off-by: Alexandru Vasile <[email protected]>
Signed-off-by: Alexandru Vasile <[email protected]>
Signed-off-by: Alexandru Vasile <[email protected]>
Signed-off-by: Alexandru Vasile <[email protected]>
Signed-off-by: Alexandru Vasile <[email protected]>
Signed-off-by: Alexandru Vasile <[email protected]>
Signed-off-by: Alexandru Vasile <[email protected]>
Signed-off-by: Alexandru Vasile <[email protected]>
Signed-off-by: Alexandru Vasile <[email protected]>
Signed-off-by: Alexandru Vasile <[email protected]>
Signed-off-by: Alexandru Vasile <[email protected]>
Signed-off-by: Alexandru Vasile <[email protected]>
Signed-off-by: Alexandru Vasile <[email protected]>
Signed-off-by: Alexandru Vasile <[email protected]>
Signed-off-by: Alexandru Vasile <[email protected]>
Signed-off-by: Alexandru Vasile <[email protected]>
Signed-off-by: Alexandru Vasile <[email protected]>
Signed-off-by: Alexandru Vasile <[email protected]>
Signed-off-by: Alexandru Vasile <[email protected]>
|
/cmd prdoc --audience node_dev,node_operator --bump patch |
Signed-off-by: Alexandru Vasile <[email protected]>
Signed-off-by: Alexandru Vasile <[email protected]>
|
/cmd prdoc --audience node_dev --bump patch |
…e_dev --bump patch'
|
All GitHub workflows were cancelled due to failure one of the required jobs. |
Signed-off-by: Alexandru Vasile <[email protected]>
|
Getting it in a backed block is half way, and it is useful. Can we also add a metric to measure the transaction time to finality? This is for example what Mythical is measuring as latency. |
The tx time to transition into different states (including finality) should be solved by: |
| /// The parachain informant service. | ||
| pub struct ParachainInformant<Block: BlockT> { | ||
| /// Relay chain interface to interact with the relay chain. | ||
| relay_chain_interface: Arc<dyn RelayChainInterface>, |
There was a problem hiding this comment.
nit: feel free to ignore, but this can also rely on generics. RelayChainInterface is Send + Sync so we just need clone here. A generic for ParachainInformant like RI: RelayChainInterface + Clone can still do it. I don't think there is a big concern with Arc approach, but previously the code did not use Arc.
All in all I posted this nit because I am curious if there was a reason for why you chose Arc here. Either way should be fine though.
There was a problem hiding this comment.
I like Arc<dyn> better in general because generics are slower to compile and more importantly, much more cumbersome to work with. So I support this decision 👍
| (sp_core::H256::from_slice(block_hash.as_ref()), submitted_at), | ||
| }; | ||
|
|
||
| if self.backed_blocks.peek(&block_hash).is_some() { |
There was a problem hiding this comment.
dq: I imagine the in_block event will hardly be sent after the block is backed, so this is a very improbable corner case, right?
| /// Ensures the RPC backed blocks reflect into the metrics and | ||
| /// performs the parachain logging. | ||
| async fn handle_import_notification(&mut self, n: RelayHeader) { | ||
| let candidate_events = match self.relay_chain_interface.candidate_events(n.hash()).await { |
There was a problem hiding this comment.
Haven't found this info that easy, but are these in order from oldest to most recent?
There was a problem hiding this comment.
Yep, had to also dig a bit to catch up with the prior context:
- it looks like we are fetching them from oldest to most recent (ie from the order they were deposited by the runtime):
| )?; | ||
|
|
||
| let mut transaction_v2_handles = Vec::with_capacity(2); | ||
| transaction_v2_handles.push(rpc_server_handle.transaction_v2_handle); |
There was a problem hiding this comment.
dq: what's the difference between the rpc_server_module and the in_memory_rpc?
There was a problem hiding this comment.
IIRC, the in-memory RPC is not externally exposed to customers. Still, it allows substrate subsystems to call into the same functionality, however, I believe we had a good reason back then to use in-memory (but cant remember offhand):
polkadot-sdk/substrate/client/service/src/lib.rs
Lines 109 to 119 in 750b547
| ); | ||
|
|
||
| // Check the monitor produces the same event | ||
| let result = tokio::time::timeout(std::time::Duration::from_secs(5), tx_monitor.next()) |
There was a problem hiding this comment.
nit: not sure how 5 seconds stands against our CI hiccups. Thought that this should be more resilient, as in 10x what we'd expect to wait under normal CI load. If 5s is the number, please ignore.
| let result = tokio::time::timeout(std::time::Duration::from_secs(5), tx_monitor.next()) | |
| let result = tokio::time::timeout(std::time::Duration::from_secs(5), tx_monitor.next()) |
sistemd
left a comment
There was a problem hiding this comment.
Nice to see the parachain informant turn into a proper type
| /// The parachain informant service. | ||
| pub struct ParachainInformant<Block: BlockT> { | ||
| /// Relay chain interface to interact with the relay chain. | ||
| relay_chain_interface: Arc<dyn RelayChainInterface>, |
There was a problem hiding this comment.
I like Arc<dyn> better in general because generics are slower to compile and more importantly, much more cumbersome to work with. So I support this decision 👍
…c-layer-metrics Signed-off-by: Alexandru Vasile <[email protected]>
Signed-off-by: Alexandru Vasile <[email protected]>
|
Why do we want to add this extra complexity here ? This can be easily done from an external application/process like this:
LE: Overall I'd put all of this monitoring stuff into https://github.com/paritytech/polkadot-introspector and keep the node lean. |
|
AFAIU it could make sense to do it, if it is not easy to do it from the outside -> how does the tool get the txs ? is it feasible to submit them from the tool ? |
This PR introduces a metric in Cumulus to measure the time it took a transaction received on the RPC layer to be included a backed block.
TransactionMonitorHandleis exposed from the transaction RPC V2 API to listen on monitoring events emitted by substratespawn_tasksnow returns the additionalTransactionMonitorHandleparachain_transaction_backed_durationTesting Done
small_network.tomlas base)Closes #8383
cc @paritytech/sdk-node