Skip to content

feat(sdk): add per-call prompt token composition metrics - #4623

Open
georgeglarson wants to merge 20 commits into
OpenHands:mainfrom
georgeglarson:feat/llm-prompt-composition-metrics
Open

feat(sdk): add per-call prompt token composition metrics#4623
georgeglarson wants to merge 20 commits into
OpenHands:mainfrom
georgeglarson:feat/llm-prompt-composition-metrics

Conversation

@georgeglarson

@georgeglarson georgeglarson commented Aug 24, 2026

Copy link
Copy Markdown
Contributor

HUMAN:

Validated. Human thinks this is correct also.


AGENT:

Reshaped per the review thread: this is no longer a runtime metric. The decomposition now ships as an offline post-run analysis toolscripts/prompt_composition_report.py — that ingests existing completion logs (LLM(log_completions=True)) and emits the per-call breakdown plus a text visualization. The only runtime change left in the diff is a 2-line logging-fidelity fix (below).

Verification beyond unit tests:

  • The decomposition logic itself is unchanged from the version validated on live harness-benchmark runs (MiniMax-M3, 41 calls; gpt-4o-mini, 37 calls; per-call data under .pr/evidence/). Those lanes were captured with the earlier runtime shape and remain as the correctness baseline — the script computes the same buckets from the same payloads.
  • Script end-to-end against real telemetry-written logs (both chat and Responses paths, produced through the actual log_completions pipeline with mocked transports): all four buckets populate, est/provider ratios land in the same 0.96–1.04 band the live lanes showed.
  • Backward compatibility: pre-fix log directories (whose tools entries lack parameter schemas) are detected and flagged tool_schema_counted=false rather than silently miscounted; garbage/error files are skipped.
  • uv run pytest tests/sdk/llm -q → 995 passed, including 14 composition tests and 2 new tests pinning the log shape through the real telemetry path. pre-commit run --files on every changed file → clean.

Sample output (synthetic but realistic fixture):

Prompt composition per call (S=system T=tool_schema H=history L=latest, bar width ~50 chars)
   seq  composition                                         est_total   provider
     0  SSSSSSSSSSSSSSTTL                                         725        700
     1  SSSSSSSSSSSSSSTTHHL                                       804        830
     2  SSSSSSSSSSSSSSTTHHHHL                                     920        960

Trend over seq:
   seq |   system | tool_schema |  history |   latest | est_total | provider |  ratio | latency_s
     0 |      608 |          85 |        0 |       32 |       725 |      700 |   1.04 |      0.00
     1 |      608 |          85 |       79 |       32 |       804 |      830 |   0.97 |      0.00
     2 |      608 |          85 |      195 |       32 |       920 |      960 |   0.96 |      0.00

Why

