feat(debug-trace-server): close the per-request accounting identity and label errors by reason - #178
feat(debug-trace-server): close the per-request accounting identity and label errors by reason#178flyq wants to merge 4 commits into
Conversation
…nd label errors by reason
Makes "did any client see a timeout?" a metric lookup instead of an
inference from three subtracted counters. Prompted by two client-visible
`-32001`s on ore (`Witness fetch deadline exceeded`, budget_ms=7999) that
were indistinguishable from any other error in `rpc_errors_total`.
Every arrival is counted once by `request_shape_total` before the first
await, and now lands in exactly one terminal bucket:
request_shape_total = rpc_requests_total
+ rpc_errors_total{reason}
+ requests_cancelled_total
`reason` splits outcomes that share a JSON-RPC code — a blown witness
deadline and an unknown transaction both leave as -32001, and only the
first is an incident. Labels: deadline_witness / deadline_block /
not_found / invalid_params / trace_failed / internal. The full
(method, reason) grid is pre-registered so an alert on deadline_witness
reads zero from boot rather than missing.
`rpc_errors_total` moves out of the `RpcMethodMetrics` derive to a
`counter!` call so one metric name never carries two label sets. Fetch
failures funnel through `data_provider_failure`, keeping the counter and
the returned code from drifting apart as call sites are added.
Also fixes a request that was literally unaccounted-for: Parity
`trace_transaction` degrades a not-found/pending/timeout to `Ok(null)`
and recorded neither counter, so the identity silently lost it. It now
counts as served, with the swallowed cause kept visible on
`debug_trace_null_results_total{reason}`.
Cancellations are recorded from the drop of the request future in
`rpc_middleware.rs` — the only layer that sees single calls and batch
entries alike, and the only place a client hang-up is observable at all,
since every completion-time metric is by definition never reached.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude review status
🛠️ Review did not finish Attempted This round did not publish: PRIOR_FINDING_INVALID in phase compile. Anything listed below is from the last round that did. Re-run the workflow or push a new commit to try again. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: aca7115a17
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
… edges - trace_block/trace_transaction record their default-shape arrival at handler entry, so the opts-less Parity pair (and its null-compat responses) no longer counts served against a zero arrival side. - A batch the server aborts over the response-size cap flags its entries' cancel guards: killed entries record no outcome instead of masquerading as client hangups. - Error responses the framework produced before any handler ran (unknown method, malformed top-level params, unparseable batch entries) are folded in by the middleware as the balanced pair shape="rejected" / reason="rejected", gated by a task-local that handler-recorded errors disarm so nothing double-counts. - Batch entries never started when the connection died stay on neither side of the identity — documented as the deliberate approximation, with cancelled correspondingly undercounting torn-down oversized batches. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0c4ca0a542
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| METHOD_DEBUG_TRACE_BLOCK_BY_NUMBER, | ||
| METHOD_DEBUG_TRACE_BLOCK_BY_HASH, | ||
| METHOD_DEBUG_TRACE_TRANSACTION, | ||
| METHOD_TRACE_BLOCK, | ||
| METHOD_TRACE_TRANSACTION, |
There was a problem hiding this comment.
This new accounting grid covers only the five tracing methods, but debug_getCacheStatus is still one of the exposed RPC methods and its handler returns directly without calling record_request_shape or record_rpc_request. Successful cache-status calls therefore disappear from both the arrival and served sides while malformed ones can still be labeled by the middleware, so dashboards checking the per-method identity will miss this method entirely; add cache-status to the accounting path or explicitly exclude it from the invariant.
AGENTS.md reference: AGENTS.md:L112-L112
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 8b95101 (the commit after the one this round reviewed): get_cache_status records a default arrival and a served count at entry — the one method outside the trace pipeline now balances against the middleware's cancelled/rejected terms — and the pre-registration grids iterate ALL_METHODS, so its series exist from boot.
…oles, dedup the identity plumbing /simplify pass over the PR. Correctness holes found by review: served was recorded before serialize_reply could still fail — one serialization failure counted as served, then the middleware fallback stacked a phantom rejected arrival+error pair on top; both tx handlers now count served only after the reply serializes, and serialize_reply records its own internal error. debug_getCacheStatus joins the identity (default arrival + served) since the middleware's cancelled/rejected terms already covered it. Plumbing dedup: the cancelled/rejected recorders take the guard's already-resolved label (one resolution point), record_framework_rejection delegates to record_rpc_error, and the single-call wrapper builds the inner future eagerly instead of cloning the service per request. Pre-registration: the reason/cancelled grid iterates ALL_METHODS, null-results registers for trace_transaction only (trace_block has no null path), and the cache-status arrival series exists from boot. Tests: error_reason assertions folded into the code-mapping test; resolution-behavior asserts left to their metrics tests; repeated identity rationale trimmed to one canonical home. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8b95101285
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| let guard = | ||
| CancelGuard::for_entry(req.method_name(), server_abort.clone()); | ||
| let req = owned_request(req); |
There was a problem hiding this comment.
When a client disconnects just after this batch entry is spawned but before Tokio first polls the spawned future, the future is dropped with guard already constructed, so CancelGuard::drop increments debug_trace_requests_cancelled_total. The handler has not reached classify_and_gate or the opts-less arrival code yet, so no debug_trace_request_shape_total sample was emitted; this puts spawned-but-never-polled entries on the cancellation side only and breaks the per-method accounting identity. Fresh evidence beyond the earlier unstarted-entry discussion is that the guard is now armed before the first handler-side arrival point.
AGENTS.md reference: AGENTS.md:L112-L112
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 8b07145 — the guard is no longer armed at spawn time: CancelGuard is constructed inside the request future (both the single-call wrapper and the spawned batch entry), so it arms on the first poll — the same poll that runs the handler's synchronous prefix and records the arrival. A future dropped before ever being polled now records neither side of the identity, and one polled at least once has its arrival on the books before the guard can fire. The label is resolved once at capture time (method_label) since the request is consumed by the inner call.
| let block_num = | ||
| self.data_provider.resolve_block_number(block_number, deadline).await.map_err(|e| { | ||
| metrics::record_rpc_error(method); | ||
| metrics::record_rpc_error(method, error_reason(&e)); |
There was a problem hiding this comment.
For debug_traceBlockByNumber/trace_block requests with the supported block-tag spelling "pending", resolve_block_number returns DataProviderError::Internal from its Pending block not supported branch, so this new error_reason(&e) path increments reason="internal". That lets any client create internal-error samples even though the failure is an unsupported input, making the new reason grid noisy for incident alerts; classify this branch as invalid params/not found instead of passing the catch-all through.
AGENTS.md reference: AGENTS.md:L114-L114
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 8b07145 with a typed variant rather than special-casing at the call site (this repo classifies errors structurally, never by message text): resolve_block_number's pending arm now returns DataProviderError::UnsupportedBlockTag, which error_reason maps to invalid_params and the shared renderer to -32602 — a client sending "pending" can no longer mint reason="internal" samples. The exhaustive matches forced every classification site to handle the new variant at compile time, which is the discipline the enum exists for. Pinned in data_provider_error_to_rpc_error_code_mapping.
| metrics::record_rpc_error(method, error_reason(&e)); | ||
| rpc_err(format!("Failed to resolve block number: {e}")) |
There was a problem hiding this comment.
When a latest/finalized/safe by-number request has to query upstream and that tag-resolution call exceeds the request deadline, resolve_block_number returns DataProviderError::Timeout; this branch records it as reason="deadline_block" but still returns rpc_err(...) (-32000) instead of the shared data_provider_failure/data_provider_error_to_rpc_error mapping (-32001). The same deadline is therefore exposed with a different JSON-RPC code depending on whether it fired during tag resolution or the later hash/block fetch, which undermines the new reason/code split.
AGENTS.md reference: AGENTS.md:L114-L114
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 8b07145 — lookup_block_by_number's resolve step now goes through the shared data_provider_failure funnel like every other fetch-path failure, so a deadline blown during tag resolution leaves as -32001 with the standard message, exactly like one blown during the hash or block fetch; reason="deadline_block" and the code now agree regardless of which stage fired. This also retired the last bespoke record+render pair (the old -32000 "Failed to resolve block number: …" wrap).
…or codes, arm-on-first-poll guards Review round 3. `pending` block tags now reject via the typed DataProviderError::UnsupportedBlockTag — reason=invalid_params and -32602 instead of client-triggerable reason=internal noise wrapped in -32000. lookup_block_by_number routes resolve errors through data_provider_failure, so a deadline blown during tag resolution leaves as -32001 exactly like one blown during the hash/block fetch. CancelGuard construction moves inside the request future: it arms on the first poll — the same poll that runs the handler's synchronous prefix and records the arrival — so a future dropped before ever being polled records neither side of the accounting identity (previously a spawned-but-never-polled batch entry counted cancelled with no arrival). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8b07145144
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if let Err(err) = batch_rp.append(rp) { | ||
| server_abort.store(true, Ordering::Relaxed); | ||
| return stamp_entry_cpu(err, &entry_cpu); |
There was a problem hiding this comment.
When a completed batch entry is the response that pushes the aggregate batch over max_response_body_size, its handler has already recorded debug_trace_rpc_requests_total before this append branch returns the batch-level error, so the client does not receive that entry's result even though the per-method ledger says it was served; other entries aborted by the same server-side cap record no outcome, leaving oversized trace batches with false served samples instead of a visible failure. Fresh evidence beyond the earlier cancellation fix is that this append-failure path runs after the entry has settled and recorded success.
AGENTS.md reference: AGENTS.md:L112-L112
Useful? React with 👍 / 👎.
2 participants
Summary
Makes "did any client see a timeout?" a metric lookup instead of an inference from three subtracted counters. Every arrival now lands in exactly one terminal bucket, and errors carry a
reasonlabel.Root cause
On 2026-08-09 ore returned two client-visible
-32001s (Witness fetch deadline exceeded,budget_ms=7999). They were indistinguishable from any other failure, becausedebug_trace_rpc_errors_totalcarries a singlemethodlabel (metrics.rs:93) — a malformedtracerConfig(-32602), an unknown transaction (-32001), a blown deadline (-32001) and a trace failure (-32000) all collapse into one series.debug_trace_upstream_deadline_exceeded_totalis not a substitute: it over-counts (the witness and full-block arms race under one deadline atdata_provider.rs:1006, contract fetches fan out per code hash), fires with no client error at all for the swallowed tip seed (data_provider.rs:496), and under-counts when single-flight fans one deadline out to N clients.Worse, one request shape was literally unaccounted-for: Parity
trace_transactiondegrades a not-found/pending/timeout toOk(null)and recorded neitherrpc_requests_totalnorrpc_errors_total, so the request vanished from both sides of the ledger.Fix
The identity that now holds per method:
reason∈deadline_witness/deadline_block/not_found/invalid_params/trace_failed/internal. The full(method, reason)grid is pre-registered so an alert ondeadline_witnessreads zero from boot rather than missing.rpc_errors_totalmoves out of theRpcMethodMetricsderive to acounter!call, so one metric name never carries two label sets.data_provider_failure(rpc_service.rs:396), keeping the counter and the returned JSON-RPC code from drifting apart as call sites are added.error_reasonis deliberately finer than the code: three reasons share-32001.trace_transactionnow counts itsnullas served (or the identity loses it) with the swallowed cause ondebug_trace_null_results_total{reason}.rpc_middleware.rs— the only layer that sees single calls and batch entries alike, and the only place a client hang-up is observable at all, since every completion-time metric is by definition never reached.Testing
cargo test --workspacegreen (138 in this bin, +3 new);fmt/clippy --all-targets --all-features/cargo sortclean.New:
error_reason_separates_deadlines_from_not_found,error_reason_labels_are_unique_and_complete,cancel_guard_arms_until_settled.Notes
Adding a label to an existing metric name is a dashboard-visible change:
debug_trace_rpc_errors_totalqueries must now aggregate overreason(sum by (method)) to reproduce the old series.