Repository navigation
otel: stop exporting fabricated token counts; add cost_unavailable - #474
Merged
Merged
Conversation
Three follow-ups on the same spans, all about not exporting numbers we cannot stand behind. **Task 3 — `gen_ai.usage.input_tokens` was 0 on every prod turn.** The cause is not this crate. Core's `collect_stream` FABRICATES a usage struct whenever no `StreamEvent::Usage` arrives: it hardcodes `prompt_tokens = 0` and estimates `completion_tokens` as `content.len() / 4` (core llm.rs:2337-2350, pearl th-eff0d0). LiteLLM at llm.smoo.ai drops that chunk for `smooth-*` aliases, so this is the common path, not an edge case. That means the "plausible" output counts in prod (62, 74, 81, 76) were never measurements either — they are character-count estimates. The old `prompt_tokens > 0 || completion_tokens > 0` gate let exactly that pair through, publishing `input = 0` beside a fabricated output. Now gated on `prompt_tokens > 0` alone, so it is both counts or neither. A test reproduces the prod signature exactly and fails on the old gate: `input_tokens: "0"`, `output_tokens: "13"` — and `cost_usd: "0.00013"`, a cost derived from that fabricated usage. **`smooai.gen_ai.cost_unavailable = "unpriced"`** is now set instead of the cost when none could be established — same attribute name and values as the TypeScript lane, so a consumer never special-cases per engine, and "someone must price this model" becomes a positive signal rather than an absence to infer. Core collapses "unparseable" into the same `None`, so that value is not distinguishable here; documented rather than guessed. **Cost is judged independently of the counts.** I first suppressed cost whenever usage was fabricated, and an existing test proved that wrong: the gateway reports cost in an HTTP header and usage in an SSE chunk, so a turn can legitimately have an authoritative cost and no usage. Suppressing it there would recreate the all-zero-rows bug this set exists to fix. The residual hazard — a locally-priced model with fabricated usage yielding `ModelPricing × 0 input` — is pinned by an assertion labelled as today's gap, and is what cost provenance will fix. Prod is not exposed: its model is not in the local pricing table, so that path yields exactly 0 and is dropped. `telemetry::record_turn_usage` now owns this whole policy in one place, so the streaming runner and `KnowledgeChatRuntime` cannot drift apart. 564 tests pass across 45 suites (563 before, +1 new). clippy -D warnings and fmt --check clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LkCz96UfUxcai5RU4LwPnG
|
brentrager
added a commit
that referenced
this pull request
Aug 18, 2026
#474 merged without a changeset, so it would not publish — same trap that nearly stranded #470 and #471. The crates.io release here is changeset-driven, and a merged fix is not a shipped fix. Claude-Session: https://claude.ai/code/session_01LkCz96UfUxcai5RU4LwPnG Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
brentrager
added a commit
that referenced
this pull request
Aug 18, 2026
…ues (#488) Bumps core 1.8.0 -> 1.10.0 and consumes what SmooAI/smooth-operator-core#177 added, closing a hole in #474 that I introduced. #474 decided "was this measured?" by testing `prompt_tokens > 0`. That inferred provenance from the VALUE, and it only worked on one of the two fabrication sites in core: the streaming one hardcodes `prompt_tokens = 0`, but the non-streaming one estimates it from the outgoing request's JSON length and so produces a plausible non-zero. The heuristic caught the first and silently waved the second through as measured. `record_turn_usage` now reads `AgentEvent::Completed.usage_estimated`. A flag carries the fact; a heuristic guesses it — the same reason `absent` beats `0`. Two new attributes, both previously impossible without the core change: - `gen_ai.usage.cost_source` = `gateway` | `estimated`, set alongside `cost_usd` from `Completed.cost_estimated`. Local `ModelPricing` returns the FREE tier for any model it doesn't recognise, so an estimate can be a wild under-count while looking exact; a billed surface must not render the two identically. - `gen_ai.response.id` from `Completed.response_id`, recorded whenever present. It joins to `LiteLLM_SpendLogs.request_id`, whose row carries the gateway's authoritative dollars AND real token counts — so it matters most exactly when the counts above are missing, and it turns "measured vs estimated" from a claim into something reconcilable. Both had to be declared `tracing::field::Empty` at span creation; `record` silently no-ops on an undeclared field, which is how the first run recorded neither while every test still compiled. `TurnUsage` carries the three engine flags but they are TELEMETRY-only — `eventual_response` still serializes exactly `{costUsd, promptTokens, completionTokens}`, now pinned by an assertion on the object's key count with all three flags set in the fixture. It also loses `Copy`, since `response_id` is an owned `String`. Verified by restoring the old `prompt_tokens > 0` gate: the new test fails with `input_tokens: "372"` — an invented count published as measured. 576 tests pass across 46 suites (+1 new). clippy -D warnings and fmt --check clean. Cargo.lock also unifies socket2 0.5.10 -> 0.6.4 for hyper-util/tokio/tokio-postgres; both versions remain in the lock and it is incidental to the core bump. Claude-Session: https://claude.ai/code/session_01LkCz96UfUxcai5RU4LwPnG Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This was referenced Aug 18, 2026
brentrager
added a commit
that referenced
this pull request
Aug 18, 2026
#488 merged without one, same as #470, #471 and #474 before it. Four for four on this ticket — the release here is changeset-driven, so a merged fix that carries no changeset publishes nothing and every consumer keeps the old crate. Claude-Session: https://claude.ai/code/session_01LkCz96UfUxcai5RU4LwPnG Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
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
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.
Follow-up to #471 (already merged). Three items from the lead, all one theme: don't export numbers we can't stand behind.
Task 3 —
input_tokens = 0on every prod turn. The cause is not this crate.Core's
collect_streamfabricates a usage struct whenever noStreamEvent::Usagearrives — it hardcodesprompt_tokens = 0and estimatescompletion_tokensascontent.len() / 4:LiteLLM at
llm.smoo.aidrops the usage chunk forsmooth-*aliases (core's own pearl th-eff0d0 says so), so this is the common path, not an edge case.This is worse than the ticket describes. The "plausible" output counts in prod — 62, 74, 81, 76 — were never measurements either. They are character-count estimates. Only the input half looked obviously wrong.
The old gate was
prompt_tokens > 0 || completion_tokens > 0, which is exactly what let that pair through: a fabricated struct hascompletion > 0, so the||publishedinput = 0beside it. Now gated onprompt_tokens > 0alone — both counts or neither.A test reproduces the prod signature and fails on the old gate:
That third value is a cost derived from the fabricated usage — a guess multiplied by a guess.
The real fix is in core (stop fabricating, or mark the struct estimated). Until then this instrumentation declines to export it.
smooai.gen_ai.cost_unavailableSet to
"unpriced"instead of the cost when none could be established — the exact attribute name and value the TS lane (#4245) ships, so a consumer never special-cases per engine.I checked the ingest before adopting it: there is no column for it, and no cost-provenance column either. It lands in the
attributesJSONB blob, readable asattributes->>'smooai.gen_ai.cost_unavailable'— unindexed and string-typed. Worth knowing before a dashboard is built on it.Core collapses TS's
"unparseable"case into the sameNoneas unpriced, so this layer genuinely cannot emit that value. Documented rather than guessed at.Cost is judged independently of the token counts
I first suppressed cost whenever usage was fabricated, reasoning that a gateway sending no usage chunk sent no cost header either. An existing test proved that wrong: cost arrives in an HTTP header, usage in an SSE chunk — two separate channels, so a turn can legitimately have an authoritative cost and no usage. Suppressing it there would have recreated the all-zero-rows bug this whole set exists to fix. Backed out.
The residual hazard is the other direction: a locally-priced model with fabricated usage yields
ModelPricing × 0 input, an undercount that still looks authoritative. It's pinned by an assertion explicitly labelled as today's gap, not desired behaviour — if it ever starts failing, provenance landed. Production is not exposed: its model isn't in the local pricing table, so that path yields exactly0and is dropped.telemetry::record_turn_usagenow owns this whole policy in one place, so the streaming runner andKnowledgeChatRuntimecan't drift apart on it.gen_ai.response.id— cannot be done hereThe Rust operator does not set it, and cannot without a core change. Core never captures the
chatcmpl-…id at all: neitherChatResponse(llm.rs:558) norStreamChunk(llm.rs:650) deserializes anidfield, andLlmResponsehas nowhere to put one. This belongs in the same core PR as cost provenance — see the report back to the lead.Correcting my own claim in #471
#471's description said the cost header "survives streaming". That was accurate about the code path (core reads
resp.headers()before consuming the SSE body) but I let it read as a claim about the data, which I did not verify and which the TS lane disputes. I have no evidence either way on whether LiteLLM populates the header on a streamed response, and this PR asserts nothing about it. Both implementations fail safe, so neither emits a wrong number.Verification
Reverting the token gate to
||and re-running fails with the prod signature quoted above; restoring passes.cargo test -p smooai-smooth-operator-server -p smooai-smooth-operatorcargo clippy --all-targets -- -D warningscargo fmt --all --checkRebased onto current
main(post-#472).docs/Operations/Observability.mdupdated in the same commit; no overlap with the files tsworker-cost touched.No changeset — the lead is driving the release.
🤖 Generated with Claude Code
https://claude.ai/code/session_01LkCz96UfUxcai5RU4LwPnG