Add hashLogger for per module hash and metrics for per module stats#3778
Add hashLogger for per module hash and metrics for per module stats#3778yzang2019 wants to merge 9 commits into
Conversation
PR SummaryLow Risk Overview Observability: new gauges Tests cover per-module categories/hashes and homomorphic invariants; IBC store tests now Reviewed by Cursor Bugbot for commit affaffb. 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).
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3778 +/- ##
==========================================
- Coverage 60.13% 59.36% -0.77%
==========================================
Files 2302 2225 -77
Lines 192117 184172 -7945
==========================================
- Hits 115533 109337 -6196
+ Misses 66260 65090 -1170
+ Partials 10324 9745 -579
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
![seidroid[bot]](https://avatars.githubusercontent.com/in/2745135?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cleanly wires FlatKV's existing per-(DB, module) LtHashes and stats into the hash logger and OTel metrics, mirroring the established memIAVL dynamic-category pattern; correctly handled upstream where categories are recomputed and re-registered each block before hashes are reported. No blocking issues found.
Findings: 0 blocking | 3 non-blocking | 0 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Stale interface docs:
sei-db/state_db/sc/flatkv/api.golines 111-116 still describe HashCategories as "the global root plus one per data DB. The set is fixed" and RecordHashes as reporting "root + per-DB". After this PR the set also includes per-(DB, module) categories and is dynamic (grows as modules first write). The "set is fixed" claim is now inaccurate and worth updating for callers, even though it has no functional impact (upstream already treats the flatKV set as dynamic). Not on a changed line, so noted here. - Minor/optional:
flatkv_module_key_count/flatkv_module_total_bytesgauges are keyed by (db, module) and only everRecorded for modules present in the current stats map; if a module ever stopped appearing its gauge would retain its last value rather than reset. In practice modules don't disappear, so this is speculative — flagging only for awareness. - Second-opinion passes produced no findings: Codex reported "No material issues found in the reviewed diff" and the Cursor review file was empty.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
No new bugs from this pass, but I'd still like a human to look at this one rather than auto-approve: it touches the core commitBatches/FinalizeImport write and import paths in FlatKV, and one of Cursor's findings (per-module gauges recorded during read-only WAL catchup) looks technically plausible to me even though this run's verifiers ruled it out — worth a second opinion before merging.
Extended reasoning...
Beyond the inline findings already on the thread, I re-examined the ruled-out "recordModuleStats pollutes gauges during read-only WAL catchup" candidate myself. recordModuleStats is called unconditionally inside commitBatches (sei-db/state_db/sc/flatkv/store_write.go), which is also invoked by catchup() (store_catchup.go:204) with no readOnly guard. catchup() itself documents (store_catchup.go:100-107) that CurrentVersion is deliberately NOT recorded there specifically because openReadOnly invokes catchup during read-only historical loads and recording would pollute the live gauge. LoadVersion(version, true) is not just a tooling path either — composite/store.go:1282 calls it from the live snapshot-export flow. So a read-only historical replay (e.g. during export) does appear to write into the same global flatkv_module_key_count/flatkv_module_total_bytes OTel gauges as the live writer, which is the same class of pollution the code explicitly avoids for `CurrentVersion" just a few lines away. This is metrics-only (self-corrects on the next live commit, no effect on state/hashes/consensus), so I'd call it low severity rather than a blocker, but I'm not confident enough in the prior "not a real bug" verdict to wave this through without a second, human look — especially since it directly parallels a documented design decision in the same file that this new code didn't follow. The rest of the PR (hash logger per-module categories/hashes, new gauges) is small, well-tested (homomorphic-sum assertions in hashlog_test.go), and low risk on its own.
There was a problem hiding this comment.
A focused follow-up that surfaces the already-tracked per-(DB, module) LtHashes through the hash logger and adds per-module key-count/byte gauges. The change is correct, well-documented, and mirrors the existing memIAVL dynamic-category pattern; only minor non-blocking observability notes.
Findings: 0 blocking | 4 non-blocking | 0 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Cursor's second-opinion pass (cursor-review.md) produced no output; Codex reported no material issues. No independent second opinion was available from Cursor.
- The dynamic per-module category set is kept in sync correctly: rootmulti.recordBlockHashes calls syncHashCategories() (which recomputes HashCategories()) before RecordHashes(), and both read the same post-Commit localMeta snapshot, so a newly-appearing module is registered before its hash is reported. Worth a brief test at the rootmulti/composite layer to lock in that a module first-appearing in a block gets its category registered before RecordHashes runs (the flatkv-level test only covers the store in isolation).
- Metrics gauge staleness: recordModuleStats only records modules currently present in ModuleStats and is only called for DBs in the
pendingset. If a module were ever fully removed from a DB's ModuleStats map, its flatkv_module_key_count/flatkv_module_total_bytes gauges would retain their last-reported value rather than dropping to zero. Likely a non-issue in practice (modules aren't removed), but worth noting. - Metric cardinality: the
moduleattribute on flatkv_module_key_count/flatkv_module_total_bytes multiplies the existingdbcardinality by the module count. Bounded in practice, but keep an eye on the total series count if the module set grows.
There was a problem hiding this comment.
Clean, well-tested follow-up that surfaces per-(DB, module) LtHashes through the hash logger and adds two per-module stat gauges. No correctness or security issues; only minor observability and doc/description-drift notes.
Findings: 0 blocking | 5 non-blocking | 2 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Observability gap (raised by Codex): the module stat gauges are recorded only from Commit's success path, so after a state-sync
FinalizeImport(or a rollback)flatkv_module_key_count/flatkv_module_total_bytesstay empty/stale until the next block commits. The comment correctly justifies excluding WAL replay/catchup (avoids publishing historical values), butFinalizeImportholds the live, final working stats — recording once at the end of import would be correct and would close the gap. Consider arecordAllModuleStats()call afterFinalizeImportsucceeds. - PR description drift: the description states
recordModuleStatsis called in bothcommitBatches(live commits) andFinalizeImport(state-sync import). The actual code calls it only fromCommitviarecordAllModuleStats, and the in-code comment explicitly excludesFinalizeImport. Update the description to match the implementation. - The Cursor second-opinion pass (
cursor-review.md) produced no output (empty file). Codex's pass produced one finding (the gauge-recording gap noted above). - 2 suggestion(s)/nit(s) flagged inline on specific lines.
| } | ||
|
|
||
| // recordModuleStats records the current per-module key-count / byte totals for one data DB, right after | ||
| // they are updated (post-Commit or post-import). stats is keyed by module name, mirroring |
There was a problem hiding this comment.
[nit] This comment says stats are recorded "post-Commit or post-import", but recordModuleStats is only ever invoked from recordAllModuleStats on the Commit success path — the recordAllModuleStats comment in store_write.go explicitly states the import path is excluded. Drop the "or post-import" clause (or add an actual post-import call) so the two comments don't contradict each other.
|
|
||
| // recordAllModuleStats records the current per-module key-count / byte totals for every data DB. Called | ||
| // only from Commit's success path — i.e. only for an actual new block being committed, never for open | ||
| // (LoadVersion/catchup), import (FinalizeImport), rollback, or a read-only historical replay — so the |
There was a problem hiding this comment.
[suggestion] The exclusion of catchup/WAL-replay here is well reasoned (replaying historical entries would clobber the live gauges). But FinalizeImport is different: it commits the live, final working stats, not a replay, so the gauges will simply stay empty until the first post-import block commits. Consider recording module stats at the end of FinalizeImport to avoid that observability gap (this is the point Codex raised).
There was a problem hiding this comment.
LGTM — this is a solid, low-risk observability follow-up. The one nit found this run (module gauges not republished on LoadVersion/Rollback) is cosmetic and self-heals at the next commit; it doesn't need to block merge.
Extended reasoning...
Overview
This PR wires already-tracked per-(data DB, module) LtHashes and key/byte stats through to the hash logger (hashlog.go) and OTel metrics (metrics.go, store_write.go). It adds two new gauges (flatkv_module_key_count, flatkv_module_total_bytes) and dynamic per-module hash-logger categories. No changes to commit/consensus logic, WAL format, or on-disk data — purely additive observability surface.
Security risks
None. No new external inputs, no auth/crypto paths touched. The only prior functional concern (a cursor[bot]/my own earlier comment about commitBatches recording module-stat gauges during read-only catchup replay, polluting the live process-global gauges) has since been fixed: recordAllModuleStats() is now only invoked from Commit()'s success path, with an explicit comment explaining why it's deliberately excluded from commitBatches/catchup.
Level of scrutiny
Appropriate scrutiny here is light — this is metrics/observability code with no bearing on state correctness or app-hash (confirmed by the non-app-hash-breaking label and the homomorphic-invariant test assertions). The one outstanding finding from this run is explicitly a nit (gauges not republished on LoadVersion/Rollback, self-healing at the next live Commit), consistent with the low-risk nature of the change.
Other factors
My earlier inline comment about the stale api.go interface doc has been marked resolved and the diff confirms it now matches hashlog.go's updated doc. Test coverage (hashlog_test.go) was updated to assert the new per-module categories and the homomorphic sum invariant for module hashes, which gives reasonable confidence in the hash-logging half of the change.
| // recordAllModuleStats records the current per-module key-count / byte totals for every data DB. Called | ||
| // only from Commit's success path — i.e. only for an actual new block being committed, never for open | ||
| // (LoadVersion/catchup), import (FinalizeImport), rollback, or a read-only historical replay — so the | ||
| // process-global gauges always reflect the live store, not a replay. | ||
| func (s *CommitStore) recordAllModuleStats() { | ||
| for _, dir := range dataDBDirs { | ||
| recordModuleStats(s.ctx, dir, s.perDBModuleWorkingStats[dir]) | ||
| } | ||
| } |
This comment was marked as low quality.
This comment was marked as low quality.
Sorry, something went wrong.
There was a problem hiding this comment.
A well-contained follow-up that surfaces the already-tracked per-(DB, module) FlatKV lattice hashes through the hash logger and adds two per-module stats gauges. The logic is correct — the dynamic category set is reconciled upstream before RecordHashes, deletion-to-zero modules are retained and re-recorded, and gauge recording is restricted to Commit's success path — with the only gap being test coverage for the new metrics.
Findings: 0 blocking | 3 non-blocking | 0 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Missing tests for the new
flatkv_module_key_count/flatkv_module_total_bytesgauges (raised by Codex): the added tests only cover per-module hash reporting, not emitted gauge values/attributes. Notably untested is the transition when a module's last key is deleted — the stats entry is retained as zero (per verify.go's residue rules) and should be re-recorded as 0, but nothing asserts this actually happens, so a future regression that prunes zeroed entries (leaving a stale non-zero gauge) would go undetected. - The Cursor second-opinion review file (
cursor-review.md) was empty, so no Cursor findings were incorporated. - No prompt-injection or suspicious instructions were found in the PR diff, title, or description.
There was a problem hiding this comment.
LGTM — the earlier findings from this PR's review history (stale api.go interface docs, and recordModuleStats firing during WAL-catchup/read-only replay) are both fixed in the current diff. The one remaining nit — module gauges not re-published on LoadVersion/Rollback — is observability-only and the author has confirmed it's an acceptable tradeoff.
What was reviewed: the dynamic per-(DB, module) hash category generation and RecordHashes reporting in hashlog.go, the new flatkv_module_key_count/flatkv_module_total_bytes gauges and their recording lifecycle in metrics.go/store_write.go, and the homomorphic-sum test assertions in hashlog_test.go. Confirmed recordAllModuleStats is scoped to Commit()'s success path only, avoiding gauge pollution from catchup/read-only replay.
Extended reasoning...
Overview
This PR wires per-(data DB, module) LtHash checksums and key-count/byte-size stats through the flatKV hash logger and OTel metrics. It touches api.go (interface docs), hashlog.go (dynamic category generation + reporting), metrics.go (two new gauges), store_write.go (wiring recordAllModuleStats into Commit's success path), and adds test coverage in hashlog_test.go.
Security risks
None. This is purely observability/instrumentation — no changes to consensus-critical commit logic, LtHash computation, or the actual data written/read. Confirmed by Cursor's own summary ("no change to commit or LtHash computation paths beyond when gauges are emitted") and by inspection: RecordHashes/HashCategories only read already-committed LocalMeta state, they don't mutate it.
Level of scrutiny
Medium — this is non-consensus, non-app-hash-breaking observability code (explicitly labeled as such), but it required a few rounds of careful review of gauge lifecycle semantics (catchup/read-only replay pollution, restart/rollback staleness) since flatKV's OTel gauges are process-global and shared across write-mode and read-only store instances. Those rounds already happened across this PR's history, and the two substantive ones were fixed.
Other factors
The PR has solid test coverage, including a homomorphic invariant check (per-module LtHashes sum to the per-DB root) which is a good correctness signal for the core hash-mixing logic. The one open nit (module gauges going stale across restart/rollback until the next commit) is explicitly accepted by the PR author as a non-blocking tradeoff, and is self-healing.
There was a problem hiding this comment.
Well-scoped follow-up that surfaces per-(DB, module) LtHashes through the hash logger and adds per-module key-count/byte gauges. The dynamic category set is correctly registered upstream (syncHashCategories runs before RecordHashes each block) and metrics are recorded only from Commit's live success path, with clear rationale. No blocking issues; only minor observations.
Findings: 0 blocking | 4 non-blocking | 1 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- The Cursor second-opinion review file (cursor-review.md) is empty — that pass produced no output. Codex reported no material findings.
- Observability nit: recordModuleStats/recordAllModuleStats only ever writes gauges for modules currently present in perDBModuleWorkingStats. If a module ever drops out of that map (e.g. all its keys deleted and the entry removed), its flatkv_module_key_count / flatkv_module_total_bytes gauge series would retain its last-recorded value rather than going to zero. Worth confirming modules are retained (with 0) rather than removed, or documenting the behavior.
- Both new gauges create one time series per (db, module) attribute pair. Module count is bounded so cardinality is fine, but worth being aware of if module names ever become unbounded/user-controlled.
- 1 suggestion(s)/nit(s) flagged inline on specific lines.
| // new modules first write into a DB — so callers must recompute this every block to detect changes | ||
| // (handled upstream by reopening/rotating the logger when the set changes). | ||
| func (s *CommitStore) HashCategories() []string { | ||
| categories := make([]string, 0, len(dataDBDirs)+1) |
There was a problem hiding this comment.
[nit] Nit: the capacity hint len(dataDBDirs)+1 no longer accounts for the per-module categories appended in the loop, so this slice will reallocate as soon as any module is present. Harmless, but consider dropping the hint or sizing it with the module count if you want to keep the pre-allocation meaningful.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit affaffb. Configure here.
| for _, dir := range dataDBDirs { | ||
| recordModuleStats(s.ctx, dir, s.perDBModuleWorkingStats[dir]) | ||
| } | ||
| } |
There was a problem hiding this comment.
Module gauges skipped on open
Medium Severity
recordAllModuleStats only runs on Commit, so flatkv_module_key_count and flatkv_module_total_bytes stay unset after a live LoadVersion or successful import until the next block. CurrentVersion is already recorded on those same success paths, and by then perDBModuleWorkingStats already holds the live store’s post-catchup totals.
Reviewed by Cursor Bugbot for commit affaffb. Configure here.
There was a problem hiding this comment.
Correct, well-documented follow-up that surfaces per-(DB, module) LtHashes through the hash logger and adds two per-module stats gauges, plus test-store cleanup in two IBC suites. No correctness or security issues found; the only gap is missing test coverage for the new metrics.
Findings: 0 blocking | 3 non-blocking | 0 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Test coverage: the new
flatkv_module_key_count/flatkv_module_total_bytesgauges have no test verifying recorded values or thedb/moduleattributes (Codex P2).hashlog_test.gocovers the hash categories/checksums well, but the metrics path is unvalidated. Consider an in-memory OpenTelemetry reader test exercisingrecordModuleStats— including a module whose KeyCount returns to zero (which correctly records 0, since entries persist in the stats map rather than being deleted) to lock in that behavior. - Cursor's second-opinion review (
cursor-review.md) was empty — that pass produced no output. - Minor:
HashCategories()pre-sizes its slice tolen(dataDBDirs)+1but now also appends the dynamic per-module categories;appendhandles the growth so this is only a missed capacity hint, not a bug.
3 participants
Describe your changes and provide context
Follow-up to the per-module LtHash refactor: FlatKV now tracks lattice hashes and stats at
(data DB, module)granularity internally, but the hash logger and metrics only surfaced theper-DB and root hashes. This PR wires the per-module data through both.
Hash logger (
hashlog.go)HashCategories()now also emits one category per(DB, module)pair currently tracked inthat DB's
LocalMeta.ModuleLtHashes, namedflatKV/db/<dir>/mod/<module>(mirrors memIAVL's<backend>/mod/<module>convention). This set is dynamic and grows as new modules first writeinto a DB. Existing categories (
flatKV/root,flatKV/db/<dir>) are unchanged.RecordHashes()reports each module's committed LtHash checksum alongside the existing rootand per-DB checksums.
reportModuleHashesandchecksumBytesOrNilhelpers to keep the loop body flat andavoid repeated nil checks.
Metrics (
metrics.go,store_write.go)flatkv_module_key_countandflatkv_module_total_bytes, both tagged withdbandmoduleattributes.recordModuleStats, called right afterLocalMeta.ModuleStatsis updated incommitBatches(live commits)Testing performed to validate your change
go test ./sei-db/state_db/sc/flatkv/...(includinghashlog_test.go, updated for the newper-module categories/hashes)
go test ./sei-db/state_db/sc/composite/...and.../rootmulti/hashlog/...gofmt -s -l ./goimports -l .clean on all touched filesmake lint