feat(client): cache-aware token accounting and a reusable span lifecycle - #28
Conversation
|
bugbot run |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
a0bd930 to
22ad56f
Compare
|
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.
|
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.
|
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.
|
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.
|
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.
|
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.
|
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.
|
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.
|
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.
|
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.
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.
22ad56f to
24f5cf3
Compare
|
bugbot run |
| + cache_creation, | ||
| output=number_or_zero(raw_usage.get("output_tokens")), | ||
| cache_read=cache_read, | ||
| cache_creation=cache_creation, |
There was a problem hiding this comment.
Span helper drops Bedrock cache keys
Medium Severity
add_cached_tokens_to_input only reads snake_case Anthropic keys, so Bedrock Converse bags (inputTokens, cacheReadInputTokens, cacheWriteInputTokens) fold to zeros on spans. parse_usage already accepts those spellings, and the helper’s own comment says Converse reports cache the same way, so metrics and span attributes would disagree for the same call.
Reviewed by Cursor Bugbot for commit 24f5cf3. Configure here.
|
bugbot run |
| if span is None or not span.is_recording(): | ||
| continue | ||
| span.set_attribute("launchdarkly.run.cancelled", True) | ||
| span.end() |
There was a problem hiding this comment.
Cancelled mark applied to every span
Medium Severity
end_unfinished_spans writes launchdarkly.run.cancelled onto every still-open span it is given. The contract limits launchdarkly.* to the invoke_agent root, and a cancelled run has to pass the open chat and execute_tool spans in too or they never export. Unlike end_span_once, there is no way to close those children without stamping the root-only key on them.
Reviewed by Cursor Bugbot for commit b2b1f3c. Configure here.
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.
asyncio.CancelledError inherits from BaseException, deliberately, so a timeout or a task.cancel() walks past every except Exception a handler writes. The blocking paths end their spans only from those clauses, so a cancelled run exports nothing at all. Not a wrong attribute: no span. The root carries the feature_flag event and every launchdarkly.* attribute, so a stranded root means the whole run never reaches AI Config Monitoring. end_unfinished_spans belongs in a finally rather than an except, because the paths an except cannot see are the entire point. Spans are left at UNSET and marked launchdarkly.run.cancelled. Nothing failed: the caller went away. That is the reasoning end_span_once already applies to an abandoned stream, and it keeps the trace agreeing with LaunchDarkly's own metrics, which record neither a success nor an error for a run that never finished. A span another path already ended is skipped. is_recording() is False once end() has run, so this is a no-op on the success path. The OTel SDK makes a second end idempotent but logs it, and that log is what would bury a real leak in noise. This has no TypeScript counterpart and needs none. An aborted request there rejects its promise and the catch catches it. Only Python routes cancellation around a handler's guards. Four tests against a real exporter, since a mock span cannot tell an ended span from an open one. Removing the guard, or marking ERROR instead of UNSET, each fails one. Found by Bugbot on the langchain-messages layer, then found in five of six handlers by auditing the rest.
b2b1f3c to
dff27f6
Compare
|
bugbot run |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
2 issues from previous reviews remain unresolved.
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit dff27f6. Configure here.
end_span_once marked every unfinished streaming span launchdarkly.stream.abandoned, whatever ended it. Both facts can be true of the same unwind and only one of them is the reason. A consumer that stops reading abandoned the stream, and that is normal. A CancelledError means something cancelled the run, usually a timeout, and the consumer never chose anything. Reporting the second as the first made a timed-out stream and a timed-out blocking call disagree in AI Config Monitoring about why they stopped, since the blocking paths already report launchdarkly.run.cancelled. cancelled wins over abandoned when both are passed, because the cancellation is the cause and the abandonment is a consequence of it. No new attribute. Both keys already exist and are already in the vocabulary lock. Two tests. Removing the branch fails one. Found by Bugbot on the openai-agents layer.
|
bugbot run |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
2 issues from previous reviews remain unresolved.
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 14ed066. Configure here.
|
bugbot run |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
2 issues from previous reviews remain unresolved.
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 14ed066. Configure here.
|
bugbot run |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
2 issues from previous reviews remain unresolved.
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 14ed066. Configure here.
|
bugbot run |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
2 issues from previous reviews remain unresolved.
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 14ed066. Configure here.
|
bugbot run |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
2 issues from previous reviews remain unresolved.
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 14ed066. Configure here.
|
bugbot run |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
2 issues from previous reviews remain unresolved.
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 14ed066. Configure here.
2 participants
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
Introduces shared token accounting and OpenTelemetry span helpers in
utils.pyfor upcoming handler migrations, and exports them from the package root. Handlers are not wired to these yet; the immediate call-site updates areto_usage_dictininvokeand judge flows and saferUsageDictconstruction in graph totals.Token normalization now treats provider
inputas inclusive of cache:parse_usagefolds accepted cache field spellings (including BedrockcacheWriteInputTokens), derivestotalfrom input+output only, and can attachinput_details.number_or_zeroreplaces bareint()for loose provider bags.UsageDictgains optionalInputTokenDetails;to_usage_dictkeeps blocking responses aligned with parsed cache breakdowns.Span telemetry building blocks add
SpanUsage,RunUsage, provider-specific mappers (add_cached_tokens_to_input,lang_chain_span_usage),set_usage_span_attributes(all sevengen_ai.usage.*keys, including OpenLLMetry aliases),set_model_identity_attributes, and lifecycle helpersend_span_once/end_unfinished_spansfor streaming and cancellation paths.Adds
test_span_usage.py(~550 lines) covering cache-direction cases, span end semantics, andto_usage_dictround-trips.Reviewed by Cursor Bugbot for commit 14ed066. Bugbot is set up for automated code reviews on this repo. Configure here.