fix(sync): correct Phoenix span extraction for multi-span traces and tool-calling agents#273
fix(sync): correct Phoenix span extraction for multi-span traces and tool-calling agents#273evduester wants to merge 10 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthrough
ChangesPhoenixSync trace deduplication and tool handling
Estimated code review effort: 4 (Complex) | ~50 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tests/unit/test_phoenix_sync.py (1)
726-766: ⚖️ Poor tradeoffConsider adding unit tests for new helper methods.
The existing
TestGetProcessedSpanIdsclass tests_get_processed_span_ids(), but the new analogous_get_processed_trace_ids()method lacks dedicated tests. Similarly,_select_representative_spans(),_extract_tools_from_span(), and_convert_openinference_tool_calls()are only covered indirectly throughsync()integration tests.Adding targeted unit tests would improve coverage for edge cases like:
_select_representative_spans()with missingstart_timeor multiple spans per trace_extract_tools_from_span()for each of the three attribute conventions_convert_openinference_tool_calls()format conversion and passthroughAs per coding guidelines: "All new features need tests."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/test_phoenix_sync.py` around lines 726 - 766, Add dedicated unit test classes for the new helper methods that currently lack direct test coverage. Create a TestGetProcessedTraceIds class similar to the existing TestGetProcessedSpanIds to test the _get_processed_trace_ids() method with scenarios for empty results, existing entities, and namespace not found exceptions. Add a TestSelectRepresentativeSpans class to test edge cases including missing start_time attributes and multiple spans per trace. Add a TestExtractToolsFromSpan class to test the method for each of the three supported attribute conventions. Add a TestConvertOpeninferenceToolCalls class to test format conversion logic and passthrough behavior. Ensure each test method follows the existing naming conventions and includes appropriate assertions and mocking setup.Source: Coding guidelines
altk_evolve/sync/phoenix_sync.py (1)
529-544: 💤 Low valueRedundant JSON schema parsing in fallback block.
Lines 537-543 attempt to parse
llm.tools.{i}.tool.json_schemaagain, but this same key was already read and parsed (or failed) at lines 521-527. If parsing succeeded, we alreadycontinued; if it failed or the key was absent, re-reading yields the same result.Consider removing this redundant block or clarifying the intent if the fallback is meant to handle a different attribute.
♻️ Proposed fix
# Fall back to building from name/description/parameters parts name = attrs.get(f"llm.tools.{i}.tool.name") if not name: continue tool: dict = {"type": "function", "function": {"name": name}} description = attrs.get(f"llm.tools.{i}.tool.description") if description: tool["function"]["description"] = description - json_schema = attrs.get(f"llm.tools.{i}.tool.json_schema") - if json_schema: - try: - schema = json.loads(json_schema) if isinstance(json_schema, str) else json_schema - tool["function"]["parameters"] = schema - except (json.JSONDecodeError, Exception): - pass + # parameters could come from a separate .parameters key if schema parsing failed above + parameters_str = attrs.get(f"llm.tools.{i}.tool.parameters") + if parameters_str: + try: + tool["function"]["parameters"] = json.loads(parameters_str) if isinstance(parameters_str, str) else parameters_str + except (json.JSONDecodeError, Exception): + pass tools.append(tool)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@altk_evolve/sync/phoenix_sync.py` around lines 529 - 544, The json_schema parsing block in the fallback section (lines 537-543) is redundant because the same llm.tools.{i}.tool.json_schema attribute was already read and parsed earlier in the code flow at lines 521-527. Since a successful parse would have already continued to the next iteration, re-attempting to parse the same attribute in the fallback will only produce the same result. Remove the redundant json_schema parsing block (the try-except block that reads and parses json_schema and assigns it to tool["function"]["parameters"]) from the fallback section to eliminate the duplicate logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@altk_evolve/sync/phoenix_sync.py`:
- Around line 529-544: The json_schema parsing block in the fallback section
(lines 537-543) is redundant because the same llm.tools.{i}.tool.json_schema
attribute was already read and parsed earlier in the code flow at lines 521-527.
Since a successful parse would have already continued to the next iteration,
re-attempting to parse the same attribute in the fallback will only produce the
same result. Remove the redundant json_schema parsing block (the try-except
block that reads and parses json_schema and assigns it to
tool["function"]["parameters"]) from the fallback section to eliminate the
duplicate logic.
In `@tests/unit/test_phoenix_sync.py`:
- Around line 726-766: Add dedicated unit test classes for the new helper
methods that currently lack direct test coverage. Create a
TestGetProcessedTraceIds class similar to the existing TestGetProcessedSpanIds
to test the _get_processed_trace_ids() method with scenarios for empty results,
existing entities, and namespace not found exceptions. Add a
TestSelectRepresentativeSpans class to test edge cases including missing
start_time attributes and multiple spans per trace. Add a
TestExtractToolsFromSpan class to test the method for each of the three
supported attribute conventions. Add a TestConvertOpeninferenceToolCalls class
to test format conversion logic and passthrough behavior. Ensure each test
method follows the existing naming conventions and includes appropriate
assertions and mocking setup.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4fe9522f-7f2d-4812-bfe7-f592a2818e09
📒 Files selected for processing (2)
altk_evolve/sync/phoenix_sync.pytests/unit/test_phoenix_sync.py
only output is trajectory["tools"]. I grepped: nothing reads it.
_select_representative_spans keeps only max(..., key=start_time). For a single linear agent run with cumulative context this is correct. |
There was a problem hiding this comment.
the PR correctly identifies and fixes both ways agentic traces were being stored wrong, and the fixes are wired up properly end-to-end.
the dedup half is solid and mergeable; the tools-extraction half is currently dead code and should either be wired up or removed, and the new parsing paths need unit tests.
|
@evduester please also update the branch |
|
Re #1 ( |
|
Pushed an update (020fcf6) addressing the review feedback: Dead code ( Missing tests — added dedicated unit tests for everything flagged as untested: "One representative span per trace" assumption — while digging into this, we found a real bug in exactly this logic: the candidate filter matched any span with an The "single linear conversation per trace" assumption itself remains as you described it — correct for single-agent sequential runs. This PR doesn't attempt to handle genuine multi-agent/parallel traces; that stays a known, accepted limitation rather than something addressed here. |
jayaramkr
left a comment
jayaramkr
left a comment
There was a problem hiding this comment.
Test coverage (my biggest ask) — fully resolved. 22 new unit tests, and I ran them: 68 passed
Multi-agent / trace-granularity caveat — addressed by design, well. The naive "one representative per trace" was reworked into a proper pipeline
…tool-calling agents
Two independent fixes to `PhoenixSync` that affect guideline generation accuracy.
Fix 1 — trace-level dedup and representative span selection:
Each agent run emits one Phoenix span per LLM call, with every subsequent span
re-including all prior messages (cumulative context window). Processing all spans
caused the same conversation steps to be analysed multiple times with growing
context, inflating guideline counts and skewing results. The fix deduplicates at
trace level (`_get_processed_trace_ids`) and picks a single representative span
per trace — the last by `start_time`, which holds the most complete message
history (`_select_representative_spans`).
Fix 2 — complete message and tool extraction from OpenInference spans:
The Phoenix REST API returns OpenInference attributes as flat indexed keys
(`llm.input_messages.{i}.message.*`, `llm.tools.{i}.tool.json_schema`) rather
than nested lists. The previous extraction code missed `tool_call_id` on tool
messages and `tool_calls` on assistant messages, producing incomplete trajectories
where tool-calling steps appeared as `content: "None"` with no linkage between
assistant calls and tool results. Guidelines generated from such trajectories
lacked tool-use context. The fix adds:
- `tool_call_id` extraction in both message loops
- an indexed-attribute reader for `llm.input_messages.{i}.*` and
`llm.output_messages.{i}.*` (including nested `tool_calls.{j}.*`)
- `_extract_tools_from_span`: parses `llm.tools.{i}.tool.json_schema` and the
list-mode `{"tool.json_schema": "..."}` format into OpenAI tool dicts
- `_convert_openinference_tool_calls`: converts OpenInference tool_call dicts to
OpenAI format (`tool_call.function.name` → `function.name`, etc.)
- `_extract_trajectory`: propagates `tool_calls` and `tool_call_id` from the
extracted message dict, choosing the right assembly path per message format
Unit tests updated: entity metadata mocks now include `trace_id` to match the
new trace-level dedup logic.
Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
…verage Addresses review feedback on the trace-level dedup half of this PR. The candidate filter previously matched any span with `input.value`/`llm.input_messages` attributes — but OpenInference sets `input.value` on every span kind (TOOL, CHAIN, AGENT), not just LLM spans. For agents that execute tools outside the LLM call itself (e.g. smolagents' CodeAgent running generated code locally), this let `_select_representative_spans` pick a TOOL-kind span as "the" representative span for a trace, producing degenerate trajectories with no model attribution. - `_is_llm_span()`: classify spans using Phoenix's canonical `span_kind` field (`LLM` vs `TOOL`/`CHAIN`/`AGENT`) instead of the `input.value` heuristic, falling back to attribute inspection only for spans where `span_kind` is absent. - `_dedupe_nested_llm_spans()` / `_is_ancestor()`: collapse multi-layer instrumentation of one logical call (e.g. three nested LLM-kind spans from stacked litellm/openai wrappers) to its innermost span via `parent_id` ancestry, so layering doesn't produce spurious candidates. - `_select_representative_spans` split into `_group_spans_by_trace` (pure grouping) + `_select_representative_span` (pick latest within one trace's already-filtered spans), orchestrated per-trace by the new `_build_trajectory_for_trace`. - `_extract_trajectory`'s message-assembly logic extracted into reusable `_assemble_openai_messages`/`_extract_usage` helpers (no behavior change). Also adds the unit tests flagged as missing in review: `_get_processed_trace_ids`, `_extract_tools_from_span` (all three attribute conventions), `_convert_openinference_tool_calls`, plus coverage for the new span-classification/dedup/selection logic. Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
- ruff format: auto-formatted both files to comply with project style - mypy/_span_id: avoid returning Any — explicitly extract and cast the span_id value - mypy/mapped_msg: rename variable to `indexed_msg` in the indexed input-message parsing loop to avoid shadowing the same name used in the flat-list loop above - mypy/_build_trajectory_for_trace: annotate `parent_of` with an explicit `dict[str, Optional[str]]` type and filter out spans with no span_id (None keys) so the type matches `_dedupe_nested_llm_spans`'s expected signature Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
evduester
force-pushed
the
fix/phoenix-span-extraction
branch
from
f53a976 to
7ea62d8
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
altk_evolve/sync/phoenix_sync.py (1)
269-280: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winParse JSON-string
tool_callsbefore storing them.
_assemble_openai_messages()passesraw_tool_callsto_convert_openinference_tool_calls(), which expects a list. If Phoenix returnsmessage.tool_callsas a JSON string, this silently iterates characters and drops the tool call.Suggested fix
- tool_calls = msg.get("message.tool_calls") or msg.get("tool_calls") + tool_calls = msg.get("message.tool_calls") or msg.get("tool_calls") + if isinstance(tool_calls, str): + tool_calls = self._parse_content(tool_calls)Apply the same normalization in the output-message branch.
Also applies to: 319-330
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@altk_evolve/sync/phoenix_sync.py` around lines 269 - 280, The `tool_calls` handling in `phoenix_sync.py` is passing through raw JSON strings, which later breaks `_convert_openinference_tool_calls()` because it expects a list. Update the message assembly in `_assemble_openai_messages()` and the output-message branch to normalize `message.tool_calls`/`tool_calls` by parsing JSON-string values into Python lists before assigning them to `mapped_msg["tool_calls"]`. Use the existing `tool_calls`/`tool_call_id` logic as the place to apply this normalization so both branches behave consistently.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@altk_evolve/sync/phoenix_sync.py`:
- Around line 131-139: In phoenix_sync.py, the span_kind check in the LLM
detection helper should treat Phoenix span_kind as authoritative, so any
explicit non-LLM kind like TOOL, CHAIN, or AGENT must return False immediately
instead of falling through to attribute-based heuristics. Update the logic
around the span.get("span_kind") handling to short-circuit on non-LLM values
before evaluating attributes, keeping the existing heuristic checks only for
spans without a definitive kind.
- Around line 335-413: The message parsing in phoenix_sync should not return
immediately after finding non-indexed prompt messages, because that bypasses
indexed llm.output_messages and tool_calls data. Update the message collection
logic in the parser method that builds messages from attrs to keep parsing
indexed OpenInference entries even when earlier messages exist, and then
de-duplicate appended results by a stable key like (type, index) so mixed
representations do not produce duplicates.
---
Outside diff comments:
In `@altk_evolve/sync/phoenix_sync.py`:
- Around line 269-280: The `tool_calls` handling in `phoenix_sync.py` is passing
through raw JSON strings, which later breaks
`_convert_openinference_tool_calls()` because it expects a list. Update the
message assembly in `_assemble_openai_messages()` and the output-message branch
to normalize `message.tool_calls`/`tool_calls` by parsing JSON-string values
into Python lists before assigning them to `mapped_msg["tool_calls"]`. Use the
existing `tool_calls`/`tool_call_id` logic as the place to apply this
normalization so both branches behave consistently.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 07f59111-d547-4d8e-bc78-a7a948ac0b53
📒 Files selected for processing (2)
altk_evolve/sync/phoenix_sync.pytests/unit/test_phoenix_sync.py
…rly return before indexed messages - _is_llm_span: if span_kind is present and non-LLM (TOOL, CHAIN, AGENT, ...) return False immediately instead of falling through to attribute heuristics; the canonical Phoenix field should be treated as authoritative in both directions, not just for the positive LLM case. - _extract_messages_from_span: do not return early after parsing flat/non-indexed message formats when the span also carries indexed llm.output_messages.* attributes; the early return was silently dropping indexed completions and tool_calls on spans that mixed input.value / non-indexed inputs with indexed OpenInference output attributes. Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
span.get() returns Any, so span_kind == "LLM" has type Any rather than bool. Wrapping with bool() satisfies the no-any-return check. Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
|
Commit 3c5b23d — CodeRabbit fixes — one good, one overcorrects Part A — _is_llm_span treats span_kind as authoritative both ways — ✅ correct. Now a span with span_kind of TOOL/CHAIN/AGENT returns False immediately instead of falling through to attribute heuristics. Well-reasoned, and covered by the new test_is_llm_span_false_for_tool_span / _for_chain_span. Part B — removing the early return before indexed parsing — CodeRabbit was right that if messages: return messages was dropping indexed completions when a span mixed non-indexed input with indexed output. But the chosen fix — if messages and not has_indexed: return messages — overcorrects. When a span has both a parseable input.value and flat llm.input_messages.* keys (the normal OpenInference LLM-span shape — both are emitted together), the non-indexed block appends the input messages, then the indexed block appends them again. I ran the actual PR code on such a span: total messages: 5 No test covers it. The suite (68 passing) never builds a span with both input.value and indexed input keys — the closest tests use one form or the other. Root cause: input and output are collapsed into one shared messages list gated by a single has_indexed flag. That coupling caused both the original drop bug and this new dup bug. The clean fix is to decide the source per message-type independently — e.g. use indexed input if any llm.input_messages.* keys exist else fall back to non-indexed input; same for output — so neither is double-counted. |
Addresses jayaram's review of commit 3c5b23d: the CodeRabbit fix for dropping indexed completions overcorrected by running both non-indexed and indexed parsing whenever indexed keys were detected, which duplicated input messages when a span carries both input.value and llm.input_messages.* simultaneously — the normal OpenInference LLM-span shape. Root cause: input and output were collapsed into one shared messages list gated by a single has_indexed flag, coupling the two sides incorrectly. Fix: choose the message source independently per side — - if llm.input_messages.* keys exist, use only the indexed input path - if llm.output_messages.* keys exist, use only the indexed output path - fall back to non-indexed (flat list / input.value / output.value) only when the corresponding indexed keys are absent This ensures a span with both input.value and indexed output attributes gets its input from the indexed path (no duplication) and its output from the indexed path (no omission), without either side double-counting. Adds a unit test with a span carrying both input.value and indexed input+output keys, asserting exactly 2 prompt messages and 1 completion with no duplicates. Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
|
@evduester the duplication fix is correct. but the code seems to have re-introduced a previously existing mypy bug which is causing CI to fail. Per claude phoenix_sync.py:308: error: Incompatible types in assignment (expression has type "Any | None", variable has type "list[dict[str, Any]]") Fix is trivial — rename the reused variable in the two non-indexed branches (e.g. tool_calls_flat, mirroring how the output side already uses tool_calls_out), or add an annotation. One-line-ish, no logic change. |
…s to fix mypy tool_calls = [] in the indexed-input block pins the type to list[dict[str, Any]]. The two non-indexed branches then reassigned the same name via msg.get(...) which returns Any | None — incompatible. Rename to msg_tool_calls in those two branches. Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
…es group Dependabot's schema requires at least a patterns or dependency-type field alongside applies-to — the entry was missing patterns, causing the config validator check to fail. Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
…erence messages
The per-side refactor introduced a regression for smolagents (and any
framework whose instrumentation uses the OpenInference multi-content
message format). When OpenInference serialises a message with multiple
content blocks (text, image, etc.) it uses the nested attribute path
llm.input_messages.{i}.message.contents.0.message_content.text
rather than the flat
llm.input_messages.{i}.message.content
key. The flat key is absent on those spans, so attrs.get() returned
None, which then became the literal string "None" via str(None) in
_convert_to_openai_format, leaving system and user messages empty.
Add an or-fallback to the nested path for both indexed input and
indexed output parsing so both attribute shapes are handled.
Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
2 participants
Summary
Fix 1 — trace-level dedup: Each agent run emits one Phoenix span per LLM call, with every subsequent span re-including all prior messages (cumulative context window). Processing all spans caused the same conversation steps to be analysed multiple times, inflating guideline counts. Now deduplicates at trace level and picks a single representative span per trace — the last by
start_time, which has the most complete message history.Fix 2 — complete message and tool extraction: The Phoenix REST API returns OpenInference attributes as flat indexed keys (
llm.input_messages.{i}.message.*,llm.tools.{i}.tool.json_schema) rather than nested lists. The old extraction missedtool_call_idon tool messages andtool_callson assistant messages.Before the fix, a tool-calling agent trajectory looked like this — the assistant call and tool results are completely unlinked:
{"role": "assistant", "content": "None"}, {"role": "tool", "content": "20"}, {"role": "tool", "content": "25"}After the fix, the trajectory is complete and valid:
{"role": "assistant", "tool_calls": [{"id": "call_abc", "type": "function", "function": {"name": "multiply", "arguments": "{\"a\": 10, \"b\": 2}"}}]}, {"role": "tool", "tool_call_id": "call_abc", "content": "20"}, {"role": "tool", "tool_call_id": "call_def", "content": "25"}Guidelines generated from the broken trajectories lacked tool-use context entirely. Fixed by adding an indexed-attribute reader for
llm.input_messages.{i}.*/llm.output_messages.{i}.*(including nestedtool_calls.{j}.*),_extract_tools_from_spanto parsellm.tools.{i}.tool.json_schema, and_convert_openinference_tool_callsto convert OpenInference tool_call dicts to OpenAI format.Test plan
uv run pytest tests/unit/test_phoenix_sync.py -v— all 46 tests passevolve sync phoenixagainst a Phoenix project with a tool-calling agent and verify the trajectory debug file shows completetool_calls/tool_call_idfields and correct tool definitionsskippedcount in sync output reflects unique traces, not individual spans🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
Bug Fixes
trace_idand skipping already-processed traces for more accurate counts.tool_callsassembly.Tests