mirror of
https://github.com/BerriAI/litellm.git
synced 2026-09-29 01:42:19 +00:00
fix(prometheus): answer /metrics without a trailing-slash redirect
app.mount("/metrics", ...) makes starlette answer the bare path with
307 Location: /metrics/. Prometheus and grafana alloy do not follow redirects,
so the scrape reads an empty body while up stays 1 - metric loss with nothing
logged.
Register both spellings as routes. ASGIRoute hands the raw ASGI triple to the
metrics app so its streaming, gzip and render-coalescing survive, instead of
buffering the whole scrape to build one Response.
Signed-off-by: hclsys <chenglunhu@gmail.com>
This commit is contained in:
parent
4aa3ff47fe
commit
7875f5b7a7
3 changed files with 80 additions and 3 deletions
|
|
@ -4553,7 +4553,10 @@ class PrometheusLogger(CustomLogger):
|
|||
from prometheus_client import REGISTRY
|
||||
|
||||
from litellm._logging import verbose_proxy_logger
|
||||
from litellm.integrations.prometheus_metrics_endpoint import make_metrics_asgi_app
|
||||
from litellm.integrations.prometheus_metrics_endpoint import (
|
||||
ASGIRoute,
|
||||
make_metrics_asgi_app,
|
||||
)
|
||||
from litellm.proxy.proxy_server import app
|
||||
|
||||
# Create metrics ASGI app
|
||||
|
|
@ -4566,8 +4569,20 @@ class PrometheusLogger(CustomLogger):
|
|||
else:
|
||||
metrics_app = make_metrics_asgi_app(REGISTRY)
|
||||
|
||||
# Mount the metrics app to the app
|
||||
app.mount("/metrics", metrics_app)
|
||||
# `app.mount("/metrics", ...)` makes starlette answer the bare `/metrics`
|
||||
# with `307 Location: /metrics/`. Prometheus and grafana alloy do not
|
||||
# follow redirects, so the scrape reads an empty body while `up` stays 1
|
||||
# -- metric loss with no error anywhere (#30079).
|
||||
#
|
||||
# Register both spellings as routes instead. `ASGIRoute` is what keeps
|
||||
# `metrics_app`'s streaming, gzip and render-coalescing intact -- a plain
|
||||
# function endpoint would have to buffer the whole scrape to hand back a
|
||||
# single Response.
|
||||
# `methods` is left at its default: starlette's `Route` resolves None to
|
||||
# exactly `["GET"]`, so naming it here would only add a mutable literal.
|
||||
metrics_route: Final = ASGIRoute(metrics_app)
|
||||
app.add_route("/metrics", metrics_route)
|
||||
app.add_route("/metrics/", metrics_route)
|
||||
verbose_proxy_logger.debug("Starting Prometheus Metrics on /metrics (no authentication)")
|
||||
|
||||
|
||||
|
|
|
|||
|
|
@ -79,6 +79,22 @@ def _chunks(body: bytes) -> Iterator[bytes]:
|
|||
return (body[start : start + RESPONSE_CHUNK_SIZE_BYTES] for start in range(0, len(body), RESPONSE_CHUNK_SIZE_BYTES))
|
||||
|
||||
|
||||
class ASGIRoute:
|
||||
"""Adapt an ASGI app so it can be registered with ``Starlette.add_route``.
|
||||
|
||||
Starlette calls a plain function endpoint as ``f(request)`` and expects a
|
||||
``Response`` back. A callable object is handed the raw ASGI triple instead,
|
||||
which lets the mounted app write its own streaming response rather than
|
||||
having the whole scrape buffered to build one.
|
||||
"""
|
||||
|
||||
def __init__(self, app: ASGIApp) -> None:
|
||||
self._app = app
|
||||
|
||||
async def __call__(self, scope: Scope, receive: Receive, send: Send) -> None:
|
||||
await self._app(scope, receive, send)
|
||||
|
||||
|
||||
def make_metrics_asgi_app(registry: CollectorRegistry) -> ASGIApp:
|
||||
renderer: Final = CoalescedScrapeRenderer(registry)
|
||||
|
||||
|
|
|
|||
|
|
@ -274,3 +274,49 @@ async def test_a_finishing_render_does_not_evict_another_that_is_still_in_flight
|
|||
assert collector.collect_calls == 1, "an unrelated render finishing evicted the render still in flight"
|
||||
for response in responses:
|
||||
assert b"gated_metric" in response.content
|
||||
|
||||
|
||||
def test_both_metrics_paths_answer_without_a_redirect(monkeypatch: pytest.MonkeyPatch):
|
||||
"""`app.mount("/metrics", ...)` answers the bare path with 307 Location:
|
||||
/metrics/. Prometheus and grafana alloy do not follow redirects, so the
|
||||
scrape reads an empty body while `up` stays 1 (#30079). Both spellings must
|
||||
return the payload directly.
|
||||
|
||||
Drives the real `_mount_metrics_endpoint` against a stand-in app so the
|
||||
wiring itself is covered, not just `ASGIRoute` in isolation.
|
||||
"""
|
||||
import sys
|
||||
import types
|
||||
|
||||
from fastapi import FastAPI
|
||||
from fastapi.testclient import TestClient
|
||||
|
||||
# Import before the stub goes in. `_mount_metrics_endpoint` resolves
|
||||
# `litellm.proxy.proxy_server` lazily, and standing a fake module in that
|
||||
# slot while this module graph is still being built changes what the real
|
||||
# imports underneath it resolve to.
|
||||
from litellm.integrations.prometheus import PrometheusLogger
|
||||
|
||||
app: Final = FastAPI()
|
||||
proxy_server: Final = types.ModuleType("litellm.proxy.proxy_server")
|
||||
proxy_server.app = app # type: ignore[attr-defined]
|
||||
monkeypatch.setitem(sys.modules, "litellm.proxy.proxy_server", proxy_server)
|
||||
|
||||
PrometheusLogger._mount_metrics_endpoint()
|
||||
|
||||
assert {route.path for route in app.router.routes if "metrics" in getattr(route, "path", "")} == {
|
||||
"/metrics",
|
||||
"/metrics/",
|
||||
}
|
||||
|
||||
client: Final = TestClient(app, follow_redirects=False)
|
||||
for path in ("/metrics", "/metrics/"):
|
||||
response = client.get(path)
|
||||
# 200 rather than the 307 the mount used to emit. What the registry
|
||||
# holds is deliberately not asserted -- this shares the process-wide
|
||||
# default REGISTRY, so another test may legitimately have drained it.
|
||||
assert response.status_code == 200, path
|
||||
assert response.headers["content-type"].startswith("text/plain"), path
|
||||
|
||||
# The streaming app still owns the response, so its behaviour survives.
|
||||
assert client.get("/metrics", headers={"accept-encoding": "gzip"}).headers["content-encoding"] == "gzip"
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue