Repository navigation
Conversation
2c68f9c to
7286f89
Compare
| ) | ||
| statBGWriterStatsResetTimestampDesc = prometheus.NewDesc( | ||
| prometheus.BuildFQName(namespace, bgWriterSubsystem, "stats_reset_timestamp_seconds"), | ||
| "Unix timestamp in seconds at which these statistics were last reset; 0 means PostgreSQL returned NULL", |
There was a problem hiding this comment.
I think it's better not to expose the metric at all if, while scanning the row, we get a NULL value from Postgres. It would definitely be less confusing than seeing a 0 in the dashboard and thinking that the last reset happened in 1970
There was a problem hiding this comment.
NULL now omits the timestamp gauge in both version paths
https://github.com/je4yfc/postgres_exporter/blob/6e73ed0/collector/pg_stat_bgwriter.go#L164-L274
| @@ -95,7 +95,13 @@ var ( | |||
| ) | |||
| statBGWriterStatsResetDesc = prometheus.NewDesc( | |||
There was a problem hiding this comment.
Could we add a comment stating that this metric will be removed in the next two releases? Mostly for us to remind ourselves to actually remove it 😬
There was a problem hiding this comment.
Added a comment marking the counter for removal after two releases
https://github.com/je4yfc/postgres_exporter/blob/6e73ed0/collector/pg_stat_bgwriter.go#L96
| @@ -87,7 +87,13 @@ var ( | |||
| ) | |||
| statCheckpointerStatsResetDesc = prometheus.NewDesc( | |||
There was a problem hiding this comment.
same as above, could we add a comment stating that this metric can be removed in the future?
There was a problem hiding this comment.
Added the same two-release removal reminder here
https://github.com/je4yfc/postgres_exporter/blob/6e73ed0/collector/pg_stat_checkpointer.go#L88
| ) | ||
| statCheckpointerStatsResetTimestampDesc = prometheus.NewDesc( | ||
| prometheus.BuildFQName(namespace, statCheckpointerSubsystem, "stats_reset_timestamp_seconds"), | ||
| "Unix timestamp in seconds at which these statistics were last reset; 0 means PostgreSQL returned NULL", |
There was a problem hiding this comment.
same as above, let's not use 0 for NULL returns
There was a problem hiding this comment.
the timestamp gauge is only emitted for non-NULL values
https://github.com/je4yfc/postgres_exporter/blob/6e73ed0/collector/pg_stat_checkpointer.go#L216-L222
| @@ -0,0 +1,206 @@ | |||
| // Copyright 2026 The Prometheus Authors | |||
There was a problem hiding this comment.
why create a new file just for this test? couldn't we reuse the existing ones?
There was a problem hiding this comment.
Because I was lazy and dumb - I fixed it. I moved the coverage into the existing bgwriter, checkpointer and database test files and removed the standalone file.
https://github.com/je4yfc/postgres_exporter/blob/6e73ed0/collector/pg_stat_bgwriter_test.go#L207
https://github.com/je4yfc/postgres_exporter/blob/6e73ed0/collector/pg_stat_checkpointer_test.go#L149
https://github.com/je4yfc/postgres_exporter/blob/6e73ed0/collector/pg_stat_database_test.go#L529
There was a problem hiding this comment.
No need to be harsh on yourself either, things happen 😅
|
|
||
| All metrics are prefixed with `pg_`. The exporter's own health is exposed via `pg_up` and `pg_exporter_*` metrics alongside the standard Go runtime metrics. | ||
|
|
||
| The last statistics-reset time is exposed as a Unix-seconds gauge in `pg_stat_bgwriter_stats_reset_timestamp_seconds`, `pg_stat_checkpointer_stats_reset_timestamp_seconds` (PostgreSQL 17+), and `pg_stat_database_stats_reset_timestamp_seconds` (with `datid` and `datname` labels). A value of `0` means PostgreSQL returned a NULL reset time, so filter for values greater than zero before calculating an age with `time() - metric`. The older `pg_stat_bgwriter_stats_reset_total`, `pg_stat_checkpointer_stats_reset_total`, and `pg_stat_database_stats_reset` families remain as deprecated counters with their existing values for compatibility; they represent timestamps, not reset counts, and should not be used with `rate()`, `increase()`, or `resets()`. |
There was a problem hiding this comment.
We don't document all our metrics in our README, it would be weird to document this one 😬
There was a problem hiding this comment.
I removed this - sorry haha.
| * [FEATURE] Export `pg_settings.pending_restart` as `pg_settings_pending_restart{name}` from the `settings` collector by @MarcWort in https://github.com/prometheus-community/postgres_exporter/pull/1396 | ||
| * [FEATURE] Add `aurora_replica_status` collector for Amazon Aurora PostgreSQL by @megative in https://github.com/prometheus-community/postgres_exporter/pull/1311 | ||
|
|
||
| * [ENHANCEMENT] Expose `pg_stat_bgwriter`, `pg_stat_checkpointer`, and `pg_stat_database` statistics reset times as `_timestamp_seconds` gauges; retain the former counter families as deprecated aliases. |
There was a problem hiding this comment.
This changelog entry doesn't follow the format we've been using above. Could you follow it?
There was a problem hiding this comment.
Also, since we're deprecating a metric, it would be nice to have a deprecation notice in the changelog as well
There was a problem hiding this comment.
I adeded tis:
https://github.com/je4yfc/postgres_exporter/blob/6e73ed0/CHANGELOG.md#L18
explicit notice naming all three deprecated counters, their replacements, and the two-release removal timeline:
https://github.com/je4yfc/postgres_exporter/blob/6e73ed0/CHANGELOG.md#L19
Signed-off-by: Joseph Erdosy <168128519+je4yfc@users.noreply.github.com>
Signed-off-by: Joseph Erdosy <168128519+je4yfc@users.noreply.github.com>
6e73ed0 to
08f755e
Compare
Signed-off-by: Joseph <168128519+je4yfc@users.noreply.github.com>
|
I think I fixed it all, I'll leave resolution to you. If you need additional fixed, let me know 🫡 |
| * [FEATURE] Export `pg_settings.pending_restart` as `pg_settings_pending_restart{name}` from the `settings` collector by @MarcWort in https://github.com/prometheus-community/postgres_exporter/pull/1396 | ||
| * [FEATURE] Add `aurora_replica_status` collector for Amazon Aurora PostgreSQL by @megative in https://github.com/prometheus-community/postgres_exporter/pull/1311 | ||
| * [BUGFIX] Expose `pg_stat_bgwriter`, `pg_stat_checkpointer`, and `pg_stat_database` statistics reset times as `_timestamp_seconds` gauges, omitting samples when PostgreSQL returns NULL, by @je4yfc in https://github.com/prometheus-community/postgres_exporter/pull/1416 | ||
| * [CHANGE] Deprecate `pg_stat_bgwriter_stats_reset_total`, `pg_stat_checkpointer_stats_reset_total`, and `pg_stat_database_stats_reset` in favor of their `_stats_reset_timestamp_seconds` gauges; the counters will be removed two releases after this deprecation, by @je4yfc in https://github.com/prometheus-community/postgres_exporter/pull/1416 |
There was a problem hiding this comment.
It's better to be explicit on what "two releases from now" means. Which is after v0.22.0
| []string{"datid", "datname"}, | ||
| prometheus.Labels{}, | ||
| ) | ||
| // Deprecated: remove this counter two releases after introducing the timestamp gauge. |
There was a problem hiding this comment.
let's be explicit about which version we're removing the metric
| []string{}, | ||
| prometheus.Labels{}, | ||
| ) | ||
| // Deprecated: remove this counter two releases after introducing the timestamp gauge. |
|
Fixed. I set them to 0.22.0. |
This is my attempt to fix: #1415
Add
pg_stat_bgwriter_stats_reset_timestamp_seconds,pg_stat_checkpointer_stats_reset_timestamp_seconds, andpg_stat_database_stats_reset_timestamp_secondsas Unix-second gauges. Atmaster@c5662d1, the background-writer (also before PostgreSQL 17), checkpointer, and database SQL conversion and emission paths turn SQLstats_resettimestamps into Unix seconds but publish counters. Preserve the legacy names, counter types, values and label sets as deprecated aliases, scheduled for removal two releases after deprecation. The new timestamp gauges omit samples when PostgreSQL returns NULL; the legacy counters retain their existing NULL-as-zero behavior. The checkpointer guard stays PostgreSQL 17+, and database labels staydatid,datname. No SQL, flag or legacy-exporter change.Reproduce the original failure with the focused collector/HTTP exposition test:
The original run fails because the gauge families are absent. Regression coverage now lives in the existing collector test files. On this PR branch,
go test ./collector -run 'TestPGStat(BGWriter|Checkpointer|Database)Collector' -count=1passes. Updated HTTP exposition excerpt:Validation:
go test ./...,go vet ./..., andgit diff --checkpassed. Tests cover both background-writer version paths, checkpointer's version guard, NULL omission, valid zero timestamps, and metric names, types and labels.LLM Disclosure: This bug was found and classified by an LLM that I was
abusinggiving the task of looking through every metric I ingest to see what could be used in a dashboard. After I verified the issue, I had that same LLM help me understand enough of your codebase to make a PR.