feat(metrics): migrate sei-cosmos to OpenTelemetry (PLT-353)#3467
Conversation
PR SummaryLow Risk Overview BaseApp gains histograms/counters/gauges for ABCI paths ( Store layer: Module keepers (bank, staking, gov, distribution, vesting, upgrade) emit OTel counters/gauges for high-value txs and upgrade plans; token amounts use Reviewed by Cursor Bugbot for commit 84551ab. 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).
|
amir-deris
changed the title
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #3467 +/- ##
==========================================
+ Coverage 59.04% 59.06% +0.02%
==========================================
Files 2187 2200 +13
Lines 181488 181679 +191
==========================================
+ Hits 107163 107317 +154
- Misses 64703 64729 +26
- Partials 9622 9633 +11
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
| planHeight metric.Int64Gauge | ||
| }{ | ||
| planHeight: must(meter.Int64Gauge( | ||
| "plan_height", |
There was a problem hiding this comment.
Instrument-name collision with sei-cosmos/x/upgrade/metrics.go.
Both files declare a Int64Gauge named plan_height with identical name + info attribute schema. The OTel-Prometheus exporter does not propagate the meter scope (seicosmos_x_upgrade vs seicosmos_x_upgrade_keeper) into the Prometheus series name — both collapse to sei_chain_plan_height. Result is either a duplicate-registration warning with one side silently dropped, or two coexisting series differentiated only by otel_scope_name where naive PromQL sum(sei_chain_plan_height) double-counts.
The x/upgrade/abci.go BeginBlocker site is the load-bearing one (fires on every block while a plan exists). The keeper-side recording at ScheduleUpgrade is one-shot and redundant — drop it, or rename to something like upgrade_pending_plan_height.
Separately: the info label is a free-form proposal string. Cardinality-unfriendly and not useful as a metric axis — keep name (bounded by released upgrade handlers, ~20 lifetime values), drop info. Move it to a span attribute / event at upgrade-schedule time if you need it.
| }, | ||
| ) | ||
| defer func() { | ||
| govMetrics.voteTotal.Add(goCtx, 1, otelmetric.WithAttributes(attribute.String("proposal_id", strconv.FormatUint(msg.ProposalId, 10)))) |
There was a problem hiding this comment.
Unbounded proposal_id label on voteTotal (and depositTotal below, and VoteWeighted).
Every governance proposal ever cast a vote on lives in the TSDB forever; multiplied by per-validator series, this grows monotonically without bound. Sei mainnet is at ~200 historical proposals today and continues linear growth.
Drop the label. A plain vote_total / deposit_total counter answers "how many votes per block?" which is the only thing the metric is actually useful for. Per-proposal vote/deposit breakdown is a query-time concern against on-chain events — not a metric axis.
| defer func() { | ||
| for _, a := range msg.Amount { | ||
| if a.Amount.IsInt64() { | ||
| bankMetrics.sendAmount.Record(goCtx, a.Amount.Int64(), otelmetric.WithAttributes(attribute.String("denom", a.Denom))) |
There was a problem hiding this comment.
Unbounded user-controlled denom label — most severe cardinality issue in this PR.
Sei supports tokenfactory denoms (factory/<creator>/<subdenom> — any address can mint) plus IBC denoms with ~100-char hash traces. Sei mainnet already has 10k+ factory denoms, growing unbounded. This same pattern appears on delegateAmount/redelegateAmount/undelegateAmount (staking), withdrawRewardAmount/withdrawCommissionAmount (distribution), and accountAmount (vesting) — every *Amount gauge across this PR.
Options, in order of preference:
- Drop
denomentirely — record a per-module amount metric without the denom axis. If you need a breakdown, that's a log/span attribute. - Bucket into
denom_class— small helper that returns"usei"/"ibc"/"factory"/"other"based on prefix. Preserves the useful axis (native vs synthetic flow) without exposing the registry to thousands of long-string labels. - If a specific dashboard needs aggregate-by-denom for native denoms, allowlist a fixed set via Prometheus
metric_relabel_configsrather than emitting them as labels.
This would be a hard block on a single-metric PR; flagging now so it doesn't go out as part of a larger migration.
| } | ||
|
|
||
| func (c *BoundedCache) emitKeysEvictedMetrics(keysToEvict int) { | ||
| storeMetrics.boundedCache.Record(context.Background(), int64(keysToEvict), otelmetric.WithAttributes(attribute.String("type", "keys_evicted"))) |
There was a problem hiding this comment.
nit: can we take a follow-up to wire the context through here or is there a blocker?
There was a problem hiding this comment.
I also looked into not using context.Background(), but this Cache type is currently only used in tests, and threading context meaningfully from ABCI → KV store → cache backend is a non-trivial SDK-wide change with marginal metric value. I would prefer to leave that for now if that's ok?
There was a problem hiding this comment.
Yeah, definitely okay to take as a follow-up for getting tracing wired
|
|
||
| // cosmos_tx_gas_exceeded | ||
| func (g *basicGasMeter) incrGasExceededCounter(errorType string, descriptor string) { | ||
| storeMetrics.gasExceeded.Add(context.Background(), 1, otelmetric.WithAttributes( |
There was a problem hiding this comment.
Same callout for context wiring.
| commitStartTime := time.Now() | ||
| defer telemetry.MeasureSince(commitStartTime, "storeV2", "sc", "commit", "latency") | ||
| defer func() { | ||
| storev2Metrics.scCommitLatency.Record(context.Background(), time.Since(commitStartTime).Seconds()) |
There was a problem hiding this comment.
Same context wiring callout
There was a problem hiding this comment.
I can address this one since the historicalAbciQuery counter would get exemplar linkage — we could correlate "this rate-limited query" directly to a specific gRPC trace and that's the most actionable OTel win here.
|
|
||
| upgradeMetrics.planHeight.Record(ctx.Context(), plan.Height, otelmetric.WithAttributes( | ||
| attribute.String("name", plan.Name), | ||
| )) |
There was a problem hiding this comment.
Missing info attribute on upgrade plan_height metric
Low Severity
The PR description specifies plan_height (and pendingPlanHeight) gauges carry both name + info labels, and the legacy telemetry call right below emits both. However, the new OTel Record calls only include attribute.String("name", plan.Name) — the info attribute is silently dropped, so the OTel metric loses the upgrade metadata that the legacy metric provides. This applies to both x/upgrade/abci.go and x/upgrade/keeper/keeper.go.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit e1348dc. Configure here.
|
Thanks for the quick turnaround — all four blockers look resolved. A few non-blocking callouts to consider for this PR or a followup:
LGTM otherwise. Happy to file these as separate issues if useful. |
| stakingMetrics.delegateTotal.Add(goCtx, 1) | ||
| // TODO(PLT-353): remove once staking_delegate_total verified | ||
| telemetry.IncrCounter(1, types.ModuleName, "delegate") | ||
| stakingMetrics.delegateAmount.Record(goCtx, msg.Amount.Amount.Int64()) |
There was a problem hiding this comment.
Staking OTel amount gauges missing denom_class attribute
Medium Severity
The staking OTel gauges (delegateAmount, redelegateAmount, undelegateAmount) record without any denomination attribute, while the equivalent bank, distribution, and vesting OTel gauges all consistently include a denom_class attribute via telemetry.DenomClass(). The legacy metrics at the same call sites still emit a denom label. The PR description also lists a denom label for these staking metrics. This inconsistency means the staking OTel metrics lose denom-level observability that the other modules preserve.
Additional Locations (2)
Reviewed by Cursor Bugbot for commit 47ba551. Configure here.
There was a problem hiding this comment.
The reason is the delegateAmount, redelegateAmount and undelegateAmount are in base denom (usei) and no need to include the attribute for this case.
For denom class, Delegate/Redelegate/Undelegate all operate on msg.Amount, which is validated against k.BondDenom(ctx) — always usei on sei-chain. Also the metric unit
Good point! Moved it to its own file.
Sounds good! Added
Added extra buckets for
Good point. Once we get to new dashboards/alerts, we can revisit this topic. |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
There are 3 total unresolved issues (including 2 from previous reviews).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 6fdfa8f. Configure here.
3 participants
Adds OTel instrumentation to
sei-cosmosfollowing the same pattern as PLT-329, PLT-330, PLT-336, PLT-339, and PLT-343.New instruments
baseapp (meter
seicosmos_baseapp)mid_block_duration— histogram, secondsend_block_duration— histogram, secondsdeliver_tx_duration— histogram, secondstx— counter, total delivered transactionstx_result— counter,resultlabel (successful/failed)tx_gas_used— gaugetx_gas_wanted— gaugecommit_duration— histogram, secondsabci_query_duration— histogram, seconds,pathlabelprocess_proposal_duration— histogram, secondsfinalize_block_duration— histogram, secondsget_tx_priority_hint_duration— histogram, secondsrun_tx_duration— histogram, seconds,modelabel (replacesMeasureThroughputSinceWithLabelsforTxCount)run_msgs_duration— histogram, seconds (replacesMeasureThroughputSinceWithLabelsforMessageCount)run_msg_latency— histogram, seconds,typelabel (replaces bothsei.cosmos.run.msg.latencyandcosmos.run.msg.latency)storev2/rootmulti (meter
seicosmos_storev2_rootmulti)sc_commit_latency— histogram, secondsss_version— gaugehistorical_abci_query— counter,success+prooflabelsiavl_total_key_bytes— gauge,store_namelabeliavl_total_value_bytes— gauge,store_namelabeliavl_total_num_keys— gauge,store_namelabelstate_sync_keys_exported— countertasks (meter
seicosmos_tasks)scheduler_retries— counterscheduler_incarnations— counterstore/types (meter
seicosmos_store_types)gas_exceeded— counter,error+descriptorlabelsbounded_cache— gauge,typelabelx/upgrade (meter
seicosmos_x_upgrade)begin_blocker_duration— histogram, secondsplan_height— gauge,namelabelx/upgrade/keeper (meter
seicosmos_x_upgrade_keeper)plan_height— gauge,namelabelx/auth/vesting (meter
seicosmos_x_auth_vesting)new_account— counteraccount_amount— gauge,denomlabelx/bank/keeper (meter
seicosmos_x_bank_keeper)send_amount— gauge,denomlabelx/distribution/keeper (meter
seicosmos_x_distribution_keeper)withdraw_reward_amount— gauge,denomlabelwithdraw_commission_amount— gauge,denomlabelx/staking/keeper (meter
seicosmos_x_staking_keeper)delegate— counterdelegate_amount— gauge,denomlabelredelegate— counterredelegate_amount— gauge,denomlabelundelegate— counterundelegate_amount— gauge,denomlabelx/gov/keeper (meter
seicosmos_x_gov_keeper)proposal— countervote— counter,proposal_idlabeldeposit— counter,proposal_idlabelNotes
TODO(PLT-353)comments pending dashboard verification.