Migrate precompile bank and giga/deps metrics to OTel (PLT-912) - #3859
Migrate precompile bank and giga/deps metrics to OTel (PLT-912)#3859amir-deris wants to merge 2 commits into
Conversation
Dual-emit the bank_new_account counter across all versioned bank precompiles and the giga fork's bank/scheduler paths, mirroring the already-migrated sei-cosmos keeper and scheduler instruments. Co-Authored-By: Claude Sonnet 5 <[email protected]>
|
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 #3859 +/- ##
==========================================
- Coverage 61.59% 60.72% -0.87%
==========================================
Files 2367 2277 -90
Lines 199909 189431 -10478
==========================================
- Hits 123137 115035 -8102
+ Misses 65806 64273 -1533
+ Partials 10966 10123 -843
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
PR SummaryLow Risk Overview Bank Giga scheduler: Reviewed by Cursor Bugbot for commit 45212c3. Bugbot is set up for automated code reviews on this repo. Configure here. |
![seidroid[bot]](https://avatars.githubusercontent.com/in/2745135?s=60&v=4){kind=small}
There was a problem hiding this comment.
Mechanically correct, purely-additive OTel dual-emit sweep: the 15-file coverage claim checks out, the v552/v555 exclusion is verified, and the giga/tasks mirror is byte-identical to sei-cosmos. No blockers; findings are about a duplicated instrument definition, a lost panic guard on a consensus-critical path, and deviations from the repo's package-local metrics.go convention.
Findings: 0 blocking | 8 non-blocking | 4 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- No tests added.
utils/metrics/metrics_util_test.goalready exists — a case using an OTelmanualReaderto assertRecordBankNewAccountbumps bothbank_new_accountand the legacynew.accountsink would lock in the dual-emit contract before PLT-353 removes the legacy half. Same for the two newgiga/deps/tasksinstruments. precompiles/bank/legacy/v601,v605, andv610previously calledtelemetry.IncrCounterdirectly and now route through the panic-recoveringSafeTelemetryIncrCounterinsideRecordBankNewAccount. Benign in practice (armon/go-metrics installs a default global sink ininit, so it doesn't panic), but it is a behavior change in version-frozen files, which sits slightly at odds with the PR description's "no behavioral change". Worth a sentence in the description given theapp-hash-breakinglabel.- Inherited from the sei-cosmos mirror, so out of scope here, but flagging for the follow-up:
scheduler_incarnationsrecords a per-round maximum viaAdd()on a monotonic counter, so the exported series is a sum of maxima rather than anything meaningful. A gauge or histogram would carry the intended signal. Same file also usescontext.Background()whereProcessAllhas a realctx.Context()available, dropping exemplar/trace linkage. - The Cursor second-opinion pass produced no output (
cursor-review.mdis empty). Codex ran and reported no material issues, which matches my own read — nothing in the diff is functionally wrong. - 4 suggestion(s)/nit(s) flagged inline on specific lines.
| "go.opentelemetry.io/otel/sdk/resource" | ||
| ) | ||
|
|
||
| var bankNewAccountCounter = mustCounter(otel.Meter("seicosmos_x_bank_keeper").Int64Counter( |
There was a problem hiding this comment.
[suggestion] This is a second, independent definition of an instrument that already exists: sei-cosmos/x/bank/keeper/metrics.go:21 declares bank_new_account on the same meter scope (seicosmos_x_bank_keeper) with the same description, unit, and kind.
Today that's harmless — the OTel SDK caches instruments by (name, description, unit, kind, number), so both resolve to the same aggregator and sum into one stream, which is presumably the intent (matching the shared legacy new.account key). The hazard is drift: if either copy's description or unit is edited later, the SDK stops deduping, logs a duplicate-metric-stream conflict, and the Prometheus exporter emits two families with the same name but different HELP text — which the Prometheus client rejects, taking out the scrape rather than just that series.
At minimum add a cross-reference comment on both declarations noting they must stay byte-identical; better, have one import the other so there's a single definition.
Separately on scope naming: every other OTel instrument in this tree (~30 files) lives in a package-local metrics.go with meter = otel.Meter("<package_path>"). This one is inline in metrics_util.go and carries a scope naming a package that is not a caller — the actual callers are precompiles/bank/* and giga/deps/xbank. Anyone filtering by instrumentation scope will attribute precompile-originated account creations to the bank keeper.
| // RecordBankNewAccount dual-emits the legacy new-account counter and its OTel | ||
| // counterpart (bank_new_account). Call from defer when creating an account. | ||
| func RecordBankNewAccount(ctx context.Context) { | ||
| bankNewAccountCounter.Add(ctx, 1) |
There was a problem hiding this comment.
[suggestion] The OTel Add sits outside the recover, which quietly drops a guard at the call sites that most need it.
12 of the 15 precompile sites this PR touches previously called SafeTelemetryIncrCounter — a wrapper that exists for exactly one reason: to stop a telemetry fault from panicking inside precompile execution. That defer now runs in sendNative during EVM execution, so a panic escaping bankNewAccountCounter.Add propagates into a consensus-critical path.
I don't have a concrete panic path (Add is nil-ctx-safe via trace.SpanFromContext, and a no-op before SetupOtelMetricsProvider runs), so this is defense-in-depth rather than a live bug. But the fix is one line, and it restores the property the original code deliberately had:
func RecordBankNewAccount(ctx context.Context) {
defer func() {
if e := recover(); e != nil {
debug.PrintStack()
}
}()
bankNewAccountCounter.Add(ctx, 1)
// TODO(PLT-353): remove once bank_new_account verified
SafeTelemetryIncrCounter(1, "new", "account")
}Worth deciding as a policy question for the whole PLT-218 sweep, not just this helper: should OTel emits on consensus paths be panic-guarded the way the legacy ones were?
| sdkerrors "github.com/sei-protocol/sei-chain/sei-cosmos/types/errors" | ||
| cosmosbanktypes "github.com/sei-protocol/sei-chain/sei-cosmos/x/bank/types" | ||
| paramtypes "github.com/sei-protocol/sei-chain/sei-cosmos/x/params/types" | ||
| seimetrics "github.com/sei-protocol/sei-chain/utils/metrics" |
There was a problem hiding this comment.
[suggestion] This is the one place the PR breaks its own fork-parity pattern. giga/deps/tasks got a verbatim copy of sei-cosmos/tasks/metrics.go, and the upstream sei-cosmos/x/bank/keeper keeps its own local metrics.go — but here the forked bank keeper reaches up into the chain-level utils/metrics instead.
That pulls x/evm/types, the OTel SDK, and the Prometheus exporter into a cosmos-fork package that otherwise only depends on sei-cosmos/*. It's a layering inversion (a vendored SDK module depending on chain-app code) and narrows the room to move things around later without an import cycle.
A local giga/deps/xbank/keeper/metrics.go mirroring sei-cosmos/x/bank/keeper/metrics.go, with the two-line inline emit at the call sites, would keep this file a clean diff against its upstream counterpart and match how the rest of the PR handles the fork. It also sidesteps the duplicate-instrument concern flagged on metrics_util.go, since the giga copy would be the same kind of near-duplicate the fork already tolerates everywhere else.
| } | ||
|
|
||
| func (s *scheduler) emitMetrics() { | ||
| taskMetrics.retries.Add(context.Background(), int64(s.metrics.retries)) |
There was a problem hiding this comment.
[nit] Verified this matches sei-cosmos/tasks/scheduler.go line for line, including comment placement — good, that's the right call for a fork mirror. Noting only that context.Background() here discards the exemplar/trace context that ProcessAll has available as ctx.Context(); if that's ever fixed upstream, this copy needs the same edit.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — purely additive OTel dual-emit for the bank new_account counter and giga scheduler metrics, no logic or state changes.
What was reviewed: the 15 near-identical sendNative/InputOutputCoins/SendCoins call sites replacing the legacy counter call with RecordBankNewAccount, the new giga/deps/tasks/metrics.go OTel instruments dual-emitted from emitMetrics(), and the shared RecordBankNewAccount helper in utils/metrics. Confirmed the legacy telemetry call is preserved inside the new helper (true dual-emit, no metric regression) and that defer ordering/placement is unchanged relative to the original code.
Extended reasoning...
Overview
This PR mechanically migrates the new-account (bank) and scheduler retries/incarnations (giga OCC scheduler) counters from the legacy armor/go-metrics-based telemetry to OpenTelemetry, dual-emitting both during the migration window. It touches 19 files, but 18 of them are near-identical one-line swaps (telemetry.IncrCounter/SafeTelemetryIncrCounter to metrics.RecordBankNewAccount(ctx.Context())) across the main and 14 versioned legacy bank precompiles plus the giga bank keeper. The only new logic is the shared RecordBankNewAccount helper and a new giga/deps/tasks/metrics.go OTel instrument file, both of which are simple counter wrappers following an existing pattern already used elsewhere in utils/metrics/metrics_util.go.
Security risks
None. This is observability-only instrumentation — no new inputs are parsed, no auth/permission logic is touched, and no state (KV store) reads/writes are added or removed. The defer call sites and their position relative to SetAccount/state mutation are unchanged from the original code, so gas metering and execution order are preserved.
Level of scrutiny
Low-to-medium. The diff is mechanically repetitive and self-evidently correct on inspection (dual-emit preserves the legacy call), but the PR touches EVM precompile execution paths and is tagged app-hash-breaking, which generally warrants care. In this case, the tag appears to be a conservative auto-label from touching precompile/keeper paths rather than a real consensus-affecting change, since no state-transition logic was modified — confirmed by reading each modified sendNative/SendCoins/InputOutputCoins site line-by-line.
Other factors
No bugs were surfaced by the bug-hunting system, and the two candidate issues raised (metric name collision between giga and sei-cosmos schedulers, and missing panic recovery around the new OTel counter Add call) were investigated and refuted — the latter is also consistent with the same no-recovery pattern already present for other OTel counters in this codebase (e.g. taskMetrics.retries.Add in scheduler.go), so it isn't a new risk introduced by this change. The PR description accurately states test coverage for the touched packages, and the pattern mirrors prior merged OTel-migration commits already on main.
Add panic recovery to RecordBankNewAccount, cross-reference comments on the three bank_new_account instrument declarations, and mirror the Giga xbank keeper fork with a local metrics.go instead of importing utils/metrics. Co-authored-by: Cursor <[email protected]>
There was a problem hiding this comment.
A clean, mechanical dual-emit sweep: 15 bank precompile files route through a new metrics.RecordBankNewAccount, and the Giga fork picks up the scheduler retries/incarnations and bank_new_account OTel instruments as byte-identical mirrors of their sei-cosmos counterparts. I verified coverage (all 16 legacy dirs accounted for; v552/v555 correctly excluded since they delegate to SendCoinsAndWei) and that giga/deps/tasks/metrics.go is identical to sei-cosmos/tasks/metrics.go. No blockers; a few consistency/robustness notes below.
Findings: 0 blocking | 6 non-blocking | 2 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- The three
bank_new_accountdeclarations (sei-cosmos/x/bank/keeper/metrics.go,utils/metrics/metrics_util.go,giga/deps/xbank/keeper/metrics.go) are kept in sync only by prose comments. Since drift silently splits the series rather than failing loudly, consider a small test that asserts the description/unit strings are equal across the three, or exporting a sharedconstfor them. giga/deps/xbank/keeper/send.goemits the OTel counter (and the baretelemetry.IncrCounter) inside adeferwith no panic recovery, while the precompile path added one specifically because "a telemetry fault here must not panic into a consensus-critical path." The keeper send path is equally consensus-critical. This is a deliberate byte-mirror ofsei-cosmos/x/bank/keeper/send.go, so fixing it here alone would break that parity — worth addressing in both, or not at all.- No tests were added.
utils/metricsalready hasmetrics_util_test.go; a case assertingRecordBankNewAccountemitsbank_new_accountand swallows a panic from a nil/failing counter would lock in the panic-safety contract the doc comment promises. - The Cursor second-opinion pass produced no output (
cursor-review.mdis empty). Codex reported no material issues, which matches my read. - 2 suggestion(s)/nit(s) flagged inline on specific lines.
| debug.PrintStack() | ||
| } | ||
| }() | ||
| bankNewAccountCounter.Add(ctx, 1) |
There was a problem hiding this comment.
[suggestion] A single recover wraps both emits, with the OTel Add first. If bankNewAccountCounter.Add ever panics, SafeTelemetryIncrCounter on the next line never runs — which breaks the PR's stated invariant that "legacy metric output is preserved on every path," and does so precisely in the failure case where you'd most want the legacy signal.
Cheap fix: emit the legacy counter first (it's already independently panic-safe via SafeTelemetryIncrCounter), so the OTel call can only ever lose its own emit:
func RecordBankNewAccount(ctx context.Context) {
defer func() {
if e := recover(); e != nil {
debug.PrintStack()
}
}()
// TODO(PLT-353): remove once bank_new_account verified
SafeTelemetryIncrCounter(1, "new", "account")
bankNewAccountCounter.Add(ctx, 1)
}| )), | ||
| incarnations: must(meter.Int64Counter( | ||
| "scheduler_incarnations", | ||
| metric.WithDescription("Maximum incarnation seen in OCC scheduler round"), |
There was a problem hiding this comment.
[nit] The description says "Maximum incarnation seen in OCC scheduler round," but the instrument is a monotonic Int64Counter that Adds the per-round max — so the exported series is the running sum of per-round maxima, not a maximum. Something like "Sum of per-round maximum incarnations in the OCC scheduler" would describe what's actually exported.
Noting this is a byte-identical mirror of sei-cosmos/tasks/metrics.go (I diffed them), so it predates this PR and any reword has to land in both files simultaneously to preserve the dedup contract. Fine to defer.
There was a problem hiding this comment.
No bugs found, but this PR has two unaddressed design comments from another automated reviewer (seidroid) that are worth a maintainer's judgment call before merge: the bank_new_account OTel instrument is now independently declared in three packages (relying on OTel's silent name/desc/unit dedup rather than a shared definition), and the Giga fork's xbank/keeper now imports the chain-level utils/metrics package, which is a layering change for that fork. Neither is a correctness bug, but they're design tradeoffs a human should sign off on given the size of this sweep (21 files) and that it touches the bank precompile's transaction path.
Beyond those, I checked the panic-recovery concern raised on RecordBankNewAccount: the defer/recover is registered before the bankNewAccountCounter.Add call, so it does cover a panic from Add (the legacy counter emit would still be skipped in that case, but execution wouldn't panic into the precompile) — this looks correctly handled in the current diff.
Extended reasoning...
This PR mechanically dual-emits OTel counters alongside existing legacy telemetry for the bank precompile new-account counter (15 near-identical files) and the Giga scheduler/bank-keeper fork. No execution or state logic changes; purely additive instrumentation with legacy paths preserved.
Security risk is minimal — this is observability code, not auth/crypto/consensus logic — though the precompile sendNative path does run during EVM transaction execution, so a panic there would be consensus-relevant. I verified the RecordBankNewAccount helper's defer/recover is correctly ordered to cover the OTel Add call.
Given the number of files touched, the fact it's on a transaction-execution path, and that another automated reviewer (seidroid) left two substantive unaddressed suggestions (duplicate instrument definition across three packages relying on implicit OTel dedup, and a layering change pulling chain-app metrics into a cosmos-fork package), I think this warrants a human sign-off on those tradeoffs rather than a shadow approval, even though no correctness bugs were found.
| bankNewAccountCounter.Add(ctx, 1) | ||
| // TODO(PLT-353): remove once bank_new_account verified | ||
| SafeTelemetryIncrCounter(1, "new", "account") |
There was a problem hiding this comment.
RecordBankNewAccount runs the OTel Add before SafeTelemetryIncrCounter under one function-scoped recover, so a panic in the OTel emit drops the legacy counter silently and the PLT-353 parity comparison skews with it. On main the precompiles emitted the legacy counter unconditionally, so ordering legacy first, or giving each emit its own recover, would hold that guarantee.
2 participants
Summary
Part of the sei-chain OTel metrics migration (PLT-218). This is the mechanical parity sweep for the
new accountcounter (telemetry.IncrCounter(1, "new", "account")), which is duplicated across versioned bank precompiles and the Giga fork.bank_new_account(OTel) alongside the existing legacy counter in all 15 precompile bank files (precompiles/bank/bank.go+ 14 versionedlegacy/v*copies), via a new shared helpermetrics.RecordBankNewAccountinutils/metrics. The helper wraps both emits in a panic recover so a telemetry fault cannot escape into precompile execution.retries/incarnationsOTel instruments into the Giga fork (giga/deps/tasks/metrics.go,scheduler.go), which previously only emitted the legacy metric.giga/deps/xbank/keeper/send.gowith inline dual-emit via a localgiga/deps/xbank/keeper/metrics.go(matching the sei-cosmos bank keeper pattern), keeping the fork free of chain-app imports.precompiles/bank/legacy/v552andv555are intentionally left untouched — theirsendNativedelegates tobankKeeper.SendCoinsAndWei, so the new-account metric is already emitted from the keeper (covered by a prior ticket).The
bank_new_accountOTel instrument is declared in three places (sei-cosmos/x/bank/keeper/metrics.go,utils/metrics,giga/deps/xbank/keeper/metrics.go) on the same meter scope so all paths merge into one series. Cross-reference comments note that description/unit must stay byte-identical across all three.No precompile execution/state logic changed — purely additive OTel instrumentation. Legacy metric output is preserved on every path. One minor behavioral nuance:
precompiles/bank/legacy/v601,v605, andv610previously called baretelemetry.IncrCounterand now route the legacy half throughSafeTelemetryIncrCounter(viaRecordBankNewAccount), which adds panic recovery. This is benign in practice and aligns those version-frozen files with the other precompile copies.Test plan
go build/gofmt -l/goimports -lclean on all touched filesgo testpasses forgiga/deps/tasks,giga/deps/xbank/keeper,utils/metrics,precompiles/bank(+ legacy subpackages)v552/v555don't need the change (keeper already emits the metric on their send path)