prometheus metrics for autobahn/avail, autobahn/data and p2p/mux#3682
prometheus metrics for autobahn/avail, autobahn/data and p2p/mux#3682pompon0 wants to merge 50 commits into
Conversation
PR SummaryMedium Risk Overview New observability: autobahn/avail records commit/app QC road indices, global block numbers, and proposal→commit / commit→commit latencies when QCs are observed. autobahn/data moves block/tx latency off Node startup: Reviewed by Cursor Bugbot for commit d1c9ddd. Bugbot is set up for automated code reviews on this repo. Configure here. |
|
The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #3682 +/- ##
==========================================
- Coverage 59.84% 58.77% -1.08%
==========================================
Files 2278 2192 -86
Lines 189140 178723 -10417
==========================================
- Hits 113187 105039 -8148
+ Misses 65876 64446 -1430
+ Partials 10077 9238 -839
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
![seidroid[bot]](https://avatars.githubusercontent.com/in/2745135?s=60&v=4){kind=small}
There was a problem hiding this comment.
Adds OpenTelemetry metrics for autobahn/avail consensus QCs and p2p/mux stream I/O, plus minor refactors. The instrumentation is reasonable, but the new mux metrics file is not gofmt-clean (will fail the lint CI), and several monotonic totals are declared as UpDownCounters.
Findings: 2 blocking | 4 non-blocking | 2 posted inline
Blockers
sei-tendermint/internal/p2p/mux/metrics/metrics.gois not gofmt-clean, which violates the repo's formatting requirement and will fail golangci/make lintin CI (see inline comment). Rungofmt -s -won the file.- 1 blocking issue(s) flagged inline on specific lines.
Non-blocking
- Repository review guidelines (
REVIEW_GUIDELINES.md) were empty/missing, so no repo-specific standards were applied beyond AGENTS.md. - The Cursor second-opinion pass (
cursor-review.md) produced no output. - In
avail/metrics.goObserveCommitQC,proposalToCommitLatency.Record(...)is called before the ordering guardlast.val.Index() >= qc.Index(), so a re-observed/stale CommitQC still records a latency sample. Consider moving the record after the guard if only fresh QCs should be measured (minor, metrics-only). - 1 suggestion(s)/nit(s) flagged inline on specific lines.
There was a problem hiding this comment.
Observability-only PR adding OpenTelemetry metrics for autobahn/avail consensus QCs and p2p/mux stream traffic. The changes are correct and well-structured (dedup guards, bounded label cardinality, safe mutex-by-value refactor); the only substantive concern is that the in-flight stream gauge can drift upward when streams are abandoned without a local Close.
Findings: 0 blocking | 4 non-blocking | 1 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- No tests were added for the new metrics helpers (ObserveCommitQC/ObserveAppQC dedup logic and the Stream metric lifecycle). A small unit test around the monotonic dedup guards (out-of-order QCs are ignored) and Open/Close idempotency would guard against regressions; acceptable to skip for an observability-only change, but worth noting.
- Cursor's second-opinion pass (cursor-review.md) produced no output, and REVIEW_GUIDELINES.md was empty on the base branch, so no repo-specific standards were applied. Codex's single medium finding (the inFlight counter leak) is incorporated as an inline comment.
- Minor: the
Sendmetric is recorded when a message is enqueued to the frame queue (stream.go:95) rather than when it is actually written to the connection, so send_bytes/send_msgs slightly lead the wire. This is a reasonable approximation but worth a comment if precise byte accounting is ever expected. - 1 suggestion(s)/nit(s) flagged inline on specific lines.
| return | ||
| } | ||
| inner.closed.local = true | ||
| inner.metrics.Close() |
There was a problem hiding this comment.
[suggestion] The inFlight up/down counter is only decremented here in the local close() path. Open() increments it once a stream's recv end is established, but the decrement never runs if a stream is abandoned without a local Close() — e.g. Mux.Run exits on a connection/read/write error, or a caller observes remote close (tryPrune in runRecv only fully prunes once both local and remote are closed) but never calls Close(). Because inFlight is a process-global counter shared across all connections, this drifts permanently upward across peer churn, making the gauge unreliable over a long-running node. Consider decrementing symmetrically on prune/teardown (e.g. from tryPrune or a runner shutdown hook) so an opened stream is always balanced by exactly one decrement. (Matches Codex's medium finding.)
There was a problem hiding this comment.
local Close is ALWAYS required by the contract on Stream object - if mux.Connect/Accept succeeds, then caller is required to call Close eventually.
seidroid
Bot
dismissed
their stale review
Superseded: latest AI review found no blocking issues.
bdchatham
left a comment
bdchatham
left a comment
There was a problem hiding this comment.
xreview summary (opentelemetry / sei-networking / systems / idiom lenses). Nice, well-scoped instrumentation — most of it is clean. Three points worth resolving before merge, plus a few smaller ones inline.
🔴 MeterProvider wiring — where is the global MeterProvider set up? otel.Meter() is a no-op unless a provider with a Prometheus/OTLP reader and a service.name resource is registered in the binary, and I don't see it in this diff. If it isn't already wired in main, every instrument here silently records nothing — worth confirming before merge.
On your weighted-histogram question: you don't need to call Record more than once. You have exactly one latency value per commit, so it's a single Record into a Float64Histogram — the sum+count counter split was solving a problem the code doesn't have. Details in the inline comment on commitToCommitLatency.
| "tendermint_internal_autobahn_avail__app_global_block_number", | ||
| metric.WithDescription("global block number of the highest observed appQC"), | ||
| )) | ||
| var proposalToCommitLatency = utils.OrPanic1(meter.Float64Histogram( |
There was a problem hiding this comment.
🟡 Add metric.WithUnit("s") on the avail latency instruments — the mux-side latency sets it but these don't, so the exporter won't append _seconds and the two packages end up with inconsistently-named latency series.
| return &RPC[API, Req, Resp]{kind, limit, req, resp} | ||
| service[kind] = &rpcConfig{ | ||
| limit: limit, | ||
| name: fmt.Sprintf("%T", utils.Zero[Req]()), |
There was a problem hiding this comment.
🟡 fmt.Sprintf("%T", ...) bakes the fully-qualified Go type into the rpc_name label, so renaming or moving the request type silently breaks dashboard continuity. Prefer an explicit, stable name string.
Weighted histograms in OTel — summary of the threadTL;DR on "how do I migrate the weighted histograms without calling There's no weighted 1. Looping recordOpts := metric.WithAttributeSet(attrSet) // build once, outside the loop
for i := uint64(0); i < blocks; i++ {
seqLatency.Record(ctx, latencySec, recordOpts)
}2. But I'd reconsider the weighted histogram itself. Replaying one shared per-commit measurement into N identical points inflates the statistical support and quietly redefines seqLatency.Record(ctx, latencySec, opts) // per-commit, unweighted → commit-health p99
blocksSequenced.Add(ctx, int64(blocks), opts) // blocks_sequenced_total → throughput
// optional: blocksPerCommit.Record(ctx, float64(blocks), opts) // batch-size distributionNow 3. If you genuinely need a pre-aggregated / weighted histogram in OTel, it's doable natively via a custom func (p *seqLatencyProducer) Observe(latencySec float64, blocks uint64) {
p.mu.Lock(); defer p.mu.Unlock()
i := sort.SearchFloat64s(p.bounds, latencySec) // first bound >= value
p.bucketCounts[i] += blocks // weight by N, O(1)
p.count += blocks
p.sum += latencySec * float64(blocks)
}
func (p *seqLatencyProducer) Produce(ctx context.Context) ([]metricdata.ScopeMetrics, error) {
p.mu.Lock()
bc := append([]uint64(nil), p.bucketCounts...) // snapshot under lock
dp := metricdata.HistogramDataPoint[float64]{
Attributes: p.attrs, StartTime: p.start, Time: time.Now(),
Count: p.count, Bounds: p.bounds, BucketCounts: bc, Sum: p.sum,
}
p.mu.Unlock()
return []metricdata.ScopeMetrics{{
Scope: instrumentation.Scope{Name: "consensus"},
Metrics: []metricdata.Metrics{{
Name: "consensus_block_sequencing_latency", Unit: "s",
Data: metricdata.Histogram[float64]{
Temporality: metricdata.CumulativeTemporality,
DataPoints: []metricdata.HistogramDataPoint[float64]{dp},
},
}},
}}, nil
}The catch: you now own the cumulative-temporality invariants (bucket counts monotonically non-decreasing + a stable On the API-ergonomics point: yes, client_golang's |
| } | ||
| // Constructed once per CommitQC, which we should afford. | ||
| attrs := metric.WithAttributeSet(attribute.NewSet( | ||
| // Timeouts capped: 20 means [20,inf) |
There was a problem hiding this comment.
I suppose we almost never have 20+ timeouts on one RoadIndex? What happens if the cluster gets stuck?
There was a problem hiding this comment.
then we don't care about this metric, I suppose.
| } | ||
|
|
||
| var observedCommitQC = newObserved[*types.CommitQC]() | ||
| var observedAppQC = newObserved[*types.AppQC]() |
There was a problem hiding this comment.
Do we need the full QC? We are only using RoadIndex, View Index, and timestamp I suppose?
There was a problem hiding this comment.
this is a simplification, there is no need to restrain it down.
| return false, nil | ||
| } | ||
| i.latestAppQC = utils.Some(appQC) | ||
| metrics.ObserveAppQC(appQC) |
There was a problem hiding this comment.
This could be much later than we actually observe AppQC for this RoadIndex right? Because the block proposal packing AppQC is optional.
There was a problem hiding this comment.
true, these metrics are describing the avail.State content.
| i.commitQCs.prune(idx) | ||
| if i.commitQCs.next == idx { | ||
| i.commitQCs.pushBack(commitQC) | ||
| metrics.ObserveCommitQC(c, commitQC) |
There was a problem hiding this comment.
Sorry I'm confused, why do we observe these QCs inside prune() instead of right after the QCs are verified?
There was a problem hiding this comment.
metrics track newest commitQC - the commitQC from the anchor is newest iff the anchor wipes the whole data state.
pompon0
changed the title
There was a problem hiding this comment.
This PR swaps the heavy k8s.io/component-base metrics dependency for the repo's own metricsgen-based prometheus wrappers and adds observability for autobahn avail/data QCs and p2p mux streams. The refactor is clean and the changes are non-app-hash-breaking; the only substantive concern is a potential slow drift of the new in_flight gauge on abnormal stream teardown.
Findings: 0 blocking | 4 non-blocking | 1 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Second-opinion passes:
cursor-review.mdandREVIEW_GUIDELINES.mdwere both empty, so no Cursor findings or repo-specific guidelines were available to merge; only the Codex pass produced output (the in_flight observation, reflected inline). - mux metrics (
latency,in_flight,send_msgs,recv_msgs,send_bytes,recv_bytes) are generated with emptyHelpstrings. Adding doc comments on the struct fields insei-tendermint/internal/p2p/mux/metrics/metrics.gowould give the exported Prometheus metrics meaningful HELP text. ObserveCommitQC/ObserveAppQCuse package-global mutex-guardedobserved*state and a process-globalGlobalregistry; in multi-node in-process test setups the cross-node dedup (last.val.Index() >= qc.Index()) will drop observations from other nodes. This is an inherent limitation of the global-metrics pattern (metrics are process-global anyway) and fine for production single-node use, but worth being aware of when interpreting test metrics.- 1 suggestion(s)/nit(s) flagged inline on specific lines.
| ctrl.Updated() | ||
| return err | ||
| } | ||
| inner.metrics.Open() |
There was a problem hiding this comment.
[suggestion] in_flight is incremented here in Open() but only decremented in the local close() path (inner.metrics.Close()). The ordering is correct for the open() error path (the increment happens only after WaitUntil succeeds, so the early s.close(inner) above doesn't over-decrement). However, because the gauge lives in a process-global registry, any stream that is opened but never has Stream.Close() called on it — e.g. a stream abandoned when Mux.Run exits on a connection/read/write error and its owning goroutine returns without closing — leaks a permanent +1. Over many reconnects the gauge can drift upward and stop reflecting the true in-flight count, and the latency sample for those streams is never recorded. Consider tying the decrement/observe to stream teardown/pruning (tryPrune) rather than only to the application calling Close().
There was a problem hiding this comment.
This PR adds Prometheus metrics for autobahn/avail, autobahn/data, and p2p/mux, refactoring metrics into metricsgen-generated code. The mux metrics are correctly guarded under the stream lock, and the data-metrics refactor is sound. Two non-blocking concerns: startup restore replays anchor QCs through prune() and pollutes latency histograms, and the PR bundles an unrelated change to generated Autobahn config defaults.
Findings: 0 blocking | 6 non-blocking | 2 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- cursor-review.md is empty — the Cursor pass produced no output to incorporate.
- REVIEW_GUIDELINES.md (from the base branch) is empty, so no repo-specific standards were applied.
- go.mod adds two new indirect deps (github.com/moby/term, github.com/modern-go/reflect2) while dropping k8s.io/component-base. This is plausible go mod tidy reconciliation, but worth a quick confirmation that the module graph churn is intended.
- The mux metrics (latency, inFlight, sendMsgs, recvMsgs, sendBytes, recvBytes) are registered with empty Help strings; consider adding help text for operator-facing Prometheus metrics.
- 2 suggestion(s)/nit(s) flagged inline on specific lines.
| return false, nil | ||
| } | ||
| i.latestAppQC = utils.Some(appQC) | ||
| metrics.ObserveAppQC(appQC) |
There was a problem hiding this comment.
[suggestion] prune() now emits metrics, but newInner() also calls prune() (line 92) to replay the persisted prune anchor on startup. On restart this makes ObserveCommitQC observe proposalToCommitLatency as now - anchor.commitQC.Proposal().Timestamp(), i.e. real latency + downtime — a spurious, downtime-inflated sample that permanently skews the histogram. It also seeds observedCommitQC with the restore time, so the first live commit records commitToCommitLatency as (restore→first-commit) rather than a genuine commit-to-commit interval. Consider only observing the latency histograms on the live pruning path (e.g. skip observation when replaying the restore anchor). ObserveAppQC only sets gauges, so it's harmless.
| Validators: validators, | ||
| MaxTxsPerBlock: 5_000, | ||
| AllowEmptyBlocks: true, | ||
| MaxTxsPerBlock: 2_000, |
There was a problem hiding this comment.
[suggestion] This changes generated Autobahn config defaults (MaxTxsPerBlock 5000→2000, AllowEmptyBlocks true→false on line 87), which alters block-production behavior for newly generated configs — with AllowEmptyBlocks:false the producer no longer seals empty interval blocks. This is unrelated to the metrics purpose of this PR; please confirm it's intentional or split it into its own change.
There was a problem hiding this comment.
A well-scoped observability PR adding Prometheus metrics for autobahn/avail, autobahn/data, and p2p/mux, plus dropping the k8s.io/component-base dependency in favor of the internal weighted histogram. The metrics wiring is correct and the dependency migration is behavior-preserving; the only notes are an unrelated config-default change bundled in and some minor metric-hygiene items.
Findings: 0 blocking | 4 non-blocking | 1 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Several new metrics have empty
Helpstrings (muxlatency/in_flight/send_msgs/recv_msgs/send_bytes/recv_bytes, and the existing p2pchannel_msgs). Prometheus best practice is a non-empty help text; consider adding descriptions in the sourcemetrics.gostructs so they propagate to the generated files. - This PR removes four previously-exported p2p metrics (
peer_send_bytes_total,peer_pending_send_bytes,router_peer_queue_send,router_channel_queue_send). Code references are cleanly removed, but any external dashboards/alerts relying on these series will silently break — worth calling out in release notes. - Second-opinion passes: Codex reported no material findings (noting it could not run
go testbecause the sandbox blocked the Go 1.25.6 toolchain download); the Cursor review file was empty, so that pass produced no output. REVIEW_GUIDELINES.md was also empty/absent, so no repo-specific standards were applied. - 1 suggestion(s)/nit(s) flagged inline on specific lines.
| Validators: validators, | ||
| MaxTxsPerBlock: 5_000, | ||
| AllowEmptyBlocks: true, | ||
| MaxTxsPerBlock: 2_000, |
There was a problem hiding this comment.
[suggestion] These config-default changes (MaxTxsPerBlock 5_000 → 2_000 and AllowEmptyBlocks true → false on the next line) are unrelated to the metrics work this PR describes, and they alter generated autobahn config / consensus behavior. Please confirm this is intentional and, ideally, mention it in the PR description since it's a behavior change bundled into an observability PR.
There was a problem hiding this comment.
This PR adds Prometheus metrics for autobahn/avail, autobahn/data and p2p/mux, replacing the k8s component-base WeightedHistogram dependency with the in-repo tmprometheus package and threading RPC names through the mux for per-stream labels. The changes are well-structured and correct; findings are non-blocking (an unrelated config default change, minor observability nits, and missing test coverage).
Findings: 0 blocking | 7 non-blocking | 2 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Cursor's second-opinion review (./cursor-review.md) is empty — that pass produced no output. Codex reported no material issues, but noted it could not run a compile check because the sandbox had Go 1.24.13 while the repo requires 1.25.6; CI should be relied on for the build/vet gate.
- No tests accompany the new observability logic. The dedup branching in ObserveCommitQC/ObserveAppQC (index/global-number ordering, the capped "timeouts" label, the +1 offset) and the Stream Open/Send/Recv/Close accounting are non-trivial and untested; a small unit test would guard against regressions.
- The old data metric was exposed as
sei_data__latency; it is nowtendermint_internal_autobahn_data_latency(namespace/subsystem). Any existing dashboards/alerts referencing the old name will break — worth calling out in release notes. - Minor observability robustness: mux
Stream.inFlightis decremented only in Close(); if a stream is Open()ed but torn down without close() being invoked, the gauge can drift upward over time. Low impact, but worth confirming close() is guaranteed on all teardown paths. - Style inconsistency in metrics struct tags: mux uses
metrics_labels:"role, rpc_name"(space after comma) while data usesmetrics_labels:"resource,stage"(no space). The generator trims correctly so output is fine, but consider standardizing. - 2 suggestion(s)/nit(s) flagged inline on specific lines.
| Validators: validators, | ||
| MaxTxsPerBlock: 5_000, | ||
| AllowEmptyBlocks: true, | ||
| MaxTxsPerBlock: 2_000, |
There was a problem hiding this comment.
[suggestion] These config default changes (MaxTxsPerBlock 5_000 → 2_000 and AllowEmptyBlocks true → false on the next line) appear unrelated to the PR's stated purpose of adding Prometheus metrics. AllowEmptyBlocks=false in particular changes block-production behavior of the generated config. Please confirm this is intentional and, if so, consider splitting it into its own commit/PR with a rationale so it isn't lost in a metrics change.
|
|
||
| //go:generate go run github.com/sei-protocol/sei-chain/sei-tendermint/scripts/metricsgen -struct=metrics | ||
| type metrics struct { | ||
| latency tmprometheus.HistogramVec `metrics_labels:"role, rpc_name" metrics_buckets:"exp(0.001, 1.3, 30)"` |
There was a problem hiding this comment.
[nit] The mux metric fields have no doc comments, so metricsgen emits Help: "" for latency/inFlight/sendMsgs/recvMsgs/sendBytes/recvBytes in metrics.gen.go. The avail and data metric structs document each field; adding one-line comments here would give these metrics proper Help text for scrapers/dashboards.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 331529e. Configure here.
There was a problem hiding this comment.
A well-scoped observability PR that migrates Autobahn avail/data and p2p/mux metrics onto the internal metricsgen-based prometheus helpers (dropping the k8s.io/component-base dependency) and adds a chain_id telemetry label. No blocking correctness issues; a couple of minor metrics-accuracy/scope notes below.
Findings: 0 blocking | 5 non-blocking | 2 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Cursor second-opinion review (cursor-review.md) was empty — that pass produced no output.
- Metric names change as part of this migration (e.g.
sei_data__latency→tendermint_internal_autobahn_data_latency, and the new avail/mux metrics). Any existing Grafana dashboards or alerting rules referencing the old names will silently stop matching; worth a heads-up in release notes / dashboard updates. - No tests accompany the change. This is understandable for pure metrics wiring, but the
ObserveCommitQC/ObserveAppQCstaleness/dedup logic (Index/GlobalNumber ordering guards, the cappedtimeoutslabel) is non-trivial and would benefit from a small unit test. - 2 suggestion(s)/nit(s) flagged inline on specific lines.
| i.commitQCs.prune(idx) | ||
| if i.commitQCs.next == idx { | ||
| i.commitQCs.pushBack(commitQC) | ||
| metrics.ObserveCommitQC(c, commitQC) |
There was a problem hiding this comment.
[suggestion] prune() now emits metrics, but newInner() also calls prune() when applying a persisted prune anchor during restart (inner.go:92). ObserveCommitQC observes proposal_to_commit_latency unconditionally on its first call (metrics.go: the now.Sub(qc.Proposal().Timestamp()) observe is outside the last, ok guard), so a restart records a single spurious sample of now - anchorProposalTimestamp, which can be arbitrarily large if the anchor QC is old / the node was down for a while. The road-index / global-block-number gauges are fine to set here (they reflect current state), but the latency histogram sample from the persisted anchor is not a real live-consensus observation. Consider skipping the latency observation (or all observations) when replaying the persisted anchor in newInner.
| MaxTxsPerBlock: 5_000, | ||
| AllowEmptyBlocks: true, | ||
| MaxTxsPerBlock: 2_000, | ||
| AllowEmptyBlocks: false, |
There was a problem hiding this comment.
[suggestion] This flips the generated-config default AllowEmptyBlocks from true to false (and MaxTxsPerBlock 5000→2000), which is a producer-behavior change unrelated to the metrics scope of this PR — with AllowEmptyBlocks=false a freshly generated config no longer advances during idle periods. If intentional, please confirm and call it out in the PR description/changelog; otherwise it looks like an accidental inclusion.
There was a problem hiding this comment.
Large, mostly-mechanical refactor moving per-instance metrics to package-level Global singletons (metricsgen pattern) and adding new autobahn/avail, autobahn/data and p2p/mux metrics. The refactor is internally consistent, but it drops the *NodeMetrics parameter from node.New and deletes NoOpMetricsProvider/DefaultMetricsProvider/NodeMetrics without updating three callers, breaking the build.
Findings: 1 blocking | 5 non-blocking | 1 posted inline
Blockers
- Build break:
node.Newlost its*NodeMetricsparameter andnode.NoOpMetricsProvider/DefaultMetricsProvider/NodeMetricswere removed, but callers outside the diff were not updated.sei-tendermint/test/e2e/node/main.go:125and:149still pass an extra trailingnil(8 args to the now 7-argnode.New) — this ispackage mainwith no build tag, sogo build ./.../go vet ./...and CI will fail to compile.inprocess/harness.go:385-388still calls the deletedtmnode.NoOpMetricsProvider()and passes the extra arg (gated behind//go:build inprocess, so only breaks tagged builds, but still references a deleted symbol). Update all three call sites to the new signature. (Confirmed by Codex; verified via source inspection — the symbols no longer exist insei-tendermint/node/.)
Non-blocking
- Cursor review file (
cursor-review.md) was empty — that second-opinion pass produced no output. - Behavior change worth confirming: metrics are now registered on the default Prometheus registry unconditionally in each package's
init(), whereas the oldDefaultMetricsProvideronly wired real metrics whencfg.Instrumentation.Prometheuswas set (otherwise no-op). After this PR thePrometheusconfig flag no longer gates metric registration/collection. Confirm this is intended. - Test isolation: tests that previously constructed fresh
NewMetrics()per node now share process-globalGlobalmetrics. Fine for counters/gauges that aren't asserted on, but any future test asserting absolute metric values could see cross-test accumulation. - Unrelated changes bundled into a metrics PR:
gen_autobahn_config.godefaultMaxTxsPerBlock5000→2000 andAllowEmptyBlockstrue→false, and therpc.Registersignature gaining anameargument. These are reasonable but orthogonal to the metrics work; a note in the PR description would help reviewers. - 1 suggestion(s)/nit(s) flagged inline on specific lines.
There was a problem hiding this comment.
A large, mostly mechanical refactor that moves per-instance Tendermint metrics to package-level Global singletons and adds new Prometheus metrics for autobahn/avail, autobahn/data, and p2p/mux. The refactor is consistent, but it removes the nodeMetrics parameter from node.New without updating the e2e node binary, which will fail to compile.
Findings: 1 blocking | 4 non-blocking | 1 posted inline
Blockers
- Compile break: this PR removes the
nodeMetrics *NodeMetricsparameter fromnode.New(sei-tendermint/node/public.go), butsei-tendermint/test/e2e/node/main.go(not touched by the PR) still callsnode.New(...)with the old 8-argument form at lines 125 and 149 (extranilin the removed metrics slot). That file ispackage mainin the same Go module with no build tags, sogo build ./.../go vet ./...(CI +make lint) will fail. Update both calls to drop the extra argument. (Independently reported by Codex; Codex's additional reference toinprocess/harness.go/NoOpMetricsProvidercould not be reproduced — no such file/symbol remains in the tree.)
Non-blocking
- Cursor produced no second-opinion output (cursor-review.md is empty), and REVIEW_GUIDELINES.md is empty, so no repo-specific guidelines were applied.
- Design note: metrics are now registered on the default Prometheus registry unconditionally in each package's init() (via Global = newMetrics()), whereas previously DefaultMetricsProvider only wired up real collectors when cfg.Prometheus was enabled. This is intentional for the refactor, but means tests now share global metric state instead of a fresh NewMetrics() per test — fine as long as no test asserts on absolute metric values.
- Metric names change as a side effect (e.g. data latency moves from
sei_data__latencytotendermint_internal_autobahn_data_latency); existing dashboards/alerts will need updating. Consistent with the PR's non-app-hash-breaking scope but worth calling out to operators. - 1 suggestion(s)/nit(s) flagged inline on specific lines.
There was a problem hiding this comment.
A large, mostly-mechanical refactor moving Tendermint metrics from per-instance *Metrics to package-level Global singletons and dropping the metrics params/providers. The migration is consistent across default-compiled code, but one build-tagged caller was missed, leaving the inprocess build broken.
Findings: 1 blocking | 3 non-blocking | 0 posted inline
Blockers
- Build break (confirms Codex):
inprocess/harness.go:387still callstmnode.New(..., tmnode.NoOpMetricsProvider(), ...), but this PR deletesNoOpMetricsProvider/NodeMetricsand removes the metrics parameter fromnode.New(now 7 params).NoOpMetricsProviderno longer exists anywhere in the repo (verified: this is the sole remaining reference), so theinprocess-tagged build fails to compile. It escapes default CI becauseharness.gois behind//go:build inprocess, but the benchmark harness andintegration_test/runner(both use theinprocesstag) will not build. Fix: drop thetmnode.NoOpMetricsProvider()argument, matching the updates already made torpc/test/helpers.goandtest/e2e/node/main.go.
Non-blocking
- Cursor's second-opinion review (
cursor-review.md) was empty — that pass produced no findings.REVIEW_GUIDELINES.mdwas also empty, so no repo-specific standards were applied. - Nit: in
sei-tendermint/internal/p2p/mux/metrics/metrics.gothe struct tags usemetrics_labels:"role, kind"(space after comma), inconsistent with other new files likedata/metrics/metrics.gowhich use"resource,stage". metricsgen trims the space (the generated[]string{"role", "kind"}is correct), so it's harmless — just a consistency nit. - The migration to
Globalsingletons means all in-process instances (e.g. multiple nodes/reactors in a single test binary) now share the same process-global metric series instead of per-instance ones. This matches how Prometheus metrics behave in production and is almost certainly intended, but worth confirming no test asserts on per-instance metric isolation.
3 participants
It will give us insight into consensus state and rpc performance.