Skip to content
PR Quorum
Sign inStart free
← Review gallery
BerriAI/litellm #33720openreviewed 5h ago

fix(prometheus): serve /metrics directly without 307 redirect

by Bryan-eng-lng+338 34 filesreviewed in 1m 5sView PR on GitHub
needs attentionThe panel surfaced issues that deserve attention before merge.6 findings · 1 high+

Reviewer panel

Correctness
deepseek/deepseek-v4-pro
4 found
Security
deepseek/deepseek-v4-pro
0 found
Architecture
deepseek/deepseek-v4-pro
2 found

Findings

01Hhighconf 85%

ASGI send/receive protocol violation: nonlocal body may be overwritten by multiple send calls

litellm/integrations/prometheus.py:3970

The inner send closure captures body via nonlocal and appends to it on each http.response.body message. The ASGI spec allows the application to send multiple body messages (e.g., for streaming). If metrics_app sends more than one body message, body += message.get("body", b"") will concatenate them, which is correct. However, the send closure also captures status_code and headers from http.response.start. If the ASGI app sends http.response.start more than once (which is a protocol violation by the app, but possible), the last one wins. More critically, the receive closure always returns an empty body and more_body=False. If metrics_app calls receive() more than once, it will get the same empty message repeatedly, which is fine. The real risk is that the send closure does not handle the http.response.start message being sent after some body messages (out of order), which would cause status_code to be updated after body has already been accumulated, potentially leading to a 200 response with a body but a status code from a later start message. This is unlikely with prometheus_client's make_asgi_app(), but the code is fragile.

02Mmedconf 85%

ASGI send/receive protocol violation may cause silent data loss

litellm/integrations/prometheus.py:3960

The _metrics_no_slash_view function manually drives the ASGI app with a custom send callable that accumulates body chunks. However, the ASGI spec allows send to be called multiple times with http.response.body messages (streaming). The current implementation concatenates all body chunks, which is correct, but it unconditionally strips the content-length header from the ASGI response and does not set a new one. If the ASGI app sends multiple body chunks, the final Response object will have no content-length header, which may cause clients to hang or misinterpret the response. Additionally, the receive callable always returns an empty body and more_body=False, which is correct for a GET request but may not match the original request scope if the route is ever used for POST requests. The fix should either preserve the original content-length header or let Starlette's Response compute it from the accumulated body.

03Mmedconf 80%

Content-Length header stripped but response may have incorrect length

litellm/integrations/prometheus.py:3971

The code strips the content-length header from the original ASGI response and lets Starlette set its own. However, if metrics_app sends multiple http.response.body messages, the accumulated body may be correct, but the original content-length (if present) would have reflected only the first body chunk. Stripping it is correct in that case. But if metrics_app sends a single body message with a correct content-length, stripping it forces Starlette to recompute it, which is also correct. The risk is that if metrics_app sends a transfer-encoding: chunked or no content-length at all, the behavior is fine. However, the code unconditionally strips content-length without checking if it matches the accumulated body length. This is not a bug per se, but it masks potential mismatches. More importantly, the code does not handle the case where metrics_app sends a response without a body (e.g., 304 Not Modified). In that case, body remains b"" and the response is empty, which is correct. No immediate bug, but the stripping logic is fragile.

04Mmedconf 80%

next() with generator may raise StopIteration if no deployment has the metric

litellm/router_strategy/simple_shuffle.py:44

The code uses next(..., None) with a default of None, so it safely returns None if no deployment has the metric. This is correct. However, the generator expression (m["litellm_params"].get(weight_by) for m in healthy_deployments if m["litellm_params"].get(weight_by) is not None) evaluates m["litellm_params"].get(weight_by) twice for each deployment that has the metric: once in the if filter and once in the expression. This is inefficient but not a bug. The real issue is that if healthy_deployments is empty, next() returns None immediately, which is correct. No bug here.

05Llowconf 90%

Unnecessary complexity: manual ASGI bridge duplicates framework functionality

litellm/integrations/prometheus.py:3960

The _metrics_no_slash_view function manually implements an ASGI-to-Request/Response bridge by driving the ASGI app with custom receive and send callables. This is fragile and duplicates functionality that Starlette already provides (e.g., starlette.middleware.wsgi.WSGIMiddleware or simply mounting the ASGI app at both paths). A simpler and more maintainable approach would be to mount the ASGI app at both /metrics and /metrics/ using Starlette's built-in routing, or to use a small middleware that rewrites the path. The current implementation introduces a custom ASGI protocol handling loop that must correctly implement the full ASGI spec (e.g., handling http.disconnect, more_body flags, etc.), increasing the risk of subtle bugs.

06Llowconf 75%

receive closure always returns empty body; may break ASGI apps that expect request body

litellm/integrations/prometheus.py:3962

The receive closure unconditionally returns {"type": "http.request", "body": b"", "more_body": False}. This is fine for GET requests to /metrics, which have no body. However, if the mounted metrics_app ever needs to read the request body (e.g., for a POST-based metrics endpoint), it will get an empty body. This is not a bug for the current Prometheus metrics endpoint, but it makes the _metrics_no_slash_view less reusable. If someone later changes the route to accept POST, this will break silently.

Want this on your own pull requests?
The same reviewer panel runs on every PR — one focused review, no noise.
Start freeLive demo