refactor(metrics): move each metric beside the code that increments it - #1205
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (15)
Included review availability: Your plan provides up to 2 included reviews per hour; 1 remains after this review. WalkthroughThe change moves feature-specific Prometheus metrics from ChangesMetric ownership migration
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Refactor Merge Risk: ⚪ Minimal · up to Metric collection remains compatible with the existing scrape contract, with no concrete current-head issue requiring changes before merge. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
✨ Simplify code
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
f5cfb86 to
71d6a9a
Compare
|
Not a blocker, and not an objection to the move: a packaging ask that this PR can satisfy for almost nothing, and that gets materially more expensive once it lands. Putting each declaration beside its owner is right, and the 14 families come through unchanged. The side effect is that And we do want it optional. The ask: in those six modules, import Two small bonuses if you do: it retires the #1235 is a reference draft of the follow-up against |
71d6a9a to
1a63dd6
Compare
Moving each counter beside the code that increments it (#1177) must not rename a metric or change its labels, because dashboards and alerts outside this repository key on them. Pin the families before any move so each move is checked against the same set.
The rate limiter is the only code that increments gateway_rate_limit_hits, so the counter now lives in rate_limit.py, registered on the shared registry. The wrapper function went with it: a one-line helper with a single caller in the same module said nothing the counter does not. Part of #1177.
budget_service is the only code that increments gateway_budget_exceeded, so the counter now lives there, registered on the shared registry, and the three call sites increment it directly. The wrapper function and its unit test go: the counter's real path is covered by the integration test that drives a zero-limit budget to a 403. Part of #1177.
The two writers are the only code that touches the four usage log metrics, so they now live in log_writer.py, registered on the shared registry, under names that read in that module (ROWS, QUEUE_DEPTH, BATCH_SIZE, FLUSH_DURATION). The lowercase aliases metrics.py exported for the writer are gone with the move. Part of #1177.
api.deps is where nearly every authentication failure is counted, so gateway_auth_failures and its record helper now live there, registered on the shared registry. The three sign-in routes already import their dependencies from api.deps and now take the helper from the same place. The helper's unit test goes: the bearer-token and key-verification tests already assert the counter through the real callers. Part of #1177.
Three modules count abandoned attempts: the platform runner, the local attempt walker, and the pipeline. The runner module already owns the upstream error classification the other two import, so gateway_abandoned_attempts and its record helper now live beside it, on the shared registry, and the walker and the pipeline take the helper from there. The helper's tests move next to the runner's own abandonment tests. Part of #1177.
…he pipeline The shared request pipeline is the only code that records token counts, request cost and inline cost settlement outcomes, so those three metrics and their record helpers now live in _pipeline.py, registered on the shared registry. The Prometheus Counter is imported under an alias there because the module already uses collections.Counter. metrics.py now holds the registry, the process collector and the HTTP request instrumentation only. Part of #1177.
…re-exports The six modules that own a metric imported Counter, Gauge and Histogram from prometheus_client directly. Importing them from gateway.metrics instead keeps one module as the gateway's dependency on prometheus_client, so making that dependency optional later changes that module alone. gateway.metrics lists the types in __all__, because mypy strict does not treat an imported name as exported. The PrometheusCounter alias in _pipeline.py stays: that module also imports collections.Counter.
3a41113 to
fe90cb6
Compare
…drop the readiness check The readiness probe judged the pool on one instantaneous reading, so a pool that filled for a moment took the pod out of rotation and a traffic spike across replicas became an outage. Drop the check, along with POOL_EXHAUSTED, request_pool_stats and PoolStats.is_saturated. #1247 tracks a check that judges the pool over time. The gauges move out of metrics.py, which #1205 had just emptied, and become a collector in core/database.py that reads the pools on each scrape. That also removes the metrics -> core.database import, which would have stopped core.database ever declaring a metric of its own. Capacity now uses the max_overflow the gateway configured rather than QueuePool's private _max_overflow, so it cannot silently understate itself. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…drop the readiness check The readiness probe judged the pool on one instantaneous reading, so a pool that filled for a moment took the pod out of rotation and a traffic spike across replicas became an outage. Drop the check, along with POOL_EXHAUSTED, request_pool_stats and PoolStats.is_saturated. #1247 tracks a check that judges the pool over time. The gauges move out of metrics.py, which #1205 had just emptied, and become a collector in core/database.py that reads the pools on each scrape. That also removes the metrics -> core.database import, which would have stopped core.database ever declaring a metric of its own. Capacity now uses the max_overflow the gateway configured rather than QueuePool's private _max_overflow, so it cannot silently understate itself. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
#1242) * feat(observability): export database pool stats and fail readiness fast when the pool is full Nothing reported how much of the SQLAlchemy connection pool was in use, so a connection leak stayed invisible until every request answered 503, and the readiness probe then queued for the full pool timeout before reporting a generic database outage. Adds a pool-stats helper over the active engines and uses it twice: four Prometheus gauges filled at scrape time (checked out, idle, overflow, capacity), labeled by pool so the metering pool is covered too; and a side-effect free saturation check ahead of the readiness query, which answers 503 immediately and names pool exhaustion as its own state. Fixes #1240 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * docs(deployment): document the database pool metrics and the pool_exhausted readiness state Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * style(db): drop a stray blank line Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * chore(web): regenerate the API client for the readiness docstring Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * refactor(observability): publish the pool stats from a collector and drop the readiness check The readiness probe judged the pool on one instantaneous reading, so a pool that filled for a moment took the pod out of rotation and a traffic spike across replicas became an outage. Drop the check, along with POOL_EXHAUSTED, request_pool_stats and PoolStats.is_saturated. #1247 tracks a check that judges the pool over time. The gauges move out of metrics.py, which #1205 had just emptied, and become a collector in core/database.py that reads the pools on each scrape. That also removes the metrics -> core.database import, which would have stopped core.database ever declaring a metric of its own. Capacity now uses the max_overflow the gateway configured rather than QueuePool's private _max_overflow, so it cannot silently understate itself. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: daavoo <davidd@mozilla.ai> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This branch was successfully deployed
3 participants
Description
Nothing changes for someone using Otari. The Prometheus scrape at
/metricsexposes the same 14 gateway metric families, with the same names, types and labels, before and after.What changes is where each metric is declared. Until now every counter lived in one shared module, so every feature that added or touched a metric edited that file. Each metric is now declared in the module that increments it: the rate limiter, the budget service, the usage log writer, the authentication dependencies, the hybrid attempt runner and the request pipeline. The shared module keeps the registry, the process collector and the HTTP request instrumentation only.
The first commit pins the set of exposed families in a test, before any move, so every move was checked against the same set. That test is the scrape diff the issue asks for.
How to test it locally
The pinned family test is
tests/unit/test_gateway_metrics.py::test_scrape_exposes_the_pinned_families. To eyeball the scrape itself, start the gateway withenable_metrics: true, hit/metrics, and compare the# HELPand# TYPElines againstmain; the lines are the same, only their order differs, because a family now registers when its owner module imports.Measured before and after (90-day churn on
mainatea1a2471versus this branch):src/gateway/metrics.pygateway.metrics(src/)REGISTRYmain.py, the middleware and endpoint)metrics.py, last 90 daysRun locally:
make lint,make typecheck,make test-unit, andtests/integration/test_metrics.pyagainst a Testcontainers PostgreSQL. The rest of the integration suite was left to CI.Not run here: the OSS edition smoke gate fails on this machine on
maintoo (model discovery against the mock provider never answers, so readiness times out), and 14 master key and dashboard session unit tests fail onmaintoo. Neither touches metrics; CI should confirm both.PR Type
Relevant issues
Fixes #1177. Part of #1171.
Checklist
tests/unit,tests/integration).make lint,make typecheck,make test). Lint, typecheck and the unit suite were run; see above for what ofmake testwas not.uv run python scripts/generate_openapi.py).AI Usage
AI Model/Tool used: Claude Code (Claude Fable 5.1)
Any additional AI details you'd like to share: The agent wrote the moves and the pinned test, ran lint, typecheck, the unit suite and the metrics integration tests, and confirmed the unrelated failures reproduce on
main.Summary
gateway.metrics./metricsmetric names, types, and labels.gateway.metricsto keep one import boundary.This improves ownership and keeps future metric changes closer to the related code.