fix(streaming): aggregate model info per invocation, not per session - #205
Open
LeonxLJX wants to merge 4 commits into
Open
fix(streaming): aggregate model info per invocation, not per session#205LeonxLJX wants to merge 4 commits into
LeonxLJX wants to merge 4 commits into
Conversation
_extract_model_info_from_trace walked trace.all_spans for every invocation, so each invocation reported the session-wide token/model aggregates and any summation triple-counted usage. The converter now records the LLM spans each invocation was built from (ConversionResult.invocation_llm_spans, kept in lockstep through ADK/GenAI conversion and deduplication) and ws_server aggregates only those spans per invocation. Fixes agentevals-dev#204
krisztianfekete
requested changes
Aug 28, 2026
krisztianfekete
left a comment
krisztianfekete
left a comment
Contributor
There was a problem hiding this comment.
Thanks, added a few comments, please check them out!
- Multi-turn: when turn count matches LLM span count, map turn n to span n; otherwise attribute all spans to the first turn (empty for the rest) so session totals stay honest instead of every turn reporting all spans. - Dedup: merge dropped duplicate invocations' LLM spans into the survivor so their token spend is not discarded; always return the (invocations, spans) tuple and updated the test call sites accordingly. - ws_server: drop the dead \if trace\ guard (convert_traces is 1:1 with traces), log loudly on invocation_llm_spans index drift, remove the unused Trace import, and stop re-filtering LLM spans (a generate_content-only trace would otherwise be dropped to nothing even with usage metadata). The model info aggregator is now a @staticmethod. - find_adk_llm_spans_in returns both call_llm and generate_content spans so a provider generate_content span's temperature/model no longer vanish. - Tests: drive a real TraceSession through _extract_invocations instead of calling the aggregator on hand-picked spans; pin the nested sub-agent double-count limitation; add a dedup span-merge test.
Author
|
Thanks for the thorough review @krisztianfekete! I've pushed
Test run on the touched areas: 249 passed (remaining full-suite failures are pre-existing env issues — API keys / installed-distribution version — unrelated to this change). |
- Add strict=True to zip() in genai_converter.py (B905) - Rename unused trace_idx to _trace_idx in ws_server.py (B007) - Run ruff format on genai_converter.py, test_genai_converter.py, test_model_info.py
- Restore 'call_llm_spans or generate_content_spans' return order as krisztianfekete requested (the combined list doesn't help because generate_content spans never carry ADK llm_request/llm_response attrs) - Fix garbled arrow characters in test_genai_converter.py docstring
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
2 participants
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.
Fixes #204
_extract_model_info_from_tracewalkedtrace.all_spansfor every invocation, so each invocation in a session reported the same session-wide token/model/provider aggregates, and anything summing them triple-counted usage.What changed
ConversionResultnow carriesinvocation_llm_spans— the LLM spans each invocation was built from, kept in lockstep through ADK conversion, GenAI single-turn conversion, and deduplication.ws_server._extract_model_info_from_trace→_extract_model_info_from_llm_spans: aggregates only the spans belonging to that invocation.Verification
tests/test_model_info.py(converter span tracking + per-invocation token aggregation + empty-span edge case).maintoo).Verified live with the zero-code examples: each invocation now reports its own
in= / out=numbers instead of the session aggregate.