Skip to content

Debounce postgres top-N query metrics - #18

Merged
sergeyfast merged 2 commits into
masterfrom
fix/pg-query-topn-churn
Aug 4, 2026
Merged

sergeyfast merged 2 commits into
masterfrom
fix/pg-query-topn-churn

Conversation

@sergeyfast

@sergeyfast sergeyfast commented Aug 4, 2026 •

Copy link
Copy Markdown
Member

Summary

  • Per-query metrics churned the per-day index: every unique queryid entering the top-N mints one series per topsrv_pg_query_* metric, and the top-set turned over on nearly every scrape. Two independent causes, both fixed here.
  • Root cause fix — selectTopStatements now rejects non-positive deltas. Ranking runs on per-scrape deltas, but nothing filtered them, and sort order among all-zero deltas is arbitrary (pdqsort is not stable). On a first scrape, an idle database, or right after pg_stat_statements_reset(), that pulled up to 20 random queryids per dimension into the top-set on every scrape — minting fresh series from a database doing no work at all. The guard lives inside the shared helper, so both the metrics top-20 and the meta push top-30 get it.
  • Debounce — per-query metrics emit only after a statement holds a top-20 spot on two consecutive scrapes (debounceTop). Covers the residual case the guard cannot: migrations, tests and ad-hoc statements legitimately top a dimension for exactly one scrape, and each one costs a permanent series. The meta push keeps its undebounced top-30 — query texts are not TSDB series, and the wider window still helps the UI resolve queries hovering on the boundary.
  • Fix data race in the statements phase — a.registry is Gathered by both the /metrics handler (app.go:180) and the push ticker (push.go:167), so Collect calls overlap and wrote prevStmts and histBuckets concurrently. That is fatal error: concurrent map writes, which instrumentedCollector's recover() cannot catch — the process dies. Pre-existing, but this PR adds state that depends on ordered once-per-scrape calls.
  • Perf — rank an index slice once and fill both widths from it, instead of sorting copies of stmtCurrent five times per call and calling it twice per scrape. At 5000 statements: 9.8 ms / 10.4 MB → 1.5 ms / 30 KB per scrape.

Design

  • TryLock rather than Lock on the statements phase: the loser skips instead of waiting. With a plain Lock it would run immediately after the winner, recompute deltas over a near-zero window, produce an empty top-set, and wipe prevTopKeys — blinding the next two scrapes on every pull/push collision.
  • prevTopKeys is replaced wholesale each scrape, so it is self-pruning and hard-bounded at 5 dimensions × 20 = 100 keys. It holds no stmtCurrent (208 B each, including query text) and no reference to the scan slice.
  • The <= 0 guard is deliberately not a "fallback to cumulative values on first scrape". Cumulative ranking pulls in long-lived queries with large historical weight but no current activity — exactly the churn this PR removes.

Behaviour change

Both documented in docs/metrics.md under How topsrv_pg_query_* statements are selected:

  • A query that alternates in and out of the top-20 never emits. A one-scrape memory has nothing to carry it across the gap. Deliberate — a statement this marginal is not worth a permanent series. Pinned by TestDebounceTopNeverEmitsAlternatingStatement.
  • After a topsrv restart or a stats reset, the first per-query metrics appear on the third scrape: the first builds the delta baseline, the second fills the top-set, the third emits. At Interval = "30s" that is a 60–90 s gap. The aggregate topsrv_pg_query_duration_seconds histogram is fed from every statement regardless of ranking, so latency percentiles have no such gap.

Test plan

  • go test -race ./... — green in CI (test check, 2m17s)
  • make test-integration — postgres / nginx / angie / smart / botlog green; also re-run under -race, no races
  • 5 unit tests in new internal/topsrv/postgres/statements_test.go — non-positive deltas, wal_bytes gating, metrics-vs-meta widths, debounce cycle, alternating statement
  • TestIntegrationPostgres extended to 3 scrapes and now asserts topsrv_pg_query_calls_total is present — without that assert the debounce could suppress per-query metrics forever and the test would stay green
  • Race regression verified by removing the guard: 4 WARNING: DATA RACE on prevStmts / histBuckets, gone once restored. Note CI runs go test -race without the integration tag, so TestIntegrationPostgresConcurrentScrape does not run there — it needs make test-integration
  • Benchmark A/B against the HEAD implementation, 5000 statements, -benchtime 200x -count 5: old 9.8 ms / 10.4 MB / 18 allocs, new 1.5 ms / 30 KB / 9 allocs
  • After deploy: confirm the per-day index series count for topsrv_pg_query_* stops growing, and that a known-hot query still reports within 3 scrapes of an agent restart

Note on make lint: 51 issues (50 goconst, 1 sloglint), all pre-existing on master and all in files this PR does not touch — verified by running the same linter on a clean master worktree. CI does not run golangci-lint (it runs go vet + staticcheck), and the local linter version is newer than what the repo was last cleaned against. Not addressed here to keep the diff scoped.

Out of scope

  • The debounce counts Collect() calls, not wall-clock time. With both a listener and a pusher configured, the window depends on who Gathered last, and its strength scales with Push.Interval. Neighbouring state in the package (appNamesTTL, bloatRefreshInterval, settingRefreshInterval) is deliberately time-bounded; this should be too. The real fix is caching a collection for a minimum interval, which is an all-collectors change.
  • The debounce is unobservable — no log line, no counter for suppressed statements. A topsrv_pg_statements_debounced gauge would make it diagnosable, but that is new public metric surface for a patch release.

- Reject non-positive deltas in top-N selection: with all-zero deltas
  (first scrape, idle database, pg_stat_statements reset) sort order
  was arbitrary and pulled random queryids into the top-set, minting
  fresh series on every scrape of an otherwise idle database
- Emit per-query metrics only after a statement holds a top-20 spot on
  two consecutive scrapes, keeping one-off queries out of the per-day
  index; the meta push keeps its undebounced top-30
- Fix data race in the statements phase: the registry is Gathered by
  both the /metrics handler and the push ticker, so overlapping Collect
  calls wrote prevStmts and histBuckets concurrently, which is a fatal
  error that instrumentedCollector's recover cannot catch
- Rank an index slice once for both widths instead of sorting copies of
  stmtCurrent five times per call, twice per scrape: 9.8ms and 10.4MB
  per scrape down to 1.5ms and 30KB at 5000 statements
- Document the selection rule in docs/metrics.md: a flapping query
  never emits, and per-query metrics reappear on the third scrape
  after a restart
- Add release.prerelease=auto to .goreleaser.yml. Without it a -rc tag
  publishes as a full release and GitHub marks it "Latest", displacing
  the last stable one — agents on the stable channel would then
  auto-update onto a release candidate
- The release job fires on any v* tag, so nothing else gated this. Two
  earlier rc tag series left no GitHub releases behind, which suggests
  they were published and then deleted by hand
@sergeyfast
sergeyfast merged commit 5b8d851 into master Aug 4, 2026
2 checks passed
@sergeyfast
sergeyfast deleted the fix/pg-query-topn-churn branch August 4, 2026 15:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant