fix: dedupe OTLP/proxy usage, correct Anthropic cache accounting, tiered pricing - #66
Conversation
…, and dashboard - Add auth_scheme: x-api-key provider config so the proxy can inject Anthropic-native auth instead of always forcing Authorization: Bearer. - Fix streaming usage extraction to merge Anthropic's message_start and message_delta events instead of the latter overwriting the former. - Fix extract_usage: Anthropic's input_tokens/cache_read_input_tokens/ cache_creation_input_tokens are disjoint, additive buckets (unlike OpenAI's subset semantics), so prompt_tokens was undercounted whenever cache reads were present. - Price cache-write tokens (cache_creation_tokens) in calculate_costs, threaded through both the proxy and OTLP recording paths. - Fix "Cache Hit Rate" (cached / (prompt + cache_creation), not cached / prompt) across ~9 backend and frontend locations that all silently read ~100% whenever cache writes dominated a request. - Add sessions.cache_creation_tokens migration with a targeted, self-healing backfill (not rebuild_sessions_from_usage, which would wipe evaluation data) and an index on usage.session_id.
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis release adds cache-creation token tracking, tiered pricing, Anthropic authentication and streaming support, cross-source usage deduplication, session migrations, and frontend cache-write reporting. The package version changes to ChangesCache accounting and provider support
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (7)
config/app.py (3)
102-134: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winNeither tier parser guarantees ascending
min_tokens._select_tierinsrc/costs.py(Lines 100-113) scans tiers in sequence and stops at the first containing range, so it depends on ascending order. Both parsers pass the source order through unchanged, which makes correct billing depend on the input file.
config/app.py#L102-L134: sort the parsed tiers bymin_tokensbefore returning them. YAML is user-authored, so out-of-order tiers are plausible here.config/pricing.py#L119-L132: apply the same sort to the LiteLLM tiers. Upstream data is currently ordered, so this change is defensive.🤖 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 `@config/app.py` around lines 102 - 134, Sort the parsed tiers by min_tokens before returning them from the parser in config/app.py lines 102-134, so _select_tier receives ascending ranges regardless of YAML order. Apply the same sorting change to the LiteLLM tier parser in config/pricing.py lines 119-132; both sites require direct updates.
238-238: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winNormalize
auth_schemewhen reading it from YAML.
src/proxy.pycomparesprovider.auth_scheme == "x-api-key"exactly. A value such asX-API-Keyor"x-api-key "silently falls back to bearer authentication, and the request fails upstream with a confusing 401. Lowercase and strip the value here.♻️ Proposed change
- auth_scheme=provider.get("auth_scheme", "bearer"), + auth_scheme=str(provider.get("auth_scheme") or "bearer").strip().lower(),🤖 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 `@config/app.py` at line 238, Normalize the value assigned to auth_scheme in the provider configuration by stripping surrounding whitespace and converting it to lowercase before constructing the provider settings. Keep the existing "bearer" default intact when the YAML key is absent, so src/proxy.py can continue its exact comparison reliably.
160-188: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winDerive the flat fallback from the first parsed tier.
Lines 165-174 read
raw_tiers[0]directly._parse_tiered_costcan drop that same entry, for example whenrangeis[None, 1000]. The flat prices then come from a tier that is not part of the final pricing.config/pricing.pyLines 152-160 uses the first parsed tier instead, so the two paths disagree. The existing testtest_parse_model_cost_skips_malformed_tierscovers this input but does not assertcost.input.Deriving
effective_flatafter_parse_tiered_costruns would also remove theeffective_flat is not flatjuggling at Lines 187-188.🤖 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 `@config/app.py` around lines 160 - 188, Update the tiered-cost handling around _parse_tiered_cost to derive the flat fallback from the first successfully parsed tier rather than raw_tiers[0], so malformed or skipped entries cannot supply fallback prices. Reuse the parsed-tier result when assigning both tiers and effective flat values, remove the effective_flat identity juggling, and preserve explicit flat-price overrides and empty-tier behavior.config/pricing.py (3)
327-332: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the stray
ponytail:marker.The comment text is useful. The
ponytail:prefix looks like a leftover authoring artifact.♻️ Proposed change
- # ponytail: raw.githubusercontent.com's IPv6 candidates are unreachable on + # raw.githubusercontent.com's IPv6 candidates are unreachable on🤖 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 `@config/pricing.py` around lines 327 - 332, Remove the stray “ponytail:” prefix from the comment while preserving the remaining explanation unchanged.
352-364: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winReuse
_remote_costsbefore re-reading the fresh cache file.While the cache is fresh, every
fetch_remote_pricing()call reads and fully parseslitellm_pricing.json, even when_remote_costsis already populated. The LiteLLM file is large, so this repeats a costly parse under_remote_lockand blocksget_remote_pricing()callers.♻️ Proposed change
with _remote_lock: if _cache_is_fresh(): + if _remote_costs: + return dict(_remote_costs) cached = _load_local_cache()🤖 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 `@config/pricing.py` around lines 352 - 364, Update the fresh-cache branch in fetch_remote_pricing around _cache_is_fresh() to return a copy of the already-populated _remote_costs before calling _load_local_cache(). Preserve the existing file-load fallback when _remote_costs is empty, including assigning and returning successfully parsed cached data.
119-132: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueTier order is taken from the upstream file.
_select_tierinsrc/costs.pydepends on ascendingmin_tokens. This parser preserves the LiteLLM order without checking it. LiteLLM data is currently ordered, so this is defensive only. Sorting bymin_tokensbefore returning removes the dependency on upstream ordering.🤖 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 `@config/pricing.py` around lines 119 - 132, Update the tier parsing function that builds ModelTier entries to sort the collected tiers by min_tokens before returning them. Preserve the existing malformed-tier skipping and None result behavior, while ensuring _select_tier receives tiers in ascending min_tokens order regardless of upstream file ordering.src/database/usage.py (1)
195-200: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse the module logger instead of a local import.
This module now imports
loggingat line 9 and definesloggerat line 30. The nestedimport loggingis redundant.♻️ Proposed cleanup
except Exception: - import logging - - logging.getLogger(__name__).warning( + logger.warning( "Failed to update session record for merged usage ts=%s", existing.ts )🤖 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 `@src/database/usage.py` around lines 195 - 200, In the exception handler for the merged usage session update, remove the nested logging import and use the module-level logger defined near the top of the module instead of logging.getLogger(__name__).
🤖 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 `@frontend/src/charts/TrendChart.tsx`:
- Around line 46-47: Update the token composition in
frontend/src/charts/TrendChart.tsx at lines 46-47 to include cache-write tokens
in the displayed input segment or add a separate cache-write segment and tooltip
value, while preserving the cache-hit denominator. Update
frontend/src/pages/OverviewTab.tsx at lines 394-396 to display
totals.cacheCreationTokens alongside the input and output breakdown.
In `@frontend/src/pages/LogsPage.tsx`:
- Around line 437-449: Update the backend cost calculation to return separate
normal-input, cache-read, and cache-write cost components using the selected
pricing tier, rather than deriving them from input_cost_usd by token ratios.
Expose these components through the existing response data and update the
LogsPage cost rendering around
totalInputTokens/cacheCost/cacheWriteCost/actualInputCost to use the returned
values directly, removing the proportional split.
In `@src/costs.py`:
- Around line 152-158: Extend the ModelTier schema with a cache-creation rate
field mapped from LiteLLM’s cache_creation_input_token_cost, then in the
cache_write_cost calculation select the active tier’s rate when available and
fall back to ModelCost.cache_write otherwise. Ensure the selected per-token rate
is applied to cache_created before converting from per-million-token pricing.
In `@src/database/usage.py`:
- Line 73: Remove the stray “ponytail:” prefix from the comment in the usage
code, preserving the remaining comment text unchanged.
- Around line 164-167: Update the cross-process race comment near the Usage
query in the relevant lookup method to clarify that with_for_update() provides
row locking only on databases that support it, specifically PostgreSQL, and does
not lock rows on SQLite. If SQLite requires equivalent race protection, replace
or supplement this mechanism with a SQLite-compatible lock while preserving the
existing behavior on other backends.
In `@src/recorder.py`:
- Line 16: Update merge_duplicate_usage and both OTLP and proxy call paths in
record_usage to include Usage.cache_creation_tokens == cache_creation_tokens in
the duplicate lookup alongside the existing token-field matches, ensuring rows
with different cache-creation values do not merge.
- Around line 52-71: The record_usage flow must prevent concurrent OTLP and
proxy requests from inserting duplicate Usage rows when merge_duplicate_usage
finds no existing match. Enforce atomic coordination using a
commit-before-insert ordering or a database-level uniqueness constraint on the
shared call key, ensuring the existing windowed duplicate lookup cannot race
into two inserts; preserve duplicate enrichment and return behavior.
In `@src/schema_migrations.py`:
- Around line 816-865: Ensure the migration adds usage.cache_creation_tokens
before the sessions backfill can query it. In the migration flow around
sessions_cache_creation_added and the usage-dependent backfill, call
_ensure_column for the existing usage table with the appropriate INTEGER NOT
NULL DEFAULT 0 definition for both supported databases, record the change in
applied consistently, and then execute the existing sessions backfill using the
newly ensured column.
---
Nitpick comments:
In `@config/app.py`:
- Around line 102-134: Sort the parsed tiers by min_tokens before returning them
from the parser in config/app.py lines 102-134, so _select_tier receives
ascending ranges regardless of YAML order. Apply the same sorting change to the
LiteLLM tier parser in config/pricing.py lines 119-132; both sites require
direct updates.
- Line 238: Normalize the value assigned to auth_scheme in the provider
configuration by stripping surrounding whitespace and converting it to lowercase
before constructing the provider settings. Keep the existing "bearer" default
intact when the YAML key is absent, so src/proxy.py can continue its exact
comparison reliably.
- Around line 160-188: Update the tiered-cost handling around _parse_tiered_cost
to derive the flat fallback from the first successfully parsed tier rather than
raw_tiers[0], so malformed or skipped entries cannot supply fallback prices.
Reuse the parsed-tier result when assigning both tiers and effective flat
values, remove the effective_flat identity juggling, and preserve explicit
flat-price overrides and empty-tier behavior.
In `@config/pricing.py`:
- Around line 327-332: Remove the stray “ponytail:” prefix from the comment
while preserving the remaining explanation unchanged.
- Around line 352-364: Update the fresh-cache branch in fetch_remote_pricing
around _cache_is_fresh() to return a copy of the already-populated _remote_costs
before calling _load_local_cache(). Preserve the existing file-load fallback
when _remote_costs is empty, including assigning and returning successfully
parsed cached data.
- Around line 119-132: Update the tier parsing function that builds ModelTier
entries to sort the collected tiers by min_tokens before returning them.
Preserve the existing malformed-tier skipping and None result behavior, while
ensuring _select_tier receives tiers in ascending min_tokens order regardless of
upstream file ordering.
In `@src/database/usage.py`:
- Around line 195-200: In the exception handler for the merged usage session
update, remove the nested logging import and use the module-level logger defined
near the top of the module instead of logging.getLogger(__name__).
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: bd213b12-1750-4592-b226-c46711c3e3df
⛔ Files ignored due to path filters (1)
frontend/public/models/qwen-color.svgis excluded by!**/*.svg
📒 Files selected for processing (34)
README.mdVERSIONconfig.example.yamlconfig/app.pyconfig/models.pyconfig/pricing.pyfrontend/src/charts/SparklineTrendPanel.tsxfrontend/src/charts/TopUsageChart.tsxfrontend/src/charts/TrendChart.tsxfrontend/src/components/SessionDetailPanel.tsxfrontend/src/hooks/useDashboardData.tsfrontend/src/i18n/zh.tsfrontend/src/model-badge.tsfrontend/src/pages/LogsPage.tsxfrontend/src/pages/OverviewTab.tsxfrontend/src/types.tsfrontend/src/utils.tsxsrc/api.pysrc/cli.pysrc/costs.pysrc/database/models.pysrc/database/sessions.pysrc/database/usage.pysrc/proxy.pysrc/recorder.pysrc/schema_migrations.pysrc/utils.pytests/test_cli.pytests/test_costs.pytests/test_database.pytests/test_pricing.pytests/test_proxy.pytests/test_recorder.pytests/test_utils.py
9e41970 to
c90035d
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
tests/test_pricing.py (2)
1685-1702: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThis guard never reaches the connection path it protects.
_RaisingOpener.openraises before any connection is attempted._IPv4OnlyHTTPSHandlertherefore never runs, so_create_ipv4_connectionnever runs either. The docstring names the globalsocket.getaddrinfopatch as the bug under guard, but that patch would have lived in the connection path.The test would still pass if someone reintroduced a global
socket.getaddrinfoassignment inside the handler or inside_create_ipv4_connection. Drive the assertion through the connection path instead. Patchpricing_module.socket.socketorpricing_module.socket.getaddrinfoto fail, and let the real opener and handler run, so the resolution step is actually exercised before the assertion.♻️ Proposed change to exercise the connection path
def test_fetch_litellm_json_does_not_mutate_global_getaddrinfo(monkeypatch): """Regression guard for the original bug: this must not monkeypatch socket.getaddrinfo globally, since the same process also resolves DNS for concurrent, unrelated live proxy traffic.""" original_getaddrinfo = pricing_module.socket.getaddrinfo + calls = {"resolved": 0} - class _RaisingOpener: - def open(self, *a, **k): - raise urllib.error.URLError("network unavailable in test") + def _failing_getaddrinfo(host, port, family, socktype): + calls["resolved"] += 1 + assert family == pricing_module.socket.AF_INET + raise OSError("network unavailable in test") - monkeypatch.setattr( - pricing_module.urllib.request, "build_opener", lambda *a, **k: _RaisingOpener() - ) + # Let the real opener and _IPv4OnlyHTTPSHandler run so the connection + # path is actually exercised. + monkeypatch.setattr(pricing_module.socket, "getaddrinfo", _failing_getaddrinfo) result = pricing_module._fetch_litellm_json() assert result is None + assert calls["resolved"] > 0, "the IPv4 connection path was never reached" + + +def test_fetch_litellm_json_restores_global_getaddrinfo(monkeypatch): + original_getaddrinfo = pricing_module.socket.getaddrinfo + + class _RaisingOpener: + def open(self, *a, **k): + raise urllib.error.URLError("network unavailable in test") + + monkeypatch.setattr( + pricing_module.urllib.request, "build_opener", lambda *a, **k: _RaisingOpener() + ) + + assert pricing_module._fetch_litellm_json() is None assert pricing_module.socket.getaddrinfo is original_getaddrinfoNote:
monkeypatchrestorespricing_module.socket.getaddrinfoat teardown, so the second test keeps the original identity assertion meaningful.🤖 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_pricing.py` around lines 1685 - 1702, Update test_fetch_litellm_json_does_not_mutate_global_getaddrinfo to use the real opener and _IPv4OnlyHTTPSHandler connection path instead of _RaisingOpener, while patching pricing_module.socket.socket or pricing_module.socket.getaddrinfo to fail after resolution is attempted. Keep the result assertion and verify pricing_module.socket.getaddrinfo retains its original identity, ensuring regressions in _create_ipv4_connection or the handler’s global DNS handling are detected.
480-507: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert the flat
cost.cache_writefor a tiers-only entry.This entry has no top-level
input_cost_per_tokenoroutput_cost_per_token, so_parse_model_entrytakes the tiers-only branch and leaves the flatcost.cache_writeasNoneeven thoughcache_creation_input_token_costis3.75e-06.calculate_costs()falls back to the flat cache-write rate when no tier matches, so a tiers-only entry with no match drops the cache-write cost. Add an assertion that preserves the intended flat value.💚 Proposed assertion for the flat cache-write rate
result = _parse_model_entry("anthropic/claude-long-context", entry) assert result is not None _, cost = result + # Flat cache_write must survive a tiers-only entry; cost paths that miss + # every tier fall back to it. + assert cost.cache_write == 3.75 first, second = cost.tiers🤖 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_pricing.py` around lines 480 - 507, Extend test_parse_model_entry_tier_carries_own_cache_write_price to assert that the parsed flat cost.cache_write equals 3.75 for this tiers-only entry. Keep the existing per-tier assertions unchanged and validate the flat fallback value alongside the tier-specific rates.
🤖 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/test_pricing.py`:
- Around line 235-251: The test_parse_model_cost_skips_malformed_tiers test only
verifies the retained tier, not the selected flat prices. Add assertions that
cost.input is 1.2 and cost.output is 4.8, confirming malformed tiers are skipped
before deriving effective_flat and matching the valid tier selected by the
LiteLLM parser.
---
Nitpick comments:
In `@tests/test_pricing.py`:
- Around line 1685-1702: Update
test_fetch_litellm_json_does_not_mutate_global_getaddrinfo to use the real
opener and _IPv4OnlyHTTPSHandler connection path instead of _RaisingOpener,
while patching pricing_module.socket.socket or pricing_module.socket.getaddrinfo
to fail after resolution is attempted. Keep the result assertion and verify
pricing_module.socket.getaddrinfo retains its original identity, ensuring
regressions in _create_ipv4_connection or the handler’s global DNS handling are
detected.
- Around line 480-507: Extend
test_parse_model_entry_tier_carries_own_cache_write_price to assert that the
parsed flat cost.cache_write equals 3.75 for this tiers-only entry. Keep the
existing per-tier assertions unchanged and validate the flat fallback value
alongside the tier-specific rates.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 7e56cc08-ecb9-4d41-abe1-c8fefd580253
⛔ Files ignored due to path filters (1)
frontend/public/models/qwen-color.svgis excluded by!**/*.svg
📒 Files selected for processing (36)
README.mdVERSIONconfig.example.yamlconfig/app.pyconfig/models.pyconfig/pricing.pyfrontend/src/charts/SparklineTrendPanel.tsxfrontend/src/charts/TopUsageChart.tsxfrontend/src/charts/TrendChart.tsxfrontend/src/components/SessionDetailPanel.tsxfrontend/src/hooks/useDashboardData.tsfrontend/src/i18n/zh.tsfrontend/src/model-badge.tsfrontend/src/pages/LogsPage.tsxfrontend/src/pages/OverviewTab.tsxfrontend/src/types.tsfrontend/src/utils.tsxsrc/api.pysrc/cli.pysrc/costs.pysrc/database/models.pysrc/database/sessions.pysrc/database/usage.pysrc/otlp.pysrc/proxy.pysrc/recorder.pysrc/schema_migrations.pysrc/utils.pytests/test_cli.pytests/test_costs.pytests/test_database.pytests/test_otlp.pytests/test_pricing.pytests/test_proxy.pytests/test_recorder.pytests/test_utils.py
🚧 Files skipped from review as they are similar to previous changes (33)
- frontend/src/hooks/useDashboardData.ts
- src/cli.py
- src/database/sessions.py
- src/api.py
- tests/test_cli.py
- config/models.py
- frontend/src/components/SessionDetailPanel.tsx
- frontend/src/charts/TrendChart.tsx
- frontend/src/i18n/zh.ts
- tests/test_utils.py
- src/recorder.py
- frontend/src/pages/OverviewTab.tsx
- frontend/src/types.ts
- tests/test_proxy.py
- VERSION
- src/database/models.py
- frontend/src/charts/TopUsageChart.tsx
- src/schema_migrations.py
- src/costs.py
- config.example.yaml
- README.md
- frontend/src/model-badge.ts
- config/app.py
- tests/test_database.py
- src/proxy.py
- config/pricing.py
- frontend/src/utils.tsx
- tests/test_recorder.py
- frontend/src/charts/SparklineTrendPanel.tsx
- src/database/usage.py
- frontend/src/pages/LogsPage.tsx
- src/utils.py
- tests/test_costs.py
Summary
This PR changes high-risk areas: cost/token accounting, provider adapter behavior, schema migration, and usage dedup between the OTLP collector and proxy.
config/pricing.py), with daily re-fetch fixes and IPv6 stall avoidance in LiteLLM pricing lookup.sessions.cache_creation_tokenswith backfill fromusage.Why / Context
OTLP-tracked sessions and proxy-recorded usage could count the same tokens twice. Cache token columns were mislabeled for Anthropic (creation vs read) causing wrong cost and dashboard totals. LiteLLM pricing lookups could stall on IPv6 and re-fetch stale data.
How It Works
cache_creation_tokensis tracked end-to-end and backfilled for pre-existing sessions.Manual QA
cache_creation_tokensfrom usage rowsTesting
uv run python -m pytest -q: pass — 712 passedpre-commit run --all-files: not run on the final diff — state before last commitRisk / Rollout / Rollback
cache_creation_tokenscolumn + backfill), safe on existing DBs.Data / Privacy Impact
Cost / Provider / Schema Impact
sessions.cache_creation_tokens+ backfillReview
AGENTS.md,.agents/commands/llm-tracker.md, and.agents/commands/pre-pr.md: yesKnown Limitations / Follow-ups
UPDATE, notrebuild_sessions_from_usage, so evaluation columns are preserved.