feat(spend): add a per-session auto-router benchmarks rollup - #35839
feat(spend): add a per-session auto-router benchmarks rollup#35839tin-berri wants to merge 9 commits into
Conversation
|
@greptileai can you reply here with a score and why that score? |
|
bugbot run |
|
Score: 4/5 The design is genuinely good — atomic upsert with all classification in SQL eliminates the read-modify-write race, the write path is off the critical request path (batched flush), tenant isolation via One correctness issue worth noting before merge: In misses: Final = counters.turns - hits
The more precise expression would be: This would make Everything else — the upsert SQL, the schema, the index choices (dedicated |
Greptile SummaryAdds a durable per-session auto-router rollup and an admin benchmarks endpoint that aggregates savings, session shape, and prompt-cache metrics
Confidence Score: 4/5The date-window overcounting and cross-pod ordering defects should be fixed before merging because they can make the new benchmark endpoint return incorrect metrics The endpoint sums lifetime session counters for sessions active in the selected range, and independently flushed turns can be classified against nonchronological state across pods Files Needing Attention: litellm/proxy/spend_tracking/auto_router_benchmarks.py, litellm/proxy/spend_tracking/auto_router_sessions.py, litellm/proxy/db/db_spend_update_writer.py
|
| Filename | Overview |
|---|---|
| litellm/proxy/spend_tracking/auto_router_sessions.py | Adds turn reduction and atomic session upserts, but cross-pod late arrivals can misclassify chronological session state |
| litellm/proxy/spend_tracking/auto_router_benchmarks.py | Adds typed benchmark derivation and aggregation, but date filtering includes lifetime counters from sessions merely active in the window |
| litellm/proxy/db/db_spend_update_writer.py | Stages and flushes auto-router turns through the spend writer; per-pod flushing contributes to the ordering issue |
| litellm/proxy/proxy_server.py | Adds an admin-only validated benchmarks endpoint and response model |
| litellm-proxy-extras/litellm_proxy_extras/migrations/20260804000000_add_auto_router_session_rollup/migration.sql | Creates the session rollup table and indexes consistently with the synchronized Prisma schemas |
| litellm/proxy/db/db_transaction_queue/spend_log_cleanup.py | Extends existing retention cleanup to prune inactive auto-router session rows by last activity |
Reviews (1): Last reviewed commit: "feat(spend): add a per-session auto-rout..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
cea0818 to
f3f954b
Compare
|
Redesigned rather than patched. All seven findings came from one root cause: the row stored a session-wide history that had to be serialized, read back and threaded before every write. Buckets are now decided by the tier's own cache record, held in a Consequences: cross-pod ordering can no longer change a bucket; expiry measures against the TTL the cache was written with; replays are priced on the cached prefix and charged on hits too; the window filters The middle bucket is deliberately "warm" rather than "same model": bucketing on what the previous turn did is what required the stored history, and it filed expired same-model turns where the warming panel could not see them. Measured on 400k sessions in a 30 day window the read aggregate is 60 ms. Eight mutations of the SQL predicates were each confirmed to fail a test in @greptileai can you reply here with a score and why that score? |
|
bugbot run |
|
Score: 4/5 The redesign is sound and the root cause — storing session-wide history that had to be read back before every write — has been correctly eliminated. Here is what earns the 4 and what holds it from 5. What earns the scoreThe core design is correct. Encoding classification inside the upsert means the write is atomic, two pods composing one session cannot produce a split brain, and there is no state to deserialise before the insert. Test strategy is the right one. SQL predicates that cannot be unit-tested in Python are covered by Retention is correct. Keying cleanup on Error handling is appropriate. Both the enqueue path ( The savings computation flows through What holds it from 5Duplicate key in the test fixture. In "expired_hits": 1,
"expired_hits": 0,Python dicts silently take the last value (
17 positional parameters in These are all minor relative to the quality of the core design and test coverage. The PR is ready for maintainer review. |
f3f954b to
ea8ed73
Compare
|
Fixed in ea8ed73, plus three findings from review. The out-of-order case was a real hole and a second round on one mechanism, so I wrote the state machine down before touching it rather than adjusting the predicate. A turn arriving before an already-recorded turn on the same tier has an unknowable cache state at its own time; a negative idle gap is not evidence of warmth. There are four guards, not three: model absent is a first visit, Walking that state machine also surfaced an adjacent hole nothing had flagged: a first visit to a new tier on an existing row, where the turn wrote no cache, fell through to The 17 positional parameters are gone. Placeholders are unpacked once from a single ordered Duplicate On the duplicated @greptileai can you reply here with a score and why that score? |
|
bugbot run |
|
5/5. All four previously flagged holes are closed, and each fix is verified by a test that was confirmed to fail without it. Out-of-order turns. The state machine is now formally correct: New-tier-no-write TTL fallthrough. Named parameters. Duplicate fixture key. Fixed, and the AST scan for other shadowed keys gives confidence this was the only one.
The integration test suite against real Postgres covers the eight state-machine mutations the description mentions, plus warming arithmetic, multi-flush accumulation, late-arrival non-rewind, and tenant isolation. The pure unit tests cover the math derivations and window clamping without a database. Coverage is thorough at both layers. |
ea8ed73 to
3bf3f72
Compare
|
Fixed in 3bf3f72, and the fix is in what the column means rather than in the guard.
Storing that also removed a condition rather than adding one. The old guard needed That leaves three cache facts on the turn, each meaning one thing: @greptileai can you reply here with a score and why that score? |
|
bugbot run |
|
5/5. The fix is correct and well-scoped. The root cause and fix are precise. The guard simplification is a consequence, not a workaround. The old code needed The SQL upsert composes correctly. Test coverage is exact. |
QA: are the dollars right?QA'd Savings: exactFour real turns in one session (haiku, haiku warm, sonnet, haiku after a 5m gap):
The cross-check that matters most, since the claim is that this tab and the usage tab cannot disagree: Same dollars on both sides. Buckets were right on live timings too: turn 2 at +6s is warm and its hit is recorded, turn 4 at an idle gap of 319s against a 300s TTL is expired rather than a fresh visit. Buckets partition turns ( Warming: two errors, and they only cancel in the netRescue is charged at the full write rate. A turn that warming rescues still reads the prefix; it does not become free. The saving is Replays are charged one window too many. A read refreshes the entry's TTL, so bridging an idle gap needs The two errors are equal and opposite whenever the whole prefix is rewritten (both are
Break-even is the number an operator would act on, and it reads roughly twice its real value. Both fixes are one term each in the upsert: Turns with no cache telemetry get a cause they did not earnFirst live run used a 3655 token prefix, which is under Anthropic's minimum cacheable length for haiku 4.5, so the provider reported usage but never cached anything. The second haiku turn was therefore recorded as a warm miss and attributed to The headline The window means "sessions that started here"Counters are lifetime and the row is selected on So the agreement with the usage tab holds on pricing, and on any window no session straddles; it is not an identity per date. Worth scoping the claim in the description, and labelling the window in the UI as sessions started in the range. Minor
VerdictSavings arithmetic ships as is; it reconciles with both spend logs and the daily rollups exactly. The two warming terms and the cause attribution are worth fixing before the tab renders them, since break-even and the miss split are the figures an operator would act on. |
3bf3f72 to
9eb3532
Compare
|
Both warming figures corrected in 9eb3532; two line changes in the upsert. A rescue is now priced at A replay is now Worth noting on the second one: Both figures are differences rather than absolutes now, which is the same rule the savings arms already follow. @greptileai can you reply here with a score and why that score? |
|
bugbot run |
|
The two warming formula corrections in the upsert are both mathematically correct: Rescued spend — Replay spend — Both figures are now differences rather than absolutes, consistent with how the savings arms work. Score: 4/5 What pushes it to 4 rather than 5 is one subtle edge in the In practice this is small — it only matters when turns arrive close together after expiry and the expired turn happened to be a miss — but it is a real misclassification that moves a cold turn into the warm bucket, understating savable misses and overstating warm hits. Everything else is solid: the primary key design, the atomic upsert that needs no read-before-write, the |
Re-QA at aaf66ee (warming estimate removed)Re-ran the live verification on the trimmed revision: four real Anthropic turns through the complexity router (haiku cold write, warm hit at +6s, sonnet on the complex turn, haiku return after a 318s gap against the 300s TTL) Savings and prompt-cache bucketing are both correct: every dollar figure reconciles exactly across SpendLogs, the session rollup, the daily table, and the endpoint, and the four turns land in the right buckets with the tier records carrying the right timestamps, TTLs, and prefixes. The warming columns, rates plumbing, and endpoint fields are cleanly gone (savable_misses is now expired_misses); endpoint validation is unchanged (401 without a key, 400 on a reversed window). The residuals from the previous round all lived in the deleted estimate, so nothing outstanding remains |
Fold each auto-routed turn into a per-(api_key, session, auto-router) row when it happens, and serve the benchmarks dashboard by summing those rows. Nothing in the feature reads LiteLLM_SpendLogs. The row carries what each tier the session used left in the prompt cache, so a turn's bucket is a question about one model's own record and the upsert answers it against the row it is already writing. Absent from that record means a first visit; present means warm or expired, on one comparison of the idle gap against the TTL the entry was written with. Nothing is read before the write, so there is no state to load or validate, and the statement is atomic.
aaf66ee to
2e2b262
Compare
|
Pushed 2e2b262. Coverage was measuring the wrong thing. That also dissolves the first_visit accumulation concern rather than renaming around it: a never-caching model contributes to no bucket at all. The naming point stands on its own and is tracked as LIT-5212. The read path lost the four-times-repeated field list. Net effect, and the deletions are the point: @greptileai can you reply here with a score and why that score? |
|
bugbot run |
|
5/5. The fix is correct and the implementation is cleaner after this commit. What was wrong and why this fixes it. The refactoring is correct. The miss attribution math holds. The four Test coverage is complete. One minor observation that is not a blocker: the |
|
@greptileai can you reply here with score & why |
|
bugbot run |
|
Score: 4/5 This is production-quality work. The architecture is sound, the test coverage is genuine (real Postgres in CI, not just mocked), and the design decisions are well-reasoned. Here's a breakdown: What's done well
What to flag before merge 1. 2. Window attribution gap for long-running sessions (low) 3. The Why not 5/5 The Everything else — the SQL correctness, the isolation model, the test strategy, the read-path design — is solid. |
|
bugbot run |
|
@greptileai can you reply here and explain why? |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 798e54d. Configure here.
|
Score: 4/5 Here's the reasoning: What earns the score: The architecture is clean. One row per What keeps it from 5/5: The rollup flush is still awaited inside Retention also requires Neither of these is a blocker — the spend path itself cannot be broken by rollup failures because they're caught — but they're the gap between good and excellent. |
…itellm_lit4712_benchmarks_backend
…rollup retention to the read horizon
|
@greptileai can you reply here with a score and why that score? |
|
bugbot run |
|
Score: 4/5 This is a well-engineered PR. Here's the breakdown: What earns a high score:
What keeps it from a 5:
|
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 270da20. Configure here.
| tiers = t.tiers || jsonb_build_object({_MODEL}, jsonb_build_array( | ||
| CASE WHEN {_REFRESHED} THEN {_STARTED_AT}::float8 ELSE {_CACHED_AT} END, | ||
| CASE WHEN {_REWROTE} THEN {_TTL}::float8 ELSE {_CACHED_TTL} END, | ||
| CASE WHEN {_REFRESHED} THEN {_PREFIX_TOKENS}::float8 ELSE {_CACHED_TOKENS} END |
There was a problem hiding this comment.
Null TTL write erases known TTL
High Severity
The _REWROTE condition in the UPSERT_SQL can incorrectly update the ttl within the tiers JSONB. It might overwrite valid TTLs with nulls when cache_creation_tokens are positive but the turn's TTL is null, or incorrectly set TTLs for unordered turns. This leads to misclassification of subsequent turns and inaccurate TTL distribution metrics.
Reviewed by Cursor Bugbot for commit 270da20. Configure here.
| horizon even with no retention configured; a shorter configured retention wins.""" | ||
| now: Final = datetime.now(timezone.utc) | ||
| horizon: Final = now - timedelta(days=AUTO_ROUTER_SESSION_RETENTION_DAYS) | ||
| return horizon if retention_seconds is None else max(horizon, now - timedelta(seconds=retention_seconds)) |
There was a problem hiding this comment.
Active old sessions never pruned
Medium Severity
Retention keys off last_turn_at, while benchmarks attribute sessions by first_turn_at. A long-lived session that started outside the 30-day read window but keeps getting turns stays invisible to every query and is never deleted, so the rollup table can grow without bound for persistent session IDs.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 270da20. Configure here.
2 participants
TLDR
Problem this solves:
LiteLLM_SpendLogs, which is the widest table in the schema and unbounded at customer scalesession_idis not on them, and a session does not close on a day boundaryHow it solves it:
LiteLLM_AutoRouterSessionrow per (api key, session, auto-router) carries the counters and, intiers, what each model that session used left in the prompt cacheLiteLLM_SpendLogsRelevant issues
LiteLLM_SpendLogson every loadcompute_savings_spendfrom feat(spend): add net auto-router savings to the cost-optimization dashboard #35521, so this tab and the usage tab cannot report different savings for the same trafficsavings.pya request was auto-routed is what tells this rollup to fold itLinear ticket
Resolves LIT-4712
Pre-Submission checklist
Screenshots / Proof of Fix
Real proxy, real Postgres, real Bedrock calls costing real money. A complexity auto-router over a haiku and an opus deployment, with
autorouter_savings_baseline_modelset to the opus one.Before, on the base commit, the same command the after leg runs:
Two real auto-routed turns on that same base proxy, to show the traffic lands and still produces no rollup:
After, on this branch, four turns in one session whose prompts classify to different tiers:
The rollup advanced from the request path alone, and the buckets sum to the turn count:
Read back through the endpoint:
Every turn reads as a first visit because these prompts are far below the provider's minimum cacheable size, so no tier ever holds a live prefix; a tier with nothing cached is cold by definition rather than warm. That is the honest reading of this traffic, and the warm, expired, unordered and unknown-TTL transitions are covered against a real Postgres in
tests/proxy_behavior/spendinstead.Restarting the proxy and sending a fifth turn in the same session keeps both tiers and keeps accumulating, which is the record round-tripping with nothing held in memory:
Window validation:
Type
🆕 New Feature
Changes
LiteLLM_AutoRouterSessiontable keyed on(api_key, session_id, model_group), with a migration and the three schema copiesauto_router_sessions.pystages a priced turn on the logging path and writes it as one atomic upsert from a dedicated scheduler job, so rollup writes never extend the wall time of the budget-commit job; the batch is sorted by session key so every pod locks rows in the same order. Cache writes establish a five-minute or one-hour TTL, reads inherit the stored TTL, and turns without evidence remain unknownauto_router_benchmarks.pysums those rows and derives every figure once, for a single router and for the totals alikeGET /auto_router/benchmarks, admin-only, typed with aresponse_modelso the dashboard client gets real typesmaximum_spend_logs_retention_periodis unset; a shorter configured retention still wins. Keyed on last activity throughSpendLogCleanup._delete_old_rows_batchedWhat does not change: pricing. Both arms of the savings comparison resolve through
compute_savings_spend, which already owns it, so this adds no pricing code and cannot disagree with the usage tab.Cache buckets
A turn lands in exactly one bucket, decided by that tier's own cache record:
Every miss therefore has exactly one cause on one denominator: cold by design, the prefix changed, the entry aged out, or unattributable. Unordered and unknown-TTL turns still count, and provider-reported hits stay in the headline hit rate even when cause attribution abstains. The response exposes five-minute, one-hour and unknown TTL counts instead of collapsing mixed traffic to one majority TTL
There is deliberately no cache-warming dollar estimate. Pricing one means modelling a daemon that does not exist, and its ping interval, stop policy and coordination with real traffic are all free parameters;
expired_missesreports the measured signal an operator would act on instead.Things a reviewer will ask about
Why the classification is in SQL. Because it needs the session's cache record, and that record is a column on the row being written. Doing it in the upsert means there is no state to load, nothing to parse or validate in Python, no batch of keys to bound, and no read that can fail: the statement is atomic and its counters compose across pods.
GREATEST/LEASTon the record and the timestamps make a late-arriving turn unable to rewind the session. The cost is that the predicates are not unit-testable in pure Python, which is why they are covered bytests/proxy_behavior/spend, a shard CI already runs against a realcimg/postgres:16.0; eight mutations of those predicates were each confirmed to fail a test.Why
api_keyis in the primary key.session_idis caller-controlled, so on its own it lets any caller write into another tenant's rollup by reusing their id. EveryLiteLLM_Daily*Spendtable carriesapi_keyin its key for the same reason. A key rotated mid-conversation splits that session's rollup rather than merging two tenants into one wrong number.Why the totals are computed server-side. Aggregating per-router percentages by averaging them is wrong, and the numbers the tab leads with are all ratios.
summarizeruns over raw counters and produces the per-router view and the totals through the same function, so the two cannot disagree.Cost at a million requests. The read touches only
LiteLLM_AutoRouterSession, which holds one row per conversation rather than per request, filtered onfirst_turn_atand served by a dedicated index on that column; a second index onlast_turn_atserves retention. Measured on 400k sessions inside a 30 day window, the aggregate runs in 60 ms. A per-model variant of this table was prototyped and measured at 1.13 s for the same window, because reconstructing a session from its tiers forces a second grouping level; that is why the record lives in a column on the session row instead. The write is one statement per auto-routed turn, batched per flush interval, and never runs on the request path.QA runbook
autorouter_savings_baseline_modelsetlitellm_session_id; after the next rollup flush the row should readturns=2with one first visit and one same-model turnGET /auto_router/benchmarks?start_date=<30d ago>&end_date=<today>and confirm, for the totals and each group, thatfirst_visit_turns + warm_turns + expired_turns + unordered_turns + unknown_ttl_turnsequalscache.turns, and that the four miss shares sum to 100model_group=<alias>and confirm every figure narrows to that routerend_dateearlier thanstart_datefor a 400, and a malformed date for a 422Final Attestation
Note
Medium Risk
Touches the hot spend-tracking path and adds durable DB writes on every auto-routed turn; design limits blast radius (non-blocking queue, no raise on rollup errors), but mis-bucketing or retention bugs would skew admin benchmarks rather than block requests.
Overview
Adds
LiteLLM_AutoRouterSessionand a write path that folds each auto-routed request into one row per(api_key, session_id, model_group)—spend, baseline savings, and prompt-cache buckets (first visit / warm / expired / unordered / unknown TTL) updated in a single Postgres upsert using a per-modeltiersJSON state.Spend tracking stages turns via
_enqueue_auto_router_turnon every request (even when spend logs are disabled), then flushes ordered upserts with the existing spend batch; failures are swallowed so rollup work cannot break billing.Read path: admin-only
GET /auto_router/benchmarksaggregates pre-folded rows (30-day window, optionalmodel_group) into savings, session shape, and cache miss attribution without scanningLiteLLM_SpendLogs. OpenAPI types are updated for the dashboard.Retention: spend-log cleanup also deletes stale rollup rows by
last_turn_at. Tests cover SQL bucketing against real Postgres and pure read-path math.Reviewed by Cursor Bugbot for commit 798e54d. Bugbot is set up for automated code reviews on this repo. Configure here.