Skip to content

Avoid deltas + simplify pebble metrics - #4100

Open
alrevuelta wants to merge 1 commit into
mainfrom
simplify-pebble-metrics
Open

Avoid deltas + simplify pebble metrics#4100
alrevuelta wants to merge 1 commit into
mainfrom
simplify-pebble-metrics

Conversation

@alrevuelta

Copy link
Copy Markdown
Contributor

Describe your changes and provide context

This PR simplifies Pebble metrics. It drops the scrape-loop trick of storing the last Pebble totals in a wall of prev* fields and adding only the difference into OTel counters, and instead observes each db.Metrics() snapshot directly.

In other words, now we directly copy Pebble metrics from db.Metrics, instead of artificially keeping the prev values and adding them in each call. Less bug prone.

Testing performed to validate your change

  • Tested in a live dashboard where I monitor most of Pebble metrics.

@cursor

cursor Bot commented Sep 4, 2026

Copy link
Copy Markdown

PR Summary

Medium Risk
Observability behavior changes for counter-style Pebble metrics (cumulative observables vs. delta-scraped counters), which can break rate queries or dashboards until validated; shutdown ordering is improved but still touches metrics on DB close.

Overview
Replaces the Pebble metrics scraper with a smaller OTel observable callback model: a background ticker refreshes an atomic db.Metrics() snapshot, and registered Float64ObservableCounter/Gauge instruments read from that snapshot on export.

Stops synthesizing counter deltas via large prev* state and addDelta; cumulative Pebble totals are exposed directly as observable counters (same metric names, different export semantics vs. per-scrape increments). Gauges and per-level series (db / level attributes) are unchanged in naming.

NewPebbleMetrics API change: no context argument; it returns a func() shutdown that stops the refresher, waits for it to exit, and unregisters the callback so Close can tear down the DB safely. Plain pebbledb.Open and MVCC OpenDB wire that into existing metricsCancel on close.

Large deletion in pebble_metrics.go (operation latency histograms that lived only in this scraper, e.g. pebble_get_latency / batch / queue depth) — MVCC still records those via mvcc/metrics.go.

Reviewed by Cursor Bugbot for commit 6870f6b. Bugbot is set up for automated code reviews on this repo. Configure here.

@github-actions

github-actions Bot commented Sep 4, 2026

Copy link
Copy Markdown

The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).

BuildFormatLintBreakingUpdated (UTC)
✅ passed✅ passed✅ passed✅ passedSep 4, 2026, 12:15 PM

@codecov

codecov Bot commented Sep 4, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 51.47059% with 165 lines in your changes missing coverage. Please review.
✅ Project coverage is 60.00%. Comparing base (df02548) to head (6870f6b).

Files with missing lines Patch % Lines
sei-db/db_engine/pebbledb/pebble_metrics.go 50.89% 25 Missing and 140 partials ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #4100      +/-   ##
==========================================
- Coverage   61.26%   60.00%   -1.27%     
==========================================
  Files        2188     2079     -109     
  Lines      192239   178338   -13901     
==========================================
- Hits       117779   107008   -10771     
+ Misses      63298    61107    -2191     
+ Partials    11162    10223     -939     
Flag Coverage Δ
sei-chain-pr 64.41% <51.47%> (?)
sei-db 69.80% <ø> (ø)
sei-db-state-db ?

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
sei-db/db_engine/pebbledb/db.go 74.33% <100.00%> (ø)
sei-db/db_engine/pebbledb/mvcc/db.go 67.89% <100.00%> (-0.09%) ⬇️
sei-db/db_engine/pebbledb/pebble_metrics.go 51.03% <50.89%> (-48.65%) ⬇️

... and 109 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@seidroid seidroid Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Solid simplification: the ~1700-line delta-tracking scrape loop is replaced by OTel observable instruments over an atomically-refreshed pebble.Metrics snapshot, and the removed latency/batch instruments were dead duplicates of the live ones in mvcc/metrics.go, so no metric is actually lost. Field mappings and the Close-before-db.Close() ordering check out; only non-blocking notes on the now-dead ctx parameter, two changed instrument kinds, and absent lifecycle tests.

Findings: 0 blocking | 3 non-blocking | 2 posted inline

Blockers

  • None at the file/PR level.

Non-blocking

  • [suggestion] No test covers the new lifecycle, which is the part with real failure modes: that the returned stop func is idempotent (sync.OnceFunc), that it waits for the refresher goroutine before returning so db.Close() is safe immediately after, and that a RegisterCallback failure degrades to a no-op stop func rather than a nil call. A small test in sei-db/db_engine/pebbledb opening a DB with EnableMetrics: true, calling the stop func twice, then closing, would pin all three cheaply.
  • 2 suggestion(s)/nit(s) flagged inline on specific lines.


// Open opens (or creates) a Pebble-backed DB at path, returning a KeyValueDB
// Open opens (or creates) a Pebble-backed DB at path, returning a KeyValueDB.
// ctx is unused: metrics collection is stopped by Close, not by cancellation.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[suggestion] ctx is now entirely unused in Open — it was only ever wired to the metrics goroutine's cancellation. Documenting a parameter as ignored leaves a trap rather than removing it: every one of the ~16 call sites (s.ctx in flatkv/store.go:761, t.Context() in several tests, context.Background() elsewhere) still reads as if cancelling that context releases the DB's background work, and it no longer does. Since Close is now the single choke point for stopping collection, dropping the parameter from the signature would make that invariant unmissable instead of a comment callers have to find. If you prefer to keep the signature stable for now, consider renaming it _ context.Context so the compiler-visible intent matches the comment.

Relatedly, metricsCancel is still typed context.CancelFunc (line 24) though it no longer comes from a context; plain func() would match what it now holds.

func (p *pebbleMetrics) declareDB() {
p.counter("pebble_compaction_count", "{count}", "Total number of compactions",
func(m *pebble.Metrics) float64 { return float64(m.Compact.Count) })
p.counter("pebble_compaction_duration", "s", "Cumulative compaction duration since DB open",

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[suggestion] pebble_compaction_duration (and pebble_flush_duration on line 171) change instrument kind from Float64Histogram to Float64ObservableCounter. Recording a cumulative value into a histogram was meaningless, so this is the right fix — but it renames the exported Prometheus series (pebble_compaction_duration_seconds_bucket/_sum/_countpebble_compaction_duration_seconds_total), so any existing dashboard panel or alert on these two will silently go empty rather than error. Every other metric keeps a compatible type, so it's worth calling out these two specifically in the PR description or a dashboard follow-up.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant