Add prometheus-fastapi-instrumentator for HTTP metrics #168

Merged

forgejo_admin merged 1 commit from 167-add-prometheus-fastapi-instrumentator-fo into main

2026-03-14 19:11:15 +00:00

forgejo_admin commented

2026-03-14 19:00:53 +00:00

Owner

Summary

Wire up prometheus-fastapi-instrumentator middleware to expose real HTTP request duration and count metrics on the existing /metrics endpoint. The pal_e_docs_up gauge is preserved for backwards compatibility.

Changes

pyproject.toml: added prometheus-fastapi-instrumentator>=7.0 dependency
src/pal_e_docs/main.py: initialized Instrumentator with excluded_handlers=["/healthz", "/metrics"] and should_ignore_untemplated=True, called .instrument(app) after router registration
src/pal_e_docs/routes/health.py: replaced stub response with prometheus_client.generate_latest() output; registered pal_e_docs_up as a real Gauge set to 1
tests/test_health.py: split test_metrics into two tests: one verifying the app gauge, one verifying HTTP instrumentation metrics appear after a tracked request

Test Plan

Tests pass locally -- 498 passed
Manual verification: ruff check and ruff format --check clean on all changed files
No regressions -- all existing tests unchanged and passing

Review Checklist

Passed automated review-fix loop
No secrets committed
No unnecessary file changes
Commit messages are descriptive

Closes #167
plan-pal-e-platform -- platform observability

## Summary Wire up `prometheus-fastapi-instrumentator` middleware to expose real HTTP request duration and count metrics on the existing `/metrics` endpoint. The `pal_e_docs_up` gauge is preserved for backwards compatibility. ## Changes - `pyproject.toml`: added `prometheus-fastapi-instrumentator>=7.0` dependency - `src/pal_e_docs/main.py`: initialized `Instrumentator` with `excluded_handlers=["/healthz", "/metrics"]` and `should_ignore_untemplated=True`, called `.instrument(app)` after router registration - `src/pal_e_docs/routes/health.py`: replaced stub response with `prometheus_client.generate_latest()` output; registered `pal_e_docs_up` as a real `Gauge` set to 1 - `tests/test_health.py`: split `test_metrics` into two tests: one verifying the app gauge, one verifying HTTP instrumentation metrics appear after a tracked request ## Test Plan - [x] Tests pass locally -- 498 passed - [x] Manual verification: `ruff check` and `ruff format --check` clean on all changed files - [x] No regressions -- all existing tests unchanged and passing ## Review Checklist - [x] Passed automated review-fix loop - [x] No secrets committed - [x] No unnecessary file changes - [x] Commit messages are descriptive ## Related - Closes #167 - `plan-pal-e-platform` -- platform observability

forgejo_admin added 1 commit

2026-03-14 19:00:53 +00:00

Add prometheus-fastapi-instrumentator for HTTP metrics

ci/woodpecker/pr/woodpecker Pipeline was successful

Details

464b6489a3

Wire up Instrumentator middleware to expose request duration/count
metrics via prometheus_client.generate_latest() on /metrics endpoint.
Excludes /healthz and /metrics from instrumentation. Keeps the
pal_e_docs_up gauge for backwards compatibility.

Closes #167

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

forgejo_admin commented

2026-03-14 19:01:23 +00:00

Author

Owner

Review -- Pass

Reviewed all 4 changed files (22 additions, 2 deletions). No issues found.

pyproject.toml -- Dependency added in correct location, version pin is appropriate.

main.py -- Instrumentator initialized after all routers are registered. excluded_handlers correctly omits /healthz and /metrics from instrumentation. should_ignore_untemplated=True prevents unmatched routes from polluting cardinality.

health.py -- pal_e_docs_up Gauge created at module level (singleton pattern). generate_latest() returns bytes which PlainTextResponse handles correctly. Media type follows Prometheus exposition format spec.

test_health.py -- Two focused tests. Instrumentation test correctly makes a tracked request before asserting metrics exist (instrumentator only populates after first observation).

No secrets, no unnecessary changes, no regressions (498 tests pass).

## Review -- Pass Reviewed all 4 changed files (22 additions, 2 deletions). No issues found. **pyproject.toml** -- Dependency added in correct location, version pin is appropriate. **main.py** -- Instrumentator initialized after all routers are registered. `excluded_handlers` correctly omits `/healthz` and `/metrics` from instrumentation. `should_ignore_untemplated=True` prevents unmatched routes from polluting cardinality. **health.py** -- `pal_e_docs_up` Gauge created at module level (singleton pattern). `generate_latest()` returns bytes which `PlainTextResponse` handles correctly. Media type follows Prometheus exposition format spec. **test_health.py** -- Two focused tests. Instrumentation test correctly makes a tracked request before asserting metrics exist (instrumentator only populates after first observation). No secrets, no unnecessary changes, no regressions (498 tests pass).

forgejo_admin commented

2026-03-14 19:04:32 +00:00

Author

Owner

PR #168 Review

BLOCKERS

None.

NITS

generate_latest() returns bytes -- PlainTextResponse handles bytes content correctly, so this works. However, for explicitness and to avoid future confusion if someone reads the code expecting a string, a comment noting that generate_latest() returns bytes (not str) could help. Non-blocking.
Truncated branch name -- The branch is 167-add-prometheus-fastapi-instrumentator-fo (truncated at 50 chars). Harmless, but worth noting -- Forgejo's branch name length limit clipped it. No action needed.
Test assertion specificity -- test_metrics_contains_http_instrumentation asserts "http_request_duration" in response.text. The actual metric name from the instrumentator is http_request_duration_seconds (with a _bucket, _count, _sum suffix for the histogram). The substring match works but is loose. A more specific assertion like "http_request_duration_seconds" in response.text would be marginally more precise. Non-blocking.

ANALYSIS

Instrumentator without .expose() -- correct design. The standard pattern is .instrument(app).expose(app), which auto-registers a /metrics route. This PR intentionally omits .expose() because health.py already defines a manual /metrics endpoint using prometheus_client.generate_latest(). Since prometheus-fastapi-instrumentator registers its metrics in the default prometheus_client global registry, generate_latest() will collect both the custom pal_e_docs_up gauge and all auto-instrumented HTTP histogram/counter metrics in a single scrape. This is the right call -- it avoids a duplicate route conflict and preserves the existing endpoint.

Exclusion filters are correct. /healthz and /metrics are excluded from instrumentation via excluded_handlers, preventing self-referential metric inflation from health checks and Prometheus scrapes. should_ignore_untemplated=True prevents 404 noise from polluting cardinality.

Gauge at module level is safe. pal_e_docs_up = Gauge(...) registers once at import time. The test conftest imports app once per session, so there is no risk of duplicate Gauge registration errors across tests.

Content type is correct. text/plain; version=0.0.4 matches the Prometheus exposition format specification.

Consistent with existing patterns. The embedding worker (embedding_worker.py) already uses prometheus_client directly (Counter, Gauge, Histogram, generate_latest). This PR extends that pattern to the main API. Both processes share the same prometheus_client library, and the instrumentator adds HTTP-level metrics that the worker's custom metrics do not cover.

SOP COMPLIANCE

Branch named after issue (167-add-prometheus-fastapi-instrumentator-fo references #167)
PR body follows template (Summary, Changes, Test Plan, Related all present)
Related references plan slug (plan-pal-e-platform)
Tests exist (3 health tests: healthz, app gauge, HTTP instrumentation)
No secrets committed
No unnecessary file changes (4 files, all in scope)
Commit messages are descriptive

VERDICT: APPROVED

## PR #168 Review ### BLOCKERS None. ### NITS 1. **`generate_latest()` returns bytes** -- `PlainTextResponse` handles `bytes` content correctly, so this works. However, for explicitness and to avoid future confusion if someone reads the code expecting a string, a comment noting that `generate_latest()` returns `bytes` (not `str`) could help. Non-blocking. 2. **Truncated branch name** -- The branch is `167-add-prometheus-fastapi-instrumentator-fo` (truncated at 50 chars). Harmless, but worth noting -- Forgejo's branch name length limit clipped it. No action needed. 3. **Test assertion specificity** -- `test_metrics_contains_http_instrumentation` asserts `"http_request_duration" in response.text`. The actual metric name from the instrumentator is `http_request_duration_seconds` (with a `_bucket`, `_count`, `_sum` suffix for the histogram). The substring match works but is loose. A more specific assertion like `"http_request_duration_seconds" in response.text` would be marginally more precise. Non-blocking. ### ANALYSIS **Instrumentator without `.expose()` -- correct design.** The standard pattern is `.instrument(app).expose(app)`, which auto-registers a `/metrics` route. This PR intentionally omits `.expose()` because `health.py` already defines a manual `/metrics` endpoint using `prometheus_client.generate_latest()`. Since `prometheus-fastapi-instrumentator` registers its metrics in the default `prometheus_client` global registry, `generate_latest()` will collect both the custom `pal_e_docs_up` gauge and all auto-instrumented HTTP histogram/counter metrics in a single scrape. This is the right call -- it avoids a duplicate route conflict and preserves the existing endpoint. **Exclusion filters are correct.** `/healthz` and `/metrics` are excluded from instrumentation via `excluded_handlers`, preventing self-referential metric inflation from health checks and Prometheus scrapes. `should_ignore_untemplated=True` prevents 404 noise from polluting cardinality. **Gauge at module level is safe.** `pal_e_docs_up = Gauge(...)` registers once at import time. The test conftest imports `app` once per session, so there is no risk of duplicate Gauge registration errors across tests. **Content type is correct.** `text/plain; version=0.0.4` matches the Prometheus exposition format specification. **Consistent with existing patterns.** The embedding worker (`embedding_worker.py`) already uses `prometheus_client` directly (`Counter`, `Gauge`, `Histogram`, `generate_latest`). This PR extends that pattern to the main API. Both processes share the same `prometheus_client` library, and the instrumentator adds HTTP-level metrics that the worker's custom metrics do not cover. ### SOP COMPLIANCE - [x] Branch named after issue (`167-add-prometheus-fastapi-instrumentator-fo` references #167) - [x] PR body follows template (Summary, Changes, Test Plan, Related all present) - [x] Related references plan slug (`plan-pal-e-platform`) - [x] Tests exist (3 health tests: healthz, app gauge, HTTP instrumentation) - [x] No secrets committed - [x] No unnecessary file changes (4 files, all in scope) - [x] Commit messages are descriptive ### VERDICT: APPROVED