fix: mount FastStream metrics for an injected collector registry - #238
Merged
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #229.
FastStreamConfigacceptsprometheus_collector_registry, and__post_init__prefers an injectedregistry over a fresh one. But
is_configuredrequiredprometheus_middleware_cls, so a servicethat injected a registry holding its own collectors and did not use the broker middleware got no
endpoint to scrape them from. The registry was accepted and did nothing, and the skip was silent
because config-level skips are deliberately quiet.
The gate now accepts either signal:
Why option 1, and not the others
The issue listed four possibilities and called option 1 "slightly odd as a rule to explain". Reading
the sibling bootstrappers made it not odd at all. The rule is mount the endpoint when something
will populate the registry: the broker middleware populates it, an injected registry arrives
already populated, and with neither there is nothing to scrape.
Three facts settle it, none of which are in the issue:
is_configured. FastAPI, Litestarand FastMCP all mount on the default path without asking. (The override at
litestar_bootstrapper.py:290is the Swagger instrument, not Prometheus.)health_checks_enableddefaults toTrue, so/health/is exposed unasked. The issue's worry about changing the HTTP surface is weaker than itlooks.
prometheus_client.REGISTRYliterally; FastAPI'sInstrumentatorand Litestar'sPrometheusControllerboth fall back to it. Only FastStream builds a private registry. Sodeleting the override for full symmetry with FastStream skips the whole OpenTelemetry instrument when
opentelemetry_middleware_clsis unset #226 would mount an endpoint serving an emptyregistry for every app that uses neither the middleware nor an injection.
Fact 3 is why the asymmetry survives, and it turns a special case into a consequence of the
private-registry design. It also disposes of the rest:
prometheus_metrics_pathto""on FastStream) swaps a gate asymmetry for aconfig-default one, making FastStream the only framework whose default path differs, and breaks
anyone relying on the default today.
prometheus_collector_registryaccepted-and-inert, which isthe exact complaint accepted as a bug in FastStream skips the whole OpenTelemetry instrument when
opentelemetry_middleware_clsis unset #226.Tests
Red-to-green, verified by reverting only
faststream_bootstrapper.pytomain: two tests go red,and the middleware case stays green.
The injected-registry test is parametrized over
middleware_clsrather than duplicated, and assertsthe counter's name appears in the response body, so it proves the mount serves the injected
registry rather than a fresh one. The third row is pinned by its own test: it is the behaviour the
issue was protecting, and it guards against a later drift into deleting the override.
That skip test also pins
not_configured_reason, which is user-visible through the documentedskipped_instrumentsAPI andbuild_summary(). That assertion independently fails onmain, so thestring is covered rather than assumed:
Review changes
is not Nonerather than truthiness, matching__post_init__two lines above. Identical behaviourtoday, since
CollectorRegistrydefines no__bool__or__len__, but a subclass defining__len__would have silently un-configured the instrument.orchain was ambiguous once a second clause was appended.bootstrap()guards on the application having one.
No ADR
One-line gate, so it fails "hard to reverse", and the "surprising without context" test is answered
by the configuration docs, which now state the rule and the private-registry reason behind it. Only
"a real trade-off" passes, and the practice here wants all three.
Verification
ruff format,ruff check,ty, 309 tests (up from 307) andmkdocs build --strict, all clean.