Avoid deltas + simplify pebble metrics - #4100
Conversation
PR SummaryMedium Risk Overview Stops synthesizing counter deltas via large
Large deletion in Reviewed by Cursor Bugbot for commit 6870f6b. 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 #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
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
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 sodb.Close()is safe immediately after, and that aRegisterCallbackfailure degrades to a no-op stop func rather than a nil call. A small test insei-db/db_engine/pebbledbopening a DB withEnableMetrics: 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. |
There was a problem hiding this comment.
[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", |
There was a problem hiding this comment.
[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/_count → pebble_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.
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 theprevvalues and adding them in each call. Less bug prone.Testing performed to validate your change