Refactor request metrics collection to per-call aggregation - #1390
Conversation
Sources no longer call Prometheus directly for per-request tracking (getLogs, getHeight, getBlockHashes, RPC's eth_* calls, SVM getInstructions). Each method now returns the (method, seconds) stats for the actual backend requests it made; SourceManager aggregates them per source, and Metrics inlines the aggregated counts/seconds into the existing envio_source_request_total / envio_source_request_seconds_total series alongside prom-client's registry. RPC's shared block/transaction/receipt loaders record through a per-source pending buffer at the loaderFn boundary, so only actual network requests count (cache/dedup hits don't). SVM HyperSync now tracks seconds for getInstructions and getBlockHashes, matching the EVM HyperSync source. Height-stream sources (HyperSync SSE, RPC WebSocket) are unchanged, still on Prometheus.SourceRequestCount.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR replaces direct Prometheus request instrumentation with per-request stats objects. Sources now return ChangesRequest stats plumbing
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Source
participant SourceManager
participant Metrics
Source->>SourceManager: return {height/result, requestStats}
SourceManager->>SourceManager: recordRequestStats(sourceState, requestStats)
Metrics->>SourceManager: getRequestStatSamples()
Metrics->>Metrics: renderSourceRequests(samples)
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Comment |
…rics-refactor-bjlbat # Conflicts: # packages/envio/src/sources/SvmHyperSyncSource.res
… text Height-stream sources (HyperSync SSE, RPC WebSocket) were the last callers of Prometheus.SourceRequestCount, splitting source-request tracking across prom-client and our own SourceManager-aggregated state. Move them onto the latter: SourceManager now records a count-only "heightSubscription" stat (no seconds — there's no request/response to time) at the point it creates a height subscription, rather than the streams recording per received message. Delete Prometheus.SourceRequestCount and its label schema, and inline the envio_source_request_total/envio_source_request_seconds_total HELP/TYPE lines directly in Metrics.renderSourceRequests instead of relying on prom-client's registry for them. renderSourceRequests now takes a flat samples array rather than chainStates, making it directly testable; add coverage for the rendered text with and without samples.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/envio/src/sources/RpcSource.res (1)
316-331: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winFailed requests silently skip
recordRequest, undercounting error-path metrics.
queryLogs(eth_getLogs), the transaction loader (eth_getTransactionByHash), and the receipt loader (eth_getTransactionReceipt) all record viaPromise.thenResolveon the success path only. If the underlying request rejects,recordRequestis never called for that attempt, and the rejection just propagates.This contradicts the invariant documented a few lines below (1040-1047): "Every actual request (cache/dedup hits never reach recordRequest) pushes here."
getKnownRawBlockWithBackoff(lines 58-72) gets this right — it records timing on both the exception and success branches. The three sites here don't, so RPC errors (rate limits, oversized-response errors, transient failures) systematically undercountenvio_source_request_*metrics precisely during the periods operators most need visibility into.🐛 Proposed fix (mirroring the block-loader's pattern)
let queryLogs = ({addresses, topicQuery}: logSelection) => { let timerRef = Performance.now() rpcClient.getLogs({ fromBlock, toBlock, ?addresses, topics: topicQuery->Array.map(filter => switch filter { | Rpc.GetLogs.Null => Nullable.null | Single(topic) => Nullable.make([topic]) | Multiple(topics) => Nullable.make(topics) } ), })->Promise.thenResolve(items => { recordRequest(~method="eth_getLogs", ~seconds=timerRef->Performance.secondsSince) items }) + ->Promise.catch(exn => { + recordRequest(~method="eth_getLogs", ~seconds=timerRef->Performance.secondsSince) + exn->Promise.reject + }) }~loaderFn=transactionHash => { let timerRef = Performance.now() Rpc.GetTransactionByHash.rawRoute ->Rest.fetch(transactionHash, ~client) ->Promise.thenResolve(res => { recordRequest( ~method="eth_getTransactionByHash", ~seconds=timerRef->Performance.secondsSince, ) res }) + ->Promise.catch(exn => { + recordRequest( + ~method="eth_getTransactionByHash", + ~seconds=timerRef->Performance.secondsSince, + ) + exn->Promise.reject + }) },Apply the analogous change to the receipt loader (
eth_getTransactionReceipt).As per coding guidelines, "Use try/catch as expressions instead of refs for tracking success/failure" — an
async/switch await ... { exception ... }rewrite (matchinggetKnownRawBlockWithBackoff's style) is a cleaner alternative to thePromise.catchchain above.Also applies to: 1061-1070, 1121-1130
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/envio/src/sources/RpcSource.res` around lines 316 - 331, The RPC loaders in RpcSource.res only call recordRequest on the success path, so rejected requests for queryLogs, eth_getTransactionByHash, and eth_getTransactionReceipt are never counted. Update these loaders to record timing on both success and failure, using the same try/catch-as-expression pattern already used by getKnownRawBlockWithBackoff so recordRequest is always invoked before rethrowing the error. Keep the existing method names in the recordRequest calls and apply the same fix consistently across the three affected request handlers.Source: Coding guidelines
🧹 Nitpick comments (6)
packages/envio/src/sources/HyperFuelSource.res (1)
484-499: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMinor: inconsistent qualification of
methodfield between the two requestStat literals.Line 306 writes
{Source.method: "getLogs", seconds: pageFetchTime}while line 498 writes{method: "getHeight", seconds}unqualified. Both compile fine givenopen Sourceat the top of the file, but the inconsistency is a small readability nit.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/envio/src/sources/HyperFuelSource.res` around lines 484 - 499, The requestStats literal in getHeightOrThrow uses an unqualified method field while the other requestStat entry in this file uses Source.method, creating a small inconsistency. Update the getHeightOrThrow record in HyperFuelSource so the method field is qualified the same way as the getLogs entry, keeping the style consistent across requestStat literals.packages/envio/src/sources/SourceManager.res (2)
3-6: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDrop the "rendered by Metrics.renderSourceRequests" pointer from the comment.
The first sentence restates the type's obvious purpose; the last sentence points to a specific downstream caller, both disallowed by the
.rescomment guideline.✏️ Suggested trim
-// Cumulative per-method request count/time for a source, aggregated from the -// requestStat arrays returned by its methods. Rendered into -// envio_source_request_* by Metrics.renderSourceRequests. +// Cumulative count/total-seconds for one (source, method) pair. type requestStatAgg = {mutable count: int, mutable seconds: float}As per coding guidelines,
**/*.res: "Don't write a comment that restates what the code already says... which callers use a value... pointers to where something is 'now defined'".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/envio/src/sources/SourceManager.res` around lines 3 - 6, Remove the downstream caller pointer from the `requestStatAgg` comment in `SourceManager.res` and trim it to only the necessary non-obvious note; keep the type definition itself unchanged and update the surrounding comment so it no longer restates the obvious purpose or mentions `Metrics.renderSourceRequests`.Source: Coding guidelines
31-39: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSame caller-pointer comment issue as the
requestStatAggtype."for Metrics.renderSourceRequests to inline into the /metrics response" names a specific caller, which the
.resguideline says to avoid.✏️ Suggested trim
-// Flattened (source, method) aggregates for Metrics.renderSourceRequests to -// inline into the /metrics response. +// Flattened (source, chain, method) view of the per-source request aggregates. type requestStatSample = {As per coding guidelines,
**/*.res: "Don't write a comment that restates what the code already says... which callers use a value...".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/envio/src/sources/SourceManager.res` around lines 31 - 39, The comment on requestStatSample repeats implementation intent and names a specific caller, which should be removed to follow the .res comment guidelines. Trim or delete the caller-pointer wording in SourceManager.res and keep only a minimal description of the type’s purpose, alongside requestStatAgg if needed, so the definition stands without referencing Metrics.renderSourceRequests or /metrics.Source: Coding guidelines
packages/envio/src/sources/Source.res (1)
10-13: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTrim the comment to the non-obvious part; drop the "who consumes this" pointer.
The first two lines capture a genuinely non-obvious invariant (cache/dedup hits aren't requests) — keep those. The last sentence ("SourceManager aggregates these per (source, method) into the envio_source_request_* metrics") describes a downstream caller/consumer, which the guideline explicitly disallows.
✏️ Suggested trim
-// A single backend request a source method actually made (cache/dedup hits -// aren't requests), with the time it took. SourceManager aggregates these -// per (source, method) into the envio_source_request_* metrics. +// A single backend request a source method actually made (cache/dedup hits +// aren't requests), with the time it took. type requestStat = {method: string, seconds: float}As per coding guidelines,
**/*.res: "Don't write a comment that restates what the code already says — module purpose, what a function does, which callers use a value, history of a refactor, or pointers to where something is 'now defined'".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/envio/src/sources/Source.res` around lines 10 - 13, Trim the documentation comment above requestStat in Source.res to keep only the non-obvious invariant that cache/dedup hits are not counted as requests, and remove the downstream usage note about SourceManager aggregating into envio_source_request_* metrics. Keep the comment focused on the meaning of requestStat itself and use requestStat as the anchor when locating the block.Source: Coding guidelines
packages/envio/src/Metrics.res (1)
21-24: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTrim the comment to just the HELP/TYPE invariant; drop the restated purpose.
The last sentence (about the metric name already being declared via
Prometheus.SourceRequestCount) is a genuinely non-obvious constraint worth keeping. The first two sentences just restate what the function computes and where the data comes from.✏️ Suggested trim
-// Samples for envio_source_request_total/envio_source_request_seconds_total, -// aggregated per (source, chain, method) by SourceManager. The HELP/TYPE lines -// for both metric names are already emitted by prom-client's registry, via -// Prometheus.SourceRequestCount — still used directly by height-stream sources. +// HELP/TYPE lines for envio_source_request_total/envio_source_request_seconds_total +// are already emitted by prom-client's registry via Prometheus.SourceRequestCount +// (still used directly by height-stream sources), so we must not repeat them here. let renderSourceRequests = (~chainStates: dict<ChainState.t>) => {As per coding guidelines,
**/*.res: "Don't write a comment that restates what the code already says — module purpose, what a function does...".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/envio/src/Metrics.res` around lines 21 - 24, Trim the comment in Metrics.res so it only preserves the non-obvious HELP/TYPE invariant tied to Prometheus.SourceRequestCount; remove the sentences that restate what SourceManager aggregates or how the samples are produced, and keep only the note that both metric names already have HELP/TYPE lines emitted by the registry and are still used directly by height-stream sources.Source: Coding guidelines
scenarios/test_codegen/test/RpcSource_test.res (1)
39-58: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider asserting on
requestStatstoo.The test now receives
requestStatsfromgetHeightOrThrow()but only destructures/assertsheight. Since this is the new feature under test in this PR, asserting the array contains aneth_blockNumberentry with a non-negativesecondsvalue would give real coverage of the new contract, not just height parsing.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scenarios/test_codegen/test/RpcSource_test.res` around lines 39 - 58, The RpcSource.getHeightOrThrow test currently only verifies height, so it does not cover the new requestStats contract. Update the test in RpcSource_test.res to destructure requestStats alongside height from getHeightOrThrow, and add an assertion that requestStats includes an eth_blockNumber entry with a non-negative seconds value. Use the RpcSource.make and getHeightOrThrow symbols to locate the test and keep the existing height assertions intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/envio/src/Metrics.res`:
- Around line 21-53: The enviro_source_request_* metrics are being emitted twice
in the same scrape because collect() appends renderSourceRequests(~chainStates)
even though PromClient.defaultRegister->PromClient.metrics already includes
Prometheus.SourceRequestCount. Update collect and/or renderSourceRequests so the
SourceManager samples are only exposed once per scrape, or rename the
scrape-time aggregate to distinct metric names; use the existing collect and
renderSourceRequests symbols to locate the duplicate emission.
In `@packages/envio/src/sources/SvmHyperSyncSource.res`:
- Around line 632-645: The catch path in SvmHyperSyncSource.getBlockHashes is
discarding already collected requestStats when queryBlockDataRange throws after
partial success. Update the error branch to return the accumulated requestStats
from the successful pages instead of an empty array, matching the RpcSource
pattern and preserving buffered stats on partial failures.
---
Outside diff comments:
In `@packages/envio/src/sources/RpcSource.res`:
- Around line 316-331: The RPC loaders in RpcSource.res only call recordRequest
on the success path, so rejected requests for queryLogs,
eth_getTransactionByHash, and eth_getTransactionReceipt are never counted.
Update these loaders to record timing on both success and failure, using the
same try/catch-as-expression pattern already used by getKnownRawBlockWithBackoff
so recordRequest is always invoked before rethrowing the error. Keep the
existing method names in the recordRequest calls and apply the same fix
consistently across the three affected request handlers.
---
Nitpick comments:
In `@packages/envio/src/Metrics.res`:
- Around line 21-24: Trim the comment in Metrics.res so it only preserves the
non-obvious HELP/TYPE invariant tied to Prometheus.SourceRequestCount; remove
the sentences that restate what SourceManager aggregates or how the samples are
produced, and keep only the note that both metric names already have HELP/TYPE
lines emitted by the registry and are still used directly by height-stream
sources.
In `@packages/envio/src/sources/HyperFuelSource.res`:
- Around line 484-499: The requestStats literal in getHeightOrThrow uses an
unqualified method field while the other requestStat entry in this file uses
Source.method, creating a small inconsistency. Update the getHeightOrThrow
record in HyperFuelSource so the method field is qualified the same way as the
getLogs entry, keeping the style consistent across requestStat literals.
In `@packages/envio/src/sources/Source.res`:
- Around line 10-13: Trim the documentation comment above requestStat in
Source.res to keep only the non-obvious invariant that cache/dedup hits are not
counted as requests, and remove the downstream usage note about SourceManager
aggregating into envio_source_request_* metrics. Keep the comment focused on the
meaning of requestStat itself and use requestStat as the anchor when locating
the block.
In `@packages/envio/src/sources/SourceManager.res`:
- Around line 3-6: Remove the downstream caller pointer from the
`requestStatAgg` comment in `SourceManager.res` and trim it to only the
necessary non-obvious note; keep the type definition itself unchanged and update
the surrounding comment so it no longer restates the obvious purpose or mentions
`Metrics.renderSourceRequests`.
- Around line 31-39: The comment on requestStatSample repeats implementation
intent and names a specific caller, which should be removed to follow the .res
comment guidelines. Trim or delete the caller-pointer wording in
SourceManager.res and keep only a minimal description of the type’s purpose,
alongside requestStatAgg if needed, so the definition stands without referencing
Metrics.renderSourceRequests or /metrics.
In `@scenarios/test_codegen/test/RpcSource_test.res`:
- Around line 39-58: The RpcSource.getHeightOrThrow test currently only verifies
height, so it does not cover the new requestStats contract. Update the test in
RpcSource_test.res to destructure requestStats alongside height from
getHeightOrThrow, and add an assertion that requestStats includes an
eth_blockNumber entry with a non-negative seconds value. Use the RpcSource.make
and getHeightOrThrow symbols to locate the test and keep the existing height
assertions intact.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1b02fd05-8656-4421-ade9-6f60292152d6
📒 Files selected for processing (17)
packages/envio/src/Metrics.respackages/envio/src/sources/HyperFuelSource.respackages/envio/src/sources/HyperSync.respackages/envio/src/sources/HyperSync.resipackages/envio/src/sources/HyperSyncSource.respackages/envio/src/sources/RpcSource.respackages/envio/src/sources/SimulateSource.respackages/envio/src/sources/Source.respackages/envio/src/sources/SourceManager.respackages/envio/src/sources/SourceManager.resipackages/envio/src/sources/Svm.respackages/envio/src/sources/SvmHyperSyncSource.resscenarios/fuel_test/test/HyperFuelHeight_test.resscenarios/test_codegen/test/RateLimit_test.resscenarios/test_codegen/test/RpcSource_test.resscenarios/test_codegen/test/helpers/MockIndexer.resscenarios/test_codegen/test/rollback/ChainMocking.res
| let (blockDatas, requestStats) = await queryBlockDataRange( | ||
| ~fromSlot=minSlot.contents, | ||
| ~toSlot=maxSlot.contents, | ||
| ) | ||
| // Keep one entry per requested slot; drop duplicates and unrelated slots. | ||
| Ok(blockDatas->Array.filter(data => requested->Utils.Set.delete(data.blockNumber))) | ||
| { | ||
| Source.result: Ok( | ||
| blockDatas->Array.filter(data => requested->Utils.Set.delete(data.blockNumber)), | ||
| ), | ||
| requestStats, | ||
| } | ||
| } catch { | ||
| | exn => Error(exn) | ||
| | exn => {Source.result: Error(exn), requestStats: []} | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Compare failure-path stat handling: RpcSource drains buffered stats; check how SVM differs.
rg -nP -C4 'drainRequestStats|pendingRequestStats' packages/envio/src/sources/RpcSource.res
rg -nP -C3 'requestStats: \[\]|catch' packages/envio/src/sources/SvmHyperSyncSource.resRepository: enviodev/hyperindex
Length of output: 2921
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the surrounding implementation of the failing path and the RPC drain pattern.
sed -n '600,660p' packages/envio/src/sources/SvmHyperSyncSource.res
printf '\n--- RPC drain path ---\n'
sed -n '1040,1060p' packages/envio/src/sources/RpcSource.res
printf '\n--- RPC getBlockHashes / catch path ---\n'
sed -n '1368,1452p' packages/envio/src/sources/RpcSource.resRepository: enviodev/hyperindex
Length of output: 5603
Preserve buffered request stats on getBlockHashes failures
If queryBlockDataRange throws after some pages succeed, getBlockHashes returns requestStats: [], dropping the requests that already ran. That makes this source go silent during partial failures; return the accumulated stats from the catch path instead, like RpcSource does.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/envio/src/sources/SvmHyperSyncSource.res` around lines 632 - 645,
The catch path in SvmHyperSyncSource.getBlockHashes is discarding already
collected requestStats when queryBlockDataRange throws after partial success.
Update the error branch to return the accumulated requestStats from the
successful pages instead of an empty array, matching the RpcSource pattern and
preserving buffered stats on partial failures.
…uest text heightSubscription never carries real timing, so its seconds line was a misleading always-zero value; drop it. Drop both HELP/TYPE blocks entirely when there are no samples to report at all.
…get-redesign-yl7t1u Brings in the block-store/ChainState rework (#1369) and per-call metrics aggregation (#1390). Auto-merged cleanly; verified envio + test_codegen compile and the scheduler, rollback, SourceManager, and IndexerState tests pass against the merged tree. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0174BGgup49cCEGmFyBW7Wyx
2 participants
Summary
Refactor RPC/HyperSync request metrics collection from direct Prometheus increments to per-call aggregation. Instead of recording metrics immediately at each request site, sources now accumulate request statistics (method name and duration) and return them alongside query results. SourceManager aggregates these per (source, method) and exposes them via a new Metrics endpoint.
Key Changes
Source interface updates:
getHeightOrThrow()andgetBlockHashes()now return wrapper types (getHeightResponse,getBlockHashesResponse) containing both the result and an array ofrequestStatentries (method name + seconds).Request tracking in RpcSource: Replaced direct
Prometheus.SourceRequestCount.increment()calls with arecordRequestcallback that accumulates stats in apendingRequestStatsarray. Stats are drained and returned with each query result viadrainRequestStats().SourceManager aggregation: Added
requestStats: dict<requestStatAgg>to track cumulative per-method counts and durations. NewrecordRequestStats()function merges incoming stats arrays into this dict. NewgetRequestStatSamples()exports flattened (source, method) aggregates.Metrics rendering: New
Metrics.renderSourceRequests()function converts SourceManager samples into Prometheus text format forenvio_source_request_totalandenvio_source_request_seconds_totalmetrics.HyperSync/HyperFuel sources: Updated to return
requestStatsarrays fromgetItemsOrThrow()andgetHeightOrThrow(). Request timing now captured at query boundaries rather than at Prometheus call sites.Test/mock updates: Updated MockIndexer, SimulateSource, and test helpers to return the new wrapper types with empty
requestStatsarrays.Implementation Details
pendingRequestStatsarray within each source'smake()function, allowing concurrent calls to share the same recording mechanism without attribution issues.Prometheus.SourceRequestCount.increment/addSeconds) are removed from request paths; the new metrics are rendered by Metrics module from SourceManager's aggregates.https://claude.ai/code/session_019LHZ2vfkopb9LwS8zTdQ8F
Summary by CodeRabbit
renderSourceRequestsoutput behavior.