Per-call prompt composition — how much of each LLM call is system prompt vs tool schemas vs conversation history vs the latest message — is the measurement needed for tool-loading and context-budget work (#4083 motivates deferred loading; this measures what it would save). The provider returns one aggregate prompt_tokens; the split only exists client-side.

The first version of this PR computed the split at call time (behind an opt-in flag). Review feedback converged on a lower-risk first step: get the decomposition into people's hands as an offline tool, validate its usefulness on real runs, and earn any runtime API surface later. This is that repackaging — the counting logic (compute_prompt_composition, the tool-schema marginal delta, the Responses payload conversion) is unchanged, just invoked post-run over logs instead of during the call.

Summary

  • New scripts/prompt_composition_report.py: point it at a completion-logs directory and it emits per-call JSONL + summary JSON + a text chart (per-call stacked buckets and a trend table over the conversation).
  • The composition helpers move out of the runtime path into a self-contained utility module; Metrics/Telemetry/LLM are byte-identical to main except the one fix below. tool_tokens is renamed tool_schema_tokens for clarity.
  • Logging-fidelity fix (2 lines): completion logs now record the finalized OpenAI-format tool schemas (cc_tools / resp_tools) instead of the ToolDefinition dumps, which carried no parameter schemas. Without this, offline analysis cannot reconstruct the tool-schema bucket — the largest bucket in the evidence lanes. No counting is added at runtime; the log simply records what was already sent.

Issue Number

Related to #4083 (this PR provides the measurement; it does not implement deferred loading).

How to Test

uv run pytest tests/sdk/llm/test_prompt_composition.py -q
uv run pytest tests/sdk/llm/test_llm_log_completions_integration.py -q

Against real logs from any run made with log_completions=True:

python scripts/prompt_composition_report.py --root <logs-dir> [--out report-dir]

Video/Screenshots

Sample script output is inline in the AGENT section above; per-call baseline data from the live lanes is attached under .pr/evidence/.

Type

  • Feature

Notes

Companion docs PR: OpenHands/docs#757 (being updated in step to document the script rather than a runtime metric).

Runtime observability (live composition during the call, context-window UI) is deliberately out of scope per the review thread; the design carries over if there is later appetite.

@github-actions

Copy link
Copy Markdown
Contributor

📁 PR Artifacts Notice

This PR contains a .pr/ directory with temporary PR-specific documents. Because this is a fork PR, the directory will be automatically removed from main immediately after merge.

georgeglarson added a commit to georgeglarson/software-agent-sdk that referenced this pull request Aug 24, 2026
…tion payloads

Address draft-PR review on prompt composition:

- Async prepare paths now compute prompt composition via asyncio.to_thread
  (the pattern aformat_messages_for_responses uses for image inlining)
  instead of running up to 5 prompt-sized synchronous tokenization passes
  on the shared event loop; sync paths stay synchronous.
- Subscription-mode Responses payloads normalize message items to
  {"role", "content"} without a "type" key; the payload converter now
  accepts that shape, so Codex subscription calls record compositions
  instead of silently skipping every call.
- Nits: is_estimate documented as reserved for a future provider-reported
  mode; PromptComposition docstring notes divergence from the
  chat-template budget counter; probe comment corrected (messages=[] works
  in litellm 1.84.1); inline test imports moved to module top; tautological
  delta assertion annotated; input_image converter branch covered; async
  aresponses composition test added.

Push-guard bypass: updating open draft PR OpenHands#4623 with review fixes,
approved by George.

Co-authored-by: openhands <openhands@all-hands.dev>
georgeglarson added a commit to georgeglarson/docs that referenced this pull request Aug 24, 2026
Mirrors the SDK docstring caveat (OpenHands/software-agent-sdk#4623):
subscription mode folds the system prompt into the first user message,
so those tokens count as history/latest on that transport.

Co-authored-by: openhands <openhands@all-hands.dev>
@rajshah4

Copy link
Copy Markdown
Member

Really nice work! Thanks for validating it against real agent runs. The decomposition is exactly the kind of measurement we need for tool-loading and context-budget studies.

I'm supportive of getting this into the SDK. One concern on the current shape: counting is always on for every LLM call, for every user. The measured cost is small (~10-20ms typical, ~31ms at 100K tokens), but it's a global tax on a metric most users won't consume.

Could we put this behind an opt-in flag (something like enable_prompt_composition on the LLM, defaulting off) so it's available when you want to troubleshoot a prompt or run a tool-loading study, without imposing the extra tokenization pass on every default user?

Drafted with help from an AI agent (OpenHands) on behalf of @rajshah4.

georgeglarson and others added 6 commits August 25, 2026 02:08
Record a per-call decomposition of estimated prompt tokens (system
prompt, tool schemas, conversation history, latest message) for every
LLM completion. Computed at the LLM completion/responses boundary where
messages and tools are final, counted with litellm's token_counter, and
recorded into Metrics via Telemetry so consumers can attribute prompt
size to components. Counts are client-side estimates (flagged via
is_estimate); provider-reported usage remains authoritative.

Co-authored-by: openhands <openhands@all-hands.dev>
…ords

Address review findings on prompt composition recording:

- Responses path now counts the finalized payload (instructions + input
  items converted to chat format) instead of the unprepared messages, so
  the record reflects what the provider received; tool serialization or
  conversion failures skip the record instead of breaking the call.
- An all-zero counting result (e.g. litellm.disable_token_counter) now
  skips the record instead of storing a bogus all-zero estimate.
- Docstrings: components do not necessarily sum to provider prompt_tokens
  (fixes the inverted direction claim), tool schema counts follow litellm's
  token_counter serialization convention rather than wire-format JSON, and
  counting cost is documented as linear in prompt size (~31 ms at ~100K
  tokens, ~61 ms at ~190K tokens measured).
- Tests: mock-tools double-count guard, controlled tool-token delta,
  all-zero skip, Responses payload converter, tool serialization failure.

Co-authored-by: openhands <openhands@all-hands.dev>
Co-authored-by: openhands <openhands@all-hands.dev>
Co-authored-by: openhands <openhands@all-hands.dev>
…tion payloads

Address draft-PR review on prompt composition:

- Async prepare paths now compute prompt composition via asyncio.to_thread
  (the pattern aformat_messages_for_responses uses for image inlining)
  instead of running up to 5 prompt-sized synchronous tokenization passes
  on the shared event loop; sync paths stay synchronous.
- Subscription-mode Responses payloads normalize message items to
  {"role", "content"} without a "type" key; the payload converter now
  accepts that shape, so Codex subscription calls record compositions
  instead of silently skipping every call.
- Nits: is_estimate documented as reserved for a future provider-reported
  mode; PromptComposition docstring notes divergence from the
  chat-template budget counter; probe comment corrected (messages=[] works
  in litellm 1.84.1); inline test imports moved to module top; tautological
  delta assertion annotated; input_image converter branch covered; async
  aresponses composition test added.

Push-guard bypass: updating open draft PR OpenHands#4623 with review fixes,
approved by George.

Co-authored-by: openhands <openhands@all-hands.dev>
…converter

Review-panel disposition (accepted tradeoff): in subscription mode the
auth-layer transform folds the system prompt into the first user message,
so those tokens are counted in history/latest rather than
system_prompt_tokens. Buckets follow the wire payload, which is the
documented contract for the Responses path; reclassifying would require
guessing at a lossy fold.

Co-authored-by: openhands <openhands@all-hands.dev>
@georgeglarson
georgeglarson force-pushed the feat/llm-prompt-composition-metrics branch from a209d86 to 208bba2 Compare August 25, 2026 06:30
georgeglarson added a commit to georgeglarson/docs that referenced this pull request Aug 25, 2026
Mirrors the SDK docstring caveat (OpenHands/software-agent-sdk#4623):
subscription mode folds the system prompt into the first user message,
so those tokens count as history/latest on that transport.

Co-authored-by: openhands <openhands@all-hands.dev>
…sition

Address rajshah4's review on draft PR OpenHands#4623: put composition counting
behind an opt-in flag (default off) so it is available for prompt
troubleshooting and tool-loading studies without imposing the extra
tokenization pass on every default user. When off, no composition is
computed on any path (chat/responses, sync/async), no records are
appended, and token_counter is never called; when on, behavior is
unchanged from before.

Push-guard bypass: updating open draft PR OpenHands#4623 with review fixes,
approved by George.

Co-authored-by: openhands <openhands@all-hands.dev>
@georgeglarson

Copy link
Copy Markdown
Contributor Author

Done in 4a9c39df: counting is now opt-in via enable_prompt_composition on the LLM, default
off. When off, no tokenization pass runs and no records are appended; a spy test asserts zero
token_counter calls on that path. When on, behavior is unchanged from the numbers above.
Companion docs PR updated too. Thanks for the review.

@georgeglarson
georgeglarson marked this pull request as ready for review August 25, 2026 08:00
@VascoSch92

VascoSch92 commented Aug 25, 2026

Copy link
Copy Markdown
Member

Thanks for the PR. I looked a little inside.

it is actually adding a little of confusion and repetition.

Lets take an example

"composition": {
    "model": "gpt-4o-mini", 
    "system_prompt_tokens": 3340, 
    "tool_tokens": 5702, 
    "history_tokens": 0, 
    "latest_message_tokens": 38, 
    "is_estimate": true, 
    "response_id": "chatcmpl-EGVEG8NSZIEywDIyNQnqt99X5R4tJ"}
  • model: you have already this information. So this is just a repetition.
  • system prompt tokens never change from the beginning of the convo. But this is saving them every turn right?
  • what it means tool_tokens? tool output? tool call? tool description? If it is the third: these are already in the system prompt (so is a subset)
  • what history_tokens? Cached ones? Are the one of the previous round? But actually you don't need to save this information because you can aways do the difference between two saved composite metrics
  • why it is estimated? I mean, this is just a computation and we have the correct data from the provider right?
  • response id is a repetition. We do have already this info.

@rajshah4

rajshah4 commented Aug 25, 2026

Copy link
Copy Markdown
Member

Thanks for taking a look, @VascoSch92 — these are fair questions to raise on a new metrics surface. A few of them are worth revisiting against the evidence traces attached to the PR (.pr/evidence/), since the live runs clarify the design choices. Using the gpt-4o-mini task-01 lane as a reference point:

  • seq 0: provider reports prompt_tokens=9172 (a single total), while the composition records system=3340, tool=5702, history=0, latest=38.

On (5) why it's an estimate — this is the crux. The provider returns one aggregate number, not a breakdown. There's no provider API that returns the 3340/5702/0/38 split; that decomposition only exists client-side. Producing that breakdown is the entire reason for the feature — if the provider gave it to us, we wouldn't need this PR. The is_estimate flag just makes the provenance explicit so nobody mistakes the client-side split for a provider-reported figure.

On (3) tool_tokens — on native-function-calling models the traces show system=3,340 and tool=5,702 as two independent, constant buckets. If the tool schemas were folded into the system prompt, system would read ~9,080 and tool would be 0 — it doesn't. They're sent as the separate tools request parameter. For the non-native path where schemas are rendered into the prompt, the PR already passes tools=None so tool_tokens=0 and there's no double count (covered by test_mock_tools_does_not_double_count_tool_schemas). Your naming point is fair though — tool_schema_tokens would be clearer than tool_tokens and would remove the ambiguity about whether it means schemas, calls, or outputs.

On (4) history derivable by diffing — not quite cleanly. seq 0 → seq 1: history goes 0 → 73, but latest_0=38. The gap is the assistant's completion (38 tokens, which lives in TokenUsage, not in any composition bucket) plus framing overhead. Reconstructing history would require a cross-list join against token_usages.completion_tokens and still only approximate the value. Storing it directly is exact, and the cost is a handful of ints per call.

On (2) system constant per turn — true in these traces (3,340 across all 9 calls of task-01), and a reasonable thing to flag. The per-call shape mirrors the existing TokenUsage / ResponseLatency records though, and it lets a caller detect a mid-conversation system change (skill activation, condenser rewrite, dynamic system message) without needing a special event type. Happy to consider whether a "preamble summary" record is worth adding, but the per-call cost is small.

On (1) and (6) model / response_id — both match the existing pattern: TokenUsage carries model, and ResponseLatency carries both model and response_id. Keeping PromptComposition consistent lets each record stand on its own. response_id specifically is the join key back to token_usages, and the docs note the two lists can diverge (skipped composition, or a response with no usage), so positional joining isn't safe.

The one genuinely actionable nit I see is the tool_tokens naming — tool_schema_tokens would be clearer. The rest looks consistent with the existing metrics design, and the traces back up the choices. Appreciate the review.

Drafted with help from an AI agent (OpenHands) on behalf of @rajshah4.

@VascoSch92

VascoSch92 commented Aug 25, 2026

Copy link
Copy Markdown
Member

On (1) and (6) model / response_id — both match the existing pattern: TokenUsage carries model, and ResponseLatency carries both model and response_id. Keeping PromptComposition consistent lets each record stand on its own. response_id specifically is the join key back to token_usages, and the docs note the two lists can diverge (skipped composition, or a response with no usage), so positional joining isn't safe.

If old code is not optimal doesn't mean that we should continue to create not optimal code.

On (5) why it's an estimate — this is the crux. The provider returns one aggregate number, not a breakdown. There's no provider API that returns the 3340/5702/0/38 split; that decomposition only exists client-side. Producing that breakdown is the entire reason for the feature — if the provider gave it to us, we wouldn't need this PR. The is_estimate flag just makes the provenance explicit so nobody mistakes the client-side split for a provider-reported figure.

It is an estimate if you can not compute that correctly. But in this case we can. So there is no motivation to have estimate here.

I think all that can be resume in a script that you can run after an agent conversation to extract the information. but at run time we don't need that.

@rajshah4

rajshah4 commented Aug 25, 2026

Copy link
Copy Markdown
Member

Fair point on the architecture — I think you're right that starting with a script is the better first step. Get the decomposition into people's hands as an offline tool, validate it's useful on real runs, and earn the runtime API surface before committing to it. No disagreement from me on that.

@georgeglarson — the decomposition logic you've built here (compute_prompt_composition, the tool-token marginal delta, the Responses payload conversion) is solid and reusable as-is for a script that ingests a conversation's logged messages + token_usages and emits the per-call breakdown. The hard part is done. Would you be open to repackaging this as a post-run analysis tool rather than a runtime metric? The test coverage carries over too. One addition worth considering: maybe we can include some visualization tools alongside the script to help people understand what's going on — a simple breakdown of where tokens go per call (system / tool schemas / history / latest), maybe a per-call trend over the conversation, would make the output much more approachable than raw JSON. Even a basic matplotlib or text-based chart would go a long way for troubleshooting.

If the script proves the value and there's later appetite for live observability (context-window UI, real-time tool-loading studies), the runtime path is already designed and can be revisited then. But starting with the script is the lower-risk way in.

Happy to run the script against the harness-benchmark suite and report back on what it surfaces.

Drafted with help from an AI agent (OpenHands) on behalf of @rajshah4.

@georgeglarson

Copy link
Copy Markdown
Contributor Author

Before I repackage this, there is data-availability to consider.

The current implementation measures the finalized request at the LLM call boundary.
At that point it has the messages and tool schemas actually being sent, after condensation, skill activation, dynamic system-message changes, and the Responses/subscription transforms.

A completed conversation's event log and token_usages do not preserve those finalized per-call request payloads.
A post-run script using only those records would have to reconstruct each prompt, so it would not retain the same measurement fidelity.
To make the offline version equivalent, we would first need to capture and persist the finalized request payload for every measured call, then run the analyzer over that trace.

The current opt-in version avoids that extra capture path: with enable_prompt_composition=False, it performs no tokenization pass and appends no composition records. When enabled, it measures the request while the required inputs are present.

I see two lossless options:

  1. Keep the default-off runtime metric in this PR. It has no counting work when disabled and gives precise per-call results when enabled.
  2. Add request-trace capture plus an offline analyzer. This moves the analysis out of the call path, but adds storage and capture plumbing that ordinary conversation logs do not currently provide.

I lean toward the current opt-in strategy because it is the smaller path and preserves the measurement.
Does this constraint change your preference between the two approaches?

@all-hands-bot

Copy link
Copy Markdown
Collaborator

🤖 OpenHands is reviewing this PR.

Head commit: b15a6250ee4816270f0878eb3752d888ea75d612
View the conversation: https://oss-agent-canvas.ngrok.dev/conversations/d4ea8987-6d3c-4f08-a839-ef9c2e13a89a

This comment was posted by an AI agent (OpenHands).

@all-hands-bot all-hands-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This review was created by an AI agent (OpenHands) on behalf of the repository maintainers.

Verdict: COMMENT (not approving)

No material bugs or security issues found. The implementation is careful: the feature is opt-in (enable_prompt_composition, default off), best-effort (counting failure silently skips the record and never affects the call), and additive-only (older persisted Metrics payloads still load — verified by test_metrics_loads_payload_without_prompt_compositions). The off-path is proven cheap: test_completion_off_never_calls_token_counter asserts zero token_counter calls when disabled. Tests are real-code-path and cover the mock-tool double-count case, the subscription-shaped Responses payload, serialization-failure fallback, and merge/diff. No pyproject.toml version bump.

Two things keep me from approving, both for a human maintainer to decide:

  1. Unresolved placement decision (runtime metric vs. offline script). @VascoSch92 raised, and @rajshah4 initially agreed, that this decomposition might be better shipped as a post-run analysis tool rather than a runtime metric. @georgeglarson pushed back with a real technical argument: the finalized per-call request (post-condensation, post-skill-activation, post-Responses/subscription transforms) is not persisted in the event log or token_usages, so an offline script cannot reproduce this measurement without adding a request-trace capture path. That's a legitimate data-availability point, but the architectural direction still isn't settled — please confirm the runtime surface is the intended home before merging.

  2. Eval-risk category. This changes the LLM call boundary (llm.py). Because the new work is gated behind a default-off flag, I would not expect any benchmark/eval impact on the default path, and the PR description includes live-run validation (MiniMax-M3 + gpt-4o-mini on harness-benchmark short suite, est/provider ratio 0.99–1.01 on the mapped-tokenizer lane). That's good evidence, but it is not an eval-monitor run confirmed by a maintainer, so per repo policy I'm leaving COMMENT rather than APPROVE.

Non-blocking notes (already discussed in-thread, no action required unless you disagree):

  • PromptComposition.is_estimate is always True with no code path setting False; it's documented as reserved for a future provider-reported mode. Either accept it as a provenance marker or drop it until that mode exists.
  • tool_tokens naming — tool_schema_tokens would be clearer about whether it means schemas vs. calls vs. outputs.

[RISK ASSESSMENT]

  • Overall PR: 🟢 LOW — default-off, additive-only, best-effort, no breaking changes, no dependency changes, comprehensive tests. Residual risk is limited to the opt-in path's accuracy (explicitly flagged via is_estimate), which is the point of the feature rather than a defect.

Repackage the per-call prompt token composition feature as an offline
analysis utility: llm.py, metrics.py, telemetry.py and the package
__init__ exports return to upstream/main content (no runtime flag, no
Metrics plumbing, no new public API surface). The PromptComposition
model moves from metrics.py into prompt_composition.py, its tool_tokens
field is renamed to tool_schema_tokens, and the docstrings now describe
offline counting over logged payloads instead of opt-in runtime
recording.

Co-authored-by: openhands <openhands@all-hands.dev>
georgeglarson and others added 5 commits August 31, 2026 14:01
Drop the runtime-wiring tests (Metrics plumbing, enable_prompt_composition
gating, per-path recording) now that composition counting is an offline
utility, and adjust the carried-over tests for the PromptComposition move
into prompt_composition.py and the tool_tokens -> tool_schema_tokens
rename. Add a converter test for subscription-shaped message items
(previously only exercised through the removed runtime path).

Co-authored-by: openhands <openhands@all-hands.dev>
Add scripts/prompt_composition_report.py, a stdlib-only CLI that rebuilds
per-call prompt token composition from LLM(log_completions=True) logs —
no runtime changes required. Ingestion and report logic live in
openhands.sdk.llm.utils.prompt_composition_report so tests can import it;
the script is a thin argparse wrapper in the spirit of
scripts/completion_logs_viewer.py.

Per log file the report converts Responses-path payloads via
responses_payload_to_chat_messages, uses chat messages directly, and
passes tools=None when raw_messages is present (mock-tools path renders
schemas into the prompt text — no double count). Logs serialize tools as
ToolDefinition dumps without parameter schemas, so when logged tools are
not OpenAI-format the row is marked tool_schema_counted=false instead of
reporting a silently wrong bucket. Output is per-call JSONL rows
(seq/usage/composition/latency_s), a summary JSON (per-bucket averages,
est/provider median ratio over fully-counted calls), and a text chart:
per-call stacked bars plus a trend table over seq.

Co-authored-by: openhands <openhands@all-hands.dev>
Add focused ingestion tests for the offline report: synthetic chat logs
(rows sequenced by call timestamp, usage joined, summary averages and
est/provider ratio), a synthetic Responses-path log (instructions +
input items converted before counting), a mock-tools log (raw_messages
present, schemas counted once via the prompt text), a native-path log
with ToolDefinition-dump tools (tool bucket marked uncountable and
excluded from the ratio), garbage files skipped, and the
calls.jsonl/summary.json writer.

Co-authored-by: openhands <openhands@all-hands.dev>
Completion logs serialized telemetry_ctx["tools"] as ToolDefinition
dumps (name/description only, no parameter schemas), so offline analysis
could not reconstruct the tool-schema portion of the request. Log the
finalized schemas actually sent instead: cc_tools (OpenAI chat format)
on the chat path and resp_tools (Responses ToolParam) on the Responses
path. Telemetry.log_llm_call still strips the duplicate kwargs["tools"],
so the top-level schemas are the single logged copy. No other runtime
behavior changes.

Co-authored-by: openhands <openhands@all-hands.dev>
Completion logs now carry the finalized tool schemas (OpenAI chat format
on the chat path, Responses ToolParam on the Responses path), so the
report normalizes both shapes to chat format for token counting and the
tool_schema bucket populates on fresh logs. Logs written before the
logging fix still serialize tools as ToolDefinition dumps; those remain
detected and flagged tool_schema_counted=false rather than miscounted,
so the script stays safe over historical log directories.

Co-authored-by: openhands <openhands@all-hands.dev>
@georgeglarson

Copy link
Copy Markdown
Contributor Author

@rajshah4 Reshaped as you suggested — the runtime metric (including the opt-in flag) is gone, and the decomposition now ships as an offline tool: scripts/prompt_composition_report.py. Point it at any completion-logs directory from a run made with the existing log_completions=True and it emits per-call JSONL, a summary, and a text chart (stacked system / tool_schema / history / latest per call, plus a trend table). The counting logic is unchanged from what the evidence lanes validated; tool_tokens is now tool_schema_tokens per your nit.

One finding worth flagging: current logs don't actually preserve the tool schemas — telemetry was logging the ToolDefinition dumps (descriptions only) and popping the OpenAI-format cc_tools from kwargs, so the tool-schema bucket (the largest one in the evidence lanes) was unreconstructable offline. The PR now includes a 2-line logging-fidelity fix so the log records the schemas exactly as sent. No counting at runtime; the log just keeps what already went over the wire. Pre-fix log directories are detected and flagged rather than miscounted, so the script is safe on historical logs.

If you're still up for running it against the harness-benchmark suite: any run with log_completions=True on this branch produces fully-sufficient logs; on older logs everything except the tool_schema bucket still computes.

Metrics/Telemetry/LLM are otherwise byte-identical to main — the runtime API surface is deferred exactly as you framed it.

@all-hands-bot

all-hands-bot commented Aug 31, 2026

Copy link
Copy Markdown
Collaborator

🚦 CI is currently failing on this PR's latest commit.

Please fix the failing checks before OpenHands reviews it - this is re-checked automatically once you push a new commit. (A maintainer can also request @all-hands-bot as a reviewer to have it reviewed regardless of CI status.)

This is an automated check - no AI was used to generate this comment.

@georgeglarson

Copy link
Copy Markdown
Contributor Author

CI note on the sdk-tests red: the only failure is tests/sdk/conversation/goal/test_render_transcript.py::test_render_transcript_for_judge, a byte-for-byte golden transcript test of conversation/goal — a subsystem this PR's diff does not touch (net diff vs main: the composition utility module, the report script, tests, and the 2-line logging-fidelity change in _finalize_completion_params). It passes locally on this exact head — individually, and in a full tests/sdk xdist run (5966 passed) — and Run tests on main is green. Looks like a CI-side flake; a rerun should clear it. The endpoint-audit red is the known fork-PR comment-permission quirk (fix in #4796); the audit itself passes.

georgeglarson and others added 4 commits August 31, 2026 14:47
Both new test modules defined a private _Args(Action) and _MockTool.
When xdist placed them on the same worker, pydantic's duplicate class
definition guard failed every Event/ToolDefinition model_validate_json
in that worker — surfacing as unrelated golden-transcript and
serialization failures only in full-suite CI runs, never single-file.
Rename each module's helpers to unique names
(_CompositionArgs/_CompositionMockTool,
_LogCompletionsArgs/_LogCompletionsMockTool).

Co-authored-by: openhands <openhands@all-hands.dev>
…ics' into feat/llm-prompt-composition-metrics
@georgeglarson

Copy link
Copy Markdown
Contributor Author

Correcting my earlier CI note — root cause found, and it was this PR's. Both new test modules defined a private _Args(Action) and _MockTool; when xdist scheduled them on the same worker, pydantic's duplicate-class-definition guard failed every Event.model_validate_json in that worker, surfacing as the golden-transcript and serialization failures. Full-suite-only by construction, which is why single-file runs and the smaller PRs never reproduced it. Fixed in ba03668ef (unique helper names per module); sdk-tests is now green. The remaining integration-test red is an unrelated event-store flake (same test green on this PR's earlier runs; the diff touches no agent-server code).

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants