Skip to content

Fix Datadog LLMObs tool spans (LLMObs.start_span does not exist in ddtrace 4.13) - #168

Merged
ForisKuang merged 2 commits into
cBioPortal:betafrom
ForisKuang:polly/fix-llmobs-spans
Sep 30, 2026
Merged

ForisKuang merged 2 commits into
cBioPortal:betafrom
ForisKuang:polly/fix-llmobs-spans

Conversation

@ForisKuang

@ForisKuang ForisKuang commented Sep 30, 2026 •

Copy link
Copy Markdown
Collaborator

Root cause

TelemetryMiddleware starts a Datadog LLM Observability tool span for each MCP tool call through _llmobs_tool_span in src/cbioportal_mcp/telemetry.py. That helper called LLMObs.start_span(span_kind="tool", name=...), but the ddtrace we pin (4.13.0 in uv.lock) has no such method. The AttributeError was caught by a bare except Exception: return None, so no LLMObs tool span has ever been created, and the LLM Observability widgets built on mcp.tool.* spans are empty. DogStatsD metrics and OTel spans are separate code paths and still work.

The existing tests never caught it because they all patch _llmobs_tool_span.

Fix

  • Starting the span. The span is started with the public LLMObs.tool(name=f"mcp.tool.{tool}", session_id=...) API and annotated with LLMObs.annotate(span=..., input_data=..., metadata=..., tags=...). The span name, input, and metadata (user_email, mcp_client_*, mcp_session_id) are unchanged.
  • LLMObs tag names are underscored in every mode: usr_id, mcp_client_kind, mcp_client_name, mcp_session_id.
    • In the production export mode (server.py: agentless_enabled=True, APM tracing on, which ddtrace calls APM_AGENTLESS), ddtrace 4.13 rewrites dots in LLMObs tag keys to underscores. In agent-proxy mode it keeps the dots. Sending the keys already underscored means Datadog stores the same names in every mode.
    • Previously the dotted names were only set with span.set_tag, which LLMObs can't filter on.
    • The APM span keeps the dotted tags (usr.id, mcp.client_kind, mcp.client.name, mcp.session.id) exactly as before.
  • Finishing the span. The span is finished in a finally around the whole tool call. On an exception, including cancellation, the error is recorded with span.set_exc_info, so the span has status: "error".
  • No-op when LLMObs is off. If LLMObs.enabled is false (no DD_API_KEY), the SDK isn't touched and no ddtrace span is created.
  • Failures are logged instead of swallowed. If LLMObs is enabled but starting, annotating or finishing the span fails, one WARNING is logged per process, then DEBUG after that. The tool call itself is never affected.
  • Docs. The LLMObs span name and tag names are documented in observability/datadog/README.md and in the TelemetryMiddleware docstring. No in-repo dashboard, monitor or doc queried LLMObs tags. The in-repo dashboards use OTel/APM span facets, which this PR doesn't change.

What error spans now contain

Error spans record error.type, error.message and error.stack.

  • The message is FastMCP's ToolError text, Error calling tool '<name>': <original exception message>. FastMCP wraps the exception before middleware sees it, and mask_error_details is not set.
  • The stack is a formatted traceback: file names, line numbers, function names and source lines of the code, including the chained original exception. It contains no local variables and no request data.
  • What the stack adds beyond the message. The traceback also carries source-code lines from the repo and any chained exceptions (including their messages). It has no local variables or request data, but a chained exception's message could itself quote input values.
  • SQL. The ClickHouse tools (clickhouse_run_select_query, clickhouse_list_tables, clickhouse_list_table_columns) catch DB errors and return {"error_message": str(e)} as a normal result. ClickHouse error text can quote SQL fragments, so it can end up in the span's output, as a successful call. This is the same output annotation as before this PR. It does not reach the error path.
  • Tool arguments are the span input, as originally designed. That includes the full SQL text for clickhouse_run_select_query.
  • Raised errors. Exceptions that do propagate come from input validation (e.g. ValueError("study_id cannot be empty"), or invalid table-name messages that can echo the value supplied), or from unexpected errors.

Test evidence

tests/test_telemetry_llmobs.py uses the real ddtrace LLMObs SDK. It has 10 test cases (9 functions, one parametrized over two values). No test makes a Datadog network call.

Capture modes:

  • agent-proxy with APM tracing off. LLMObs events are captured at the LLMObs span writer's enqueue.
  • Prod export mode (agentless + APM tracing on). The LLMObs payload rides the APM trace, so it's captured at the APM trace writer's write.
    • A fake API key is used.
    • http.client connects are refused and recorded, and the fixture asserts no connection was attempted.
    • ddtrace's instrumentation telemetry is kept off the agentless intake.
