feat(runner): declare that our input token count excludes cached tokens - #5720
feat(runner): declare that our input token count excludes cached tokens#5720mmabrouk wants to merge 1 commit into
Conversation
|
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 |
Railway Preview Environment
|
The API prices a model call by handing token counts to litellm, which expects an inclusive prompt count and derives ordinary input by subtracting the cache details from it. This runner emits the opposite. `gen_ai.usage.input_tokens` holds uncached tokens only, with cache read and cache creation counts beside it. The pinned Pi implementation confirms it; it reconstructs a prompt count as input plus cache read plus cache write. The OpenTelemetry GenAI contract says `input_tokens` already includes cached tokens. Agenta ingests OTLP from third-party instrumentation as well as from this runner, and ingest maps both into the same bucket, so the API cannot tell the two contracts apart. Whichever it assumes is wrong for the other, and the wrong assumption misprices by about tenfold in either direction. So say which one this is. Every span that carries an input token count now also carries `agenta.usage.input_tokens_includes_cache = false`. One shared constant, and both tracer paths stamp it at the same place they stamp the token counts. The tests assert the two as a biconditional: a span carries the marker exactly when it carries an input token count. A future stamping site that forgets the marker fails the suite rather than shipping an unpriceable span. The API side of this contract maps the attribute and treats an absent marker as inclusive, per the OpenTelemetry meaning, so an older runner reaching a newer API produces the pre-existing undercount rather than a new overcharge. Tests: the full runner suite passes, 1,495 tests across 99 files, and `tsc --noEmit` is clean. Five new tests cover both tracers, a parent that carries no input tokens and so needs no marker, a cost-only run, and a turn with no cache counts. Claude-Session: https://claude.ai/code/session_01RkWWQUNNzRbaB5jnCAdjYA
10ed84c to
6477d28
Compare
e9d76b1 to
f0e68e4
Compare
Stacked on #5717. Set the base to that branch, so this diff shows only its own change.
This is one half of a contract. The API half maps the attribute and decides what to do with it. This half should merge first, so that by the time the API starts trusting the marker, the runner is already sending it.
The problem
The API prices a model call by handing token counts to litellm. Litellm expects an inclusive prompt count and derives ordinary input by subtracting the cache details from it.
This runner emits the opposite.
gen_ai.usage.input_tokensholds uncached tokens only, with cache-read and cache-creation counts beside it. The pinned Pi implementation confirms it: it reconstructs a prompt count as input plus cache read plus cache write.The OpenTelemetry GenAI contract says
input_tokensalready includes cached tokens.Agenta ingests OTLP from third-party instrumentation as well as from this runner, and ingest maps both into the same bucket. So the API cannot tell the two contracts apart, and whichever it assumes is wrong for the other. Measured against the pinned litellm, the wrong assumption misprices by roughly tenfold, in one direction or the other.
The change
Say which contract this is. Every span that carries an input token count now also carries
agenta.usage.input_tokens_includes_cache = false. One shared constant, stamped on both tracer paths at the same place they stamp the token counts.Why this is safe under version skew
The API half treats an absent marker as inclusive, which is the OpenTelemetry meaning. The runner and the services deploy as independently versioned artifacts, so an old runner will reach a new API. Under that default, the skew reproduces the pre-existing undercount, which is the bug we already have, rather than producing a new tenfold overcharge. A fix should never introduce an error worse than the one it repairs.
Verification
The full runner suite passes, 1,495 tests across 99 files, and
tsc --noEmitis clean.Five new tests cover both tracers, a parent that carries no input token count and therefore needs no marker, a cost-only run, and a turn with no cache counts.
The tests assert the two facts as a biconditional: a span carries the marker exactly when it carries an input token count. A future stamping site that forgets the marker fails the suite rather than shipping a span the API cannot price.
Notes for the reviewer
src/callssetAttributeat all, and only two places write an input token count.stampRunCostwrites cost onto parent spans and never carries an input token count, so the invariant holds there without a marker.