fix: convert Langfuse AGENT observations regardless of parent - #314
fix: convert Langfuse AGENT observations regardless of parent#314poshinchen wants to merge 2 commits into
Conversation
ec72436 to
5586628
Compare
| return content, error | ||
| # Strands OTEL format: {"message": "<JSON string of [{\"text\": ...}]>", "id": "..."} | ||
| if "message" in obs_output: | ||
| return self._parse_tool_result_message(obs_output["message"]), None |
There was a problem hiding this comment.
Issue: The Strands OTEL message branch always returns error=None, unlike the result/status branch above which surfaces failures. If a Strands OTEL tool result represents a failure, the error is silently dropped, so failure-detection / root-cause evaluators would treat a failed tool call as successful.
Suggestion: Confirm whether the OTEL TOOL output ever carries error/status information (e.g. a status field or an error marker inside message). If it can, surface it as the error return value; if the format genuinely never encodes tool errors, add a brief comment noting this limitation so it's a documented decision rather than an accidental gap.
|
Issue: The PR title, description, and "Testing" section only describe the AGENT routing fix (#311). However roughly half the diff adds Strands OTEL TOOL parsing — Suggestion: Update the description to document the tool-parsing changes and link #312 (and reflect them in the Testing section), or split them into a separate PR to keep each change focused per the contributor checklist. |
|
Assessment: Comment (non-blocking) The core fix is correct and well-reasoned: gating Review themes
Nice, targeted root-cause fix with a clear explanation of the LangChain-vs-Strands nesting difference. |
Strands SDK traces nest the AGENT observation under a framework SPAN, so the null-parent gate on the combined CHAIN/AGENT branch dropped every Strands agent invocation. No AgentInvocationSpan was produced, and every evaluator calling _get_last_turn() failed with ValueError. Route AGENT to _convert_agent_invocation unconditionally; keep the null-parent check on CHAIN, where only the root chain is the agent invocation (LangChain emits a CHAIN per sub-chain). Fixes strands-agents#311
5586628 to
99fd32a
Compare
|
@strandly-the-agent can you review this PR? |
strandly-the-agent
left a comment
There was a problem hiding this comment.
Verdict: the narrow fix is correct — verified with a repro, existing suite (49/49), and a read of the Strands tracer source. One real 🟡 should-fix: the same nesting problem the docstring calls out for CHAIN also applies to AGENT, but only CHAIN gets a root-only guard. Everything below is checked against src/strands_evals/providers/langfuse_provider.py at pr314 (5c5bc3e) vs merge-base c5461d0.
1. Routing fix itself — correct
_convert_observation (langfuse_provider.py:229-232): AGENT now converts unconditionally, CHAIN keeps the null-parent gate. LangChain behavior is unaffected — test_child_chain_is_skipped and TestLangChainEndToEnd still pass, and I confirmed obs.type on the installed SDK (langfuse 3.15.0) is a required str field on Observation (not Optional), so the "obs.type is None" concern from the brief doesn't apply — ruled out, not a live risk.
2. 🟡 Should-fix: no root-only guard for AGENT ⇒ multi-agent traces get >1 AgentInvocationSpan, silently corrupting output/eval inputs
Confirmed via Strands SDK source (strands/agent/agent.py:1470 → tracer.start_agent_span, strands/telemetry/tracer.py:687-740): every Agent() invocation emits its own invoke_agent <name> span, with no distinction between a top-level call and a sub-agent invoked from inside a tool/Swarm/Graph node. So any Strands multi-agent trace (agent-as-tool, Swarm, Graph) produces multiple obs.type=="AGENT" observations in one Langfuse trace — this is the mainstream shape for Strands multi-agent apps, not a contrived edge case.
Repro (/tmp/repro/repro_multiagent.py, orchestrator + sub-agent-as-tool, both type=="AGENT"):
_extract_output → 'Flights: AA123, UA456' (should be the orchestrator's answer)
_get_last_turn().agent_response → 'Flights: AA123, UA456' (same — used by ~12 evaluators, evaluator.py:136)
turn[0].session_history and turn[1].session_history both contain the SAME full tool-call list
Root cause chain:
langfuse_provider.py:769-777(_extract_output) andtrace_extractor.py:43-85/evaluator.py:128-136(_get_last_turn) both pick the trace's lastAgentInvocationSpanin list order — the sub-agent's, not the orchestrator's.trace_extractor.py:47(_find_tool_execution_spans) collects allToolExecutionSpans in the trace once, then attaches that full list to every turn (trace_extractor.py:60-68) — duplicated tool history per turn._fetch_all_pages(langfuse_provider.py:130-139) never sorts observations, and Langfuse'sobservations.get_manyhas noorder_byparam at all — so "last in list" isn't even chronological, it's whatever order the API happens to return.
Not a regression from this PR — I checked out the merge-base and reran the same shape through the pre-existing SPAN+name="invoke_agent" fallback (langfuse_provider.py:236-241, unchanged by this diff, no parent check either): it already produced 2 AgentInvocationSpans and the same wrong _extract_output before this PR. What this PR changes is that the new mainstream ingestion path (Langfuse v4-style type=="AGENT") now also hits this latent gap — previously it hit a different bug (0 spans → loud ValueError), now it silently succeeds with plausible-but-wrong data for multi-agent traces, while correctly fixing the single-agent case (the reported issue #311).
Sibling mappers already carry the fix for this exact class of problem: mappers/langchain_otel_session_mapper.py:143-152 and mappers/openinference_session_mapper.py:256-261 explicitly keep only the root AgentInvocationSpan when a trace yields more than one, with a comment calling out multi-agent nesting. langfuse_provider.py has no equivalent for AGENT (or CHAIN, or the SPAN fallback, for that matter).
Minimal in-scope mitigation, if you want it in this PR (otherwise worth a tracking issue so it's not lost): after converting a trace's observations, when >1 AgentInvocationSpan came from AGENT/SPAN-fallback routing, keep only the one whose source observation has no ancestor (via obs.parent_observation_id, which Langfuse does give you, unlike the ADOT-based mappers that fall back to a start-time heuristic) that is itself one of the other AgentInvocationSpan observations. That's ordering-independent, which matters since observations aren't sorted here.
3. Other correctness notes
- A separate
SPAN-named-invoke_agentobservation coexisting with anAGENTobservation in the same trace doesn't add a new failure mode beyond #2 above — both routes feed the same unguarded_convert_agent_invocationcall, so it just adds another entry to the same "multiple AgentInvocationSpans" bucket. - Exception handling is unaffected by this diff —
_convert_observations(langfuse_provider.py:186-193) already catches and logs per-observation conversion failures.
4. Docstring — accurate but has a gap
The new Note (langfuse_provider.py:215-219) correctly explains why AGENT and CHAIN differ, and in the same breath says "CHAIN emits one per sub-chain, only the root is the agent invocation" — but Strands' own AGENT observations have the identical multiplicity property (per #2), and the docstring doesn't mention it. Given the fix is deliberately silent on that symmetry, worth a one-line caveat so it doesn't read as "AGENT is always 1-per-trace." Also ⚪ nit: the class-level bullet list a few lines up (langfuse_provider.py:199-203) still doesn't mention AGENT at all, even though the Routing table right below it does.
Nothing here should block landing the single-agent fix (#311) — it's correct and the tests for it pass. I'd just make sure the multi-agent gap doesn't get lost; happy to file a tracking issue if that's preferred over holding this PR.
|
@poshinchen do you want to pull in the multi agent fix here too? |
|
Follow-up to my review above. @opieter-aws — re your question to Still recommend landing the #311 fix — it turns a loud ✅ Evidence ledger — what I ran myselfReviewed
Not verified: no live model calls or real Langfuse instance — the Strands/LangChain span shapes are source-verified, then replayed as fixtures through the real provider code. 1. 🔴 The wrong answer, reproduced end to end (raised from 🟡)Orchestrator + sub-agent, both Why this is the mainstream shape and not a contrived one: 2. Genuinely new: this isn't Strands-only, and the docstring rationale doesn't holdThe new docstring says "LangChain emits a CHAIN per sub-chain and only the root chain is the agent invocation". That isn't what the installed Langfuse integration does: Replaying a LangGraph supervisor + 2 sub-agents in that shape: 3. Why a list-order fix (like the sibling mappers') isn't safe on this path
The scored output flips. Worth noting 4. Mitigation prototype (~20 lines, not applied to this branch)Mirrors the sibling mappers but keys on parent linkage over the raw observations — that detail matters: Strands' intermediate def _drop_nested_agent_invocations(self, spans, observations):
agent_ids = {s.span_info.span_id for s in spans if isinstance(s, AgentInvocationSpan)}
if len(agent_ids) < 2:
return spans
parent_of = {obs.id: obs.parent_observation_id for obs in observations}
def nested_under_agent(obs_id):
seen, cur = {obs_id}, parent_of.get(obs_id)
while cur is not None and cur not in seen:
if cur in agent_ids:
return True
seen.add(cur); cur = parent_of.get(cur)
return False
return [s for s in spans
if not isinstance(s, AgentInvocationSpan) or not nested_under_agent(s.span_info.span_id)]Called as The last row is the case to be careful about: siblings under a non-agent If you'd rather keep this PR narrow, the cheap alternative is landing the three multi-agent cases as 5. Suggested test for the gap (passes against this branch today)Follows the file's def test_two_agent_observations_both_convert(self, provider, mock_client):
"""Nested AGENT (orchestrator -> sub-agent) both become AgentInvocationSpan.
Pins the multi-agent fanout: TraceExtractor emits one turn per span and
_get_last_turn takes the last, so evaluators score the sub-agent's answer.
If AGENT is later restricted to the outermost observation, this moves with it.
"""
spans = self._get_spans(
provider, mock_client,
[
_obs("o-orch", "t1", "AGENT", name="invoke_agent Orchestrator",
obs_input="Book a flight", obs_output="Booked!", parent_observation_id=None),
_obs("o-sub", "t1", "AGENT", name="invoke_agent FlightFinder",
obs_input="find flights", obs_output="found 3 options", parent_observation_id="o-orch"),
],
)
agents = [s for s in spans if isinstance(s, AgentInvocationSpan)]
assert len(agents) == 2
assert {a.agent_response for a in agents} == {"Booked!", "found 3 options"}Housekeeping
Open questions for `poshinchen` (non-blocking)
All of the above is one agent's pass with the repros attached — worth a human sanity-check before you act on it, particularly the call on whether the multi-agent fix belongs in this PR or the next one. |
Description
LangfuseProvidernever produced anAgentInvocationSpanfor Strands agent traces instrumented via OTEL → Langfuse.The routing in
_convert_observationgated bothCHAINandAGENTtypes onobs.parent_observation_id is None:The null-parent check is correct for LangChain
CHAINobservations (LangChain emits aCHAINper sub-chain, and only the root chain is the agent invocation). But Strands SDK traces always nest theAGENTobservation under a frameworkSPAN:Because the AGENT observation has a parent, it was silently skipped. The resulting session contained only
InferenceSpanandToolExecutionSpanobjects, so every evaluator that calls_get_last_turn()(which requires at least oneAgentInvocationSpan) failed withValueError. The reporter observed 0/70 production Strands traces producing anAgentInvocationSpan.Changes
AGENTobservations to_convert_agent_invocationregardless of parent — Strands nests them under a SPAN, so requiring a null parent drops every agent invocation.CHAIN, where only the root chain is the agent invocation.The
SPANfallback (namestarts withinvoke_agent),GENERATION, andTOOLpaths are unchanged.Related Issues
Fixes #311
Documentation PR
N/A
Type of Change
Bug fix
Testing
Added
TestLangfuseAgentTypecovering:test_agent_with_parent_produces_agent_invocation— the exact scenario from [BUG] LangfuseProvider skips AGENT observations with parent (Strands agent traces never produce AgentInvocationSpan) #311 (AGENT nested under a SPAN parent) now yields anAgentInvocationSpan.test_root_agent_produces_agent_invocation— AGENT at the root still converts.Existing
test_child_chain_is_skippedstill passes, confirming LangChain sub-chain filtering is unaffected. Fulltests/strands_evals/providers/test_langfuse_provider.pysuite passes; ruff clean.hatch run prepareChecklist
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.