fix(api): price by the producer's token contract, and never guess a custom price - #5722
Open
mmabrouk wants to merge 2 commits into
Open
fix(api): price by the producer's token contract, and never guess a custom price#5722mmabrouk wants to merge 2 commits into
mmabrouk wants to merge 2 commits into
Conversation
dosubot
Bot
added
the
size:L
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
Contributor
Railway Preview Environment
|
…ustom price Three defects, all found by an independent review of the cached-token pricing change. The cache fix overpriced every standards-compliant producer. It folded the cache buckets into the prompt count before pricing, which is right for the Agenta runner, whose input count excludes cached tokens. The OpenTelemetry contract says the opposite: `gen_ai.usage.input_tokens` already includes them. Agenta ingests OTLP from third-party instrumentation too, and ingest maps both into the same bucket, so the code could not tell the two apart. Measured against the pinned litellm with 13,556 input tokens and 13,463 cache reads, it charged $0.026079 where the answer is $0.002519, a 10.35x overcharge on both the OpenAI and Anthropic routes. Producers now declare their contract. `agenta.usage.input_tokens_includes_cache` maps to `ag.meta.usage.input_tokens_includes_cache`, and the summation happens only when a producer says its input excludes cached tokens. An absent marker means inclusive, following OpenTelemetry, so an older runner reaching a newer API reproduces the pre-existing undercount instead of a new overcharge. The runner emits the marker in a companion change. All the folding lives in one function so a future canonical migration has one thing to change. The request-model fallback could quote a confident wrong price. Managed custom models are chosen as a connection slug plus a model id, and the tracer stamps the slug into `gen_ai.system` and the bare id into `gen_ai.request.model`. A customer deployment named `gpt-5.3-codex` would therefore have received litellm's public OpenAI price. The fallback is now withheld when a custom base URL or endpoint is present, or when a provider identity exists and litellm does not recognize it. Litellm agrees from its own side: it refuses to price an unrecognized provider prefix. Response-model pricing is deliberately left alone, because codex is the only harness that sets it and its provider name is not in litellm's list, so guarding it would un-price every codex span. A measured zero did not propagate. The guard that preserves a reported cost tests for presence, but the writer still wrote only when something was non-zero, so a child that genuinely cost zero left its parent with no attribute. The accumulator now carries a separate "measured" signal, set when the cost dictionary exists rather than when its value is non-zero, and the writer fires on that. Writing zero unconditionally would have turned missing measurements into measured zeros, which is the opposite error. Tokens and errors keep their current behavior on purpose: there, zero is the identity rather than a measurement, and writing it would attach a cumulative block to every span in every trace. Two test problems came with the same review. The pricing oracle was circular, calling the same litellm function with the same constructed arguments; the expected prices are now literals, plus a test that captures the exact argument tuple handed to litellm. And one test put the workflow root, the agent span and the leaf in a single ingest call, proving a topology production does not have. It is now two batches, and it asserts what really happens: the reported cost settles on the agent span, which is the root of its own batch, and the workflow root gets no cost at ingest. Tests: 1,664 passing in OSS and 1,923 in EE. The marker's three states, absent, true and false, are proven end to end through real OTLP ingest on the same span. Claude-Session: https://claude.ai/code/session_01RkWWQUNNzRbaB5jnCAdjYA
…red nothing Two blocking defects found by a second review pass. The custom-connection guard covered only the request-model lookup. The response model won unconditionally and skipped it. That rested on a false premise: that codex is the only harness setting a response model. The Pi tracer sets `gen_ai.response.model` for every assistant message, falling back to the message's own model, and for a managed custom connection the provider on that same span is the customer's connection slug. So a custom connection was still priced at public rates through the response model, which is exactly what the guard exists to prevent. Reproduced: a span with a custom base URL and provider `acme-gateway` reporting `gpt-5.3-codex` was priced at $0.00315. The guard now covers the response model, the request model and the legacy parameters path alike. Codex keeps its price through an explicit trusted identity rather than through the accident of setting a response model. `TRUSTED_PRICING_PROVIDERS` maps an Agenta provider name to the litellm provider whose published prices genuinely apply, with one entry today and one sentence saying why. The lookup resolves through the map before testing membership, so a value that is not itself a litellm provider fails closed and withholds the estimate. A custom base URL still outranks a trusted identity, because a gateway in front of a public catalog charges its own prices. Estimation also ran on spans that measured nothing. The token buckets were read with defaults, so a span with no token metrics yielded four zeros, called litellm anyway, got zero back, and wrote a cost record. The presence of that record is what the new measured signal keys on, so a span that measured nothing propagated a cumulative zero cost to every ancestor and the root claimed the run was measured and free. Estimation now requires at least one priceable token bucket to be present. A bucket holding zero still prices, because a fully cached turn is a real measurement. A bucket holding only a total does not, because a total prices nothing on its own. The test that should have caught this used a tool span as its child, and tool spans skip cost calculation entirely, so it never exercised estimation at all. It now uses a priceable span and asserts the child as well as the parent. Tests: 1,680 passing in OSS and 1,939 in EE. Claude-Session: https://claude.ai/code/session_01RkWWQUNNzRbaB5jnCAdjYA
mmabrouk
force-pushed
the
fix/token-rollup-invariant
branch
from
August 3, 2026 22:19
cee277a to
394b0e1
Compare
mmabrouk
force-pushed
the
fix/cache-pricing-semantics
branch
from
August 3, 2026 22:19
73c744e to
d48a915
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stacked on #5718. Set the base to that branch, so this diff shows only its own change.
Addresses three blocking findings from an independent staff-level review of #5714. Pairs with #5720, the runner half of the token-contract marker.
1. The cache fix overpriced every standards-compliant producer
#5714 folds the cache buckets into the prompt count before pricing. That is right for the Agenta runner, whose input count excludes cached tokens. The OpenTelemetry contract says the opposite:
gen_ai.usage.input_tokensalready includes them.Agenta ingests OTLP from third-party instrumentation as well as from its own runner, and ingest maps both into the same bucket, so the code could not tell them apart. Measured against the pinned litellm 1.92.0 with
input_tokens = 13,556andcache_read = 13,463:gpt-5.3-codexclaude-sonnet-4-6Producers now declare their contract.
agenta.usage.input_tokens_includes_cachemaps toag.meta.usage.input_tokens_includes_cache, and the summation runs only when a producer says its input excludes cached tokens.An absent marker means inclusive, following OpenTelemetry. That default is deliberate. The runner and the services deploy as independently versioned artifacts, so an old runner will reach a new API. Under this default that skew reproduces the pre-existing undercount, which is the bug we already have, instead of a fresh tenfold overcharge. A fix must never introduce an error worse than the one it repairs.
All the folding lives in one function, so a future canonical migration has one thing to change.
2. The request-model fallback could quote a confident wrong price
Managed custom models are chosen as
<connection-slug>/<model-id>. The tracer splits that and stamps the slug intogen_ai.systemand the bare id intogen_ai.request.model. So a customer deployment namedgpt-5.3-codexwould have received litellm's public OpenAI price. That is worse than no estimate.The fallback is now withheld when either:
litellm.provider_list.Litellm agrees from its own side:
cost_per_token(model="my-gateway/gpt-4o-mini")refuses to price an unrecognized provider prefix.Response-model pricing is deliberately left alone. Codex is the only harness that sets a response model, and its provider name is
openai-codex, which is not in litellm's list. Extending the guard there would un-price every codex span and regress the fix this stack exists to deliver.Provider identity had to be read from three paths,
ag.meta.provider.name,ag.meta.providerandag.meta.system, because different adapters land it in different places. Normalizing those is a separate change to ingest semantics.3. A measured zero did not propagate
#5714 made the preservation guard test for presence, but the writer still fired only when a computed value was non-zero. So a child that genuinely cost zero left its parent with no attribute at all.
The accumulator now carries a separate
measuredsignal, set when the cost dictionary exists rather than when its value is non-zero, and the writer fires on that. Writing zero unconditionally would have turned a missing measurement into a measured zero, which is the opposite error.Tokens and errors keep their current behavior on purpose. There, zero is the identity rather than a measurement: a model call with zero tokens carries no information, the error roll-up uses a bare integer whose default is zero, and writing zeros would attach a cumulative block to every span in every trace.
Test problems from the same review, also fixed
The pricing oracle was circular. It called the same litellm function with the same constructed arguments and compared production against itself. Expected prices are now literals from the pinned version, plus a new test that captures the exact argument tuple handed to litellm for both producer contracts.
One test modelled a topology production does not have. It put the workflow root, the agent span and the leaf into a single ingest call. Production sends the workflow span and the runner subtree in two separate OTLP requests. It is now two batches, and it asserts what really happens: the reported cost settles on the agent span, which is the root of its own batch, and the workflow root gets no cost at ingest. Anything needing a whole-trace total has to aggregate stored spans.
Verification
1,664 passing in OSS unit tests, 1,923 in EE.
The marker's three states, absent and
trueandfalse, are proven end to end by pushing the same OTLP span through real ingest three ways and asserting three prices. Four custom-connection cases assert no price is written even though the model is priceable by name, with two non-regression tests pinning that a known provider still prices and that a custom connection does not suppress the response model.Known gap, deliberately out of scope
A recognized but differently priced provider is still priced at the bare model name. Measured: litellm charges
azure/gpt-4o-miniat $0.000165 per thousand prompt tokens againstopenai/gpt-4o-miniat $0.00015. Building the qualified identity would fix it and litellm understands that form, but it changes pricing for spans that are correct today and would silently un-price any model whose catalog key is bare-only. That is a separate, testable change.