mirror of
https://github.com/tiennm99/DocsGPT.git
synced 2026-10-12 18:15:33 +00:00
Keep tool output out of span errors and fix trace status and listing
Tool results, denial comments and tool exception text now reach a span only as a capture-gated preview; span.error is a fixed message, since it is stored and exported regardless of content settings. A turn whose stream yields an error event is recorded as failed. Search traces are listed by a query copied into the small summary column, so the Logs timeline never reads the spans JSONB. Adds GraphRAG span tests.
This commit is contained in:
1 parent
285fdc914c
commit
efec1a2f83
9 files changed
+134
-13
No files matched your search
@@ -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)
|
||||
|
||||
@@ -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:
|
||||
|
||||
@@ -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,
|
||||
|
||||
@@ -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,
|
||||
}
|
||||
|
||||
@@ -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
|
||||
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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"
|
||||
@@ -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()
|
||||
|
||||
@@ -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"]
|
||||
Reference in new issue
Block a user