diff --git a/docsgpt/agents/tool_executor.py b/docsgpt/agents/tool_executor.py index 155ea925..2c7b446d 100644 --- a/docsgpt/agents/tool_executor.py +++ b/docsgpt/agents/tool_executor.py @@ -81,11 +81,15 @@ def finish_tool_span(span: Any, data: Dict[str, Any]) -> None: if data.get("result") is not None: span.preview("result", data.get("result")) span_status = _TOOL_SPAN_STATUS.get(status, tracing.STATUS_OK) + # ``error`` and ``result`` are tool output or user text (a denial + # comment), so they travel only as previews, which honour the content + # capture settings; ``span.error`` is exported and stored unconditionally. + if data.get("error"): + span.preview("error", data["error"]) if span_status == tracing.STATUS_ERROR: - span.error = str(data.get("error") or data.get("result") or "tool error")[:500] - span.set(**{"error.type": str(data.get("error_type") or "ToolError")}) - elif data.get("error"): - span.error = str(data["error"])[:500] + error_type = str(data.get("error_type") or "ToolError") + span.error = f"Tool call failed ({error_type})" + span.set(**{"error.type": error_type}) span.end(span_status) @@ -1021,7 +1025,11 @@ class ToolExecutor: try: outcome = yield from self._execute(tools_dict, call, llm_class_name) except Exception as exc: - span.end(error=exc) + # A tool's exception text can quote its response; like tool + # output it goes only into the capture-gated preview. + span.preview("error", str(exc)) + span.error = f"Tool call failed ({type(exc).__name__})" + span.end(tracing.STATUS_ERROR, attributes={"error.type": type(exc).__name__}) raise except GeneratorExit: span.end(tracing.STATUS_CANCELLED) diff --git a/docsgpt/api/answer/routes/base.py b/docsgpt/api/answer/routes/base.py index 469271b7..f23b82f3 100644 --- a/docsgpt/api/answer/routes/base.py +++ b/docsgpt/api/answer/routes/base.py @@ -860,6 +860,13 @@ class BaseAnswerResource: # error silently — the exact shape of the bug being fixed here. if stream_error: query_metadata.setdefault("error", stream_error) + # A yielded error (e.g. a failed workflow node) ends the + # generator normally, so no span raised; the user still saw + # the turn fail, and its trace should say so. A pause below + # overrides this, as it does for the message row. + trace = tracing.current_trace() + if trace is not None: + trace.outcome = tracing.STATUS_ERROR # ---- Paused: save continuation state and end stream early ---- if paused: diff --git a/docsgpt/api/user/analytics/routes.py b/docsgpt/api/user/analytics/routes.py index cb4486e5..570c141e 100644 --- a/docsgpt/api/user/analytics/routes.py +++ b/docsgpt/api/user/analytics/routes.py @@ -99,9 +99,10 @@ def _trace_branch(name: str, sources_sql: str, scope: str) -> dict: return { "name": name, "level": "CASE WHEN t.status = 'error' THEN 'error' ELSE 'info' END", - # A search's query sits in its retrieval span's preview; graph builds - # are named after their source. - "summary": "COALESCE(t.spans->0->'preview'->>'query', t.name, t.source)", + # A search is listed by its query (copied into the small ``summary`` + # at flush so this never detoasts ``spans``); graph builds are named + # after their source. + "summary": "COALESCE(t.summary->>'query', t.name, t.source)", "where": [f"t.source IN {sources_sql}", scope], "sql": f""" SELECT '{name}' AS event_type, diff --git a/docsgpt/tracing/core.py b/docsgpt/tracing/core.py index 47a2c49a..aa0f3b07 100644 --- a/docsgpt/tracing/core.py +++ b/docsgpt/tracing/core.py @@ -431,6 +431,14 @@ class Trace: def to_record(self) -> Dict[str, Any]: """The ``request_traces`` row for this (finished) trace.""" + summary = self.summary() + # Traces without a log row of their own (searches) are listed by + # their query; keep it in the small summary so listing never reads + # ``spans``. Dropped with the other previews when content is blocked. + if not self.content_blocked and self.spans: + query = self.spans[0].previews.get("query") + if isinstance(query, str): + summary["query"] = query spans = [] for s in self.spans: entry: Dict[str, Any] = { @@ -464,7 +472,7 @@ class Trace: "duration_ms": int(round(self.duration_ms or 0)), "span_count": len(spans), "dropped_spans": self.dropped_spans, - "summary": self.summary(), + "summary": summary, "spans": spans, "otel_trace_id": self.otel_trace_id, } diff --git a/tests/api/answer/test_trace_wiring.py b/tests/api/answer/test_trace_wiring.py index 49fcd6e0..9153c424 100644 --- a/tests/api/answer/test_trace_wiring.py +++ b/tests/api/answer/test_trace_wiring.py @@ -132,6 +132,17 @@ class TestTraceLifecycle: assert any('"type": "error"' in s for s in stream) assert flushed[0].status == "error" + def test_yielded_error_marks_trace_error(self, flask_app, mock_mongo_db): + """A failed workflow node yields an error event instead of raising.""" + from docsgpt.api.answer.routes.base import BaseAnswerResource + + with flask_app.app_context(), _captured_flushes() as flushed: + _run( + BaseAnswerResource(), + _agent([{"type": "error", "error": "node failed"}]), + ) + assert flushed[0].status == "error" + def test_abandoned_stream_still_flushes(self, flask_app, mock_mongo_db): from docsgpt.api.answer.routes.base import BaseAnswerResource diff --git a/tests/api/user/test_traces_routes.py b/tests/api/user/test_traces_routes.py index cb918493..a0b2bbbe 100644 --- a/tests/api/user/test_traces_routes.py +++ b/tests/api/user/test_traces_routes.py @@ -167,7 +167,14 @@ class TestLogsTraceSummaries: assert row["trace"]["ref"] == {"field": "activity_id", "value": "act-1"} def test_search_traces_are_listed(self, app, pg_conn): - record = _trace(pg_conn, request_id=None, source="mcp", name="mcp", status="error") + record = _trace( + pg_conn, + request_id=None, + source="mcp", + name="mcp", + status="error", + summary={"llm_calls": 0, "query": "how do I deploy"}, + ) _trace(pg_conn, request_id=None, source="graph_extraction", name="graph_extraction Docs") search_rows = _logs(app, pg_conn, "owner", {"event_type": "search"}).json["logs"] assert len(search_rows) == 1 diff --git a/tests/retriever/test_graph_rag.py b/tests/retriever/test_graph_rag.py index 5203eee1..802bfc99 100644 --- a/tests/retriever/test_graph_rag.py +++ b/tests/retriever/test_graph_rag.py @@ -1021,3 +1021,41 @@ class TestPersonalizedPageRankWithoutScipy: docs = rag._get_data() assert [doc["text"] for doc in docs] == ["near", "far"] + + +class TestTraceSpans: + """GraphRAG searches and query embeddings are recorded in the execution trace.""" + + @pytest.fixture(autouse=True) + def _trace(self, monkeypatch): + from docsgpt import tracing + from docsgpt.core.settings import settings + + monkeypatch.setattr(settings, "TRACES_ENABLED", True) + monkeypatch.setattr(settings, "TRACES_CAPTURE_CONTENT", True) + self.trace = tracing.start_trace(source="stream", capture_otel_context=False) + with tracing.activate(self.trace): + yield + + def test_search_is_a_retrieval_span(self, _patch_llm_creator): + rag = _make_retriever() + docs = [{"text": "alpha", "title": "Doc A", "source": "a.md"}] + with patch.object(rag, "_get_data", return_value=docs): + assert rag.search("new question") == docs + (span,) = [s for s in self.trace.spans if s.kind == "retrieval"] + assert span.name == "retrieval GraphRAGRetriever" + assert span.attributes["docsgpt.retriever"] == "GraphRAGRetriever" + assert span.attributes["gen_ai.data_source.id"] == "src1" + assert span.attributes["docsgpt.chunk_count"] == 1 + assert span.previews["chunks"][0]["title"] == "Doc A" + + def test_embed_query_is_an_embedding_span(self): + embedder = Mock() + embedder.embed_query.return_value = [0.5] + with patch( + "docsgpt.retriever.graph_rag.get_embeddings", return_value=embedder + ): + assert GraphRAGRetriever._embed_query(object(), "q") == [0.5] + (span,) = self.trace.spans + assert span.kind == "embedding" + assert span.attributes["gen_ai.operation.name"] == "embeddings" diff --git a/tests/tracing/test_tool_spans.py b/tests/tracing/test_tool_spans.py index 6f2447db..899b6a8b 100644 --- a/tests/tracing/test_tool_spans.py +++ b/tests/tracing/test_tool_spans.py @@ -82,7 +82,9 @@ class TestExecute: _drain(executor.execute({}, ToolCall(id="c", name="x", arguments="{}"), "L")) span = trace.spans[0] assert span.status == "error" - assert span.error == "no such tool" + # Tool output stays in the capture-gated preview, never in ``error``. + assert span.error == "Tool call failed (ToolError)" + assert span.previews["result"] == "no such tool" def test_raised_error(self, trace): def inner(executor, call): @@ -94,6 +96,8 @@ class TestExecute: _drain(executor.execute({}, ToolCall(id="c", name="x", arguments="{}"), "L")) assert trace.spans[0].status == "error" assert trace.spans[0].attributes["error.type"] == "RuntimeError" + assert trace.spans[0].error == "Tool call failed (RuntimeError)" + assert trace.spans[0].previews["error"] == "api down" def test_nested_retrieval_parents_to_tool(self, trace): def inner(executor, call): @@ -109,6 +113,22 @@ class TestExecute: assert retrieval.parent_id == tool.id +def test_tool_content_is_not_kept_when_capture_is_off(trace, monkeypatch): + monkeypatch.setattr(settings, "TRACES_CAPTURE_CONTENT", False) + + def inner(executor, call): + executor.tool_calls.append( + {"tool_name": "t", "result": "secret tool output", "status": "error"} + ) + return "", call.id + yield # pragma: no cover + + _drain(_executor_with(inner).execute({}, ToolCall(id="c", name="x", arguments="{}"), "L")) + trace.finish() + record = trace.to_record() + assert "secret tool output" not in str(record) + + class _Handler(LLMHandler): def parse_response(self, response): # pragma: no cover - unused raise NotImplementedError @@ -169,7 +189,8 @@ class TestPausedCalls: _drain(_Handler().handle_tool_calls(agent, [call], {"0": {"name": "telegram"}}, [])) (span,) = trace.spans assert span.status == "denied" - assert span.error == "not allowed" + assert span.error is None + assert span.previews["error"] == "not allowed" class TestContinuation: @@ -227,7 +248,8 @@ class TestContinuation: assert agent_span.status == "ok" assert tool_span.parent_id == agent_span.id assert tool_span.status == "denied" - assert tool_span.error == "too risky" + assert tool_span.error is None + assert tool_span.previews["error"] == "too risky" def test_client_result_is_recorded(self, trace): agent = self._agent() diff --git a/tests/tracing/test_trace.py b/tests/tracing/test_trace.py index 03a4caca..437f1b53 100644 --- a/tests/tracing/test_trace.py +++ b/tests/tracing/test_trace.py @@ -353,3 +353,22 @@ class TestOutcome: trace.outcome = "paused" trace.finish(status="error") assert trace.status == "error" + + +class TestRecordQuery: + def test_first_span_query_is_copied_into_summary(self): + trace = tracing.start_trace(source="search") + with tracing.activate(trace): + with tracing.span(tracing.KIND_RETRIEVAL, "retrieval") as s: + s.preview("query", "how do I deploy") + trace.finish() + assert trace.to_record()["summary"]["query"] == "how do I deploy" + + def test_query_is_dropped_when_content_is_blocked(self): + trace = tracing.start_trace(source="search") + with tracing.activate(trace): + with tracing.span(tracing.KIND_RETRIEVAL, "retrieval") as s: + s.preview("query", "leaky") + tracing.mark_content_blocked() + trace.finish() + assert "query" not in trace.to_record()["summary"]