Skip to content

Prometheus /metrics improvment proposals - #536

Open
divad1196 wants to merge 4 commits into
14.0-monitoring_prometheus_queuejobfrom
14.0-monitoring_prometheus_queuejob_divad1196
Open

divad1196 wants to merge 4 commits into
14.0-monitoring_prometheus_queuejobfrom
14.0-monitoring_prometheus_queuejob_divad1196

Conversation

@divad1196

Copy link
Copy Markdown

Context

2 different proposals in 1 PR:

  1. I would recommend on-the-fly gathering.
    In [ADD] monitoring_prometheus_queue_job #529 it was mentioned that `15 pods would repond" but that's an infrastructure matter: only one pod should be queried at the time.
  2. If we still want to use the cron and prometheus.metrics table, we can drop the last commit.
    The new code
    1. Merge the abstract class into prometheus.metrics
    2. Clean up the Gauge / Registry code. This avoid concurrency issues.

@gurneyalex

Shared gauges mixed data between parallel requests and between databases.
This also avoids cleaning up stale series.
…s.metric

The abstract model held no state, it was only an extension point.
The cron and the prometheus.metric table only existed to pass values from
the cron process to the HTTP workers. Querying the database directly when
/metrics is scraped removes the cron, the table and the staleness.
@divad1196
divad1196 requested review from florentx and gurneyalex and removed request for gurneyalex October 1, 2026 09:59
@divad1196 divad1196 changed the title 14.0 monitoring prometheus queuejob divad1196 Prometheus /metrics improvment proposals Oct 1, 2026
@divad1196
divad1196 force-pushed the 14.0-monitoring_prometheus_queuejob_divad1196 branch from 9bda634 to 6476454 Compare October 1, 2026 11:40

@florentx florentx left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO these changes make sense.

I don't have experience with Prometheus, though.

@divad1196

Copy link
Copy Markdown
Author

@gurneyalex @yankinmax @vvrossem Opinion?

@vvrossem

Copy link
Copy Markdown

@gurneyalex @yankinmax @vvrossem Opinion?

For context on #535: it only took the per-request CollectorRegistry from 6f17f3c, because it fixes deleted metrics still showing up in /metrics (that's what the test a review caught). The rest of #536 isn't in #535 since #535 forward-ports #529 and #536 isn't merged there yet. Whatever gets decided here, #535 can follow.

I don't know Prometheus well enough to have a strong opinion on cron vs on-the-fly. Sorry.

@divad1196
divad1196 added this pull request to stack #537 October 13, 2026 08:11
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.

3 participants