fix(seidb-metrics): use shared fine-grained buckets for memiavl/flatkv/mvcc histograms#3771
fix(seidb-metrics): use shared fine-grained buckets for memiavl/flatkv/mvcc histograms#3771blindchaser wants to merge 1 commit into
Conversation
…v/mvcc histograms The memiavl, flatkv, and pebble MVCC latency histograms used the OTel default bucket boundaries (5s, 10s, 25s, ...), which are useless for percentile queries over millisecond-scale per-block operations: every sample lands in the first bucket, so Grafana p95/p99 cannot resolve. Apply the shared smetrics.LatencyBuckets (10us-5min) to the latency histograms, and ByteSizeBuckets/CountBuckets to the mvcc batch-size and iterator histograms, matching the pattern already used by pebbledb/pebble_metrics.go, storev2 rootmulti, and app metrics. Co-authored-by: Cursor <cursoragent@cursor.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 #3771 +/- ##
==========================================
- Coverage 59.92% 59.47% -0.46%
==========================================
Files 2288 2235 -53
Lines 190180 185586 -4594
==========================================
- Hits 113971 110370 -3601
+ Misses 66049 65385 -664
+ Partials 10160 9831 -329
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9f82ba98e5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| "flatkv_import_latency", | ||
| metric.WithDescription("Time taken to import FlatKV snapshot data"), | ||
| metric.WithUnit("s"), | ||
| metric.WithExplicitBucketBoundaries(smetrics.LatencyBuckets...), |
There was a problem hiding this comment.
For state-sync imports that run longer than 5 minutes, smetrics.LatencyBuckets tops out at 300s (sei-db/common/metrics/buckets.go), so every multi-minute/hour import lands only in the +Inf bucket and histogram_quantile cannot distinguish a 6-minute import from a multi-hour one. The migration docs explicitly note snapshot export/import can take hours when state grows (docs/migration/seidb_migration.md), so flatkv_import_latency should keep longer/default boundaries or use a separate long-operation bucket set while the per-block latencies use the fine-grained buckets.
Useful? React with 👍 / 👎.
![seidroid[bot]](https://avatars.githubusercontent.com/in/2745135?s=60&v=4){kind=small}
There was a problem hiding this comment.
A low-risk, mechanical change applying shared fine-grained histogram buckets to memiavl/flatkv/mvcc latency and size metrics, correctly mirroring existing patterns with no name/label changes. One metric (pebble_batch_size) gets byte-scaled buckets while it actually records an operation count, which undermines resolution for that specific histogram.
Findings: 0 blocking | 3 non-blocking | 1 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Cursor's second-opinion review (cursor-review.md) was empty — that pass produced no output. Codex contributed the one substantive finding, which is captured inline.
- Optional: consider whether all remaining latency histograms in the three files are covered; the diff appears complete, but a quick grep for Float64Histogram without WithExplicitBucketBoundaries in these files would confirm none were missed.
- 1 suggestion(s)/nit(s) flagged inline on specific lines.
| "pebble_batch_size", | ||
| metric.WithDescription("Size of batches written to PebbleDB"), | ||
| metric.WithUnit("By"), | ||
| metric.WithExplicitBucketBoundaries(smetrics.ByteSizeBuckets...), |
There was a problem hiding this comment.
[suggestion] pebble_batch_size records int64(len(ops)) (an operation count) at batch.go:170, not bytes, yet this assigns ByteSizeBuckets (256B–1GB). Batches almost always have far fewer than 256 ops, so every sample falls in the lowest bucket — reproducing the exact p95/p99 resolution failure this PR aims to fix. The metric's unit is also "By" while it records a count. Either use smetrics.CountBuckets to match the current recorded value, or record the actual batch byte size (e.g. batch.Len()) to match the By unit and the "Size of batches" description.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| "pebble_batch_size", | ||
| metric.WithDescription("Size of batches written to PebbleDB"), | ||
| metric.WithUnit("By"), | ||
| metric.WithExplicitBucketBoundaries(smetrics.ByteSizeBuckets...), | ||
| )), | ||
| pendingChangesQueueDepth: must(meter.Int64Gauge( | ||
| "pebble_pending_changes_queue_depth", |
There was a problem hiding this comment.
🟡 This PR adds smetrics.ByteSizeBuckets (256B–1GB) to the pebble_batch_size histogram, but the only recording site (writeBatchOps in batch.go) records batchSize := int64(len(ops)) — a count of batch operations, not a byte size. Since op counts (tens–thousands) fall far below the first ByteSizeBuckets boundary of 256, nearly every sample will land in the lowest bucket, reproducing the exact histogram_quantile resolution problem this PR is fixing elsewhere; it should use smetrics.CountBuckets instead, matching the analogous iteratorIterations metric two lines below.
Extended reasoning...
The bug: metrics.go:165 applies metric.WithExplicitBucketBoundaries(smetrics.ByteSizeBuckets...) to the pebble_batch_size histogram (otelMetrics.batchSize, an Int64Histogram). ByteSizeBuckets (defined in sei-db/common/metrics/buckets.go:17-20) is [256, 1KB, 4KB, 16KB, 64KB, 256KB, 1MB, 4MB, 16MB, 64MB, 256MB, 1GB] — designed for byte-size data.
Where it's recorded: The only call site is writeBatchOps in sei-db/db_engine/pebbledb/mvcc/batch.go:169-178:
batchSize := int64(len(ops))
...
otelMetrics.batchSize.Record(ctx, batchSize)ops is the []batchOp slice passed in — one entry per Set/Delete call being written in that pebble batch. So the value recorded is an operation count, not a byte size, despite the metric's unit (By) and description ('Size of batches written to PebbleDB') suggesting otherwise. That unit/name mismatch pre-dates this PR, but the bucket-boundary choice is new in this diff and is what actually determines whether the histogram is usable.
Why this defeats the PR's own goal: Real-world batch op counts per commit are typically in the tens-to-low-thousands range — well below the first ByteSizeBuckets boundary of 256. That means nearly all samples fall into the le="256" bucket, and histogram_quantile cannot resolve p95/p99 for this metric — precisely the 'everything piles into one bucket' failure mode this PR's description says it's fixing for the other histograms.
Step-by-step proof:
- A block commits a changeset with, say, 40 Set/Delete ops →
writeBatchOpsis called withlen(ops) == 40. batchSize := int64(len(ops))→batchSize = 40.otelMetrics.batchSize.Record(ctx, 40)records the value40into a histogram bucketed at[256, 1024, 4096, ...].40 < 256, so this sample (and virtually every other realistic sample, since op counts rarely exceed a few thousand) lands in thele="256"bucket.- Querying
histogram_quantile(0.95, rate(pebble_batch_size_bucket[5m]))returns ≈256 (or whatever the lowest boundary is) regardless of the actual distribution of batch sizes, because all the mass is in one bucket — no resolution.
The fix: Use smetrics.CountBuckets ([1, 5, 10, 50, 100, 500, 1000, 5000, 10000, 100000, 1000000]) instead of ByteSizeBuckets for this histogram, matching the pattern already correctly applied two lines below to iteratorIterations — a genuinely analogous per-operation count metric that uses CountBuckets.
Severity: This is an observability/dashboard-resolution issue only — no functional/runtime impact, no crash, no data loss. It just means this one specific histogram (pebble_batch_size) won't have useful percentiles, which is a quality gap relative to the PR's stated intent but doesn't block any behavior. Marking as nit.
1 participant
Summary
0, 5, 10, 25, ...seconds. Per-block operations are millisecond-scale, so every sample lands in the lowest bucket and Grafanahistogram_quantilep95/p99 cannot resolve (verified against a live pacific-1 node's:26660/metrics).smetrics.LatencyBuckets(10us-5min) to those latency histograms, plusByteSizeBuckets/CountBucketsto the mvccpebble_batch_sizeandpebble_iterator_iterationshistograms.pebbledb/pebble_metrics.go,storev2/rootmulti/metrics.go, andapp/metrics.go; no metric names or labels change, only bucket layout.Motivation: the gas-repricing storage measurements (docs/storage work) need backend p99s; today only
_sum/_countinterval means are usable.Test plan
go build ./sei-db/...go test ./sei-db/state_db/sc/memiavl/ ./sei-db/state_db/sc/flatkv/ ./sei-db/db_engine/pebbledb/...gofmt -s/goimportscleanMade with Cursor