Skip to content

Fix: record the tool exception on the execute_tool span - #710

Open
AmaadMartin wants to merge 2 commits into
mainfrom
fix/execute-tool-span-record-exception
Open

Fix: record the tool exception on the execute_tool span#710
AmaadMartin wants to merge 2 commits into
mainfrom
fix/execute-tool-span-record-exception

Conversation

@AmaadMartin

Copy link
Copy Markdown
Owner

Please ensure you have read the contribution guide before creating a pull request.

Link to Issue or Description of Change

  1. Link to an existing issue (if applicable):

  2. Or, if no issue exists, describe the change:

Problem: callToolAsync has no catch, so a tool that throws leaves its execute_tool span with status UNSET, no exception event and none of the gen_ai.tool.* attributes. Every OTel backend renders that span as a success, which makes tool failures invisible in Cloud Trace, in an OTLP collector and in the dev UI. adk-python does not have this gap: trace_tool_call takes an error argument and records it.

Solution: traceToolCall now accepts the thrown value. When it is present the function records the exception, sets error.type and sets an ERROR status. callToolAsync calls it from a catch block and rethrows the value unchanged, so telemetry stays an observation and never becomes control flow. A new resolveErrorType in core/src/utils/error_utils.ts derives the label with the reference's precedence: a self-classified errorType, then an HTTP status, then the class name.

Design notes

  • The status check is duck-typed, not instanceof ApiError. The repo bans instanceof for SDK class detection because it fails when two copies of a package share one runtime. A duck-typed check needs a plausibility bound, so the status must fall in the existing MIN_HTTP_STATUS/MAX_HTTP_STATUS range. Without it an unrelated status: 0 field would be reported as an HTTP code.
  • resolveErrorType uses error.constructor.name, mirroring Python's type(error).__name__, so a subclass that never assigns this.name still reports its own class. The published core build is not minified (core/build.js sets minify: bundle, true only for the optional build:bundle target), so this is stable for the default artifact. A consumer who minifies a bundle gets mangled class names.
  • The span status message is the failure type, not the error message. Tool content must not reach a field that shouldAddRequestResponseToSpans cannot gate.
  • Parity precedence. In the reference an exception always outranks a response-derived error type. This change deliberately adds no response-derived errorType parameter, because nothing in this change would read it. Any future response-derived classification must be evaluated only when error is absent.
  • Scope. The invocation, invoke_agent and call_llm spans have the same gap. They are out of scope here and queued separately.

Collision check (gh pr list --repo AmaadMartin/adk-js --state open --limit 1000)

No open PR records the thrown exception on the execute_tool span. Three overlap and none of them lands this change:

Testing Plan

Unit Tests:

  • I have added or updated unit tests for my change.
  • All unit tests pass locally.
npx vitest run --project unit:core core/test/telemetry/tracing_test.ts \
  core/test/utils/error_utils_test.ts \
  core/test/telemetry/tool_exception_span_test.ts \
  core/test/agents/functions_test.ts
# 4 files, 82 tests passed
npm run lint          # clean
npm run format:check  # clean
npm run build         # clean
npm run docs:check    # clean

core/test/telemetry/tool_exception_span_test.ts drives handleFunctionCallList through a real NodeTracerProvider, SimpleSpanProcessor and InMemorySpanExporter, and asserts the exported span. That is the integration-level signal, so I added no tests/integration/** file: a separate one would duplicate it at a much higher runtime cost. This was a decision, not an omission.

Coverage: core/src/utils/error_utils.ts is at 100% line and branch. Every line and branch this change adds to tracing.ts and functions.ts is covered. @vitest/coverage-v8 reports one uncovered synthetic branch on the finally in callToolAsync, which is a pre-existing block that ends the span on every path. I kept it.

Proof the tests can fail. I ran each mutation against the new tests and recorded the failure:

Mutation Test that failed Message
Delete the catch in callToolAsync 3 span tests expected +0 to be 2
throw e -> return undefined exception reaches the caller expected { result: undefined } to deeply equal { error: 'sku not found' }
Drop span.setStatus, keep the attribute 2 span tests, 1 unit test expected "spy" to be called with arguments: [ { code: 2, message: 'TypeError' } ]
Swap the errorType and status branches precedence test expected '429' to be 'QUOTA_EXCEEDED'
Remove the error !== undefined guard 2 success-path guards expected 2 not to be 2
Drop the HTTP range bound out-of-range status test expected '0' to be 'MyToolError'
constructor.name -> name 4 tests expected 'Error' to be 'MyToolError'

Manual End-to-End (E2E) Tests:

  1. Register a tool whose implementation throws.
  2. Start the dev server with tracing enabled and run a turn that calls the tool.
  3. Confirm the execute_tool <name> span is marked failed, shows an exception event with the thrown message, and carries error.type.
  4. Confirm the turn still completes with the tool's error as the function response, which proves the change altered only what is recorded.

Checklist

  • I have read the CONTRIBUTING.md document.
  • I have performed a self-review of my own code.
  • I have commented my code, particularly in hard-to-understand areas.
  • I have added tests that prove my fix is effective or that my feature works.
  • New and existing unit tests pass locally with my changes.

Amaad Martin added 2 commits August 6, 2026 02:51
A tool that throws leaves its execute_tool span with status UNSET, no
exception event and no gen_ai.tool.* attributes, so every OTel backend
renders the failed call as a success.

traceToolCall now accepts the thrown value and, when present, records the
exception, sets error.type and sets an ERROR status. callToolAsync calls it
from a catch block and rethrows the value unchanged. This matches
adk-python's trace_tool_call error handling.
resolveErrorType gets unit cases for each precedence rule. traceToolCall
gets cases for the error path and a regression guard on the success path.
A new file drives handleFunctionCallList through a NodeTracerProvider and
asserts the exported span, which is the integration-level proof that the
wiring works end to end.
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