RSPEED-2941: fix REST API metrics when root_path is set#1614
Conversation
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughThe changes modify Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
ca1dede to
e096022
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/app/main.py`:
- Line 12: Replace the private import get_route_path with a local path
computation: remove the import of get_route_path and wherever
get_route_path(scope) is used (the call site referencing scope), compute the
route path using scope["path"].removeprefix(scope.get("root_path", "")) and use
that local variable (e.g., path) instead of calling the private helper; ensure
the replacement uses scope, "root_path", and removeprefix so behavior matches
Starlette's internal logic.
In `@tests/unit/app/test_main_middleware.py`:
- Around line 15-26: Change _make_scope's return type from dict to the ASGI
Scope type by updating its signature to return Scope and importing Scope (e.g.,
from starlette.types import Scope); update the local variable annotation to
scope: Scope = { ... } so the constructed dict matches the Scope type (adjust
header types if needed) and keep the rest of the helper unchanged.
🪄 Autofix (Beta)
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
Run ID: 4b252b5f-3677-4142-829f-ed66be625d09
📒 Files selected for processing (2)
src/app/main.pytests/unit/app/test_main_middleware.py
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: E2E: library mode / ci / group 2
- GitHub Check: E2E: server mode / ci / group 2
- GitHub Check: E2E: server mode / ci / group 3
- GitHub Check: E2E: server mode / ci / group 1
- GitHub Check: E2E: library mode / ci / group 3
- GitHub Check: E2E: library mode / ci / group 1
🧰 Additional context used
📓 Path-based instructions (3)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Use absolute imports for internal modules:from authentication import get_auth_dependency
Import FastAPI dependencies with:from fastapi import APIRouter, HTTPException, Request, status, Depends
Import Llama Stack client with:from llama_stack_client import AsyncLlamaStackClient
Checkconstants.pyfor shared constants before defining new ones
All modules start with descriptive docstrings explaining purpose
Uselogger = get_logger(__name__)fromlog.pyfor module logging
Type aliases defined at module level for clarity
Use Final[type] as type hint for all constants
All functions require docstrings with brief descriptions
Complete type annotations for parameters and return types in functions
Usetyping_extensions.Selffor model validators in Pydantic models
Use modern union type syntaxstr | intinstead ofUnion[str, int]
UseOptional[Type]for optional type hints
Use snake_case with descriptive, action-oriented function names (get_, validate_, check_)
Avoid in-place parameter modification anti-patterns; return new data structures instead
Useasync deffor I/O operations and external API calls
HandleAPIConnectionErrorfrom Llama Stack in error handling
Use standard log levels with clear purposes: debug, info, warning, error
All classes require descriptive docstrings explaining purpose
Use PascalCase for class names with standard suffixes: Configuration, Error/Exception, Resolver, Interface
Use ABC for abstract base classes with@abstractmethoddecorators
Use@model_validatorand@field_validatorfor Pydantic model validation
Complete type annotations for all class attributes; use specific types, notAny
Follow Google Python docstring conventions with Parameters, Returns, Raises, and Attributes sections
Files:
src/app/main.pytests/unit/app/test_main_middleware.py
src/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Pydantic models extend
ConfigurationBasefor config,BaseModelfor data models
Files:
src/app/main.py
tests/{unit,integration}/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
tests/{unit,integration}/**/*.py: Use pytest for all unit and integration tests
Do not use unittest; pytest is the standard for this project
Usepytest-mockfor AsyncMock objects in tests
Use markerpytest.mark.asynciofor async tests
Unit tests require 60% coverage, integration tests 10%
Files:
tests/unit/app/test_main_middleware.py
🔇 Additional comments (2)
src/app/main.py (1)
158-184: Path normalization is correctly applied before metrics route checks.Using the normalized route path before
app_routes_pathsmembership and metric labels fixes the root-path mismatch and preserves expected counter/histogram behavior.tests/unit/app/test_main_middleware.py (1)
183-236: Great regression coverage for root_path path-label behavior.These tests validate both prefixed (
root_pathset) and non-prefixed flows and assert the exact metric labels/status values expected from the middleware fix.
The metrics middleware reads scope["path"] directly, which contains the full request path including any root_path prefix (e.g., /api/lightspeed/v1/infer). Since app_routes_paths stores only application-level paths (e.g., /v1/infer), the path check fails and metrics are never recorded for any API traffic routed through 3scale. Use get_route_path(scope) from starlette._utils to strip the root_path prefix before matching, consistent with how Starlette Router matches routes internally. Signed-off-by: Major Hayden <major@redhat.com>
e096022 to
3951e59
Compare
Description
The
RestApiMetricsMiddlewarereadsscope["path"]directly, which contains the full request path including anyroot_pathprefix set by FastAPI (e.g.,/api/lightspeed/v1/infer). Sinceapp_routes_pathsstores only application-level paths (e.g.,/v1/infer), the middleware's path check fails and both thels_rest_api_calls_totalcounter andls_response_duration_secondshistogram are never recorded for API traffic routed through 3scale.This causes the Grafana "Request Rate by Path" and "Error Rate" panels to show blank, and "P95 Response Latency" to show only
/metrics(the only path where Prometheus scrapes bypass 3scale and hit the pod directly).The fix uses
get_route_path(scope)fromstarlette._utilsto strip theroot_pathprefix before matching, consistent with how Starlette's Router matches routes internally.Type of change
Tools used to create PR
Related Tickets & Documents
Checklist before requesting a review
Testing
pytest tests/unit/app/test_main_middleware.py -vtest_rest_api_metrics_strips_root_path: verifies that whenroot_path="/api/lightspeed"and path is/api/lightspeed/v1/infer, metrics are recorded with label/v1/infertest_rest_api_metrics_no_root_path_unchanged: verifies no behavioral change whenroot_pathis not setROOT_PATH=/api/lightspeedand verify Grafana panels populateSummary by CodeRabbit
Bug Fixes
Tests