You've already forked postgres_exporter
mirror of
https://github.com/prometheus-community/postgres_exporter.git
synced 2026-10-23 12:08:57 +03:00
* 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>
60 lines
2.0 KiB
Go
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)
|
|
}
|
|
})
|
|
}
|
|
}
|