fix(mavvrik_focus): carry token counts (prompt/completion/total) in FOCUS Tags - #33694
Conversation
|
@greptileai please review. |
Greptile SummaryThis PR fixes missing LLM token counts (
Confidence Score: 5/5Safe to merge — the change is strictly additive to Tags JSON on one Mavvrik-only code path and cannot affect Vantage or CloudZero exports. All four defensive-coding gaps raised in the prior review have been addressed in this revision, and the new tests exercise every newly added branch. No existing assertions were weakened; the only test-file change is adding a missing No files require special attention.
|
| Filename | Overview |
|---|---|
| litellm/integrations/mavvrik_focus/mavvrik_focus_logger.py | Adds _with_token_tags() helper to zip token counts from the pre-transform frame into the FOCUS Tags JSON column; all previously flagged issues (malformed JSON, non-dict Tags, missing Tags column guard) are addressed in this version |
| tests/test_litellm/integrations/mavvrik_focus/test_mavvrik_focus_logger.py | Adds columns: list = [] to the _Frame mock (necessary to avoid AttributeError when _with_token_tags reads data.columns) and adds seven new unit tests covering the major branches of _with_token_tags, including malformed-JSON recovery and non-dict Tags recovery |
Reviews (8): Last reviewed commit: "fix(mavvrik_focus): also carry cache tok..." | Re-trigger Greptile
Greptile SummaryThis PR adds
Confidence Score: 4/5The change is additive, confined to a single Mavvrik-specific file, and backed by E2E proof against a live Postgres database with real completions. The core logic is correct and the approach is well-reasoned. The three defensive gaps — missing Tags column guard, unvalidated json.loads return type, and implicit row-ordering reliance — are all export-path-only and require unexpected inputs or future transformer changes to manifest. litellm/integrations/mavvrik_focus/mavvrik_focus_logger.py — specifically the _with_token_tags helper.
|
| Filename | Overview |
|---|---|
| litellm/integrations/mavvrik_focus/mavvrik_focus_logger.py | Adds _with_token_tags() to merge prompt/completion tokens into the FOCUS Tags column; logic is sound but has minor defensive gaps. |
| tests/test_litellm/integrations/mavvrik_focus/test_mavvrik_focus_logger.py | Adds four unit tests for _with_token_tags() branches and fixes _Frame mock; existing coverage is preserved. |
Reviews (2): Last reviewed commit: "fix(mavvrik_focus): also derive total_to..." | Re-trigger Greptile
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
yuneng-berri
left a comment
There was a problem hiding this comment.
Please see if the Greptile comments are applicable.
I do not believe we have pl as a dependency, and adding one just for the integration will introduce bloat. Can you accomplish this without the pl dependency
|
Addressed the flagged concern: wrapped On the @greptileai please re-review. |
d9365a6 to
9fa8fd5
Compare
|
@greptileai please re-review. |
FOCUS v1.2 has no standard column for LLM token counts, and the shared FocusTransformer used by every destination (Mavvrik, Vantage, CloudZero) drops prompt_tokens/completion_tokens even though the source query already selects them. Merge the two counts into the existing Tags JSON column, which is the spec's own escape hatch for non-standard fields, inside the Mavvrik-only export path so no shared transformer changes.
Add columns attribute to the _Frame test double so _with_token_tags does not raise AttributeError on the existing empty-export parametrized case, and add dedicated unit tests for _with_token_tags covering the merge, no-token-columns, and row-count-mismatch paths.
total_tokens has no stored column in LiteLLM_DailyUserSpend at all, so it can't be selected like prompt_tokens/completion_tokens. Derive it as their sum in _with_token_tags, only when both source counts are present for a row, and add tests covering the sum and the partial-data case.
json.loads on the existing Tags value had no error handling; a malformed value would raise JSONDecodeError and abort the entire export window instead of just skipping that row's token merge.
cache_creation_input_tokens and cache_read_input_tokens are selected by the same database.py query as prompt_tokens/completion_tokens and dropped by the same transformer. Add them to _TOKEN_TAG_KEYS.
9fa8fd5 to
4b3905c
Compare
Relevant issues
Replaces #33551, retargeted to
litellm_internal_staging(same 3 commits, already reviewed by Greptile at 5/5 onlitellm_oss_staging).Mavvrik FOCUS exports do not carry LLM token usage (
prompt_tokens,completion_tokens,total_tokens), even though LiteLLM's own DB and UI have this data. FOCUS v1.2 has no standard column for token counts, so the sharedFocusTransformer(used by every FOCUS destination: Mavvrik, Vantage, CloudZero) dropsprompt_tokens/completion_tokensduringtransform(), even thoughdatabase.py's query already selects them fromLiteLLM_DailyUserSpend.total_tokenshas no stored column at all; it is derived here as the sum of the other two.Pre-Submission checklist
make test-unit@greptileaiand received a Confidence Score of at least 4/5 (5/5 on the identical commits against fix(mavvrik_focus): carry token counts (prompt/completion/total) in FOCUS Tags #33551)Type
Bug Fix
Changes
FocusTransformer.transform()(shared core code, not touched by this PR) builds the final FOCUS-shaped frame via an explicit columnselect(...)that enumerates ~35 output columns.prompt_tokens/completion_tokensare not in that list, so they are silently dropped between the raw query result and the exported CSV. This is true for every destination built on this transformer, not just Mavvrik.total_tokensisn't stored inLiteLLM_DailyUserSpendat all, so there is nothing to drop or select for it.Rather than changing the shared transformer (which would affect Vantage and CloudZero too), this PR merges the token counts into the existing
TagsJSON column, entirely insideMavvrikFocusLogger._export_window(), a Mavvrik-only file.Tagsis FOCUS v1.2's own escape hatch for non-standard fields, and core already uses it to carryteam_id,model,custom_llm_provider, etc._export_window()holds both the pre-transform frame (data, still has the token columns) and the post-transform frame (normalized, tokens already dropped) at the same point, right before serialization. The_with_token_tags()helper zips the two frames by row position (transform()only adds/renames columns and never filters or reorders rows, so a 1:1 row correspondence is guaranteed), addsprompt_tokens/completion_tokensas extra string keys into each row's existingTagsJSON, and additionally addstotal_tokensas their sum when both source counts are present for that row (omitted otherwise, to avoid emitting a partial/wrong total).No changes to
litellm/integrations/focus/database.pyorlitellm/integrations/focus/transformer.py.Also fixes the
_Frametest double intest_mavvrik_focus_logger.py(was missing acolumnsattribute, which_with_token_tagsreads) and adds unit tests for_with_token_tags: merge with total_tokens, partial-column no-total, no-token-columns no-op, and row-count-mismatch no-op.Note:
litellm_internal_stagingis currently missing the earlierfix(mavvrik): advance metricsMarker after upload; fix scheduler startupfix (#31068), which only merged intolitellm_oss_stagingand was never synced forward. This PR's diff applies cleanly regardless, sincemavvrik_focus_logger.pywas otherwise identical between the two branches at the time of this PR, but the metricsMarker gap is a separate pre-existing issue worth tracking independently.Screenshots / Proof of Fix
E2E verified on the QA VM using a native LiteLLM proxy (not Docker) against a dedicated Postgres database (
litellm_tokentest), with real Azuregpt-4o-minicompletions and a real Mavvrik sandbox connection.FOCUS_CRON_OFFSETwas used to fire the daily export job a few minutes after startup; test rows were backdated one day in Postgres so the daily window (which only exports strictly-past dates) would pick them up.Before fix, commit
9076c3334760d4c4d6be4b2555c874e9d49c2733(unpatchedmavvrik_focus_logger.py)3 real completions produced this Postgres row:
Resulting
Tagsvalue in the exported CSV, no token counts despite Postgres having them:After fix (prompt/completion tokens), commit
b33978e5cf...3 new real completions produced this Postgres row:
Resulting
Tagsvalue:After adding total_tokens, commit
f7df2ec14c...3 new real completions produced this Postgres row:
Export log:
Resulting
Tagsvalue in the exported CSV, downloaded from GCS and decompressed.total_tokensnow present and equal toprompt_tokens + completion_tokens(48 + 274 = 322), matching Postgres exactly:All other FOCUS columns (
BilledCost,ConsumedQuantity,ChargePeriodStart/End, etc.) are unchanged across all three runs, confirming the fix is additive toTagsonly.Unit tests (
tests/test_litellm/integrations/mavvrik_focus/test_mavvrik_focus_logger.py), run against a real litellm install with polars/pytest-asyncio available: