Migrate precompiles/bank and giga/deps metrics to OTel (PLT-912) - #3859
Migrate precompiles/bank and giga/deps metrics to OTel (PLT-912)#3859amir-deris wants to merge 10 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 <noreply@anthropic.com>
|
The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3859 +/- ##
==========================================
- Coverage 61.74% 60.71% -1.03%
==========================================
Files 2381 2275 -106
Lines 201667 189311 -12356
==========================================
- Hits 124513 114948 -9565
+ Misses 66074 64253 -1821
+ Partials 11080 10110 -970
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 OCC scheduler: Giga Legacy Reviewed by Cursor Bugbot for commit f72d14b. 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.
![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 <cursoragent@cursor.com>
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)
}
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.
There was a problem hiding this comment.
The OTel dual-emit changes themselves are clean and correctly mirrored, but the PR delivers only 1 of the 15 precompile bank files its description claims to migrate — 14 legacy/v* copies are untouched, so the stated parity sweep is not done and parts of the description are factually wrong. A few smaller consistency/robustness issues are noted inline.
Findings: 1 blocking | 8 non-blocking | 3 posted inline
Blockers
- Parity sweep is not delivered (confirmed, also flagged by Codex). The description says
bank_new_accountis dual-emitted in "all 15 precompile bank files (precompiles/bank/bank.go+ 14 versionedlegacy/v*copies)", but the diff touches onlyprecompiles/bank/bank.go. All 14 legacy versions still emit the legacy counter only:v562,v580,v600,v603,v606,v610(bare),v614,v620,v630,v640,v65,v66, plusv601/v605(baretelemetry.IncrCounter). The description's "minor behavioral nuance" paragraph — claimingv601,v605,v610now route throughSafeTelemetryIncrCounter— describes a change that is not in this diff. Either the edits were dropped (a 15-file sweep would not fit in +124/-8) or the description needs correcting; as-is, replay/tracing through legacy precompile versions produces nobank_new_accountseries.
Non-blocking
- The Cursor second-opinion pass produced no output (
cursor-review.mdis empty) — that review lane effectively did not run for this PR. - No tests added.
RecordBankNewAccountis a new exported helper on a consensus-adjacent path with non-trivial semantics (recover + dual emit); a smallutils/metricstest asserting both emits fire, and that a panicking OTel provider does not escape, would lock in the contract the description leans on. - Three near-identical
must/mustCountergeneric helpers now exist (sei-cosmos/x/bank/keeper,giga/deps/xbank/keeper,giga/deps/tasks, plusutils/metrics). Fork isolation is a legitimate reason to duplicate, bututils/metrics.mustCounteris a non-generic one-off that could just use the samemust[V any]shape for consistency. - The
sei-cosmos/tasks/scheduler.goemitMetricssignature change (plumbingctx.Context()instead ofcontext.Background()) and thescheduler_incarnationsdescription rewording are outside the stated scope ("precompile bank and giga/deps"). Both look like sensible improvements — worth calling out in the description so reviewers of sei-cosmos know an existing instrument's description changed. - The PR carries the
app-hash-breakinglabel, but the change is described (and appears) to be purely additive instrumentation with no state/execution effect. Worth confirming the label is intentional — a spurious app-hash-breaking label affects release/upgrade handling. - 3 suggestion(s)/nit(s) flagged inline on specific lines.
| debug.PrintStack() | ||
| } | ||
| }() | ||
| bankNewAccountCounter.Add(ctx, 1) |
There was a problem hiding this comment.
[suggestion] Ordering under the shared recover: if bankNewAccountCounter.Add panics, the outer recover swallows it and SafeTelemetryIncrCounter never runs — so an OTel fault silently drops the legacy counter too, which is a regression against the pre-PR behavior at every call site.
Since the whole point of this helper is that legacy output is preserved on every path, either emit the legacy counter first, or give the OTel Add its own recover:
func RecordBankNewAccount(ctx context.Context) {
// TODO(PLT-353): remove once bank_new_account verified
defer SafeTelemetryIncrCounter(1, "new", "account")
defer func() {
if e := recover(); e != nil {
debug.PrintStack()
}
}()
bankNewAccountCounter.Add(ctx, 1)
}| } | ||
|
|
||
| func (s *scheduler) emitMetrics() { | ||
| taskMetrics.retries.Add(context.Background(), int64(s.metrics.retries)) |
There was a problem hiding this comment.
[suggestion] This PR updates the sei-cosmos twin to plumb the real context (emitMetrics(ctx.Context()), sei-cosmos/tasks/scheduler.go:268/288) but the Giga fork copy keeps context.Background(). Since this file is meant to mirror upstream, it starts out already drifted — and it loses the exemplar/trace correlation the upstream change was presumably for. Suggest applying the same emitMetrics(ctx context.Context) signature here and calling defer s.emitMetrics(ctx.Context()) in ProcessAll.
| if !accExists { | ||
| defer telemetry.IncrCounter(1, "new", "account") | ||
| defer func() { | ||
| bankMetrics.newAccount.Add(ctx.Context(), 1) |
There was a problem hiding this comment.
[nit] The precompile helper wraps its OTel Add in a recover on the stated grounds that "a telemetry fault here must not panic into a consensus-critical path" (utils/metrics/metrics_util.go:77). This keeper send path is equally consensus-critical, but Add is called bare here and at line 174. It matches the sei-cosmos pattern, so this may be a deliberate consistency choice — but if the panic-safety rationale holds for precompiles it holds here, and the two paths should agree either way.
There was a problem hiding this comment.
This PR hasn't addressed bdchatham's most recent comment (2026-08-05T17:33:43Z): RecordBankNewAccount still runs the OTel Add before SafeTelemetryIncrCounter under a single recover, so a panic in the OTel emit would silently drop the legacy counter it's meant to be validated against during the PLT-353 comparison window. That's the last unresolved comment in the thread, so I'll leave this for a human to confirm before merging.
Extended reasoning...
Overview
This PR adds OTel dual-emit counters for the bank new_account metric across 15 precompile files and the Giga fork's xbank keeper, plus mirrors OCC scheduler retries/incarnations counters into the Giga fork. It's purely additive telemetry — no precompile execution or state-transition logic changes, and legacy metrics are preserved.
Security risks
None from an attack-surface standpoint — this is observability code, not auth/consensus logic. The only 'consensus-relevant' risk is a panic in the new OTel code path escaping into precompile execution (touching sendNative, a transaction-path method), which the PR's own RecordBankNewAccount helper defends against via recover().
Level of scrutiny
Given the change is purely additive telemetry with an existing panic guard, this doesn't need production-critical-code-level scrutiny for correctness of business logic. However, the ordering of operations inside a shared recover block is a real, still-open design question raised by a human reviewer (bdchatham) in the most recent PR comment, which is why I'm not approving outright — that class of comment (unaddressed reviewer feedback) is a signal a human should follow up on before merge, even though the underlying risk is low (defense-in-depth, not a confirmed live bug).
Other factors
Earlier bot review rounds (seidroid) raised layering-inversion and duplicate-instrument-definition concerns for the Giga xbank keeper; those appear to have already been addressed in the current diff (the Giga keeper now has its own local metrics.go mirroring the sei-cosmos pattern rather than importing utils/metrics). The bug-hunting system found no bugs this run, and the candidate issues it ruled out (giga scheduler context.Background() fork-parity, legacy v552/v555 not migrated) both fall under expected, intentional PR scope per the description. Test coverage on the new code paths is partial per Codecov, which is typical for defer/panic-recovery branches.
amir-deris
changed the title
There was a problem hiding this comment.
Purely additive OTel dual-emit instrumentation for bank_new_account and the OCC scheduler counters; no state, gas, or precompile logic changes, and legacy counters remain on every path. No blockers — the notes are about emit ordering/panic-safety consistency, the prose-only contract keeping three duplicated instrument declarations byte-identical, and missing test coverage.
Findings: 0 blocking | 10 non-blocking | 4 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Cursor's second-opinion pass produced no output (
cursor-review.mdis empty). Codex reported "No material issues found in the PR diff." - The
bank_new_accountinstrument is now declared in three places (sei-cosmos/x/bank/keeper/metrics.go,giga/deps/xbank/keeper/metrics.go,utils/metrics/metrics_util.go) andscheduler_retries/scheduler_incarnationsin two, with byte-identical description/unit enforced only by comments. Drift silently splits the series (OTel logs a duplicate-instrument conflict and keeps both streams) — consider exported description/unit constants plus a small test asserting they match across declarations, which would also have caught thescheduler_incarnationswording fix needing to land in both copies. - No new tests.
giga/deps/taskscontains no_test.gofiles at all, so the test-plan line "go testpasses forgiga/deps/tasks" is vacuous for the newmetrics.go/emitMetricschange.utils/metricsalready hasmetrics_util_test.go; a manual-reader test assertingRecordBankNewAccountbumps both the OTel and legacy counters would be cheap and would pin the dual-emit contract. - The PR carries the
app-hash-breakinglabel, but the diff is metrics-only with no state/gas/logic change that I can see — worth confirming the label is intentional (likely inherited from a base merge) so it doesn't misdirect release gating. - Informational:
emitMetricsnow receives the request context and runs from adefer, so it may fire with an already-cancelled context. The current OTel SDK's synchronous counters ignore cancellation, so nothing drops today; noting only because the previous code usedcontext.Background()and was immune by construction. - I could not run
go buildin this environment (command not permitted), so compilation is unverified here. I did confirm statically thatmust/meterare declared only once per package ingiga/deps/tasksandgiga/deps/xbank/keeper, and thatcontextis already imported ingiga/deps/tasks/scheduler.go. - 4 suggestion(s)/nit(s) flagged inline on specific lines.
| debug.PrintStack() | ||
| } | ||
| }() | ||
| bankNewAccountCounter.Add(ctx, 1) |
There was a problem hiding this comment.
[suggestion] The new OTel emit runs first, and both emits share a single recover(). If bankNewAccountCounter.Add ever panics, the recover swallows it and SafeTelemetryIncrCounter is never reached — so the legacy new.account counter silently stops incrementing, which contradicts the PR's "legacy metric output is preserved on every path" guarantee. Since the whole point of the recover is that you don't fully trust the new instrument, emit the legacy counter first (or give each emit its own recover) so a fault in the new path can't starve the one we still depend on.
| if !accExists { | ||
| defer telemetry.IncrCounter(1, "new", "account") | ||
| defer func() { | ||
| bankMetrics.newAccount.Add(ctx.Context(), 1) |
There was a problem hiding this comment.
[suggestion] Unlike the precompile path — where RecordBankNewAccount is explicitly wrapped in a recover because "a telemetry fault here must not panic into a consensus-critical path" — this Add is unguarded and runs from a defer inside InputOutputCoins/SendCoins, which is equally consensus-critical (a panic in a defer propagates). This does match the existing sei-cosmos/x/bank/keeper/send.go precedent, so it's not a blocker, but the two paths now reason about the same risk differently. At minimum, put telemetry.IncrCounter before the OTel Add here and in SendCoins (line 174) so the legacy counter can't be lost to a fault in the new instrument.
|
|
||
| func (s *scheduler) emitMetrics() { | ||
| func (s *scheduler) emitMetrics(ctx context.Context) { | ||
| taskMetrics.retries.Add(ctx, int64(s.metrics.retries)) |
There was a problem hiding this comment.
[nit] Same ordering point as the bank paths: this runs from defer s.emitMetrics(...) in ProcessAll, and the OTel Adds precede their legacy counterparts unguarded. Emitting telemetry.IncrCounter first keeps the legacy series intact regardless of what the new instruments do.
| ) | ||
|
|
||
| var ( | ||
| meter = otel.Meter("seicosmos_tasks") |
There was a problem hiding this comment.
[nit] This file is a byte-identical copy of sei-cosmos/tasks/metrics.go (same blob), but it's the only one of the three mirrored metrics files without the "same name/scope so the series merge; keep description/unit byte-identical or the OTel SDK stops deduping" comment that giga/deps/xbank/keeper/metrics.go and sei-cosmos/x/bank/keeper/metrics.go both carry. It's also the copy holding the corrected scheduler_incarnations description that must stay in lockstep with sei-cosmos — the place drift is most likely. Please add the same comment here.
Superseded: latest AI review found no blocking issues.
There was a problem hiding this comment.
Purely additive OTel dual-emit instrumentation across the bank precompile, the Giga fork's bank keeper and OCC scheduler, plus a context.Context thread-through in emitMetrics. No correctness or security problems found; the notes are about the fragility of three hand-synced instrument declarations, missing test coverage, and inconsistent panic hardening.
Findings: 0 blocking | 8 non-blocking | 3 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 issues. - No tests were added. The test plan only claims existing tests still pass, but
RecordBankNewAccount, the Gigabank_new_accountcounter, and the Giga scheduler counters are all new and uncovered. A test using an OTelmanual.Readerthat emits from all three paths and asserts exactly onebank_new_accountstream is collected would both cover the new code and mechanically enforce the byte-identical-description invariant the comments describe. - All
bank_new_accountevents merge into one series with no attribute distinguishing precompile-originated from keeper-originated creations, so dashboards can't break the number down by source. That's presumably the intent, but adding asourceattribute is cheap now and a breaking dashboard change later — worth a conscious decision before this lands. - The PR carries the
app-hash-breakinglabel while the description states no execution/state logic changed (and the diff supports that — metrics emission consumes no gas and touches no store). Worth confirming the label is intentional, since it gates the release process. - Five new
TODO(PLT-353)markers are added across four files. Make sure PLT-353 enumerates every legacytelemetry.IncrCountercall site so none is missed at cleanup time. - 3 suggestion(s)/nit(s) flagged inline on specific lines.
| // so precompile-originated and keeper-originated new-account events merge | ||
| // into a single bank_new_account series. Keep description/unit byte-identical | ||
| // across all three declarations or the OTel SDK stops deduping the instrument. | ||
| var bankNewAccountCounter = mustCounter(otel.Meter("seicosmos_x_bank_keeper").Int64Counter( |
There was a problem hiding this comment.
[suggestion] The bank_new_account instrument identity (name, scope, description, unit) is now declared independently in three packages — here, sei-cosmos/x/bank/keeper/metrics.go, and giga/deps/xbank/keeper/metrics.go — and the only thing keeping them in sync is a comment. If any one drifts, the OTel SDK silently emits two conflicting streams; nothing fails at compile time and no test catches it.
Understood that giga/deps is deliberately import-free of chain-app code so a shared constant isn't available everywhere. But at minimum, a test that registers a manual.Reader meter provider, drives all three paths, and asserts exactly one bank_new_account metric is collected would turn a silent regression into a failing build. Same argument applies to scheduler_retries/scheduler_incarnations, now duplicated between sei-cosmos/tasks and giga/deps/tasks.
| defer func() { | ||
| // TODO(PLT-353): remove once bank_new_account verified | ||
| telemetry.IncrCounter(1, "new", "account") | ||
| bankMetrics.newAccount.Add(ctx.Context(), 1) |
There was a problem hiding this comment.
[suggestion] This Add (and the one at line 176) is unguarded, while RecordBankNewAccount in utils/metrics deliberately wraps the equivalent call in a recover() with the rationale that "a telemetry fault here must not panic into a consensus-critical path." InputOutputCoins/SendCoins run in DeliverTx and are just as consensus-critical, so the reasoning applies equally here. Non-blocking since this mirrors the pre-existing sei-cosmos/x/bank/keeper/send.go shape, but if the panic-safety concern is real for the precompile it's real here too — consider a shared guarded helper (or a short comment explaining why these sites don't need one).
Minor: the emit order is inverted relative to sei-cosmos/x/bank/keeper/send.go (legacy first here, OTel first there). Since these files are meant to be mirrors, matching the order makes future diffs between them easier to read.
| ) | ||
|
|
||
| // bankMetrics.newAccount mirrors sei-cosmos/x/bank/keeper/metrics.go's | ||
| // instrument of the same name/scope so the two dual-emit paths merge into a |
There was a problem hiding this comment.
[nit] "the two dual-emit paths" — there are three declarations of this instrument (this file, sei-cosmos/x/bank/keeper/metrics.go, and utils/metrics.bankNewAccountCounter); the other two comments correctly say "all three." More importantly, this comment doesn't name utils/metrics, so someone editing the description here would only know to check one of the two other sites. Worth listing all three, given this comment is the only mechanism enforcing the invariant.
There was a problem hiding this comment.
Purely additive OTel dual-emit instrumentation for bank_new_account and the OCC scheduler counters; no execution or state logic changes, and legacy telemetry is preserved on every path. No blockers — the notes are about the hand-maintained "keep byte-identical" invariant now spanning three instrument declarations, inconsistent panic-guarding of the new Add calls, and the absence of any test for the one new helper.
Findings: 0 blocking | 6 non-blocking | 3 posted inline
Blockers
- None at the file/PR level.
Non-blocking
cursor-review.mdis empty — the Cursor pass produced no output, so this review reflects only my own analysis plus Codex's (which reported no material issues).- No tests added.
RecordBankNewAccountis the only new logic in the PR andutils/metricsalready has a test file (metrics_util_test.go) with a working manual-reader pattern. A smoke test asserting it records 1 and returns cleanly with a nilcontext.Context(reachable via a zero-valuesdk.Context, whoseContext()returns nil) would be cheap and would pin the panic-recovery contract the doc comment promises. - The PR carries the
app-hash-breakinglabel while the description states "No precompile execution/state logic changed — purely additive OTel instrumentation." I agree with the description: nothing here consumes gas or touches state. Worth reconciling the label before merge so it isn't misleading in the release notes. - 3 suggestion(s)/nit(s) flagged inline on specific lines.
| defer func() { | ||
| // TODO(PLT-353): remove once bank_new_account verified | ||
| telemetry.IncrCounter(1, "new", "account") | ||
| bankMetrics.newAccount.Add(ctx.Context(), 1) |
There was a problem hiding this comment.
[suggestion] Two things here, both also applying to the identical block at line 176:
-
Emit order is flipped relative to the sei-cosmos counterpart.
sei-cosmos/x/bank/keeper/send.go:149-151does OTelAddfirst, thentelemetry.IncrCounter; this does the reverse. Sincetelemetry.IncrCounterhere is the unguarded variant (notSafeTelemetryIncrCounter), a panic in the legacy sink would skip the OTel emit entirely and silently under-countbank_new_accounton exactly the paths this PR is trying to instrument. Matching the sei-cosmos ordering makes the new counter the one that survives. -
No panic guard on the new
Add.RecordBankNewAccount's doc comment justifies itsrecover()with "a telemetry fault here must not panic into a consensus-critical path" — butInputOutputCoins/SendCoinsrun insideDeliverTx, which is equally consensus-critical, and so doesemitMetricsingiga/deps/tasks/scheduler.go. A panic escaping here is recovered by baseapp into a tx failure, and because it would depend on node-local telemetry configuration it's a (low-probability) nondeterminism vector. I don't think OTel'sAddcan realistically panic, and this matches the pre-existing unguarded sei-cosmos pattern, so it isn't blocking — but the guard should be applied consistently across all four new call sites or dropped from the precompile as unnecessary.
| // instrument of the same name/scope (and giga/deps/xbank/keeper/metrics.go's) | ||
| // so precompile-originated and keeper-originated new-account events merge | ||
| // into a single bank_new_account series. Keep description/unit byte-identical | ||
| // across all three declarations or the OTel SDK stops deduping the instrument. |
There was a problem hiding this comment.
[suggestion] This "keep description/unit byte-identical across all three declarations" invariant is now maintained by hand across three packages, enforced only by comments. This PR is itself evidence the invariant drifts — the scheduler_incarnations description fix in sei-cosmos/tasks/metrics.go was needed for exactly this reason.
Consider a tiny dependency-free leaf package exporting the name/description/unit as consts. It would have no chain-app imports, so all three sites could import it — including giga/deps/xbank/keeper, which the PR description says is deliberately kept import-free of chain-app code, and sei-cosmos/x/bank/keeper, which can't import utils/metrics (cycle). Failing that, a test in the root module asserting the three declared descriptions agree would catch drift at CI time rather than in a metrics dashboard.
| ) | ||
|
|
||
| var ( | ||
| meter = otel.Meter("seicosmos_tasks") |
There was a problem hiding this comment.
[nit] This file is byte-identical to sei-cosmos/tasks/metrics.go and intentionally reuses the seicosmos_tasks meter scope and instrument names so the two forks' series merge — but unlike the three bank_new_account declarations, it carries no comment saying so. The seicosmos_ scope name inside the Giga fork reads like a copy-paste slip without one. Worth mirroring the comment style used in giga/deps/xbank/keeper/metrics.go so a future description tweak on either side doesn't silently break the dedupe.
There was a problem hiding this comment.
This PR has had substantial back-and-forth review (seidroid[bot] across four rounds, plus a human comment from bdchatham), and while the highest-signal concern — the shared recover() ordering in RecordBankNewAccount that could silently drop the legacy counter — has been fixed (legacy SafeTelemetryIncrCounter now runs before the OTel Add), several other raised points still look open in the current diff: the Giga fork's bankMetrics.newAccount.Add calls in giga/deps/xbank/keeper/send.go remain unguarded by a recover despite the precompile helper's stated rationale that telemetry faults on consensus-critical paths need one, and giga/deps/tasks/metrics.go still lacks the cross-reference comment the other two mirrored metrics files carry.
I didn't find any new bugs beyond what's already been discussed, but given the unresolved review threads and the fact this PR touches consensus-critical bank/scheduler code paths (even though the changes are additive-only), I think it's worth a human pass to confirm which of the outstanding suggestions the author intends to address before merging.
Extended reasoning...
Overview
Additive OTel instrumentation across the bank precompile (precompiles/bank/bank.go), the Giga fork's bank keeper (giga/deps/xbank/keeper) and scheduler (giga/deps/tasks), and the sei-cosmos scheduler/bank keeper. Adds a RecordBankNewAccount helper in utils/metrics, threads real context into emitMetrics, and corrects an instrument description. No precompile/state-transition logic changes; legacy telemetry paths are preserved.
Security risks
None directly — this is observability-only instrumentation, not auth/crypto/permission logic. The main risk class is a telemetry fault (panic in an OTel Add call) propagating into a consensus-critical DeliverTx/precompile path, which is exactly what the extensive review thread has focused on.
Level of scrutiny
Medium-high, despite being "just metrics." The code runs on every native bank transfer and every OCC scheduler round, i.e. hot, consensus-critical paths, and the PR explicitly makes a panic-safety claim ("a telemetry fault here must not panic into a consensus-critical path") that needs to actually hold everywhere it's asserted. The bug hunting system found no confirmed bugs this run, and the one candidate that was investigated (OTel panic under a shared recover silently dropping the legacy counter) matches an issue the human/bot reviewers already raised and which the current code appears to have fixed by reordering the two emits.
Other factors
The PR has multiple rounds of detailed review from seidroid[bot] and one comment from a human (bdchatham), several of which (duplicate instrument declarations across three packages, unguarded Add() calls in the Giga fork's keeper, a missing consistency comment in giga/deps/tasks/metrics.go) don't appear resolved in the current diff. None of these looks like a live correctness bug on their own — they're mostly defense-in-depth and consistency asks — but the volume of open threads and the consensus-path context argue for a human owner explicitly deciding which to address versus defer, rather than a bot approval closing out the PR.
There was a problem hiding this comment.
Purely additive OTel dual-emit instrumentation; no state, gas, or execution-path changes, and legacy counters are preserved on every touched path. No blockers — the notable items are the Giga InputOutputCoins emission semantics now diverging from the canonical sei-cosmos keeper on error paths, three hand-copied instrument declarations kept in sync only by comments, and no tests pinning any of it.
Findings: 0 blocking | 11 non-blocking | 3 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Cursor's second-opinion file (
cursor-review.md) is empty — that pass produced no output. Findings below merge only Codex's single P1 with this review. - No tests added.
giga/deps/xbank/keeper/send_test.gohas noInputOutputCoinscoverage at all, so the new count-then-emit path (and the "no emit on error" behavior it introduces) is unverified. A unit test with an in-memory OTel reader asserting onebank_new_accountincrement per created account — and the intended count on an erroring output — would pin the semantics the new comments describe. must[V any]is now duplicated in four packages (sei-cosmos/tasks,giga/deps/tasks,sei-cosmos/x/bank/keeper,giga/deps/xbank/keeper) plus a fifthmustCountervariant inutils/metrics.giga/deps/xbank/keeper/metrics.goalready importssei-cosmos/telemetry, so the "import-free of chain-app code" constraint doesn't prevent sharing one helper.- Both new recover blocks bind
e := recover()but only calldebug.PrintStack(), discarding the panic value — a telemetry fault leaves a stack with no message identifying which instrument failed. Consistent with the existingSafeTelemetryIncrCounter, but loggingewould cost nothing. - Hardening is asymmetric: the bank paths get recover wrappers with the rationale "must not panic into a consensus-critical path," while
emitMetricsin bothsei-cosmos/tasksandgiga/deps/taskscallstaskMetrics.*.Addandtelemetry.IncrCounterunguarded from adeferinProcessAll— equally consensus-critical. Pre-existing for sei-cosmos, but the Giga copy is new code in this PR and could have been guarded. - Threading
ctx.Context()into the deferredemitMetricsinstead ofcontext.Background()is safe with the current SDK —Int64Counter.Addnever checksctx.Err(), and the context only feeds exemplar sampling (trace.SpanContextFromContext, which is nil-safe). Worth stating explicitly since the deferred call may run with an already-cancelled context if exemplars are enabled later. - The frozen
precompiles/bank/legacy/v*copies still calldefer metrics.SafeTelemetryIncrCounter(1, "new", "account")(legacy only, no OTel). Expected for versioned precompiles, but it means legacynew.accountcan exceedbank_new_accountwhen older precompile versions execute under tracing/replay — relevant when comparing the two series during PLT-353 verification. - The PR carries the
app-hash-breakinglabel while the description states it is purely additive instrumentation with no execution/state changes — which the diff supports. Confirm the label is intentional (or path-auto-applied) so release tooling isn't misled. - 3 suggestion(s)/nit(s) flagged inline on specific lines.
| k.ak.SetAccount(ctx, k.ak.NewAccountWithAddress(ctx, outAddress)) | ||
| } | ||
| } | ||
| recordNewAccounts(ctx.Context(), int64(newAccountCount)) |
There was a problem hiding this comment.
[suggestion] This changes when the legacy counter fires, and diverges from the canonical fork. Previously defer telemetry.IncrCounter(1, "new", "account") ran at function exit regardless of outcome; now, if an earlier output creates an account and a later output fails AccAddressFromBech32/AddCoins and returns (lines 125–132), recordNewAccounts is never reached and both counters lose the event.
The upstream keeper this file mirrors kept the deferred shape — sei-cosmos/x/bank/keeper/send.go:148-152 wraps bankMetrics.newAccount.Add + telemetry.IncrCounter in a defer func(){...}() inside the loop. Since the whole point of the three parallel declarations is that they merge into one bank_new_account series, the same logical event now counts differently depending on which fork executed, and legacy new.account shifts on error paths during the PLT-353 dual-emit comparison window.
Not a correctness/state issue (an error return reverts the account creation anyway, so arguably the new behavior is the more accurate one) — but pick one and make it explicit. Either match upstream:
defer func() { recordNewAccounts(ctx.Context(), int64(newAccountCount)) }()or keep the current placement and note in the comment that Giga intentionally excludes rolled-back accounts. (Codex flagged this as P1; I'd scope it to metrics-accuracy rather than blocking.)
| // into a single bank_new_account series. Keep description/unit byte-identical | ||
| // across all three declarations or the OTel SDK stops deduping the instrument. | ||
| var bankNewAccountCounter = mustCounter(otel.Meter("seicosmos_x_bank_keeper").Int64Counter( | ||
| "bank_new_account", |
There was a problem hiding this comment.
[suggestion] The instrument identity ("bank_new_account", description, "{count}") is now hand-copied into three packages, and dedup silently degrades into two conflicting series if any copy drifts — enforced only by prose comments. Consider exporting the three strings as constants from one location that all three import (sei-cosmos/telemetry would work; giga/deps/xbank/keeper already depends on it), or add a test that asserts the literals match so drift fails CI instead of quietly splitting the metric.
| accExists := p.accountKeeper.HasAccount(ctx, receiverSeiAddr) | ||
| if !accExists { | ||
| defer metrics.SafeTelemetryIncrCounter(1, "new", "account") | ||
| defer metrics.RecordBankNewAccount(ctx.Context()) |
There was a problem hiding this comment.
[nit] Worth noting alongside the Giga change: the defer here means the counter still fires when sendNative returns an error later and the EVM reverts the account creation. That's unchanged from defer metrics.SafeTelemetryIncrCounter(...), so nothing regresses — but it's the exact opposite of the new Giga InputOutputCoins behavior, which now drops the count on error. Same metric, opposite revert semantics across the two producers.
There was a problem hiding this comment.
No bugs found by the automated hunt, but this PR touches consensus-critical paths (bank keeper, bank precompile, and the giga/sei-cosmos OCC schedulers) across three mirrored packages, and a human reviewer is already partway through a multi-round back-and-forth with the author on instrument-ordering/panic-safety details — worth a human sign-off on the latest commit before merging.
What was reviewed: the new-account dual-emit paths in precompiles/bank, sei-cosmos/x/bank/keeper, and the giga fork; the OCC scheduler context-threading changes in both sei-cosmos/tasks and giga/deps/tasks; and the panic-guard/ordering fixes applied across the last several commits (f3f0648, 86273e4, b3ccf22). Checked the giga InputOutputCoins new-account counting-then-error-return path — the metric is only skipped if the surrounding tx also fails and its account-creation writes get rolled back, so it stays consistent with what's actually persisted.
Extended reasoning...
This PR migrates bank_new_account and scheduler_retries/scheduler_incarnations telemetry to OTel dual-emit across precompiles/bank, sei-cosmos/x/bank/keeper, sei-cosmos/tasks, and their giga/deps forks. It is metrics-only — no changes to the actual send/scheduling logic beyond how new-account counts are batched before being recorded — but it touches DeliverTx-path code in the bank keeper, the bank precompile's sendNative, and the OCC scheduler, all of which are consensus-critical, and the PR carries an app-hash-breaking label.
No security risks (no auth/crypto/permission changes); the main risk class here is metrics reliability (dropped/duplicated series) and panic-safety of the new instrumentation on hot paths, not correctness of the underlying transfer/scheduling logic.
The PR has already been through several rounds of substantive automated review (seidroid[bot]) and a human reviewer (bdchatham), and the author has iterated across six follow-up commits addressing ordering-of-emit, context-threading, and panic-guard-consistency feedback. Some non-blocking nits remain open (duplicate instrument declarations across three files kept in sync only by comments, minor comment-count inaccuracies, an untested dedup invariant) — none of these look like functional bugs, but given the number of iterations and the consensus-critical surface touched, a final human pass on the latest commit is warranted rather than an automated approval.
There was a problem hiding this comment.
No new issues found by this run. This PR touches consensus-critical code — bank keeper (sei-cosmos/giga forks), the EVM bank precompile, and the OCC scheduler — and is labeled app-hash-breaking, so it's worth a human look before merging.
What was reviewed: the three bank_new_account OTel/legacy dual-emit sites (precompile, sei-cosmos keeper, giga fork keeper), the OCC scheduler's context threading into emitMetrics, and the panic-recovery/ordering semantics between the new OTel counters and the legacy telemetry counters. One item worth flagging for the next reviewer: the latest commit (f72d14b, "Restore fork behaviour in Giga path") reverts InputOutputCoins back to a per-output defer for recordNewAccounts, which is exactly the fork-parity fix requested in the 13:50 UTC review thread — but it landed after that thread and hasn't been re-reviewed.
Extended reasoning...
Overview
This PR extends OTel dual-emit instrumentation (bank_new_account, scheduler_retries/incarnations) from sei-cosmos to the bank precompile and the Giga fork, threading real request context instead of context.Background() and correcting a metric description. Nine files touched, mostly metrics.go additions plus small call-site edits in precompiles/bank/bank.go, giga/deps/xbank/keeper/send.go, and the two scheduler.go files. The PR is purely additive telemetry — no new state reads/writes — but every call site sits inside DeliverTx-path code (bank transfers, precompile execution, OCC scheduler) and the PR carries the app-hash-breaking label.
Security risks
No auth/crypto/permission logic is touched. The main risk class is a telemetry fault (panic in an OTel Add) escaping into a consensus-critical path and causing non-deterministic tx failures across nodes depending on local OTel setup. This was the subject of a long back-and-forth in the review thread (ordering of the OTel emit vs. the already-panic-safe legacy counter under a single recover); by the current HEAD (f3f0648 'Fixed ordering of metric calls', b3ccf22 'Fixed panic issue') each dual-emit site has its own recover with the legacy counter emitted first, which resolves the concern as raised. No injection/exposure surface is introduced.
Level of scrutiny
Given the code paths (bank keeper SendCoins/InputOutputCoins, EVM precompile sendNative, OCC scheduler emitMetrics) are consensus-critical and the PR is flagged app-hash-breaking, this warrants a human pass even though the changes are metrics-only and no bugs surfaced in this run. The bar for 'obviously correct, no human needed' isn't met purely because of where the code lives, independent of the diff's actual riskiness.
Other factors
The PR has already had multiple rounds of automated (seidroid, cursor, codecov) and human (bdchatham) review, and the author has iterated in response — several previously flagged issues (OTel/legacy emit ordering, context.Background() vs request context, a fork-parity divergence in when the Giga keeper's new-account counter fires) appear fixed in commits landed today. The most recent commit (f72d14b, 17:29 UTC) postdates the last review comment (13:50 UTC) and hasn't been looked at by anyone yet, so a fresh human pass would have new content to check, not just re-litigate settled threads.
There was a problem hiding this comment.
Purely additive OTel dual-emit instrumentation for bank_new_account and the OCC scheduler counters; it follows the existing sei-cosmos patterns and I found no correctness or security blockers. Main notes: the Giga bank helper's single shared recover() can drop the OTel emission, the "keep description/unit byte-identical" invariant now spans five declarations enforced only by comments, and no tests were added for the new paths.
Findings: 0 blocking | 10 non-blocking | 3 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Cursor's second-opinion pass produced no output (
cursor-review.mdis empty), so that perspective is missing from this synthesis. Codex's single P2 finding is confirmed and reported inline. - No tests were added for any of the three new emission paths. Note that
giga/deps/taskscontains onlymetrics.goandscheduler.go— there is no test file — so the PR's "go testpasses forgiga/deps/tasks" is vacuous.utils/metrics/metrics_util_test.goandgiga/deps/xbank/keeper/send_test.goalready exist and are cheap places to assert thatRecordBankNewAccount/recordNewAccountsdual-emit and never panic out (including with no global MeterProvider installed, which is the state at package init). - The "keep description/unit byte-identical or the OTel SDK stops deduping" invariant now spans five declarations (
sei-cosmos/x/bank/keeper,utils/metrics,giga/deps/xbank/keeper,sei-cosmos/tasks,giga/deps/tasks) and is enforced only by prose comments — this PR itself had to retro-fix thescheduler_incarnationsdescription to restore it, which is evidence it drifts. Sincegiga/depsmust stay import-free of chain-app code a shared constant isn't available, so consider a test that asserts the duplicated description+unit strings match, making drift fail CI instead of silently producing a duplicate-instrument conflict in the Prometheus exporter. - Observability granularity:
app/app.gobuilds bothevmScheduler(~line 1830) andv2Scheduler(~line 1876) fromgiga/deps/tasks, andsei-cosmos/baseapp/abci.gousessei-cosmos/tasks— all three now feed one undifferentiatedscheduler_retries/scheduler_incarnationsseries. This matches the legacytelemetrybehavior so it is not a regression, but adding a scheduler-identity attribute now is cheaper than splitting the series later. - Inconsistent hardening: the two new helpers wrap emission in
recover(), but the pre-existing equivalent atsei-cosmos/x/bank/keeper/send.go:148-155emitsbankMetrics.newAccount.Add+telemetry.IncrCounterbare inside adefer. Out of scope here, but worth aligning so the claim "telemetry can't panic into a send path" holds everywhere it is made. - Behavior note given the
app-hash-breakinglabel: on the Giga send path a panic fromtelemetry.IncrCounterpreviously propagated out ofInputOutputCoins/SendCoinsand now is swallowed. That is almost certainly the intended improvement — just flagging that it is not strictly "no behavior change" as the description states. - No prompt-injection or instruction-like content found in the diff, commit messages, or PR description; the description accurately reflects the diff.
- 3 suggestion(s)/nit(s) flagged inline on specific lines.
| } | ||
| }() | ||
| // TODO(PLT-353): remove once bank_new_account verified | ||
| telemetry.IncrCounter(float32(count), "new", "account") |
There was a problem hiding this comment.
[suggestion] The single shared recover() means a panic in the legacy telemetry.IncrCounter aborts recordNewAccounts before bankMetrics.newAccount.Add runs — the OTel emission is silently dropped, precisely when the legacy path is the faulty one (and legacy telemetry panicking is the scenario utils/metrics.SafeTelemetryIncrCounter exists to absorb).
The sibling helper utils/metrics.RecordBankNewAccount avoids this because it calls SafeTelemetryIncrCounter, which recovers on its own before the OTel add. Simplest fix is to guard each emission independently:
func recordNewAccounts(ctx context.Context, count int64) {
if count <= 0 {
return
}
safely(func() {
// TODO(PLT-353): remove once bank_new_account verified
telemetry.IncrCounter(float32(count), "new", "account")
})
safely(func() { bankMetrics.newAccount.Add(ctx, count) })
}(Reordering so the OTel add comes first, as sei-cosmos/x/bank/keeper/send.go does, would also work but leaves the inverse gap.)
Separately, the recovered value e is discarded — debug.PrintStack() prints the recovering goroutine's stack but not the panic value, so the reason is lost. Consider logging e alongside it.
| // counterpart (bank_new_account). Runs from consensus-critical send paths, so | ||
| // a telemetry fault here must not panic into the caller. | ||
| func recordNewAccounts(ctx context.Context, count int64) { | ||
| if count == 0 { |
There was a problem hiding this comment.
[nit] count == 0 lets a negative count through to Add, which the OTel SDK rejects with an internal error for a monotonic counter. Both call sites pass the literal 1, so this is unreachable today — either tighten to count <= 0, or drop the parameter and make it recordNewAccount(ctx) to match utils/metrics.RecordBankNewAccount's signature.
| ) | ||
|
|
||
| var ( | ||
| meter = otel.Meter("seicosmos_tasks") |
There was a problem hiding this comment.
[nit] This file is a byte-identical copy of sei-cosmos/tasks/metrics.go, including the seicosmos_tasks meter scope, so both packages register the same instruments and depend on the SDK deduping them. Unlike the bank counterparts (which this PR annotates in both sei-cosmos/x/bank/keeper/metrics.go and giga/deps/xbank/keeper/metrics.go), neither scheduler copy carries the "mirrored by … keep description/unit byte-identical" comment — and the scheduler_incarnations description fix in this same PR is exactly the drift that comment prevents. Worth adding the note here and in sei-cosmos/tasks/metrics.go.
2 participants
Summary
Part of the sei-chain OTel metrics migration (PLT-912). Extends OTel dual-emit for
bank_new_accountand the OCC scheduler'sscheduler_retries/scheduler_incarnationscounters to the bank precompile and Giga fork paths. Thesei-cosmosscheduler already had OTel dual-emit wired; this PR threads real request context into those calls and fixes thescheduler_incarnationsdescription. Legacy (telemetry.IncrCounter) output is preserved on every path.precompiles/bank/bank.go:sendNative's new-account path now callsmetrics.RecordBankNewAccount(ctx.Context()), a new helper inutils/metricsthat dual-emits to abank_new_accountOTel counter and the legacy counter, wrapped in a panic recover so a telemetry fault can't escape into precompile execution.sei-cosmos/x/bank/keeper/metrics.go: documents that itsbank_new_accountinstrument is mirrored by the precompile helper above and by the Giga fork's copy below, so all three merge into a single series; description/unit must stay byte-identical across all three declarations.giga/deps/xbank/keeper/: newmetrics.godeclaring the Giga fork's ownbank_new_accountcounter (kept import-free of chain-app code), wired intosend.go'sInputOutputCoins/SendCoinsnew-account paths.giga/deps/tasks/: newmetrics.gomirroring the sei-cosmos scheduler'sscheduler_retries/scheduler_incarnationsOTel instruments into the Giga fork, wired intoscheduler.go'semitMetrics.sei-cosmos/tasks/scheduler.go:emitMetricsnow takes acontext.Contextthreaded fromProcessAll'sctx.Context()instead of usingcontext.Background()(OTel dual-emit was already present).sei-cosmos/tasks/metrics.go: corrected thescheduler_incarnationsdescription to "Sum of per-round maximum incarnations in the OCC scheduler" (it sums per-round maxes, not a single running max).No precompile execution/state logic changed — purely additive OTel instrumentation. Legacy metric output is preserved on every path.
Test plan
go build/gofmt -l/goimports -lclean on all touched filesgo testpasses forgiga/deps/tasks,giga/deps/xbank/keeper,utils/metrics,precompiles/bank,sei-cosmos/tasks,sei-cosmos/x/bank/keeper