Merge pull request #2380 from thepetk/fix-metrics-with-proxy

RSPEED-3444: fix REST API metrics middleware route discovery and root_path handling

Author: tisnik

src/app/main.py

@@ -10,7 +10,8 @@
 from fastapi.middleware.cors import CORSMiddleware
 from fastapi.responses import JSONResponse
 from ogx_client import APIConnectionError, AsyncOgxClient
-from starlette.routing import Mount, Route, WebSocketRoute
+from fastapi.routing import iter_route_contexts
+
 from starlette.types import ASGIApp, Message, Receive, Scope, Send
 
 import version
@@ -212,7 +213,7 @@ async def __call__(self, scope: Scope, receive: Receive, send: Send) -> None:
         # requests with the full prefixed path (/api/lightspeed/v1/infer) but
         # app_routes_paths contains only application-level paths (/v1/infer).
         # Strip the prefix so the path check and metric labels match the routes.
-        root_path = scope.get("root_path", "")
+        root_path: str = app.root_path
         path: str = scope["path"]
         if root_path and path.startswith(root_path + "/"):
             path = path[len(root_path) :]
@@ -292,9 +293,9 @@ async def send_wrapper(message: Message) -> None:
 routers.include_routers(app)
 
 app_routes_paths = [
-    route.path
-    for route in app.routes
-    if isinstance(route, (Mount, Route, WebSocketRoute))
+    rc.original_route.path
+    for rc in iter_route_contexts(app.routes)
+    if hasattr(rc.original_route, "path") and rc.original_route.path
 ]
 
 # Register pure ASGI middlewares.  Middleware execution order is the reverse of

tests/unit/app/test_main_middleware.py

@@ -9,7 +9,14 @@
 from pytest_mock import MockerFixture
 from starlette.types import Message, Receive, Scope, Send
 
-from app.main import GlobalExceptionMiddleware, RestApiMetricsMiddleware
+from app.main import (
+    GlobalExceptionMiddleware,
+    RestApiMetricsMiddleware,
+    app_routes_paths,
+)
+from app.main import (
+    app as fastapi_app,
+)
 from models.api.responses.error import InternalServerErrorResponse
 
 
@@ -189,6 +196,7 @@ async def test_rest_api_metrics_strips_root_path(
 ) -> None:
     """Middleware must strip root_path so prefixed requests still match routes."""
     mocker.patch("app.main.app_routes_paths", ["/v1/infer"])
+    mocker.patch.object(fastapi_app, "root_path", "/api/lightspeed")
     mock_measure_duration = mocker.patch(
         "app.main.recording.measure_response_duration", return_value=nullcontext()
     )
@@ -201,9 +209,9 @@ async def ok_app(_scope: Scope, _receive: Receive, send: Send) -> None:
     middleware = RestApiMetricsMiddleware(ok_app)
     collector = _ResponseCollector()
 
-    # Simulate 3scale forwarding /api/lightspeed/v1/infer with root_path set.
+    # Simulate 3scale forwarding /api/lightspeed/v1/infer — scope carries no root_path.
     await middleware(
-        _make_scope("/api/lightspeed/v1/infer", root_path="/api/lightspeed"),
+        _make_scope("/api/lightspeed/v1/infer"),
         _noop_receive,
         collector,
     )
@@ -241,3 +249,57 @@ async def ok_app(_scope: Scope, _receive: Receive, send: Send) -> None:
     assert collector.status_code == 200
     mock_measure_duration.assert_called_once_with("/v1/infer")
     mock_record_call.assert_called_once_with("/v1/infer", 200)
+
+
+@pytest.mark.asyncio
+async def test_rest_api_metrics_uses_app_root_path_not_scope(
+    mocker: MockerFixture,
+) -> None:
+    """Middleware must read root_path from app.root_path, not scope["root_path"].
+
+    The scope carries an empty root_path while app.root_path holds the real prefix.
+    If the middleware reads from the scope it will not strip the prefix, the path
+    will not match any route, and no metric will be recorded — causing both
+    mock_measure_duration and mock_record_call assertions to fail.
+    """
+    mocker.patch("app.main.app_routes_paths", ["/v1/infer"])
+    mocker.patch.object(fastapi_app, "root_path", "/api/lightspeed")
+    mock_measure_duration = mocker.patch(
+        "app.main.recording.measure_response_duration", return_value=nullcontext()
+    )
+    mock_record_call = mocker.patch("app.main.recording.record_rest_api_call")
+
+    async def ok_app(_scope: Scope, _receive: Receive, send: Send) -> None:
+        await send({"type": "http.response.start", "status": 200, "headers": []})
+        await send({"type": "http.response.body", "body": b"ok"})
+
+    middleware = RestApiMetricsMiddleware(ok_app)
+    collector = _ResponseCollector()
+
+    # scope["root_path"] is explicitly empty while app.root_path is "/api/lightspeed".
+    # The middleware must use app.root_path to strip the prefix correctly.
+    scope = _make_scope("/api/lightspeed/v1/infer")
+    scope["root_path"] = ""
+    await middleware(scope, _noop_receive, collector)
+
+    assert collector.status_code == 200
+    mock_measure_duration.assert_called_once_with("/v1/infer")
+    mock_record_call.assert_called_once_with("/v1/infer", 200)
+
+
+# ---------------------------------------------------------------------------
+# app_routes_paths population
+# ---------------------------------------------------------------------------
+
+
+def test_app_routes_paths_contains_application_routes() -> None:
+    """app_routes_paths must include routes registered via include_router.
+
+    FastAPI >= 0.137 stores included routers as _IncludedRouter objects that
+    the old isinstance(route, (Mount, Route, WebSocketRoute)) filter silently
+    drops.  iter_route_contexts() resolves them correctly.  If this test fails
+    with only 4 entries (the FastAPI built-ins), the fix has been reverted.
+    """
+    assert "/liveness" in app_routes_paths
+    assert "/readiness" in app_routes_paths
+    assert len(app_routes_paths) > 4