Test Checks
test_tool_call_emits_one_llmobs_tool_span one tool span mcp.tool.ping, status ok, session_id, underscored LLMObs tags, input/output/metadata
test_prod_mode_llmobs_tags_are_underscored_and_apm_tags_dotted prod export mode: LLMObs payload tags usr_id/mcp_client_kind/mcp_client_name/mcp_session_id (no dotted keys); APM span has usr.id/mcp.client_kind/mcp.client.name/mcp.session.id
test_erroring_tool_emits_llmobs_span_with_error status error, error:1, error type/message recorded
test_each_tool_call_gets_its_own_span one root span per sequential call
test_overlapping_calls_get_distinct_root_spans two concurrent in-flight calls → distinct span and trace IDs, both roots, correct outputs
test_worker_thread_child_span_is_parented_to_tool_span[asyncio|anyio] an LLMObs span opened in asyncio.to_thread / anyio.to_thread.run_sync is a child of the tool span
test_cancelled_call_finishes_span_with_error a cancelled call still exports a finished span with status error (CancelledError)
test_noop_when_llmobs_not_enabled LLMObs.tool is never called and nothing is logged when LLMObs is disabled
test_span_creation_failure_warns_once_and_never_breaks_tool one WARNING across two failing calls; tool results unaffected

Fail-before / pass-after, same test file

Source Result
bd2d698 (original bug) 0 LLMObs events. Every span test fails.
b29f27d (first commit of this PR) 1 failed, 9 passed. test_tool_call_emits_one_llmobs_tool_span fails with KeyError: 'usr_id' because agent-proxy mode exported dotted keys.
9b565d1 (head) 10 passed

On b29f27d the prod-mode test passes. In that mode ddtrace already rewrote the keys to usr_id etc., which is what the cross-review captured. So the real defect was that tag names differed by export mode, and the first revision's tests and PR text claimed dotted keys. The concurrency, worker-thread and cancellation tests also pass on b29f27d. They are regression coverage and did not reveal a bug.

Gates

  • Full suite: uv run pytest collects 315 test cases: 246 passed, 69 skipped, 0 failed. The skips are pre-existing.
  • Lint: ruff check src/cbioportal_mcp/telemetry.py tests/test_telemetry_llmobs.py passes with no errors. The repo-wide ruff check src tests reports 110 errors, the same 110 as the bd2d698 baseline.

Notes

  • Production (main) has the same bug. Once this is approved, an identical PR against main can follow.
  • After deploy, the LLM Observability tool-span widgets should start filling in. Use the underscored tag names in LLMObs queries.
  • server.py still passes the deprecated ml_app= (ddtrace 4.x prefers agent_service). It still works. I left it alone because it's out of scope.

🤖 Generated with Claude Code

Foris Kuang and others added 2 commits September 30, 2026 08:04
…trace 4.13)

_llmobs_tool_span called LLMObs.start_span(span_kind="tool", ...), which is not
part of the ddtrace 4.13.0 LLMObs API. The AttributeError was swallowed, so no
LLMObs tool span was ever created and the LLM Observability widgets stayed empty.

- Start the span with the public LLMObs.tool(name=..., session_id=...) API and
  annotate input, metadata and the user/client/session tags with LLMObs.annotate
  (tags are also set on the APM span as before).
- Finish the span in a finally around the tool call, recording the exception
  (set_exc_info) on error or cancellation so the span has status "error".
- No-op when LLMObs is not enabled (no DD_API_KEY) without touching the SDK.
- If LLMObs is enabled but the SDK fails, log one WARNING per process (then
  DEBUG) instead of silently dropping spans; the tool call is never affected.
- Add tests that run the real ddtrace LLMObs SDK and capture the exported span
  events in-process (no Datadog network).

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

Co-authored-by: omnigent <noreply@omnigent.ai>
… mode

In the production export mode (agentless + APM tracing on) ddtrace 4.13
rewrites dots in LLMObs tag keys to underscores, while agent-proxy mode keeps
them dotted, so the same tag had different names depending on configuration.
Send the LLMObs tags as usr_id, mcp_client_kind, mcp_client_name and
mcp_session_id so the stored names are identical in every mode; the APM span
keeps the dotted tags unchanged.

Tests: capture the LLMObs payload in the prod export mode at the APM trace
writer (fake key, outbound HTTP refused), and cover overlapping concurrent
calls, worker-thread child spans (asyncio and anyio), and cancellation.
Document the LLMObs tag names in observability/datadog/README.md.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

Co-authored-by: omnigent <noreply@omnigent.ai>
@ForisKuang
ForisKuang merged commit 6ddc23c into cBioPortal:beta Sep 30, 2026
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant