feat(client): cache-aware token accounting and a reusable span lifecycle - #28
Draft
apucacao wants to merge 3 commits into
Draft
feat(client): cache-aware token accounting and a reusable span lifecycle#28apucacao wants to merge 3 commits into
apucacao wants to merge 3 commits into
Conversation
This was referenced Aug 11, 2026
Author
|
bugbot run |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Six handlers each hand-write their own usage attributes today, which is how they came to disagree. This adds the shared writers they will move onto, so the numbers have one author. parse_usage grows cache handling. It read three key-pair spellings and dropped every cache field, so Anthropic undercounted input by the whole cached portion of a call: a turn that reads 19,971 tokens from cache and writes 3,580 more reports input_tokens: 3, and 3 is what we billed a dashboard on. It now folds every accepted spelling, including Bedrock Converse's cacheWriteInputTokens, and reports the breakdown in input_details. The fold stays provider-blind, which is the contract handlers must respect: return raw usage with the cache fields intact, or an input figure that already includes cache with the fields omitted. Returning a pre-folded input alongside the fields double-counts. add_cached_tokens_to_input applies the Anthropic-shaped rule at the call site, and lang_chain_span_usage the LangChain-shaped one, because the direction differs per provider and centralising it would silently double-count for two providers out of three. SpanUsage is the type that means "the folding is already done". set_usage_span_attributes writes all seven attributes every time, zeros included, because an absent attribute drops a span from every query that groups on usage, which reads as "no cached tokens" rather than "this handler forgot to say". It also owns the two OpenLLMetry aliases, which previously lived beside the completion text and were computed off Anthropic's cache-excluding input field, so they disagreed with the canonical numbers on the same span. RunUsage counts whether any turn reported usage rather than testing the total for zero, so a failed run can put its partial spend on the root while a run that never completed a call correctly says nothing. All-zero attributes would assert the run cost nothing. number_or_zero replaces bare int(...), which raised on None. An emitted NaN is worse than an emitted 0, because the metric guard tests `> 0` and that is false for NaN, so the metric vanishes instead of reading low. end_span_once makes the streaming teardown idempotent and marks an abandoned stream without failing it. Stopping early is a normal thing for a consumer to do, and LaunchDarkly's own metrics record neither success nor error for it, so ERROR would put two dashboards in disagreement about one run. It tracks id(span) because an OTel span is not guaranteed hashable. UsageDict gains input_details, which broke graph.py's UsageDict(**dict) splat. Now built with named arguments, so the next member added here cannot silently arrive from a dict with no business filling it.
parse_usage started reporting a cache breakdown, but invoke and the judge runner both built UsageDict by hand from three keys, so input_details was always None on the blocking path while the streaming path handed back the nested dict. The two paths disagreed about the same run. Both now go through to_usage_dict, so the mapping has one author and cannot drift again. graph.py keeps building its own: a graph total is a sum across nodes and carries no per-call breakdown, and it says so. Found by Bugbot on #28.
Handlers hold None for every span whenever the OpenTelemetry SDK is absent, and every other helper in this family already no-ops on it. This one did not, so a streaming cleanup path in a finally crashed with AttributeError on an install without the otel extra: the one place that should be hardest to break was the one place that was not guarded. Fixed in the shared helper rather than at six call sites, because all six handlers use it the same way and the next handler would hit the same edge. Found by Bugbot on #30.
apucacao
force-pushed
the
ag/py-telemetry-usage-layer
branch
from
August 11, 2026 20:43
a0bd930 to
22ad56f
Compare
Author
|
bugbot run |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 22ad56f. Configure here.
Author
|
bugbot run |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 22ad56f. Configure here.
Author
|
bugbot run |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 22ad56f. Configure here.
Author
|
bugbot run |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 22ad56f. Configure here.
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.
Adds the shared token-accounting and span-lifecycle helpers the six handlers will move onto, so the numbers have one author instead of six hand-rolled copies.
Additive. No handler uses these yet, so no behaviour changes in this PR.
The bug this fixes
parse_usageread three key-pair spellings and dropped every cache field. Anthropic reports cache reads and writes besideinput_tokensrather than inside it, so a turn that read 19,971 tokens from cache and wrote 3,580 more reported an input of 3. That 3 is what dashboards were billed on.What changed
parse_usagefolds every accepted cache spelling, including Bedrock Converse'scacheWriteInputTokens, and reports the breakdown ininput_details.add_cached_tokens_to_inputandlang_chain_span_usageapply the per-provider rule at the call site, because the direction differs by provider and centralising it would double-count for two out of three.SpanUsageis the type that means the folding is already done.set_usage_span_attributeswrites all sevengen_ai.usage.*keys every time, zeros included, because an absent attribute drops a span from every query that groups on usage. It also owns the two OpenLLMetry aliases, which previously lived beside the completion text and were computed off Anthropic's cache-excluding input field, so they disagreed with the canonical numbers on the same span.RunUsagecounts whether any turn reported usage rather than testing the total for zero, so a failed run can report partial spend while a run that never completed a call correctly says nothing.number_or_zeroreplaces bareint(...), which raised onNone. An emittedNaNis worse than a 0, because the metric guard tests> 0and that is false forNaN, so the metric vanishes instead of reading low.end_span_oncemakes teardown idempotent and marks an abandoned stream without failing it.UsageDictgainsinput_details, which brokegraph.py'sUsageDict(**dict)splat. Now built with named arguments, so the next member added cannot silently arrive from a dict with no business filling it.Where this sits
Builds on the contract (#27). The content layer (#29) sits on top, and the six handler PRs depend on both.
Tests: 625 to 671.
Note
Overview
Fixes undercounted Anthropic/Bedrock token totals by making
parse_usagefold cache tokens intoinput, and exposes the breakdown via newUsageDict.input_details/InputTokenDetails.Adds shared span helpers for upcoming handler work:
SpanUsage/RunUsage, provider-specific folding (add_cached_tokens_to_input,lang_chain_span_usage),set_usage_span_attributes,set_model_identity_attributes, and idempotentend_span_once. Call sites now build usage throughto_usage_dictso cache details are not dropped.Graph totals stay cache-free and are built with named
UsageDictfields to avoid silent splat of new members.Reviewed by Cursor Bugbot for commit 22ad56f. Bugbot is set up for automated code reviews on this repo. Configure here.