1
0
mirror of https://github.com/prometheus-community/postgres_exporter.git synced 2026-10-23 12:08:57 +03:00
Files
postgres_exporter/collector/instance_test.go
Matthew Wimpelberg 637fd77e36 Run database-scoped collectors against every discovered database (#1378)
* Run database-scoped collectors against every discovered database

collector.NewRuntime only ever built one PostgresCollector, bound to
DataSourceNames[0]. This meant per-database collectors like
stat_user_tables, statio_user_tables and statio_user_indexes silently
never ran for any database beyond the first configured DSN, whether
that list came from a static, comma-separated DATA_SOURCE_NAME or
from --auto-discover-databases.

Naively running every collector against every discovered database
isn't safe either: most collectors (bgwriter, wal, stat_activity,
stat_statements, locks, ...) read catalog views or functions that
already report on the whole instance from any single connection, so
running them again from a second database would re-report the same
rows and the registry would panic on the duplicate metric (as already
happened once, for stat_activity, in #533).

This adds an explicit scope to each registered collector (clusterScope
vs databaseScope) and a multiInstanceCollector that fans the
database-scoped ones out across every DSN exporter.Exporter currently
targets (static list or, with autodiscovery enabled, freshly
enumerated databases), while cluster-scoped collectors still only ever
run once, against the originally configured DSN. The target list is
re-evaluated on every scrape, so databases created or dropped after
startup are picked up without restarting the exporter, matching the
autodiscovery behavior already used by the legacy exporter.Exporter
metrics.

Also gives statio_user_indexes a datname label, matching its sibling
per-database collectors: without it, index metrics from two databases
with a same-named index would collide once run concurrently.

Signed-off-by: Matthew Wimpelberg <120263653+mwimpelberg28@users.noreply.github.com>

* Address review: rename cluster scope to server, drop multiInstanceCollector

sysadmind flagged two issues in review: PostgreSQL doesn't have a "cluster",
so collectorScope should say "server" instead; and the multiInstanceCollector
built a full server+database collector set for every explicitly configured
DSN, which panics the registry if two of those DSNs turn out to be the same
physical server (e.g. two databases on one server, each listed separately in
DATA_SOURCE_NAME).

Follows the plan from #1326 instead: exactly one primary connection
(DataSourceNames[0]) ever runs server-scoped collectors, so they can never be
duplicated no matter how many databases are discovered. PostgresCollector
gains a WithDatabaseDiscovery option that, when auto-discovery is enabled,
queries pg_database on that same primary connection and runs only the
database-scoped collectors against every other database found, over its own
connection, reconciled on every scrape.

This drops collector's dependency on exporter.Target/TargetDSNs, since
discovery is now entirely self-contained within the collector package.

Signed-off-by: Matthew Wimpelberg <120263653+mwimpelberg28@users.noreply.github.com>

* Address review: fold database discovery into PostgresCollector, fix datname collisions

sysadmind flagged that database_discovery.go duplicated DSN-rewriting logic
that config.DSN already owns, and that the whole databaseDiscovery type
(its own collect loop, cache of *PostgresCollector, mutex) duplicated what
PostgresCollector already knows how to do: distinguish server- from
database-scoped collectors and run them.

config.DSN gains WithDatabase, built on its existing parser, and instance
gains withDatabase to use it, so no connection-string parsing is duplicated.
databaseDiscovery, buildDatabaseCollector, and onlyScope/scopeFilter are all
gone; collectFromConnection takes an optional scope filter so the single
PostgresCollector can run every collector against its primary connection and
just the database-scoped ones against each discovered database. No state
needs to persist across scrapes: the primary connection already opens and
closes a fresh sql.DB every scrape, so discovered-database connections do
the same instead of being cached.

Also fixes the brittle "unknown" datname fallback in pg_statio_user_indexes:
datname comes from current_database(), which Postgres guarantees is never
NULL, so it's scanned as a plain string instead of falling back to a shared
sentinel that could collide across databases.

Testing locally (as sysadmind asked) surfaced a related bug the redesign
didn't originally cover: pg_scrape_collector_duration_seconds and
pg_scrape_collector_success carried no datname label, so running the same
collector against multiple databases produced the exact duplicate-metric
registry error from sysadmind's repro. Fixed by adding a datname label to
both, sourced from a new instance.database field populated via
current_database() during setup().

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Signed-off-by: Matthew Wimpelberg <120263653+mwimpelberg28@users.noreply.github.com>

* Address review: bound discovery concurrency, fix socket DSNs, gate statio_user_indexes datname

ArthurSens and Copilot's latest review pass flagged four issues in the
database-discovery path added by this PR:

- collectDiscoveredDatabases launched one goroutine per discovered
  database with no bound, so a server with many databases could open more
  connections than it can spare in a single scrape. It's now bounded by a
  semaphore sized by a new AutoDiscoverDatabasesMaxConcurrency config field
  (--auto-discover-databases.max-concurrency, default 10).

- The per-database goroutine called instance.setup() with no context at
  all, so a stalled connect/auth could block wg.Wait() well past
  CollectionTimeout. setup() (and queryVersion) now take a context, and
  each discovered database gets one deadline covering both connecting and
  scraping.

- config.DSN.WithDatabase always serialized through GetConnectionString's
  URL form, but pq's own URL parser rejects a percent-encoded "/" in the
  host — so any discovered-database connection built from a Unix-socket
  primary DSN (the documented `host=/var/run/postgresql/` form) produced a
  DSN string sql.Open couldn't even parse. GetConnectionString now detects
  a socket-shaped host and falls back to key=value serialization, the only
  form pq accepts for it.

- pg_statio_user_indexes gained an unconditional datname label last round
  to fix a duplicate-metric collision, but that collision can only happen
  once database discovery runs the collector concurrently against more
  than one database. Installs that never enable
  --auto-discover-databases got a new label on an existing metric for no
  reason. The label (on this collector and on the
  pg_scrape_collector_duration_seconds/success meta-metrics added for the
  same reason) is now only added when WithDatabaseDiscovery is active.

Separately, ArthurSens and Copilot also asked why NewRuntime still only
passes DataSourceNames[0] into the native PostgresCollector, so a
comma-separated multi-server DATA_SOURCE_NAME still only gets
database-scoped collectors on the first server. That line is unchanged
from master and predates this PR — it's a pre-existing limitation of the
native collector framework (this PR only fans database-scoped collectors
out across databases discovered on a single server), not something to fix
here. Replying on the review thread to flag it as a good follow-up instead.

Assisted-by: Claude <noreply@anthropic.com>
Signed-off-by: Matthew Wimpelberg <120263653+mwimpelberg28@users.noreply.github.com>

* Address review: enforce CollectionTimeout across discovery batches

collectDiscoveredDatabases gave each batch its own fresh
context.WithTimeout(context.Background(), ...), so a scrape could run well
past CollectionTimeout once enough batches of discovered databases had run.
Thread a single ctx through the whole call via a new forEachDatabase
worker-pool helper so the deadline actually bounds the total time.

Signed-off-by: Matthew Wimpelberg <120263653+mwimpelberg28@users.noreply.github.com>
Assisted-by: Claude <noreply@anthropic.com>

* Address review: replace DataSourceNames []string with a single DataSourceName

DataSourceNames[0] has been hardcoded wherever the registry-based collectors
(and /probe) pick a primary connection, ever since database auto-discovery
was added; the array never actually enabled discovery across multiple
servers there. Meanwhile the legacy Exporter genuinely iterated the whole
slice to scrape multiple independent servers from one process via a
comma-separated DATA_SOURCE_NAME.

Collapse this into a single DataSourceName string everywhere, removing the
ambiguity for good. This is a breaking change: DATA_SOURCE_NAME containing a
comma is now a startup error. Multi-server scraping from a single exporter
is the job of the multi-target /probe endpoint, not this field.

Signed-off-by: Matthew Wimpelberg <120263653+mwimpelberg28@users.noreply.github.com>
Assisted-by: Claude <noreply@anthropic.com>

* Address review: drop env var for auto-discover-databases.max-concurrency

Per review feedback, new flags shouldn't gain env var bindings going
forward since they aren't required for security reasons. Keep the flag
CLI-only and drop the README mention of PG_EXPORTER_AUTO_DISCOVER_DATABASES_MAX_CONCURRENCY.

Signed-off-by: Matthew Wimpelberg <120263653+mwimpelberg28@users.noreply.github.com>
Assisted-by: Claude <noreply@anthropic.com>

* Revert "Address review: replace DataSourceNames []string with a single DataSourceName"

This reverts commit b19a55eea484bd7bf9565dd9e7f2e5bbb18c7643.

Per @ArthurSens on #1378: the legacy exporter package genuinely depends on
DATA_SOURCE_NAME accepting a comma-separated list to scrape multiple
independent servers from a single process. Collapsing it to a single DSN
would silently break that path for existing users. Go back to accepting
the array here; the actual deprecation (if any) should be its own,
intentional follow-up PR.

Signed-off-by: Matthew Wimpelberg <120263653+mwimpelberg28@users.noreply.github.com>
Assisted-by: Claude <noreply@anthropic.com>

* Address review: extract collect closure, skip redundant version query

Pull the closure passed to forEachDatabase out into a named
collectDatabase variable for readability, and have withDatabase carry
over the primary's already-known version so setup() skips re-querying
it for each discovered database on the same server.

Signed-off-by: Matthew Wimpelberg <120263653+mwimpelberg28@users.noreply.github.com>
Assisted-by: Claude <noreply@anthropic.com>

---------

Signed-off-by: Matthew Wimpelberg <120263653+mwimpelberg28@users.noreply.github.com>
Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
2026-10-01 16:05:43 -04:00

60 lines
2.0 KiB
Go

// Copyright The Prometheus Authors
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.
package collector
import (
"testing"
"github.com/blang/semver/v4"
)
func TestInstanceWithDatabase(t *testing.T) {
tests := []struct {
name string
dsn string
want string
wantErr bool
}{
{name: "postgresql URI", dsn: "postgresql://user:pass@localhost:5432/postgres?sslmode=disable", want: "postgresql://user:pass@localhost:5432?dbname=other&sslmode=disable"},
{name: "postgres URI", dsn: "postgres://localhost/postgres", want: "postgres://localhost?dbname=other"},
{name: "connstring", dsn: "host=localhost port=5432 dbname=postgres", want: "postgresql://localhost:5432?dbname=other"},
{name: "unparsable", dsn: "not a dsn", wantErr: true},
}
for _, test := range tests {
t.Run(test.name, func(t *testing.T) {
primary := &instance{dsn: test.dsn, wrapLargeCounters: true, version: semver.MustParse("15.2.0")}
got, err := primary.withDatabase("other")
if test.wantErr {
if err == nil {
t.Fatalf("withDatabase() error = nil, want error")
}
return
}
if err != nil {
t.Fatalf("withDatabase() error = %v", err)
}
if got.dsn != test.want {
t.Fatalf("withDatabase() dsn = %q, want %q", got.dsn, test.want)
}
if got.wrapLargeCounters != primary.wrapLargeCounters {
t.Fatalf("withDatabase() wrapLargeCounters = %v, want %v", got.wrapLargeCounters, primary.wrapLargeCounters)
}
if !got.version.EQ(primary.version) {
t.Fatalf("withDatabase() version = %v, want %v", got.version, primary.version)
}
})
}
}