Skip to content

feat(ethexe-rpc): Metrics middleware for RPC methods#5371

Queued
ecol-master wants to merge 96 commits intomasterfrom
feat/rpc-metrics-middleware
Queued

feat(ethexe-rpc): Metrics middleware for RPC methods#5371
ecol-master wants to merge 96 commits intomasterfrom
feat/rpc-metrics-middleware

Conversation

@ecol-master
Copy link
Copy Markdown
Member

@ecol-master ecol-master commented Apr 21, 2026

Closes: #5387

@gear-tech/dev

Dmitry Kuzmin and others added 30 commits February 6, 2026 13:47
@ecol-master ecol-master linked an issue Apr 28, 2026 that may be closed by this pull request
@ecol-master ecol-master changed the base branch from kuzmindev/feat/producer-send-promises-hashes to master May 4, 2026 15:45
@ecol-master
Copy link
Copy Markdown
Member Author

@claude review

@claude
Copy link
Copy Markdown

claude Bot commented May 4, 2026

Claude finished @ecol-master's task in 3m 48s —— View job


Code Review

This PR replaces per-method manual counters with a generic Tower middleware layer that intercepts all jsonrpsee method calls and records calls_started_total, calls_finished_total (with status label), call_duration_seconds, and calls_in_flight for configured methods. The design is clean and the integration with jsonrpsee's RpcServiceBuilder is correct.

high: calls_in_flight gauge leak on future cancellation

File: ethexe/rpc/src/metrics.rs lines 85-92

If the BoxFuture returned from call() is dropped before resolving (server shutdown, connection close), on_outgoing_response never runs and calls_in_flight stays permanently incremented. spawner.rs already demonstrates the correct fix with scopeguard::guard, and scopeguard is already a dependency of ethexe-rpc.

Fix: wrap the decrement in a RAII guard rather than relying on on_outgoing_response:

let future = self.service.call(request);
Box::pin(async move {
    metrics.on_incoming_request();
    // Guard ensures decrement even on cancellation.
    let _guard = scopeguard::guard((), |_| metrics.calls_in_flight.decrement(1));
    let started_at = Instant::now();
    let response = future.await;
    metrics.on_outgoing_response(started_at, &response); // remove calls_in_flight decrement from here
    response
})

medium: inner mod metrics shadows the metrics crate

File: ethexe/rpc/src/metrics.rs line 99

The inner module name conflicts with the metrics crate, requiring #[allow(clippy::module_inception)] and the super::metrics disambiguation on line 25. Renaming to mod types or mod definitions removes the ambiguity without touching anything else.

medium: TRACKED_METHODS missing the read-only injected methods

File: ethexe/rpc/src/metrics.rs lines 35-39

injected_getTransactionPromise and injected_getTransactions are absent. The same rationale for tracking program_calculateReplyForHandle latency applies to getTransactions (batch capped at 100 IDs) and getTransactionPromise.

low: inconsistent tracing style in server.rs

File: ethexe/rpc/src/apis/injected/server.rs line 186

trace is imported at line 40, but line 186 uses tracing::trace!. Inconsistent with the relay.rs cleanup in this same PR.


Overall the middleware approach is the right design. The cancellation-safety gap with calls_in_flight is the main thing to address before merging.

@ecol-master
Copy link
Copy Markdown
Member Author

@claude review-delta

@claude
Copy link
Copy Markdown

claude Bot commented May 4, 2026

Claude finished @ecol-master's task in 1m 56s —— View job


Delta Review

Reviewing changes since the previous review. The relevant commit is acc7d48 ("fix: claude review").


Fixed

  • High: calls_in_flight gauge leak — correctly addressed with scopeguard::guard in metrics.rs:88-89. The decrement now runs on drop, so cancellation no longer leaks the gauge.

Still Open

medium: inner mod metrics shadows the metrics crate (metrics.rs:110)

The #[allow(clippy::module_inception)] suppression is still present. The inner module is still named metrics, shadowing the metrics crate name. Renaming to mod types or mod definitions eliminates the suppression and the ambiguity. Fix this →

medium: TRACKED_METHODS missing read-only injected methods (metrics.rs:35-39)

injected_getTransactionPromise and injected_getTransactions are still absent from the tracked set. Both methods are part of the public injected API surface and are worth monitoring — getTransactions already has a batch-size cap (100 IDs) that makes latency spikes interesting to observe. Fix this →

low: inconsistent tracing style in server.rs:186

tracing::trace! is used at line 186 while trace is already imported at line 40. All other call sites use the short form. Fix this →


New observation

The acc7d48 refactor removed on_incoming_request()/on_outgoing_response() and made all MethodMetrics fields pub. Combined with pub use metrics::* at metrics.rs:21, MethodMetrics and its raw Counter/Gauge/Histogram fields are now part of the crate's public API. This is harmless since callers can only call metrics-crate operations on those handles, but if encapsulation matters here, keeping the helper methods and private fields would be the cleaner interface.

Comment thread ethexe/rpc/src/apis/injected/relay.rs
Comment thread ethexe/rpc/src/metrics.rs Outdated
Comment thread ethexe/rpc/src/metrics.rs Outdated
Comment thread ethexe/rpc/src/metrics.rs Outdated
@ecol-master ecol-master added this pull request to the merge queue May 6, 2026
Any commits made after this event will not be merged.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ethexe-rpc: Implement metrics middleware for RPC

4 participants