Expose a Prometheus-scrapable metrics endpoint for multiprocess workers #3191

Activity

1. IsmaelMartinez

   added

   enhancementNew feature or requestNew feature or request

   on Sep 8, 2026
2. utsab345 commented on Sep 9, 2026

   

   utsab345

   on Sep 9, 2026

   Contributor

   More actions

   I dug through the current telemetry wiring and the native Prometheus route is feasible, including the gunicorn multiprocess part. What is already in place: the pr_agent.commands counter and the pr_agent.command span, with the exporter selected in _create_metric_exporter (pr_agent/telemetry/meter.py:45) off the ExporterType values (console, otlp, none) configured under [otel].

   The plan for a first class scrape target:

   1. Dependency. Add prometheus_client and opentelemetry-exporter-prometheus pinned to the same 1.43.0 as the otel SDK already in pyproject.toml. PrometheusMetricExporter plugs straight into the existing PeriodicExportingMetricReader, so the meter does not need restructuring.

   2. Exporter type. Add prometheus to ExporterType and to the [otel] config, with an optional port. _create_metric_exporter returns a PrometheusMetricExporter bound to a CollectorRegistry.

   3. Scrape endpoint. The webhook apps are FastAPI (github_app, gitlab_webhook, azuredevops_server_webhook), so a small shared helper can attach a GET /metrics route serving generate_latest(REGISTRY) with the CONTENT_TYPE_LATEST header. No new server or process is needed.

   4. Multiprocess mode. gunicorn already runs preload_app = True with UvicornWorkers, which is the fork model prometheus_client multiprocess mode is built for. That explains the commented out from prometheus_client import multiprocess still sitting at pr_agent/servers/gunicorn_config.py:4. Wiring: set PROMETHEUS_MULTIPROC_DIR and prepare the directory in when_ready, add child_exit calling multiprocess.mark_process_dead(worker.pid), and back the /metrics handler with a MultiProcessCollector.

   5. One latent gap to note. The PeriodicExportingMetricReader background thread does not survive fork, so under multiprocess workers the otlp and console exporters already have a flush problem today. The prometheus route avoids that by pushing into the shared multiprocess registry from the worker side, but the reader lifecycle deserves an explicit comment either way.

   The native endpoint fits the codebase cleanly. I am happy to open the PR for it, or to take the other scope (document the OTel Collector route and remove the dead import) if the maintainers prefer the lighter answer. Can I take this one? Assigning me works.
3. IsmaelMartinez

   assigned

   utsab345

   on Sep 9, 2026
4. IsmaelMartinez commented on Sep 9, 2026

   

   IsmaelMartinez

   on Sep 9, 2026

   CollaboratorAuthor

   More actions

   Thanks, this is a thorough read and the native route is the one I would take. Assigned.

   The commented-out multiprocess import at gunicorn_config.py:4 being the tell is a good catch, and the child_exit plus PROMETHEUS_MULTIPROC_DIR wiring is exactly the missing half.

   Two things for the PR. Adding prometheus_client and opentelemetry-exporter-prometheus needs to be flagged in the description as a dependency change, and please keep the endpoint opt-in via ExporterType so nothing new is exposed by default. Your point 5 on the PeriodicExportingMetricReader not surviving fork is a separate bug worth its own issue rather than folding in here.
5. utsab345 commented on Sep 9, 2026

   

   utsab345

   on Sep 9, 2026

   Contributor

   More actions

   Implemented exporter_type = "prometheus" in PR #3238 (branch feature/prometheus-metrics-3191). Following the maintainer's guidance I flagged the dependency change in the PR description.

   Summary of what landed:

   1. A new prometheus OTEL.EXPORTER_TYPE renders the pr_agent.commands counter as a native GET /metrics endpoint on the gunicorn-served apps (github_app, gitlab_webhook, azuredevops_server_webhook, gitea_app). The endpoint is mounted only when this exporter is selected, so a default deployment exposes nothing new.
   2. Multiprocess correctness under preload_app = True: gunicorn provisions a shared PROMETHEUS_MULTIPROC_DIR in when_ready before the first fork and deregisters workers in child_exit; each worker writes its own per-pid state file and /metrics merges them with a MultiProcessCollector.
   3. Dependency decision: I added only prometheus-client==0.21.1. I initially assumed opentelemetry-exporter-prometheus would plug into PeriodicExportingMetricReader as an exporter, but after checking the package that assumption was wrong: it ships a PrometheusMetricReader and its docs state no multiprocessing support, which is exactly the requirement here, and it only exists as a 0.x pre-release. So I implemented a small in-house bridge on prometheus_client directly and called out the deviation in the PR description for maintainer review.
   4. Scope: the exporter is command-metrics only (a startup warning makes the no-spans behavior explicit), and point 5 from my earlier comment — the PeriodicExportingMetricReader background thread not surviving fork for otlp/console — is left out of this PR as a separate issue, as suggested.
   5. Verification: unit tests cover the export translation, a multiprocess end-to-end test across two interpreter processes, config/validation, the endpoint mounting guard, and the import-order safety that keeps prometheus_client out of the gunicorn master. Full unit suite passes (4459), ruff and pre-commit clean.
6. utsab345

   added a commit that references this issue on Sep 10, 2026

   feat(telemetry): expose a native prometheus scrape endpoint

   ...

   fc71075
7. IsmaelMartinez

   closed this as completedin #3238on Sep 10, 2026