Skip to content

fix: include cached tokens in input_tokens - #4080

Merged
opieter-aws merged 2 commits into
strands-agents:mainfrom
opieter-aws:opieter-aws/token-count-ts-py-parity
Sep 1, 2026
Merged

opieter-aws merged 2 commits into
strands-agents:mainfrom
opieter-aws:opieter-aws/token-count-ts-py-parity

Conversation

@opieter-aws

@opieter-aws opieter-aws commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

Description

The TypeScript tracer emitted the provider’s raw inputTokens as gen_ai.usage.input_tokens, which for Bedrock Converse and Anthropic-direct excludes cached tokens. This violates the OTEL GenAI semantic conventions, where input_tokens must include cached tokens.

For subset-cache providers (OpenAI, Gemini) where inputTokens already includes cache, the function returns the value unchanged, so there is no double counting. This brings TS to parity with the Python fix in #3886.

Related Issues

Follows #3886

Documentation PR

No documentation changes required.

Type of Change

Bug fix

Testing

Ran the TypeScript telemetry unit suite in both Node and browser environments (npx vitest run src/telemetry/__tests__/tracer.test.node.ts src/telemetry/__tests__/meter.test.ts), plus npm run type-check, npm run lint, and npx prettier. Added regression coverage for the disjoint-cache path on both endModelInvokeSpan and endAgentSpan (cached tokens folded into input_tokens) and for the subset-cache path (fold-in is a no-op, no double count).

  • I ran hatch run prepare

Checklist

  • I have read the CONTRIBUTING document
  • I have reviewed and understand every line of code in this PR, including any generated by AI tools, and I can explain why it works
  • My change is focused and reasonably small; I have split unrelated work into separate PRs
  • I have added any necessary tests that prove my fix is effective or my feature works
  • I have updated the documentation accordingly
  • I have added an appropriate example to the documentation to outline the feature, or no new docs are needed
  • My changes generate no new warnings
  • Any dependent changes have been merged and published

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.**

@github-actions github-actions Bot added bug Something isn't working python Pull requests that update python code area-model Related to models or model providers complexity/low Touched functions have low cognitive complexity (<=10) size/xs strands-running labels Aug 31, 2026
@github-actions

Copy link
Copy Markdown
Contributor

Issue: The description lists Anthropic-direct among the disjoint-cache providers this fix corrects, but the shared totalPromptTokens helper does not fold cache into input_tokens for Anthropic-direct. Per the #3546 known-limitation test in streaming.test.ts, Anthropic-direct reports cache as a separate counter yet computes totalTokens = inputTokens + outputTokens, so it is arithmetically indistinguishable from a subset provider and the cache read is dropped (returns raw inputTokens).

So in practice only Bedrock Converse (whose totalTokens includes the cache counters) actually gets the corrected, cache-inclusive input_tokens from this change; Anthropic-direct will continue to under-report until the adapter-side normalization in #3546 lands.

Suggestion: Reword the description so it doesn't claim Anthropic-direct is fixed here — either scope it to Bedrock Converse, or explicitly call out Anthropic-direct as a still-open follow-up (#3546). No code change needed; this is about setting accurate expectations for the cost-backend behavior the PR promises.

@github-actions

Copy link
Copy Markdown
Contributor

Assessment: Approve (with one minor description clarification)

Clean, well-scoped fix that centralizes the correction in _setUsageAttributes, so all four span emit sites (agent + model-invoke + the two others) inherit it. It reuses the existing totalPromptTokens helper already relied on by meter.ts and agent.ts, and mirrors the Python _total_prompt_tokens emit-site fix (#3886) for cross-SDK parity. Logic verified: disjoint folds cache in, subset is a genuine no-op, and output_tokens/total_tokens/cache subset attributes are untouched.

Review Categories
  • Correctness: Verified against the OTel GenAI semconv intent and Python parity; the self-guarding heuristic (in + out === total) prevents double counting for subset-cache providers. Usage.totalTokens is required, so no NaN/undefined edge case.
  • Testing: Good integration coverage at both endModelInvokeSpan and endAgentSpan, plus the helper's own unit tests. Optional: a subset no-op assertion on the endAgentSpan/accumulated path would round out symmetry, but the shared helper makes this low-risk.
  • Documentation accuracy: The description overstates provider coverage (Anthropic-direct) — see the inline comment.

Nice reuse of the existing helper to keep the tracer, meter, and agent token math consistent.

@opieter-aws
opieter-aws marked this pull request as ready for review August 31, 2026 23:51
@opieter-aws
opieter-aws requested a review from a team as a code owner August 31, 2026 23:51
@opieter-aws opieter-aws changed the title fix!: include cached tokens in input_tokens fix: include cached tokens in input_tokens Aug 31, 2026
@github-actions

github-actions Bot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

Re-reviewed after the update — code is unchanged; the deltas are the title reclassification and a reworded description.

Title fix!:fix:: This reclassification looks correct. The parity PR #3886 (fix: count cached tokens in context-size and compaction baseline) was itself shipped as non-breaking even though it changed the value of existing derived properties (context_size/projected_context_size). Telemetry attribute values aren't part of the semver code API, so treating this emit-value correction the same way is consistent precedent. 👍

One residual nit on the reworded description: it still frames "Bedrock Converse and Anthropic-direct excludes cached tokens" as the problem being corrected, but per the #3546 known limitation this change only actually folds cache into input_tokens for Bedrock Converse — Anthropic-direct stays an undercount until adapter-side normalization lands. Worth scoping that sentence to Bedrock (or flagging Anthropic-direct as a follow-up) so the cost-backend expectation is precise. No code change needed.

Assessment stands: Approve.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area-model Related to models or model providers bug Something isn't working complexity/low Touched functions have low cognitive complexity (<=10) python Pull requests that update python code size/xs

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants