[Bug]: MCP tracing middleware leaves span status `Unset` on failed `tools/call`, so tool failures are invisible to error queries

[aarushitandon0]

What happened?

createTracingMiddleware and createMetricsMiddleware live about 40 lines apart in the same file and classify the same failed tool call differently.

Signal	Failed tools/call	Where
jaeger.mcp.tool.calls	status="error" (correct)	middleware.go#L118-L123
span Status.Code	Unset (wrong)	middleware.go#L79-L84

The tool-error branch sets error.type and records the exception, but never calls span.SetStatus:

// middleware.go:73-84
result, err := next(ctx, method, req)
if err != nil {
    span.RecordError(err)
    span.SetStatus(codes.Error, err.Error())   // transport error -> Error
    return result, err
}
if callResult, ok := result.(*mcp.CallToolResult); ok && callResult.IsError {
    span.SetAttributes(otelsemconv.ErrorType(errorTypeTool))
    if toolErr := callResult.GetError(); toolErr != nil {
        span.RecordError(toolErr)
    }
    // no span.SetStatus here, status stays Unset
}

This is self-contradictory on a single span. error.type is stable semconv defined as "describes a class of error the operation ended with," with "If the operation has completed successfully, instrumentations SHOULD NOT set error.type." The middleware sets it, asserting the operation did not succeed, while leaving the status field that every consumer reads at Unset.

This is not a rare path. The MCP Go SDK converts every plain handler error into CallToolResult{IsError:true} with a nil Go error, so the err != nil branch is never reached for handler errors, only for *jsonrpc.Error and transport failures, neither of which mcptools handlers produce:

// go-sdk@v1.6.1 mcp/server.go:344-353
res, out, err := h(ctx, req, in)
if err != nil {
    if wireErr, ok := err.(*jsonrpc.Error); ok {
        return nil, wireErr          // -> middleware err != nil branch
    }
    var errRes CallToolResult
    errRes.SetError(err)
    return &errRes, nil              // every mcptools handler error lands here
}

That routes 44 error returns across all 9 tools through the Unset path: trace not found, invalid trace_id, span_ids exceeds maximum limit, no root span found in trace, and so on. SDK-side argument-schema validation failures (server.go#L322-L327) take the same route and use SetError too, so they are the same shape as above.

GetError() is non-nil on every case reached through this path, since the SDK always uses SetError when converting a handler error. So RecordError fires and the error text survives on the span as an exception event, only the status field is wrong. There is a second, theoretical shape, IsError set directly without SetError, which would leave a span with no status, no description, and no event, but the one place in the codebase that constructs a result that way (uiToolErrorResult in the gateway) sits in middleware that is wired to run before this tracing middleware and short-circuits before it, so it never produces a span at all today. Noted for completeness since the fix below closes that gap defensively at no cost, but it is not a live path.

Impact: Jaeger's own error tooling keys off StatusCodeError specifically, and treats Unset as not-an-error.

• get_trace_errors.go#L73: if span.Status().Code() == ptrace.StatusCodeError
• memory/tenant.go#L274: error=true matches only StatusCodeError, which is what search_traces(with_errors: true) resolves to

So when the MCP server is traced into Jaeger, the natural dogfooding setup, and the one an evaluation harness for #9135 would use, a failed tools/call cannot be found by search_traces(with_errors: true) or get_trace_errors. Jaeger's own error-finding tools cannot find Jaeger's own tool failures. Anyone reconciling the jaeger.mcp.tool.calls{status="error"} rate against traces gets a nonzero metric and zero matching spans.

What this does not affect, scoping this down deliberately:

• The agent's view is correct. IsError and the message reach the LLM intact. This is purely how the server records what happened.
• Metrics are already right. No change needed there. They are the reference for what the span should say.
• Not a data-loss bug. The error text is still on the span as an exception event, so a human opening the span sees it. What breaks is status-based filtering and error-rate aggregation.
• Not the AG-UI/ACP tool-status path. How isError is translated onward to the browser is a separate layer ([Bug]: [ai-sidecar] Tool results are double-wrapped in raw_output, so the AG-UI gateway never flattens them #9289 and the sidecar ACP status handling). This issue is confined to the Go MCP server's own OTel span.
• Not the "empty vs error" question. Handlers that succeed while returning nothing, or that report a soft failure in output.Error with IsError:false, e.g. get_span_details.go#L111-L117, are untouched here and are a separate design question. This issue is only about calls already classified as errors.

Steps to reproduce

Drives the real server, real handlers, both middlewares, over an in-memory transport, and calls get_trace_topology for a trace the store does not have. The handler returns errors.New("trace not found"), which the SDK converts into CallToolResult{IsError: true} with a nil Go error.

The test asserts both halves of the contradiction at once, so it passes on current main and documents the inconsistency rather than merely failing. The final assertion can be flipped to codes.Error to turn this into the regression test a fix needs.

Lives in package mcptools as it reuses newTraceCapture, newMetricsCapture, findMetricDataPoint, and assertHasStringAttribute from the existing middleware_test.go.

// Copyright (c) 2026 The Jaeger Authors.
// SPDX-License-Identifier: Apache-2.0

package mcptools

import (
	"context"
	"iter"
	"testing"
	"time"

	"github.com/modelcontextprotocol/go-sdk/mcp"
	"github.com/stretchr/testify/assert"
	"github.com/stretchr/testify/mock"
	"github.com/stretchr/testify/require"
	"go.opentelemetry.io/collector/pdata/ptrace"
	"go.opentelemetry.io/otel/codes"
	"go.opentelemetry.io/otel/sdk/metric/metricdata"
	"go.uber.org/zap"

	"github.com/jaegertracing/jaeger/cmd/jaeger/internal/extension/jaegerquery/querysvc"
	"github.com/jaegertracing/jaeger/internal/metrics"
	depstoremocks "github.com/jaegertracing/jaeger/internal/storage/v2/api/depstore/mocks"
	tracestoremocks "github.com/jaegertracing/jaeger/internal/storage/v2/api/tracestore/mocks"
	"github.com/jaegertracing/jaeger/internal/telemetry"
	"github.com/jaegertracing/jaeger/internal/telemetry/otelsemconv"
)

// TestToolErrorSpanStatusVsMetricStatus drives a real mcptools server, real
// handlers, both middlewares, in-memory transport, and calls get_trace_topology
// for a trace the store does not have. The handler returns
// errors.New("trace not found"), which the MCP SDK converts into
// CallToolResult{IsError: true} with a nil Go error.
//
// The test asserts BOTH halves of the contradiction at once, so it passes on
// current main and documents the inconsistency rather than merely failing:
//
//	metrics middleware -> jaeger.mcp.tool.calls{status="error"}
//	tracing middleware -> span Status.Code = Unset
//
// Flip the final assertion to codes.Error to turn this into the regression test
// that fails without a fix.
func TestToolErrorSpanStatusVsMetricStatus(t *testing.T) {
	traceCap := newTraceCapture(t)
	metricCap := newMetricsCapture(t)

	reader := &tracestoremocks.Reader{}
	// Empty iterator => querysvc yields no traces => handler: "trace not found".
	reader.On("GetTraces", mock.Anything, mock.Anything).Return(
		iter.Seq2[[]ptrace.Traces, error](func(func([]ptrace.Traces, error) bool) {}),
	)
	svc := querysvc.NewQueryService(reader, &depstoremocks.Reader{}, querysvc.QueryServiceOptions{})

	server := NewServer(telemetry.Settings{
		Logger:         zap.NewNop(),
		Metrics:        metrics.NullFactory,
		MeterProvider:  metricCap.provider,
		TracerProvider: traceCap.provider,
	}, svc, DefaultConfig())

	ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second)
	defer cancel()

	serverTransport, clientTransport := mcp.NewInMemoryTransports()
	serverSession, err := server.Connect(ctx, serverTransport, nil)
	require.NoError(t, err)
	defer serverSession.Close()

	client := mcp.NewClient(&mcp.Implementation{Name: "repro-client", Version: "0.0.0"}, nil)
	clientSession, err := client.Connect(ctx, clientTransport, nil)
	require.NoError(t, err)
	defer clientSession.Close()

	res, err := clientSession.CallTool(ctx, &mcp.CallToolParams{
		Name:      "get_trace_topology",
		Arguments: map[string]any{"trace_id": "00000000000000000000000000000abc"},
	})
	require.NoError(t, err, "no transport-level error: the failure is carried in the result")

	t.Log("=== TOOL RESULT ===")
	t.Logf("  IsError = %v", res.IsError)
	for _, c := range res.Content {
		if tc, ok := c.(*mcp.TextContent); ok {
			t.Logf("  content = %q", tc.Text)
		}
	}
	require.True(t, res.IsError, "the tool call failed")

	// --- what the tracing middleware recorded -------------------------------
	require.NoError(t, traceCap.provider.ForceFlush(ctx))
	spans := traceCap.exporter.GetSpans()
	require.NotEmpty(t, spans)

	toolSpanName := mcpMethodToolsCall + " get_trace_topology"
	toolSpan, found := spans[0], false
	for _, s := range spans {
		if s.Name == toolSpanName {
			toolSpan, found = s, true
		}
	}
	require.True(t, found, "expected a span named %q", toolSpanName)

	t.Log("=== SPAN (tracing middleware) ===")
	t.Logf("  name         = %s", toolSpan.Name)
	t.Logf("  Status.Code  = %v", toolSpan.Status.Code)
	t.Logf("  Status.Descr = %q", toolSpan.Status.Description)
	for _, a := range toolSpan.Attributes {
		t.Logf("  attr %s = %s", a.Key, a.Value.Emit())
	}
	t.Logf("  events (RecordError) = %d", len(toolSpan.Events))

	// --- what the metrics middleware recorded -------------------------------
	rm := metricCap.collect(t)
	counter := findMetricDataPoint[metricdata.Sum[int64]](t, rm, "jaeger.mcp.tool.calls")

	t.Log("=== METRIC (metrics middleware) ===")
	var toolCallStatus string
	for _, p := range counter.DataPoints {
		status, _ := p.Attributes.Value("status")
		toolName, _ := p.Attributes.Value("gen_ai.tool.name")
		method, _ := p.Attributes.Value("mcp.method.name")
		t.Logf("  value=%d status=%-9q tool=%-20q method=%q",
			p.Value, status.AsString(), toolName.AsString(), method.AsString())
		if toolName.AsString() == "get_trace_topology" {
			toolCallStatus = status.AsString()
		}
	}

	// --- the contradiction, asserted ----------------------------------------
	assert.Equal(t, metricStatusError, toolCallStatus,
		"metrics middleware classifies the failed call as an error")
	assert.Equal(t, codes.Unset, toolSpan.Status.Code,
		"BUG: tracing middleware leaves the same failed call at Unset")
	assertHasStringAttribute(t, toolSpan.Attributes,
		string(otelsemconv.ErrorType("").Key), errorTypeTool)

	t.Log("=== VERDICT ===")
	t.Logf("  same call: metric=%q, span status=%v", toolCallStatus, toolSpan.Status.Code)
}

Verified at commit b3076cc4337324387d71cd330d58c1cda49372e0, saved as cmd/jaeger/internal/extension/jaegerquery/internal/mcptools/middleware_repro_test.go:

• gofmt -l clean
• go vet ./...mcptools/ clean
• go test -run TestToolErrorSpanStatusVsMetricStatus -count=1 PASS
• Full package suite ./...mcptools/... all ok, no interference with existing tests

Expected behavior

Status.Code = Error on a call the same middleware package already counts as an error in jaeger.mcp.tool.calls{status="error"}.

The proposed fix:

 	if callResult, ok := result.(*mcp.CallToolResult); ok && callResult.IsError {
 		span.SetAttributes(otelsemconv.ErrorType(errorTypeTool))
 		if toolErr := callResult.GetError(); toolErr != nil {
 			span.RecordError(toolErr)
+			span.SetStatus(codes.Error, toolErr.Error())
+		} else {
+			span.SetStatus(codes.Error, "tool returned an error result")
 		}
 	}

The else branch is defensive. No current caller reaches it, since the one place that constructs an IsError result without SetError sits in middleware that never reaches the tracing layer, see above. It costs nothing and closes the gap if that ever changes.

Note on two tests that pin the current behaviour: TestTracingMiddlewareToolCallResultError (middleware_test.go#L107) and TestTracingMiddlewareToolCallResultErrorWithoutConcreteError (#L194) both assert codes.Unset, so this fix flips two existing assertions. Flagging that rather than burying it.

The property those tests guard is that a tool error stays distinguishable from a transport error. The transport tests at L64/L84/L142 assert codes.Error plus a Status.Description carrying the error text. That distinction survives the fix, because it is carried by error.type=tool_error, which is present only on the tool-error path and is already asserted in both tests. Only the literal status value changes.

TestTracingMiddlewareToolCallSuccess (#L45) also asserts Unset and must stay that way. The fix does not touch the success path.

If maintainers consider Unset a deliberate choice rather than an oversight, the metrics/traces disagreement is still worth resolving in one direction or the other, and I am happy to reframe this as a discussion instead of a bug.

Relevant log output

=== TOOL RESULT ===
  IsError = true
  content = "trace not found"
=== SPAN (tracing middleware) ===
  name         = tools/call get_trace_topology
  Status.Code  = Unset
  Status.Descr = ""
  attr gen_ai.operation.name = execute_tool
  attr gen_ai.tool.name = get_trace_topology
  attr error.type = tool_error
  events (RecordError) = 1
=== METRIC (metrics middleware) ===
  value=1 status="error"   tool="get_trace_topology" method="tools/call"
  value=1 status="success" tool=""                   method="initialize"
  value=1 status="success" tool=""                   method="notifications/initialized"
=== VERDICT ===
  same call: metric="error", span status=Unset

Screenshot

No response

Additional context

Verification notes. Repro run against commit b3076cc4337324387d71cd330d58c1cda49372e0, real NewServer, real handlers, in-memory transport. gofmt, go vet, and go test all clean, including the full package suite with no interference with existing tests. Output above is captured verbatim from that run.

The SDK error-conversion path and the StatusCodeError consumers were read, not executed. I did not stand up a live Jaeger-tracing-Jaeger deployment to confirm the end-user query behaviour, though tenant.go:274 and get_trace_errors.go:73 are unambiguous about the criterion.

The count of 44 handler error returns is a grep of return .*(errors\.New|fmt\.Errorf) across the non-test handler files and may include a small number of paths not reachable via tools/call.

I have not surveyed whether any downstream dashboard in the wild depends on the current Unset behaviour.

Related but distinct: #9289 covers how the tool result envelope is translated onward to the browser. This issue is confined to the Go MCP server's own OTel span, a different layer.

Jaeger backend version

main b3076cc

SDK

No response

Pipeline

No response

Stogage backend

in-memory, via tracestoremocks.Reader / querysvc.NewQueryService in the repro; the impact section separately cites internal/storage/v2/memory/tenant.go as a real consumer

Operating system

No response

Deployment model

No response

Deployment configs

[kashvipeehu24]

I’d like to work on this issue and submit a focused implementation PR.

My proposed approach is to update the tracing middleware so that "CallToolResult{IsError: true}" sets the span status to "codes.Error", using the concrete tool error message when available and a defensive fallback otherwise. I’ll also update the existing tracing tests that currently expect "codes.Unset", while keeping the success and transport-error behavior unchanged.

I’ll verify the change with the relevant "mcptools" test suite, "go vet", and formatting checks, and keep the PR limited to the MCP tool-error span-status inconsistency described here.

[dishafaujdar]

Opened #9348