Repository navigation
fix(seidb): report only the current migration boundary on the snapshot gauge - #4327
Conversation
…t gauge Co-authored-by: Cursor <cursoragent@cursor.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
PR SummaryLow Risk Overview MigrationMetrics now uses an New Reviewed by Cursor Bugbot for commit 793ab83. Bugbot is set up for automated code reviews on this repo. Configure here. |
|
The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).
|
There was a problem hiding this comment.
The PR swaps the synchronous boundary-snapshot gauge for an Int64ObservableGauge whose callback reports only the current boundary, or "complete" once the target version is reached. That fixes the unbounded series growth. The callback is unregistered when the router context is cancelled, reads its state under the existing mutex, and all three call sites are updated. The new ManualReader tests cover reporting only the current boundary, the "complete" label, and stopping after Close. I found no issues.
Findings: 0 blocking | 0 non-blocking | 0 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- None at the file/PR level.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4327 +/- ##
==========================================
- Coverage 67.53% 66.64% -0.89%
==========================================
Files 2181 2077 -104
Lines 167804 157492 -10312
==========================================
- Hits 113325 104966 -8359
+ Misses 54469 52516 -1953
Partials 10 10
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
Successfully created backport PR for |
* main: (21 commits) Backport evmonly parse, app-hash and changeset perf fixes from giga-1 (#4345) Remove the oracle module behind a v6.8 upgrade (#4319) fix(seidb): report only the current migration boundary on the snapshot gauge (#4327) fix(flatkv): keep 10 old checkpoints instead of mirroring memIAVL's count (#4322) Backport Autobahn execute-loop and produced-tx metrics from giga-1 (#4330) Add giga.storage.receipts to toggle the Autobahn receipt store (#4333) optimize gather phase (#4326) Regenerate the Unreleased changelog as a plain PR list (#4336) Bump sei-protocol/go-ethereum to v1.15.7-sei-21 (#4332) Fail dynamic-gas precompile out-of-gas as an EVM out-of-gas call (#4318) Add dashboard and topology option for Autobahn e2e (#4167) Add Giga fetch/serve and BlockDB prune metrics (#4329) Add eth_getLogs to the EVM-only Giga RPC (#4308) Generate v6.8 precompiles (#4320) Add [giga] app.toml section and honor it on the Autobahn node (#4323) feat(evmonly): add eth_estimateGas via existing libraries (#4325) Use Pebble batch directly in SS (#4300) Fix pruning issue in SS causing huge disk spike (#4321) Make Autobahn always run the EVM-only executor, disable/remove some integration tests (#4316) reduce seal lock contention (#4314) ...
…ei-protocol#4436) On atlantic-2, archive-0-0-0, snapshotter-0 and state-sync-node-0 finished the EVM migration (all 123,876,555 keys moved, `seidb_migration_version` went to 1), but the FlatKV migration dashboard still shows them as migrating, at 99.3% with ~875k EVM keys left in memIAVL. The migration is complete; two gauges report stale values, and only on nodes that export state-sync snapshots. A snapshot export opens a read-only composite store at the snapshot height, and when that height is before completion, the `MigrationManager` built for that handle records version 0 on the process-wide `seidb_migration_version` gauge. Nothing records 1 again until restart. Separately, `rootmulti.Store.Snapshot` records `iavl_total_num_keys` only for stores that exported at least one node, so once the memIAVL `evm` tree is empty its last pre-migration value is exported forever. This is the same retention problem sei-protocol#4327 fixed for `seidb_migration_boundary_snapshot`. `migration.BuildRouter` now takes `RouterOption`s, and `WithoutTelemetry()` gives the router's `MigrationManager` `newLocalMigrationMetrics()` instead of the OTel-backed instance. `CompositeCommitStore.buildRouter` passes it for derived stores (the `LoadVersionReadOnly` view and `Copy`), so only the live store publishes migration metrics. `Snapshot` sets the per-store totals to zero on each store header, so a store with no nodes records 0. Converting the version gauge to an observable gauge would also work, but it leaves read-only handles publishing the other migration counters, so they are cut off at the router instead. No consensus, state, or wire-format impact: only metric emission changes, and the option is variadic, so existing `BuildRouter` callers are unchanged. A store absent from an export entirely (rather than exported empty) still keeps its last value; that does not occur for `evm` after the migration. Affected nodes show correct values after deploy, once they restart and export their next snapshot. `TestLoadVersionReadOnlyDoesNotReportMigrationVersion` and `TestSnapshotReportsZeroKeysForEmptyStore` each fail without their half of the fix; the migration, composite and rootmulti suites pass. --------- Co-authored-by: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> Co-authored-by: Cursor <cursoragent@cursor.com>
seidb_migration_boundary_snapshotgrows without bound during a migration. Every 10 secondsMigrationMetricsrecorded the current boundary as a newboundary_hexlabel on a synchronous OTel gauge, and the OTel Go SDK keeps every attribute set a synchronous gauge has recorded and exports all of them on each scrape. On arctic-1, each migrating node exports about 15.7k series for this metric, growing by 360 per hour, which is about 615k active series in prod Prometheus. Because every series has the value 1, no query can tell which one is the current boundary, so the metric cannot serve its purpose either.The boundary snapshot is now an
Int64ObservableGauge.registerBoundarySnapshotregisters a callback,observeBoundarySnapshot, that reports only the current boundary at each collection, orcompleteonce the version reachestargetVersion. The callback is unregistered when the router context is cancelled, so a rebuilt or closed router, including a read-only handle, stops reporting. This removes the 10-second snapshot loop and the interval parameter ofNewMigrationMetrics. The metric name, value, and label stay the same.There is no consensus, state, or wire-format impact; the change is limited to metrics. New tests in
migration_metrics_test.gouse a manual reader to check that only the current boundary is exported, that the label iscompleteat the target version, and that nothing is exported afterClose. Themigration,composite, androotmultipackage tests pass.