RSPEED-3444: fix REST API metrics middleware route discovery and root_path handling - #2380
RSPEED-3444: fix REST API metrics middleware route discovery and root_path handling#2380thepetk wants to merge 1 commit into
Conversation
Walkthrough
ChangesRoot path metrics matching
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 7✅ Passed checks (7 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 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 |
fastapi app instead of ASGI scope
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/unit/app/test_main_middleware.py`:
- Around line 253-288: The new test duplicates
test_rest_api_metrics_strips_root_path; remove
test_rest_api_metrics_uses_app_root_path_not_scope or change it to use a
conflicting non-empty scope root_path while keeping fastapi_app.root_path set,
then assert metrics use the application root.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: c7065dbc-1fec-44da-9aed-6ca74752b197
📒 Files selected for processing (2)
src/app/main.pytests/unit/app/test_main_middleware.py
📜 Review details
⏰ Context from checks skipped due to timeout. (5)
- GitHub Check: E2E: server mode / ci / group 2
- GitHub Check: E2E: library mode / ci / group 1
- GitHub Check: E2E: server mode / ci / group 3
- GitHub Check: E2E: library mode / ci / group 2
- GitHub Check: E2E Tests for Lightspeed Evaluation job
🧰 Additional context used
📓 Path-based instructions (3)
**/*
📄 CodeRabbit inference engine (Custom checks)
**/*: Flag meaningful O(n^2)+ algorithms on non-trivial inputs, including handlers and Kubernetes list operations.
Flag N+1 patterns that list items and then query once per item, including Kubernetes API and database access.
Flag expensive work inside loops, including API calls, JSON parsing, and regex compilation.
Flag unbounded growth in caches, watchers, or buffers when eviction or limits are missing.
Flag missing pagination or limits on list operations and API endpoints.
Flag secrets or tokens logged in plaintext or hardcoded in source.
Flag API endpoints missing authentication or authorization.
Flag injection vulnerabilities, including SQL injection, command injection, and path traversal.
Flag sensitive data leaked in API responses, WebSocket messages, or logs.
Flag Kubernetes Secrets and Red Hat secrets missing OwnerReferences.
Files:
src/app/main.pytests/unit/app/test_main_middleware.py
src/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
src/**/*.py: Use absolute imports for internal modules and follow the prescribed FastAPI and Llama Stack import conventions.
All modules must begin with descriptive docstrings; uselogger = get_logger(__name__)fromlog.pyfor module logging; package__init__.pyfiles must contain brief package descriptions.
Define shared constants in the centralconstants.pymodule, add descriptive comments, and annotate constants withFinal[type].
Use complete type annotations for function parameters, return types, class attributes, and type aliases; prefer specific types overAny, use modern union syntax, and usetyping_extensions.Selffor model validators.
All functions and classes require descriptive Google-style docstrings, including appropriateParameters,Returns,Raises, andAttributessections.
Use descriptive snake_case, action-oriented function names such asget_,validate_, andcheck_; use PascalCase class names with standard suffixes such asConfiguration,Error/Exception,Resolver, andInterface.
Avoid modifying input parameters in place; return a newly constructed data structure instead.
Useasync deffor I/O operations and external API calls; API endpoints should raise FastAPIHTTPExceptionwith appropriate status codes and handle Llama StackAPIConnectionError.
Usefrom log import get_loggerand standard logger levels:debugfor diagnostics,infofor general execution,warningfor unexpected conditions or potential problems, anderrorfor serious failures.
Configuration models must extendConfigurationBase, setextra="forbid"to reject unknown fields, use Pydantic validators for custom validation, and use types such asOptional[FilePath],PositiveInt, andSecretStrwhere appropriate.
Abstract interfaces must useABCand@abstractmethoddecorators.
Never commit secrets or keys; use environment variables for sensitive data.
Files:
src/app/main.py
tests/unit/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Use pytest for unit tests, shared fixtures in
conftest.py,pytest-mockfor mocks,pytest.mark.asynciofor async tests, and maintain at least 60% unit-test coverage.
Files:
tests/unit/app/test_main_middleware.py
🧠 Learnings (3)
📚 Learning: 2026-06-24T13:45:37.249Z
Learnt from: Jdubrick
Repo: lightspeed-core/lightspeed-stack PR: 1971
File: src/utils/markdown_repair.py:31-36
Timestamp: 2026-06-24T13:45:37.249Z
Learning: In the lightspeed-stack repository, docstrings must use the section header name "Parameters:" (not "Args:") for function arguments, even if the project references Google Python docstring conventions. Ensure docstrings follow the project’s established "Parameters:" header format for any documented function parameters.
Applied to files:
src/app/main.pytests/unit/app/test_main_middleware.py
📚 Learning: 2026-07-06T15:26:18.398Z
Learnt from: Jdubrick
Repo: lightspeed-core/lightspeed-stack PR: 2071
File: src/models/config.py:2416-2422
Timestamp: 2026-07-06T15:26:18.398Z
Learning: In this repo’s Python code under src/**, don’t treat differences in string concatenation style as a style inconsistency when Black has effectively forced (or made clearer) use of explicit `+` string concatenation in multi-line logger/string expressions. If adjacent-literal implicit concatenation is avoided/changed specifically to accommodate Black’s formatting in these call sites, accept the `+` usage and don’t recommend converting it solely for consistency with nearby blocks that use implicit concatenation.
Applied to files:
src/app/main.py
📚 Learning: 2026-07-17T19:25:05.325Z
Learnt from: Jdubrick
Repo: lightspeed-core/lightspeed-stack PR: 2166
File: src/utils/saved_prompts.py:129-157
Timestamp: 2026-07-17T19:25:05.325Z
Learning: For any endpoint that handles saved prompts and calls `src/utils/saved_prompts.py::create_saved_prompt`, treat the endpoint as the validation boundary. Before calling `create_saved_prompt`, validate the incoming saved-prompt name and content, specifically using `validate_saved_prompt_name` and then persist (store) the normalized value it returns. Do not call `create_saved_prompt` with unvalidated/raw name/content.
Applied to files:
src/app/main.py
🔇 Additional comments (3)
src/app/main.py (1)
215-215: LGTM!tests/unit/app/test_main_middleware.py (2)
12-18: LGTM!
198-198: LGTM!Also applies to: 211-213
|
Will mark it again as "ready for review" once I complete a final round of tests |
1ebf238 to
d6fd77e
Compare
d6fd77e to
b5a3069
Compare
fastapi app instead of ASGI scope|
I see the same failures in many different PRs currently opened. I don't believe they are related to my changes tbh. |
PR updated. Was able to reproduce and verify the fix works |
Description
Fix REST API middleware-level metrics never recording.
FastAPI 0.137.0(fastapi#15745) refactoredinclude_router()to include routers with lazy_IncludedRouterwrapper objects inapp.routesinstead of copying individualRouteinstances. The old list comprehension filtered withisinstance(route, (Mount, Route, WebSocketRoute)), which silently drops_IncludedRouterobjects — leavingapp_routes_pathscontaining only the 4 built-in FastAPI routes (/openapi.json,/docs,/docs/oauth2-redirect,/redoc). No application route ever matched, so metrics were neverrecorded regardless of the
root_pathsetting.FastAPI was pinned to 0.141.1 in the LCORE-2922 dependency update, merged via PR #2358 / commit
18c0876d.Additionally,
RestApiMetricsMiddlewarereadroot_pathfromscope.get("root_path", ""). In older versions of FastAPI/Starlette the scope field was never populated by the framework for this use case, so it always returned"", the prefix-stripping logic never activated, and every prefixed path failed to be recorded.About the fix:
Replaced the route discovery list comprehension with FastAPI's official
iter_route_contexts()API (PR fastapi#15785, discussion #15791), which correctly resolves all registered routes including the lazy_IncludedRouterwrappers introduced in 0.137. We also readroot_pathfromapp.root_pathinstead of the ASGI scope, which is the actual source of the configured value. Together these two changes ensureapp_routes_pathsis fully populated and that proxy-prefixed paths are correctly stripped, so middleware-level metrics record for every API endpoint regardless of deployment configuration.Type of change
pyproject.toml+uv.lock]requirements.*.txtfor Konflux]Tools used to create PR
Related Tickets & Documents
Checklist before requesting a review
Testing
To reproduce:
On
main:service.root_path: /api/lightspeedconfigured (that simulates 3scale), then send requests using the full prefixed path the proxy will forward:/metricsafterwards:ls_rest_api_calls_totalshould have zero samples despite successful responses. Metrics lkels_llm_token_sent_totalshould be recorded cause they are being handled inside the scope of the endpoint.To verify the fix end-to-end:
After the fix, repeat the same requests.
/metricsshould show:Unit test coverage:
test_rest_api_metrics_strips_root_path— updated to patchapp.root_pathdirectly (scope carries noroot_path, matching actual runtime behaviour).test_rest_api_metrics_no_root_path_unchanged— unchanged; confirms emptyroot_pathdeployments are unaffected.test_rest_api_metrics_uses_app_root_path_not_scope— regression test; fails if the middleware is switched back to reading from the scope.test_app_routes_paths_contains_application_routes— new; assertsapp_routes_pathscontains application routes beyond the 4 FastAPI built-ins. Fails if theiter_route_contexts()call is reverted to the oldisinstancefilter.Summary by CodeRabbit
/v1/infer.