Skip to content

fix: count cached tokens in context-size and compaction baseline - #3886

Merged
opieter-aws merged 8 commits into
strands-agents:mainfrom
opieter-aws:opieter-aws/fix-linting
Aug 27, 2026
Merged

opieter-aws merged 8 commits into
strands-agents:mainfrom
opieter-aws:opieter-aws/fix-linting

Conversation

@opieter-aws

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

Copy link
Copy Markdown
Contributor

Description

Context-window sizing and proactive compaction size the prompt as inputTokens + outputTokens. That undercounts cached tokens on providers where the cache read is reported additional to inputTokens (Bedrock Converse, Anthropic-direct): a 90k-token cache is currently underreported, so context utilization looks near-zero, proactive compaction never fires, and the run hits a hard context-window error instead of compacting.

On providers that fold the cache read into inputTokens (OpenAI, Gemini, LiteLLM) the same code path is already correct, so the fix handles both conventions without assuming either.

Two existing read-only properties change value on cache-reporting providers:
AgentResult.context_size / AgentResult.projected_context_size (Python), and the equivalent contextSize / projectedContextSize (TypeScript). On a disjoint-cache provider they now report the full prompt the model processed rather than the uncached remainder, e.g. a turn with inputTokens=10, cacheReadInputTokens=5848 now reports 5858 instead of 10.

Related Issues

#3546

Documentation PR

N/A

Type of Change

Bug fix

Testing

  • I ran hatch run prepare

Ran the Python telemetry, event-loop, and conversation-manager unit suites and the TypeScript meter, agent, and streaming suites; both cover the subset and disjoint conventions plus the no-cache collapse using the token counts captured in #3546. Python lint/type/complexity and TypeScript lint/type-check are clean.

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.

@opieter-aws
opieter-aws marked this pull request as ready for review August 19, 2026 15:29
@opieter-aws
opieter-aws requested a review from a team as a code owner August 19, 2026 15:29
@opieter-aws
opieter-aws requested a review from chaynabors August 19, 2026 15:29
@github-actions github-actions Bot added bug Something isn't working area-context Session or context related complexity/low Touched functions have low cognitive complexity (<=10) size/m strands-running labels Aug 19, 2026
@codecov

codecov Bot commented Aug 19, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

Comment thread strands-py/src/strands/telemetry/metrics.py Outdated
Comment thread strands-py/src/strands/telemetry/metrics.py
@github-actions

Copy link
Copy Markdown
Contributor

Issue: This changes the runtime value of the public read-only properties AgentResult.context_size / projected_context_size (and TS contextSize / projectedContextSize) on cache-reporting providers — a mostly-cached turn will jump from a handful of tokens to the full prompt. #3546 itself notes this "deserves a release note: anyone charting those fields will see a step." The PR is not labeled api/needs-review and the description lists documentation as not updated.

Suggestion: Add a release note documenting the behavior change on these public properties, and consider whether an api/needs-review label / API-reviewer signoff is warranted since the observable contract of a public API is shifting.

@github-actions

Copy link
Copy Markdown
Contributor

Assessment: Comment / Request Changes

Well-scoped fix with good cross-SDK parity and tests that assert the subset/disjoint/collapse invariant (as #3546 explicitly asked for). The main concern is that the convention-detection heuristic doesn't fully deliver the coverage the docstring claims.

Review themes

Nice work routing both SDKs through a single shared helper and covering the invariant rather than field presence.

@yonib05
yonib05 requested review from Unshure and removed request for chaynabors August 23, 2026 15:36
@github-actions

Copy link
Copy Markdown
Contributor

Issue (follow-up to the review — the file isn't in this PR's diff so noting it here): the new _full_prompt_tokens / fullPromptTokens helpers now depend on the disjoint/subset convention, but the Usage type doc (strands-py/src/strands/types/event_loop.py and the TS Usage interface in models/streaming.ts) still describes totalTokens as "Total number of tokens (input + output)". That contradicts the disjoint convention (input + output + cacheRead + cacheWrite) the helper relies on, and #3546 fix #1 explicitly asks for this contract to be written on Usage.

Suggestion: When touching this area, update the Usage field docs to state whether the cache counters are inside inputTokens and whether totalTokens includes them, so the next adapter/consumer doesn't have to re-derive the arithmetic. (Optional here if you'd prefer to keep this PR narrowly scoped and address the Usage contract in a dedicated change.)

@github-actions

Copy link
Copy Markdown
Contributor

Updated assessment (re-review of 944cae2): Approve

The fix bugs commit resolves both code-level findings from my first pass:

