feat(serve): route-log per-request routing stats for profile-config serve#118
Conversation
…erve Support --routing-log-file on `serve --config` (previously rejected), add per-request cache-token breakdown, and expose GET /v1/routing/session-stats for session-scoped aggregates. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Brian Newsom <brnewsom@nvidia.com>
5f56008 to
31aed1d
Compare
WalkthroughRouting logs now capture cache-related token metrics and actual models, support session aggregation through a new HTTP endpoint, and integrate routing-log processing with v2 ChangesRouting Log Statistics
Estimated code review effort: 4 (Complex) | ~45 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
switchyard/lib/endpoints/__init__.py (1)
26-59: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
RoutingLogStatsEndpointis listed in__all__but missing from__getattr__— package-level import will raiseAttributeError.
__all__now advertisesRoutingLogStatsEndpoint, but the lazy-loader__getattr__has no matching branch, sofrom switchyard.lib.endpoints import RoutingLogStatsEndpointfails at runtime withAttributeError: module 'switchyard.lib.endpoints' has no attribute 'RoutingLogStatsEndpoint'. This contradicts the stated goal of exposing the endpoint through the package.🐛 Proposed fix
elif name == "ResponsesEndpoint": from switchyard.lib.endpoints.responses_endpoint import ( ResponsesEndpoint, ) return ResponsesEndpoint + elif name == "RoutingLogStatsEndpoint": + from switchyard.lib.endpoints.routing_log_stats_endpoint import ( + RoutingLogStatsEndpoint, + ) + return RoutingLogStatsEndpoint raise AttributeError(f"module {__name__!r} has no attribute {name!r}")🤖 Prompt for 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. In `@switchyard/lib/endpoints/__init__.py` around lines 26 - 59, Update the lazy-loader __getattr__ to add a RoutingLogStatsEndpoint branch that imports and returns RoutingLogStatsEndpoint from its endpoint module, matching the existing lazy-loading branches and making the symbol advertised by __all__ available through package-level imports.
🧹 Nitpick comments (2)
tests/test_serve_profile_config.py (1)
140-140: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert CLI-to-route-table processor propagation.
This now passes
routing_log_file, but the test only checks registered models and never verifies that_cmd_serve_profile_configattachedRoutingLogResponseProcessor. The later test injects the processor directly, so a regression in this forwarding path would go undetected.🤖 Prompt for 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. In `@tests/test_serve_profile_config.py` at line 140, Update the test using _serve_args and _cmd_serve_profile_config to assert that the generated route table includes RoutingLogResponseProcessor when routing_log_file is provided. Keep the existing registered-model assertions and ensure the assertion verifies CLI argument propagation rather than relying on the later test’s direct processor injection.switchyard/lib/profiles/switchyard_adapter.py (1)
59-84: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocstring doesn't mention the new failure mode.
call()'s docstring says only "Run the wrapped Profile and translate its response for the caller." but the method can now raiseSwitchyardProcessorErrorif a processor throws or returns a non-ChatResponse. As per coding guidelines,switchyard/**/*.pydocstrings should "document behavior, important invariants, and relevant errors."📝 Proposed docstring update
- """Run the wrapped Profile and translate its response for the caller.""" + """Run the wrapped Profile and translate its response for the caller. + + Raises: + SwitchyardProcessorError: If a response processor raises or returns + a value that is not a ``ChatResponse``. + """🤖 Prompt for 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. In `@switchyard/lib/profiles/switchyard_adapter.py` around lines 59 - 84, Update the `call()` method docstring to document that response processors may cause `SwitchyardProcessorError` when they raise an exception or return a value that is not a `ChatResponse`; retain the existing description of running the profile and translating its response.Source: Coding guidelines
🤖 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 `@docs/cli_reference.md`:
- Around line 229-231: Add --routing-log-file PATH to the v2 command synopsis
near the existing flags, keeping the documented routing-log-file option
consistent between the synopsis and flags table.
In `@tests/test_serve_profile_config.py`:
- Around line 95-103: Update the HTTP mocking in the affected test in
tests/test_serve_profile_config.py to use respx instead of the hand-written
loopback server. Replace the socket-binding server setup and route responses
through respx while preserving the test’s existing provider response payload and
assertions.
---
Outside diff comments:
In `@switchyard/lib/endpoints/__init__.py`:
- Around line 26-59: Update the lazy-loader __getattr__ to add a
RoutingLogStatsEndpoint branch that imports and returns RoutingLogStatsEndpoint
from its endpoint module, matching the existing lazy-loading branches and making
the symbol advertised by __all__ available through package-level imports.
---
Nitpick comments:
In `@switchyard/lib/profiles/switchyard_adapter.py`:
- Around line 59-84: Update the `call()` method docstring to document that
response processors may cause `SwitchyardProcessorError` when they raise an
exception or return a value that is not a `ChatResponse`; retain the existing
description of running the profile and translating its response.
In `@tests/test_serve_profile_config.py`:
- Line 140: Update the test using _serve_args and _cmd_serve_profile_config to
assert that the generated route table includes RoutingLogResponseProcessor when
routing_log_file is provided. Keep the existing registered-model assertions and
ensure the assertion verifies CLI argument propagation rather than relying on
the later test’s direct processor injection.
🪄 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: CHILL
Plan: Enterprise
Run ID: 5c9a1de8-6083-44c5-b750-66676dbe8180
📒 Files selected for processing (10)
benchmark/run_manifest.pydocs/cli_reference.mdswitchyard/cli/switchyard_cli.pyswitchyard/lib/endpoints/__init__.pyswitchyard/lib/endpoints/routing_log_stats_endpoint.pyswitchyard/lib/processors/routing_log_response_processor.pyswitchyard/lib/profiles/switchyard_adapter.pytests/test_routing_log_response_processor.pytests/test_run_manifest.pytests/test_serve_profile_config.py
|
Addressed the remaining review feedback in
I kept the loopback provider mock because this path uses Rust Validation: Ruff passed; mypy passed (167 files); strict MkDocs passed; focused tests passed (13); full hermetic suite passed (2,020 passed, 12 skipped, 43 integration tests deselected). No live provider calls were made. |
Signed-off-by: Brian Newsom <brnewsom@nvidia.com>
a576369 to
5c371f7
Compare
|
@BrianNewsom can you share a bit more about the use-case for the endpoint? I can think of some ideas of how this would be useful - but making sure I understand your perspective and end goal for the endpoint. |
Signed-off-by: Brian Newsom <brnewsom@nvidia.com>
Updated the diff with a "why" section and rebased! |
Signed-off-by: Brian Newsom <brnewsom@nvidia.com>
2 participants
Why
When a switchyard server is run to serve a proxy, it can keep ongoing cost statistics and write them to a file. If run in the context of an evaluation, this proxy then only has aggregate evaluation level statistics, and loses fidelity of any individual request.
This diff allows segmentation of the routing stats so that a consumer can have task level cost attribution.
Summary
Extends the
--routing-log-filerouting log (added in #116) in three ways:--routing-log-fileonserve --config. Previously this path raisedSystemExitand rejected the flag outright; it now wires aRoutingLogResponseProcessorinto every served profile and target.run_manifesttask rollup) now carrycached_tokensandcache_creation_tokensalongside prompt/completion/total, extracted from OpenAI Chat, Responses, and Anthropic usage shapes.GET /v1/routing/session-stats?session_id=.... A new endpoint returns session-scoped model/token aggregates from the durable log, contributed viaget_endpoint()on the processor (available on both serve paths).#116 intentionally rejected
--routing-log-fileonserve --configwith aSystemExit. This PR removes that guard and implements the feature there instead. This is a deliberate scope expansion, not an oversight — calling it out explicitly so it's a reviewed decision.Why the processor attaches at the adapter (
ProfileSwitchyard)serve --configserves a heterogeneous mix — Rust-defined profiles (passthrough,random-routing, ...), Python-defined profiles, and passthrough targets. The Rust and passthrough profiles have no Python-visible response chain to inject into, so the only universal seam for a Python cross-cutting response processor isProfileSwitchyard.call, which every profile funnels through before translation. The processor loop there mirrors the existingchain.py/noop.pypattern (SwitchyardProcessorErrorwrapping +ChatResponsetype guard). Processors run afterprofile.run()returns, i.e. after any in-profile evict/retry, so the log records the final response.Behavior with no session_id
Logging is unchanged and never gated on session identity: every request still appends one line, with
session_id: nullwhen absent. Such records are simply invisible to the new session-scoped endpoint (they can't match a queriedsession_id) and still roll up under their task inrun_manifest.Notes
snapshot_sessionre-reads and re-parses the full log per call (documented in its docstring) — sized for benchmark runs, not a production server with millions of sessions. The read is lock-free so snapshots don't contend with request logging.RoutingLogStatsEndpointadded toswitchyard/lib/endpoints/__init__.__all__for consistency with sibling contributed endpoints.Validation
ruff checkclean,mypyclean (167 files)test_classifier_planner_chain—ProxyContextmissingrecord_submodel_call, a stale-Rust-build issue that fails identically onmain)Summary by CodeRabbit
New Features
serve --config.GET /v1/routing/session-stats.Documentation