Apply specific variant types for ChainIndexingMetrics#1612
Apply specific variant types for ChainIndexingMetrics#1612
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 3 Skipped Deployments
|
|
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughConvert chain indexing metrics to a discriminated-union model (Queued/Backfill/Realtime/Completed), refactor deserialization to build/validate per-chain variants, update mocks and a client test, add DeepPartial utility, and wrap per-chain checkpointBlock in a new ChainIndexingStatus object. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Greptile OverviewGreptile Summary
Confidence Score: 4/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant Consumer as SDK Consumer
participant Client as PonderClient.metrics()
participant Fetch as fetch(/metrics)
participant DPM as deserializePrometheusMetrics
participant Inv as invariant_includesRequiredMetrics
participant Build as buildUnvalidatedPonderIndexingMetrics
participant BuildChain as buildUnvalidatedChainIndexingMetrics
participant Zod as schemaPonderIndexingMetrics
Consumer->>Client: metrics()
Client->>Fetch: HTTP GET /metrics
Fetch-->>Client: Prometheus text
Client->>DPM: parse text -> PrometheusMetrics
DPM-->>Client: PrometheusMetrics
Client->>Inv: validate required metric names
loop for each chain in ponder_sync_block labels
Inv->>Inv: validate per-chain seconds + flag conflict
end
Client->>Build: build {appSettings, chains}
loop for each chain reference
Build->>BuildChain: derive discriminated union variant
BuildChain-->>Build: {type, ...fields}
end
Build-->>Client: DeepPartial<PonderIndexingMetrics>
Client->>Zod: validate discriminated union + types
Zod-->>Client: PonderIndexingMetrics
Client-->>Consumer: resolved metrics
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Additional Comments (1)
Adding If backward compatibility is required, gate this requirement (e.g., treat missing metric as “not supported” and fall back to prior logic), or bump major version / document the minimum Ponder version this SDK now requires. |
There was a problem hiding this comment.
Pull request overview
This pull request refactors the ChainIndexingMetrics type in the Ponder SDK from a single interface with boolean flags to a discriminated union with four distinct states. This improves type safety and makes the state machine more explicit and easier to reason about.
Changes:
- Converted
ChainIndexingMetricsfrom an interface with conditional boolean fields to a discriminated union of four state variants:Queued,Backfill,Realtime, andCompleted - Updated deserialization logic to build the appropriate state based on Prometheus metrics values
- Enhanced invariant checking to validate state-specific constraints at the Prometheus metrics level
- Updated test mocks and test assertions to reflect the new data model
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/ponder-sdk/src/indexing-metrics.ts | Defines the new discriminated union type structure with four state variants, replacing the previous single interface with boolean flags |
| packages/ponder-sdk/src/deserialize/utils.ts | Adds DeepPartial utility type for handling partial/unvalidated data structures during deserialization |
| packages/ponder-sdk/src/deserialize/indexing-metrics.ts | Updates deserialization logic to construct appropriate state variants based on Prometheus metrics, with enhanced invariant validation |
| packages/ponder-sdk/src/deserialize/indexing-metrics.mock.ts | Updates mock test data to use the new discriminated union structure with explicit type fields |
| packages/ponder-sdk/src/client.test.ts | Updates test assertion to match the new error message format for conflicting metrics |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| * Represents the indexing metrics for a chain that is currently in | ||
| * the real-time indexing phase by a Ponder app. It means that | ||
| * the backfill phase transitioned to completed phase, as there was | ||
| * no "config end block" specified for the chain. |
There was a problem hiding this comment.
The phrase "transitioned to completed phase" is confusing because "Completed" is a separate distinct state in this type system. Consider clarifying that the backfill phase finished (rather than saying it "transitioned to completed phase"), and since there was no config end block, indexing continues in real-time mode. For example: "Represents the indexing metrics for a chain that has finished the backfill phase and is currently in the real-time indexing phase, as there was no 'config end block' specified for the chain."
| * Represents the indexing metrics for a chain that has completed indexing by | ||
| * a Ponder app. It means that the backfill phase transitioned to completed phase. | ||
| * No more blocks are required to be indexed for the chain at this point. |
There was a problem hiding this comment.
The phrase "transitioned to completed phase" is redundant and could be clearer. Since this IS the Completed state, consider simplifying to: "Represents the indexing metrics for a chain that has finished indexing. The backfill phase completed and reached the configured end block for the chain. No more blocks are required to be indexed."
| * Represents the indexing metrics for a chain that has completed indexing by | |
| * a Ponder app. It means that the backfill phase transitioned to completed phase. | |
| * No more blocks are required to be indexed for the chain at this point. | |
| * Represents the indexing metrics for a chain that has finished indexing. | |
| * The backfill phase completed and reached the configured end block for the chain, | |
| * if one was specified. No more blocks are required to be indexed for the chain. |
tk-o
left a comment
tk-o
left a comment
There was a problem hiding this comment.
Self-review completed.
|
|
||
| # HELP ponder_historical_completed_indexing_seconds Number of seconds that have been completed | ||
| # TYPE ponder_historical_completed_indexing_seconds gauge | ||
| ponder_historical_completed_indexing_seconds{chain="10"} 34242 |
There was a problem hiding this comment.
ponder_historical_completed_indexing_seconds is useful to know if chain indexing is queued.
|
|
||
| const schemaSerializedChainIndexingMetricsCompleted = z.object({ | ||
| type: z.literal(ChainIndexingMetricTypes.Completed), | ||
| targetBlock: schemaBlockRef, |
There was a problem hiding this comment.
Perhaps we just need a better name here, i.e. lastSyncedBlock or lastIndexedBlock. This is for a completed phase, so there must be some "last"/"end"/"target" block defined.
| * @returns Unvalidated (possibly incomplete) Chain Indexing Metrics | ||
| * to be validated by {@link schemaSerializedChainIndexingMetrics}. | ||
| */ | ||
| function buildUnvalidatedChainIndexingMetrics( |
There was a problem hiding this comment.
This function collects all per-chain metrics and translated them into business-layer data model. Value returned by this function is unvalidated, but then is quickly checked by schemaPonderIndexingMetrics schema.
| const chainReferences = prometheusMetrics.getLabels("ponder_sync_block", "chain"); | ||
|
|
||
| // Validate per-chain invariants. | ||
| for (const chainReference of chainReferences) { | ||
| const ponderHistoricalCompletedIndexingSeconds = prometheusMetrics.getValue( | ||
| "ponder_historical_completed_indexing_seconds", | ||
| { chain: chainReference }, | ||
| ); | ||
|
|
||
| // Invariant: historical completed indexing seconds must be a non-negative integer. | ||
| if ( | ||
| typeof ponderHistoricalCompletedIndexingSeconds !== "number" || | ||
| !Number.isInteger(ponderHistoricalCompletedIndexingSeconds) || | ||
| ponderHistoricalCompletedIndexingSeconds < 0 | ||
| ) { | ||
| ctx.issues.push({ | ||
| code: "custom", | ||
| input: ctx.value, | ||
| message: `'ponder_historical_completed_indexing_seconds' metric for '${chainReference}' chain must be a non-negative integer. Received: ${ponderHistoricalCompletedIndexingSeconds}`, | ||
| }); | ||
| } | ||
|
|
||
| const ponderSyncIsComplete = prometheusMetrics.getValue("ponder_sync_is_complete", { | ||
| chain: chainReference, | ||
| }); | ||
|
|
||
| const ponderSyncIsRealtime = prometheusMetrics.getValue("ponder_sync_is_realtime", { | ||
| chain: chainReference, | ||
| }); | ||
|
|
||
| // Invariant: `ponder_sync_is_complete` and `ponder_sync_is_realtime` cannot | ||
| // both be `1` at the same time. | ||
| if (ponderSyncIsComplete === 1 && ponderSyncIsRealtime === 1) { | ||
| ctx.issues.push({ | ||
| code: "custom", | ||
| input: ctx.value, | ||
| message: `'ponder_sync_is_complete' and 'ponder_sync_is_realtime' metrics cannot both be 1 at the same time for chain ${chainReference}`, | ||
| }); | ||
| } | ||
| } |
There was a problem hiding this comment.
These invariants allow buildUnvalidatedChainIndexingMetrics to work on validated per-chain metric values.
| * const update: PartialConfig = { b: { y: { z: true } } }; | ||
| * ``` | ||
| */ | ||
| export type DeepPartial<T> = { |
There was a problem hiding this comment.
Very useful type while working with partially undefined data.
| // Act & Assert | ||
| await expect(ponderClient.metrics()).rejects.toThrowError( | ||
| /Invalid serialized Ponder Indexing Metrics.*Chain Indexing Metrics cannot have both `indexingCompleted` and `indexingRealtime` as `true`/, | ||
| /Invalid serialized Ponder Indexing Metrics.*'ponder_sync_is_complete' and 'ponder_sync_is_realtime' metrics cannot both be 1 at the same time for chain 10/, |
There was a problem hiding this comment.
Better error message referring directly to metric names.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@packages/ponder-sdk/src/deserialize/indexing-metrics.ts`:
- Around line 75-141: In buildUnvalidatedChainIndexingMetrics, treat missing
per-chain metric values as "Queued" instead of falling through to Backfill:
change the ponderHistoricalCompletedIndexingSeconds check to treat both 0 and
undefined/null as queued (e.g., if ponderHistoricalCompletedIndexingSeconds ==
null || ponderHistoricalCompletedIndexingSeconds === 0) and ensure the
subsequent checks for ponderSyncIsComplete and ponderSyncIsRealtime only
consider a value of 1 as true (leave them as strict === 1) so undefined/null
won't be misinterpreted; update logic around latestSyncedBlock and
backfillTotalBlocks handling accordingly so missing metric values don't produce
an unintended Backfill result.
In `@packages/ponder-sdk/src/deserialize/utils.ts`:
- Around line 31-37: DeepPartial currently treats any non-primitive as a nested
object (so Map/Set/Date/RegExp/Function get recursed into); update the
conditional in the DeepPartial type to exclude built-in non-plain objects by
adding a guard: after the array branch check add a case like "T[P] extends Date
| Function | Map<any, any> | Set<any> | WeakMap<any, any> | WeakSet<any> |
RegExp ? T[P] : DeepPartial<T[P]>" so that the type alias DeepPartial<T> only
recurses into plain object shapes and returns the original type for those
built-ins.
In `@packages/ponder-sdk/src/indexing-metrics.ts`:
- Around line 116-136: The doc comment for the interface
ChainIndexingMetricsRealtime uses ambiguous phrasing "the backfill phase
transitioned to completed phase"; update the comment above the
ChainIndexingMetricsRealtime declaration to clearly state that the backfill
phase has finished and the chain has entered realtime indexing mode (i.e.,
backfill completed and indexing continues with no target end block), replacing
the ambiguous "transitioned to completed phase" wording with a concise phrasing
like "the backfill phase has finished and the chain entered realtime mode" so
readers of the ChainIndexingMetricsRealtime docs understand the intended state.
|
@greptile review |
|
@greptile review |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/ponder-sdk/src/indexing-status.ts (1)
30-37:⚠️ Potential issue | 🟡 MinorJSDoc on
chainsis now slightly inaccurate.Line 32 still says "Map of indexed chain IDs to their block reference," but the value type is now
ChainIndexingStatus, notBlockRef.📝 Suggested doc fix
/** - * Map of indexed chain IDs to their block reference. + * Map of indexed chain IDs to their indexing status. * * Guarantees: * - Includes entry for at least one indexed chain. */ chains: Map<ChainId, ChainIndexingStatus>;
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Add `backfillTotalBlocks` field to `ChainIndexingMetricsQueued` The `ponder_historical_total_blocks` metric is calculated for all indexed chains during Ponder app initialization, including queued chains. When indexing multiple chains, typically one chain is in backfill phase while others are queued. Since `ponder_historical_total_blocks` can be useful in both phases, both `ChainIndexingMetricsQueued` and `ChainIndexingMetricsBackfill` now include the `backfillTotalBlocks` field to reference this metric.
![vercel[bot]](https://avatars.githubusercontent.com/in/8329?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@packages/ponder-sdk/src/indexing-metrics.ts`:
- Around line 63-96: There's a typo in the JSDoc for BackfillTotalBlocks: remove
the extra period in the phrase "Ponder metric.." so it reads "Ponder metric."
Update the comment above the export type BackfillTotalBlocks (the block starting
with "Number of blocks required to be indexed during backfill.") to correct that
punctuation.
|
@greptile review |
Additional Comments (1)
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /** | ||
| * Map of indexed chain IDs to their block reference. | ||
| * | ||
| * Guarantees: |
There was a problem hiding this comment.
The doc comment for PonderIndexingStatus.chains is now inaccurate: it still says the map values are a “block reference”, but the type was changed to ChainIndexingStatus (wrapping checkpointBlock). Update the comment to reflect the new shape to avoid confusing consumers.
| /** | ||
| * Is indexing completed for the chain? | ||
| * | ||
| * This will be true when the backfill has been completed, | ||
| * and a specified end block for the chain has been reached. | ||
| * A {@link BlockRef} to the "highest" block that has been discovered by RPCs | ||
| * and stored in the RPC cache as of the time the metric value was captured. | ||
| */ | ||
| indexingCompleted: boolean; | ||
| latestKnownBlock: BlockRef; |
There was a problem hiding this comment.
The JSDoc for latestKnownBlock describes it as the highest block discovered via RPC and stored in the RPC cache. However, the deserializer currently derives this field from ponder_sync_block (closest-to-tip synced block). Please align the documentation with the actual source of truth, or adjust the deserialization logic to populate this field from a metric that truly represents “latest known block”.
| * discovered by RPCs and stored in the RPC cache. | ||
| */ | ||
| export interface ChainIndexingMetricsRealtime { | ||
| type: typeof ChainIndexingMetricTypes.Realtime; |
There was a problem hiding this comment.
Field name and documentation claim latestKnownBlock represents latest discovered block by RPCs, but it actually contains the latest synced/indexed block from ponder_sync_block metric
1 participant
Lite PR
Tip: Review docs on the ENSNode PR process
Summary
ChainIndexingMetricstype have been collected as a discriminated union.ChainIndexingMetricsQueuedandChainIndexingMetricBackfillboth includebackfillTotalBlocksfield to support ENSIndexer use case where, at the ENSIndexer app start, all block refs must be fetched from RPC and cached for further use.ChainIndexingMetricsRealtimeincludeslatestKnownBlockfieldChainIndexingMetricsCompletedincludestargetBlockfieldPonderIndexingStatusnow includes thecheckpointBlockfield for each indexed chain.Why
Testing
Notes for Reviewer (Optional)
Pre-Review Checklist (Blocking)