feat: decouple metrics scraping from metrics computation by jsilvela · Pull Request #8003 · cloudnative-pg/cloudnative-pg

jsilvela · 2025-07-09T15:24:56Z

github-actions · 2025-07-09T15:25:09Z

❗ By default, the pull request is configured to backport to all release branches.

To stop backporting this pr, remove the label: backport-requested ◀️ or add the label 'do not backport'
To stop backporting this pr to a certain release branch, remove the specific branch label: release-x.y

jsilvela · 2025-07-10T13:50:09Z

/test depth=push test_level=4 feature_type=basic

github-actions · 2025-07-10T13:50:20Z

@jsilvela, here's the link to the E2E on CNPG workflow run: https://github.com/cloudnative-pg/cloudnative-pg/actions/runs/16196975122

jsilvela · 2025-07-10T14:07:06Z

pkg/management/postgres/metrics/collector.go

 	for name, userQuery := range q.userQueries {
 		queryLogger := log.WithValues("query", name)
-		collector := QueryCollector{
+		queryRunner := QueryRunner{


OK, way too many things in the monitoring code are called a collector.
Computing the next value of a metric is not "collecting".
If I get the OK from maintainers, would like to do a lot of renaming. Perhaps a separate PR would be better though.

100% agree on the changes; the metrics code was hard to follow. I'd support it on this PR or a separate one... My gut is that a separate PR might be easier to review [rename only, no logic changes].

My gut is that a separate PR might be easier to review [rename only, no logic changes].

Yep, my thoughts too

jsilvela · 2025-07-10T14:14:23Z

/test depth=push test_level=4 feature_type=observability

github-actions · 2025-07-10T14:14:35Z

@jsilvela, here's the link to the E2E on CNPG workflow run: https://github.com/cloudnative-pg/cloudnative-pg/actions/runs/16197557109

jsilvela · 2025-07-10T15:32:39Z

Gah, metrics E2E failing. Manually taking out the OK-to-merge label (from smoke tests)

jmealo

I meant to leave this as a PR review not a comment. Ooops.

jmealo · 2025-07-10T15:40:36Z

pkg/management/postgres/metrics/collector.go

 	for name, userQuery := range q.userQueries {
 		queryLogger := log.WithValues("query", name)
-		collector := QueryCollector{
+		queryRunner := QueryRunner{


100% agree on the changes; the metrics code was hard to follow. I'd support it on this PR or a separate one... My gut is that a separate PR might be easier to review [rename only, no logic changes].

pkg/management/postgres/metrics/collector.go

jmealo

Looks good overall!
When I was troubleshooting some issues with CNPG, I received help from some Gophers in the Go slack who hinted that the project may have some race conditions in patterns used throughout. This was months ago and I don't remember the specifics. All of th Go I've done is just trying to fix bugs in CNPG; so I could be way off-base here.

Possible Race Condition

I think there may be a race condition between Update() and Collect(). The Update() method writes to computedMetrics while Collect() reads
from it. It looks like Update() is called from a ticker goroutine (every 30s) and Collect() is called by Prometheus HTTP handler (meaning they can execute concurrently without synchronization)? 🤔

I think something like this could work?

  type QueriesCollector struct {
      // ... existing fields ...
      computedMetrics []prometheus.Metric
      mu              sync.RWMutex  // Add this
  }

Then use q.mu.Lock() in Update() and q.mu.RLock() in Collect().

Alternatively:
Instead of a mutex, we could possibly use an atomic pointer to swap the entire metrics slice atomically? (I would lean towards this)

  type QueriesCollector struct {
      // ... existing fields ...
      computedMetrics atomic.Pointer[[]prometheus.Metric]
  }

This would eliminate lock contention between Update and Collect operations.

Error handling

Since the user queries are user supplied, would it make sense to add more context to errors in createMetricsFromUserQueries()? For example: when instance.IsPrimary() fails, it might be helpful to wrap the error with more context about what operation was being attempted.

jsilvela · 2025-07-10T16:14:41Z

@jmealo you're right about the race condition. While it wouldn't lead to corruption/destruction, we could call Collect at a point where Update was in progress, and we could lose metrics during collection.
The mutex is right, I think

jsilvela · 2025-07-10T16:51:50Z

@jmealo thinking about the lock more, I think we might not need it. Instead of clearing the computedMetrics field at the beginning of Update, and progressively appending to it, we can build a slice with everything, and then do q.computedMetrics = generatedMetrics in one fell swoop.

jmealo · 2025-07-10T18:04:37Z

@jmealo thinking about the lock more, I think we might not need it. Instead of clearing the computedMetrics field at the beginning of Update, and progressively appending to it, we can build a slice with everything, and then do q.computedMetrics = generatedMetrics in one fell swoop.

This looks right to me. 👍

Signed-off-by: Jaime Silvela <jaime.silvela@mailfence.com> Signed-off-by: Armando Ruocco <armando.ruocco@enterprisedb.com>

Co-authored-by: Jeff Mealo <jmealo@protonmail.com> Signed-off-by: Jaime Silvela <jaime.silvela@mailfence.com> Signed-off-by: Armando Ruocco <armando.ruocco@enterprisedb.com>

Signed-off-by: Jaime Silvela <jaime.silvela@mailfence.com> Signed-off-by: Armando Ruocco <armando.ruocco@enterprisedb.com>

Signed-off-by: Armando Ruocco <armando.ruocco@enterprisedb.com>

Signed-off-by: Jaime Silvela <jaime.silvela@mailfence.com> Signed-off-by: Armando Ruocco <armando.ruocco@enterprisedb.com>

Signed-off-by: Armando Ruocco <armando.ruocco@enterprisedb.com>

Signed-off-by: Leonardo Cecchi <leonardo.cecchi@enterprisedb.com>

leonardoce · 2025-10-28T10:29:44Z

/ok-to-merge

jsilvela requested a review from a team as a code owner July 9, 2025 15:24

jsilvela marked this pull request as draft July 9, 2025 15:25

dosubot bot added the size:L This PR changes 100-499 lines, ignoring generated files. label Jul 9, 2025

cnpg-bot added backport-requested ◀️ This pull request should be backported to all supported releases release-1.22 release-1.25 release-1.26 labels Jul 9, 2025

dosubot bot added the enhancement 🪄 New feature or request label Jul 9, 2025

jsilvela force-pushed the dev/jsilvela-metrics branch 2 times, most recently from 7f9da77 to 196f54f Compare July 10, 2025 13:49

jsilvela marked this pull request as ready for review July 10, 2025 13:52

jsilvela commented Jul 10, 2025

View reviewed changes

cnpg-bot added the ok to merge 👌 This PR can be merged label Jul 10, 2025

jsilvela removed the ok to merge 👌 This PR can be merged label Jul 10, 2025

jmealo reviewed Jul 10, 2025

View reviewed changes

pkg/management/postgres/metrics/collector.go Outdated Show resolved Hide resolved

jmealo reviewed Jul 10, 2025

View reviewed changes

jsilvela requested review from NiccoloFei and litaocdl as code owners July 11, 2025 08:22

dosubot bot added size:XL This PR changes 500-999 lines, ignoring generated files. and removed size:L This PR changes 100-499 lines, ignoring generated files. labels Jul 11, 2025

refactor: implement runnable to perform updates on a ticker

e155a07

Signed-off-by: Jaime Silvela <jaime.silvela@mailfence.com> Signed-off-by: Armando Ruocco <armando.ruocco@enterprisedb.com>

leonardoce force-pushed the dev/jsilvela-metrics branch from 3f9cabf to 23ccdb9 Compare October 28, 2025 08:34

jsilvela and others added 17 commits October 28, 2025 11:04

Update pkg/management/postgres/metrics/collector.go

47bc962

Co-authored-by: Jeff Mealo <jmealo@protonmail.com> Signed-off-by: Jaime Silvela <jaime.silvela@mailfence.com> Signed-off-by: Armando Ruocco <armando.ruocco@enterprisedb.com>

refactor: avoid need for locks

fc0c0fc

Signed-off-by: Jaime Silvela <jaime.silvela@mailfence.com> Signed-off-by: Armando Ruocco <armando.ruocco@enterprisedb.com>

fix: add timeout to compute metrics

b7415d6

Signed-off-by: Jaime Silvela <jaime.silvela@mailfence.com> Signed-off-by: Armando Ruocco <armando.ruocco@enterprisedb.com>

chore: improve error traceability as suggested by jmealo

32377e0

Signed-off-by: Jaime Silvela <jaime.silvela@mailfence.com> Signed-off-by: Armando Ruocco <armando.ruocco@enterprisedb.com>

fix: update apidoc

eff169d

Signed-off-by: Jaime Silvela <jaime.silvela@mailfence.com> Signed-off-by: Armando Ruocco <armando.ruocco@enterprisedb.com>

fix: spellcheck

d1e6527

Signed-off-by: Jaime Silvela <jaime.silvela@mailfence.com> Signed-off-by: Armando Ruocco <armando.ruocco@enterprisedb.com>

fix: go mod

d3e713d

Signed-off-by: Jaime Silvela <jaime.silvela@mailfence.com> Signed-off-by: Armando Ruocco <armando.ruocco@enterprisedb.com>

fix: thread safety (Collect interface)

1699187

Signed-off-by: Armando Ruocco <armando.ruocco@enterprisedb.com>

feat: convert the periodic run approach to a TTL one

d16e191

Signed-off-by: Jaime Silvela <jaime.silvela@mailfence.com> Signed-off-by: Armando Ruocco <armando.ruocco@enterprisedb.com>

Update pkg/management/postgres/webserver/metricserver/pg_collector.go

af54e58

Signed-off-by: Jaime Silvela <jaime.silvela@mailfence.com> Signed-off-by: Armando Ruocco <armando.ruocco@enterprisedb.com>

chore: review

cfca145

Signed-off-by: Armando Ruocco <armando.ruocco@enterprisedb.com>

fix: respect default behaviour if we cannot fetch the cluster

4ec65e0

Signed-off-by: Armando Ruocco <armando.ruocco@enterprisedb.com>

feat: add a way to keep track of metric generation

07b7c35

Signed-off-by: Armando Ruocco <armando.ruocco@enterprisedb.com>

feat: add cache hit and miss

85e6167

Signed-off-by: Armando Ruocco <armando.ruocco@enterprisedb.com>

webhook: add warning and improve field description

fb53876

Signed-off-by: Armando Ruocco <armando.ruocco@enterprisedb.com>

chore: clarify description

5b56a95

Signed-off-by: Armando Ruocco <armando.ruocco@enterprisedb.com>

refactor: rethink cache miss

cb0436c

Signed-off-by: Armando Ruocco <armando.ruocco@enterprisedb.com>

leonardoce force-pushed the dev/jsilvela-metrics branch from 23ccdb9 to cb0436c Compare October 28, 2025 10:05

leonardoce approved these changes Oct 28, 2025

View reviewed changes

docs: clarify output caching

13b6216

Signed-off-by: Leonardo Cecchi <leonardo.cecchi@enterprisedb.com>

leonardoce added do not backport This PR must not be backported - it will be in the next minor release and removed backport-requested ◀️ This pull request should be backported to all supported releases release-1.25 release-1.26 release-1.27 labels Oct 28, 2025

leonardoce merged commit fde2400 into main Oct 28, 2025
44 checks passed

leonardoce deleted the dev/jsilvela-metrics branch October 28, 2025 10:43

Conversation

jsilvela commented Jul 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Jul 9, 2025

Uh oh!

jsilvela commented Jul 10, 2025

Uh oh!

github-actions bot commented Jul 10, 2025

Uh oh!

jsilvela Jul 10, 2025

Choose a reason for hiding this comment

Uh oh!

jmealo Jul 10, 2025

Choose a reason for hiding this comment

Uh oh!

jsilvela Jul 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jsilvela commented Jul 10, 2025

Uh oh!

github-actions bot commented Jul 10, 2025

Uh oh!

jsilvela commented Jul 10, 2025

Uh oh!

jmealo left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmealo Jul 10, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jmealo left a comment

Choose a reason for hiding this comment

Possible Race Condition

Error handling

Uh oh!

jsilvela commented Jul 10, 2025

Uh oh!

jsilvela commented Jul 10, 2025

Uh oh!

jmealo commented Jul 10, 2025

Uh oh!

leonardoce commented Oct 28, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

jsilvela commented Jul 9, 2025 •

edited

Loading

jsilvela Jul 10, 2025 •

edited

Loading

jmealo left a comment •

edited

Loading