Resolved
  • Correctness / Anthropic-direct: docstring narrowed to the providers the heuristic actually detects, the arithmetic ambiguity is now documented as an explicit "Known limitation" (deferred to adapter-side normalization in [BUG] Usage cache-token fields mix two incompatible conventions, over-counting cost on OpenAI/Gemini/LiteLLM cache hits #3546), and boundary-guarding regression tests were added in both SDKs (test_latest_context_size_undercounts_anthropic_direct_cache / the TS fullPromptTokens case).
  • Robustness: _full_prompt_tokens now reads all three counters via usage.get(..., 0), eliminating the KeyError risk on partial usage dicts.

Verified locally: the telemetry context_size/full_prompt suite (19) and the event-loop baseline suite (5) pass.

Remaining items are minor and non-blocking: the Usage type-doc contract (#3546 fix #1, reasonably out of scope here) and a release note for the public-property value change. Nice, thorough follow-up.

@github-actions

Copy link
Copy Markdown
Contributor

Updated assessment (re-review of 485d41d "Include tracer"): Approve

The new commit extends the cache-aware prompt sizing into the tracer, and it's cleanly done.

Review notes
  • Tracer parity ✅: end_model_invoke_span and end_agent_span now route gen_ai.usage.prompt_tokens / input_tokens through _full_prompt_tokens, so span telemetry matches the metrics/compaction fix. input + output == total still holds on both conventions, and cache read/write are emitted as a breakdown — consistent with GenAI semantic conventions.
  • Test coverage ✅: dedicated subset and disjoint-cache regression tests for both spans (test_end_model_invoke_span_counts_disjoint_cache_tokens → 38, test_end_agent_span_counts_disjoint_cache_tokens → 85), all using full-object equality assertions. The Anthropic-direct limitation tests remain. Ran the telemetry + event-loop suites locally: 223 passed.
  • Minor (inline): the _full_prompt_tokens docstring lost the "Known limitation" note that had resolved an earlier thread — worth restoring one line. Non-blocking.

Remaining non-blocking items from earlier still stand: the Usage type-doc contract (#3546 fix #1) and a release note for the public property/telemetry value change. Solid, well-tested extension.

Comment thread strands-py/src/strands/telemetry/metrics.py
@github-actions

Copy link
Copy Markdown
Contributor

Re-review of 6335f67 (merge of main): Approve — no change to prior assessment.

This update is a merge from main to keep the branch current; there are no new source changes since 485d41d. Verified after the merge:

  • Effective PR diff unchanged (14 files); tracer + metrics fix and the subset/disjoint-cache regression tests are intact.
  • Ran the telemetry + event-loop suites locally against the merged tree: 225 passed (the merge picked up 2 additional tracer tests). Green.

The only open item remains the minor, non-blocking one from the last round: the _full_prompt_tokens docstring hasn't restored its "Known limitation" note. The two earlier non-blocking follow-ups (Usage type-doc contract per #3546 fix #1, and a release note for the public property/telemetry value change) also still stand. None block merge.

Comment thread strands-py/tests/strands/telemetry/test_metrics.py
Comment thread strands-py/src/strands/telemetry/metrics.py Outdated
Comment thread strands-py/src/strands/event_loop/event_loop.py Outdated
@github-actions

Copy link
Copy Markdown
Contributor

Re-review of 0c6ccdf ("Rename to totalPromptTokens"): Approve

This commit is a clean refactor that also picks up prior feedback:

What changed & verification
  • Helper extraction + naming parity ✅: TS now has a single totalPromptTokens(usage) in streaming.ts (marked @internal, so it's not a new public API surface), and the Python helper was renamed _full_prompt_tokens_total_prompt_tokens to match. grep confirms no stale references remain. The cache-convention logic is now centralized in one named function per SDK rather than inlined in meter.ts/agent.ts.
  • Docs follow-up resolved ✅: the public property docstrings (context_size, projected_context_size, latestContextSize, AgentResult accessors, meter.ts) now accurately describe the cache-inclusive semantics instead of the stale "input token count". This addresses the documentation gap I raised earlier.
  • "Known limitation" now guarded ✅: streaming.test.ts adds 5 cases mirroring the Python suite — disjoint (adds), subset (no double-count), cache read+write, no-cache collapse, and the Anthropic-direct undercount as a boundary-guard test with a clear #3546 comment. This is a better home for that note than the docstring.
  • Ran both suites locally: Python 225 passed; TS unit-node 191 passed, no type errors.

Only remaining non-blocking item: a release note for the public property / telemetry value change. Nice work tightening this up.

@opieter-aws
opieter-aws merged commit a6c2e5e into strands-agents:main Aug 27, 2026
48 of 49 checks passed
@opieter-aws
opieter-aws deleted the opieter-aws/fix-linting branch August 27, 2026 18:14
@codefre

codefre commented Aug 28, 2026

Copy link
Copy Markdown

I’m seeing a behavior after the recent telemetry changes that are causing an interoperability issue with AgentCore GenAI Observability.

After the change, gen_ai.usage.input_tokens appears to represent the full prompt size, including cache-read and cache-write tokens, while the cache token counts are also emitted separately.

For example:
gen_ai.usage.input_tokens: 12,737
cache read: 11,634
cache write: 1,099
derived uncached input: 4

AgentCore GenAI Observability appears to interpret the full input_tokens value as normal billable input and then adds the cache-read/write costs separately, which results in apparent double-counting in the cost display.

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

Labels

area-context Session or context related bug Something isn't working complexity/low Touched functions have low cognitive complexity (<=10) size/m

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants