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 ignored due to path filters (1)
📒 Files selected for processing (8)
Included review availability: Your plan provides up to 2 included reviews per hour; 1 remains after this review. WalkthroughPrometheus support is now optional. The application provides no-op metrics objects when the dependency is absent and refuses startup when metrics are enabled without it. The Docker image installs the metrics extra, and documentation describes the requirement. ChangesOptional metrics support
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Refactor Merge Risk: ⚪ Minimal · up to The optional metrics installation and startup behavior are consistent across packaging, Docker, and the documented configuration paths. This is ready to merge. 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Full details: Title checkExplanation The title clearly describes the change and uses imperative mood, but it starts with the unsupported Full details: Docstring CoverageExplanation Docstring coverage is 19.05% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 21 functions across 3 files. (5 skipped: 5 unsupported.)
✨ 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 |
ed70f94 to
d157da5
Compare
d157da5 to
1f1e552
Compare
prometheus-client was a core dependency imported on every startup, whether or not anything scraped. enable_metrics gates only the middleware and the /metrics route, so a deployment that never scrapes still paid the import, the ProcessCollector registration and 14 always-incrementing counters nothing could read. Most of the import cost is prometheus_client.exposition pulling in http.server and wsgiref.simple_server, which nothing else here needs. It moves to a metrics extra. gateway/metrics.py becomes the seam: it re-exports Counter, Gauge, Histogram, CollectorRegistry, ProcessCollector and generate_latest, real when the extra is installed and no-op stands-in when it is not, so the declarations and the record_* helpers stay callable either way and no call site needs a guard. Types are taken from the real library under TYPE_CHECKING, which the dev group installs, so mypy checks the declarations against it regardless of the runtime environment. enable_metrics with the extra absent refuses to start rather than registering a scrape that answers with an empty body, which would read as a broken exporter rather than a missing install. BREAKING CHANGE: a source install that sets enable_metrics must now install the metrics extra (pip install gateway[metrics]) or the gateway refuses to start. The Docker image installs it, so image-based deployments are unaffected. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1f1e552 to
3a51798
Compare
Description
Rebased onto
mainafter #1205 landed, which is what this draft said it waswaiting for. The
!in the title is still a real decision for a maintainer toconfirm rather than mine to make.
Prometheus stops being something every deployment pays for.
prometheus-clientis a core dependency today, imported on every startup whether or not anything
scrapes:
enable_metricsgates only the request middleware and the/metricsroute, so a gateway that never scrapes still pays the import, registers a
process collector, and keeps counters incrementing that nothing can read.
Measured on this branch: the import is 32ms and 138 modules, of which
prometheus_client.expositionis 25ms. It pulls inhttp.server,wsgiref.simple_server,socketserver,socket,selectand the wholeemailpackage, none of which anything else here needs. Against a 2.66simport gateway.mainthe time is about 1.2%, so the case is the dependencysurface (a process that never scrapes currently imports a WSGI server and an
email parser) rather than the milliseconds.
It becomes a
metricsextra, alongside the existingocrands3ones.#1205 is what makes the seam clean: with every metric now declared beside the
code that increments it,
gateway/metrics.pyis purely the re-export pointthose declarations go through, so intercepting it there covers all of them.
It re-exports the metric types, real when the extra is installed and no-op
stand-ins when it is not. The declarations still run and every recorder stays
callable either way, so nothing on a hot path needs a guard and no call site
changes. Types come from the real library under
TYPE_CHECKING, which the devgroup installs unconditionally, so mypy checks every declaration against the
real signatures regardless of what the runtime environment has.
The fallback covers the custom-collector surface too, which #1205 introduced:
Collectorstays subclassable andCollectorRegistry.registerstays callable,because
gateway/core/database.pyregisters a pool collector at import time.Setting
enable_metricswithout the extra refuses to start, with the installhint. That is the one case worth failing on: registering the scrape on top of
no-op metrics would answer with an empty body, which reads as a broken exporter
rather than a missing install.
The Docker image installs the extra (
--extra metrics), so image-based andotari.ai deployments are unaffected and a scrape stays one config flag away. A
source install that sets
enable_metricsneedspip install gateway[metrics].How to test it locally
tests/unit/test_gateway_metrics.pyis unchanged and still pins the exposedfamilies, so the present case is covered by what already covered it. The new
file covers the absent case: it blocks
prometheus_clientin a subprocess andasserts the module imports, that every recorder across the six modules that now
declare metrics no-ops, that
labels()chains, that the pool collector stillsubclasses and registers, and that the scrape body is empty, plus the three
startup outcomes (off and absent starts, on and absent refuses, on and present
starts).
To see the real absent case rather than the simulated one, which is what the OSS
edition smoke gate installs:
Confirmed on the rebase:
False, the app builds, and the last line raises withpip install gateway[metrics].uv run --frozen --no-dev python scripts/oss_edition_smoke.pyalso passes end to end in that same no-extrasenvironment.
Run locally:
make lint,make typecheck, the OSS edition smoke gate, and thefull
tests/unitsuite. The integration suite was left to CI; nothing heretouches a route, a model or a migration.
Notes for review
uv lockon this machine(uv 0.11.7) rewrites ~1000 lines of
uv.lock, addingsys_platformmarkersacross every package, none of it related to this change. The lines this
change actually needs were applied by hand instead, and
uv lock --checkaccepts the result. Worth a second pair of eyes, and worth knowing that
whoever next runs
uv lockhere will produce that churn.uv sync --frozen --no-devwith no extras, so from here on it boots andserves a completion with prometheus absent. The no-op branch cannot rot into
dead code.
gateway[metrics], nototari[metrics]. The distribution name isgateway, which is whatdocs/files.mdalready uses for theocrextra.Noting in passing that
S3FileStoresayspip install otari[s3], which lookswrong for the same reason; left alone as out of scope.
enable_metrics,so that counters stop incrementing when it is off. The metric objects are
module-level and built at import time, and the import chain reaches them
before config is loaded, so that would need an indirection layer at every
declaration and call site. The import is the cost worth removing; an increment
on an in-memory float is not.
^buildand^refactorare both skipped incliff.toml, butprotect_breaking_commits = true, so the!is what keepsthis in the release notes. It should be there, since an upgrade can refuse to
start. If a maintainer would rather not mark it breaking, it needs a different
type to stay visible.
PR Type
Relevant issues
Follow-up to #1205 (#1177), now merged. Discussed there; no issue filed yet.
Checklist
tests/unit,tests/integration).make lint,make typecheck,make test). Lint, typecheck, the OSS edition smoke gate and the full unit suite were run; the integration suite was left to CI.AI Usage
AI Model/Tool used: Claude Code (Claude Opus 5)
Any additional AI details you'd like to share: Opened as a reference draft at
the request of the repository owner, to accompany the packaging comment on #1205.
The agent wrote the seam, the validation, the tests and the docs, ran lint,
typecheck, the smoke gate and the unit suite, and verified the absent case
against a real runtime-only environment rather than only the subprocess
simulation.
🤖 Generated with Claude Code
Summary
prometheus-clientan optionalmetricsextra.enable_metricsis enabled without the extra.Source installations that enable metrics must now install
gateway[metrics].