Conversation
CHr15F0x
left a comment
There was a problem hiding this comment.
@zakazaka95 Many thanks for your contribution! I had a look and I'd like to request a few changes to the solution:
- Please drop the "cancelled" failure reason - it's too general and we may be unlucky enough during a shutdown that an in-flight scrape request will land exactly on part of the "cancelled" failures being already registered while others will be not. Either way graceful shutdown should not count into failures.
- Please add an add-transaction-specific gateway timeout config field, with a reasonably high default value (like 60s), which for most users will be indistinguishable from the current default, which is no timeout. This way the request will time out and the failure will be recorded in metrics with the correct reason. This field should be applicable to all three
add_*_transactionmethods. - Feel free to keep "gateway_requests_in_flight" if you find it useful, however InFlightRequest should not record the latency, because it would then include requests cancelled during a graceful shutdown, or due to a disconnected client, which imo shouldn't be taken into account at all and will just pollute the latency histogram. Ofc you still need to decrement "gateway_requests_in_flight" in the guard.
Please do not hesitate to ask if you have more questions!
|
When the new |
|
|
Addressed the review in |
Make stuck gateway submissions visible and bound how long add-transaction requests can wait.
gateway_requests_in_flight. Dropping a future only decrements the gauge; it does not record a failed request or latency.--rpc.gateway-add-transaction-timeout(default 60 seconds) for invoke, declare and deploy-account submissions. Actual timeouts are recorded through the existing timeout failure and latency metrics.Closes #3545. No dependencies were added.
Validation of the September 7 review follow-up (
500ef873):git diff --checkpassed.chunks_exact_to_as_chunkslint incrates/crypto/src/hash/poseidon/hash.rs:19; that unrelated code was left unchanged.The focused RPC cancellation regression passed during the earlier implementation; it was not rerun for this timing-helper inline.