Skip to content

Commit 893539d

Browse files
betegonclaude
andauthored
fix(core): set span.status to error when MCP tool returns JSON-RPC error response (#20082)
When an MCP tool returns a JSON-RPC error response (i.e. the response has an `error` field instead of `result`), the span was being ended with `status=ok`. This meant `failure_rate()` read 0% in the MCP insights dashboard even when tools were consistently failing. ## Root cause `completeSpanWithResults` always ended the span without checking whether the response was an error. The `captureError` path uses `getActiveSpan()`, which returns `null` outside a `withActiveSpan()` context — so errors were never reflected in span status. ## Fix Pass `!!message.error` as a `hasError` flag to `completeSpanWithResults`. When `true`, set `SPAN_STATUS_ERROR` with `message: 'internal_error'` directly on the stored span before ending it. ## Changes - `correlation.ts` — `completeSpanWithResults` accepts optional `hasError` param; sets error status when true - `transport.ts` — passes `!!message.error` when completing spans on outgoing send - `transportInstrumentation.test.ts` — two new tests: error response sets error status; success response does not Closes #20083 --------- Co-authored-by: claude-sonnet-4-6 <noreply@anthropic.com>
1 parent 4be2e67 commit 893539d

File tree

9 files changed

+131
-2
lines changed

9 files changed

+131
-2
lines changed

dev-packages/e2e-tests/test-applications/node-express-v5/src/mcp.ts

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -47,6 +47,10 @@ server.prompt('echo', { message: z.string() }, ({ message }, extra) => ({
4747
],
4848
}));
4949

50+
server.tool('always-error', {}, async () => {
51+
throw new Error('intentional error for span status testing');
52+
});
53+
5054
const transports: Record<string, SSEServerTransport> = {};
5155

5256
mcpRouter.get('/sse', async (_, res) => {

dev-packages/e2e-tests/test-applications/node-express-v5/tests/mcp.test.ts

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -120,6 +120,23 @@ test('Should record transactions for mcp handlers', async ({ baseURL }) => {
120120

121121
// TODO: When https://github.com/modelcontextprotocol/typescript-sdk/pull/358 is released check for trace id equality between the post transaction and the handler transaction
122122
});
123+
124+
await test.step('error tool sets span status to internal_error', async () => {
125+
const toolTransactionPromise = waitForTransaction('node-express-v5', transactionEvent => {
126+
return transactionEvent.transaction === 'tools/call always-error';
127+
});
128+
129+
try {
130+
await client.callTool({ name: 'always-error', arguments: {} });
131+
} catch {
132+
// Expected: MCP SDK throws when the tool returns a JSON-RPC error
133+
}
134+
135+
const toolTransaction = await toolTransactionPromise;
136+
expect(toolTransaction).toBeDefined();
137+
expect(toolTransaction.contexts?.trace?.op).toEqual('mcp.server');
138+
expect(toolTransaction.contexts?.trace?.status).toEqual('internal_error');
139+
});
123140
});
124141

125142
/**

dev-packages/e2e-tests/test-applications/node-express/src/mcp.ts

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -47,6 +47,10 @@ server.prompt('echo', { message: z.string() }, ({ message }, extra) => ({
4747
],
4848
}));
4949

50+
server.tool('always-error', {}, async () => {
51+
throw new Error('intentional error for span status testing');
52+
});
53+
5054
const transports: Record<string, SSEServerTransport> = {};
5155

5256
mcpRouter.get('/sse', async (_, res) => {

dev-packages/e2e-tests/test-applications/node-express/tests/mcp.test.ts

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -126,6 +126,23 @@ test('Should record transactions for mcp handlers', async ({ baseURL }) => {
126126
expect(promptTransaction.contexts?.trace?.data?.['mcp.method.name']).toEqual('prompts/get');
127127
// TODO: When https://github.com/modelcontextprotocol/typescript-sdk/pull/358 is released check for trace id equality between the post transaction and the handler transaction
128128
});
129+
130+
await test.step('error tool sets span status to internal_error', async () => {
131+
const toolTransactionPromise = waitForTransaction('node-express', transactionEvent => {
132+
return transactionEvent.transaction === 'tools/call always-error';
133+
});
134+
135+
try {
136+
await client.callTool({ name: 'always-error', arguments: {} });
137+
} catch {
138+
// Expected: MCP SDK throws when the tool returns a JSON-RPC error
139+
}
140+
141+
const toolTransaction = await toolTransactionPromise;
142+
expect(toolTransaction).toBeDefined();
143+
expect(toolTransaction.contexts?.trace?.op).toEqual('mcp.server');
144+
expect(toolTransaction.contexts?.trace?.status).toEqual('internal_error');
145+
});
129146
});
130147

131148
/**

dev-packages/e2e-tests/test-applications/tsx-express/src/mcp.ts

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -47,6 +47,10 @@ server.prompt('echo', { message: z.string() }, ({ message }, extra) => ({
4747
],
4848
}));
4949

50+
server.tool('always-error', {}, async () => {
51+
throw new Error('intentional error for span status testing');
52+
});
53+
5054
const transports: Record<string, SSEServerTransport> = {};
5155

5256
mcpRouter.get('/sse', async (_, res) => {

dev-packages/e2e-tests/test-applications/tsx-express/tests/mcp.test.ts

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -123,6 +123,23 @@ test('Records transactions for mcp handlers', async ({ baseURL }) => {
123123

124124
// TODO: When https://github.com/modelcontextprotocol/typescript-sdk/pull/358 is released check for trace id equality between the post transaction and the handler transaction
125125
});
126+
127+
await test.step('error tool sets span status to internal_error', async () => {
128+
const toolTransactionPromise = waitForTransaction('tsx-express', transactionEvent => {
129+
return transactionEvent.transaction === 'tools/call always-error';
130+
});
131+
132+
try {
133+
await client.callTool({ name: 'always-error', arguments: {} });
134+
} catch {
135+
// Expected: MCP SDK throws when the tool returns a JSON-RPC error
136+
}
137+
138+
const toolTransaction = await toolTransactionPromise;
139+
expect(toolTransaction).toBeDefined();
140+
expect(toolTransaction.contexts?.trace?.op).toEqual('mcp.server');
141+
expect(toolTransaction.contexts?.trace?.status).toEqual('internal_error');
142+
});
126143
});
127144

128145
/**

packages/core/src/integrations/mcp-server/correlation.ts

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -81,19 +81,23 @@ export function storeSpanForRequest(transport: MCPTransport, requestId: RequestI
8181
* @param requestId - Request identifier
8282
* @param result - Execution result for attribute extraction
8383
* @param options - Resolved MCP options
84+
* @param hasError - Whether the JSON-RPC response contained an error
8485
*/
8586
export function completeSpanWithResults(
8687
transport: MCPTransport,
8788
requestId: RequestId,
8889
result: unknown,
8990
options: ResolvedMcpOptions,
91+
hasError = false,
9092
): void {
9193
const spanMap = getOrCreateSpanMap(transport);
9294
const spanData = spanMap.get(requestId);
9395
if (spanData) {
9496
const { span, method } = spanData;
9597

96-
if (method === 'initialize') {
98+
if (hasError) {
99+
span.setStatus({ code: SPAN_STATUS_ERROR, message: 'internal_error' });
100+
} else if (method === 'initialize') {
97101
const sessionData = extractSessionDataFromInitializeResponse(result);
98102
const serverAttributes = buildServerAttributesFromInfo(sessionData.serverInfo);
99103

packages/core/src/integrations/mcp-server/transport.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -121,7 +121,7 @@ export function wrapTransportSend(transport: MCPTransport, options: ResolvedMcpO
121121
}
122122
}
123123

124-
completeSpanWithResults(this, message.id, message.result, options);
124+
completeSpanWithResults(this, message.id, message.result, options, !!message.error);
125125
}
126126
}
127127

packages/core/test/lib/integrations/mcp-server/transportInstrumentation.test.ts

Lines changed: 62 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -182,6 +182,68 @@ describe('MCP Server Transport Instrumentation', () => {
182182
// Trigger onclose - should not throw
183183
expect(() => mockTransport.onclose?.()).not.toThrow();
184184
});
185+
186+
it('should set span status to error when JSON-RPC error response is sent', async () => {
187+
const mockSpan = {
188+
setAttributes: vi.fn(),
189+
setStatus: vi.fn(),
190+
end: vi.fn(),
191+
isRecording: vi.fn().mockReturnValue(true),
192+
};
193+
startInactiveSpanSpy.mockReturnValue(mockSpan as any);
194+
195+
await wrappedMcpServer.connect(mockTransport);
196+
197+
// Simulate an incoming tools/call request
198+
const jsonRpcRequest = {
199+
jsonrpc: '2.0',
200+
method: 'tools/call',
201+
id: 'req-err-1',
202+
params: { name: 'always-error' },
203+
};
204+
mockTransport.onmessage?.(jsonRpcRequest, {});
205+
206+
// Simulate the MCP SDK sending back a JSON-RPC error response
207+
const jsonRpcErrorResponse = {
208+
jsonrpc: '2.0',
209+
id: 'req-err-1',
210+
error: { code: -32603, message: 'Internal error: tool threw an exception' },
211+
};
212+
await mockTransport.send?.(jsonRpcErrorResponse as any);
213+
214+
expect(mockSpan.setStatus).toHaveBeenCalledWith({ code: 2, message: 'internal_error' });
215+
expect(mockSpan.end).toHaveBeenCalled();
216+
});
217+
218+
it('should not set error span status for successful JSON-RPC responses', async () => {
219+
const mockSpan = {
220+
setAttributes: vi.fn(),
221+
setStatus: vi.fn(),
222+
end: vi.fn(),
223+
isRecording: vi.fn().mockReturnValue(true),
224+
};
225+
startInactiveSpanSpy.mockReturnValue(mockSpan as any);
226+
227+
await wrappedMcpServer.connect(mockTransport);
228+
229+
const jsonRpcRequest = {
230+
jsonrpc: '2.0',
231+
method: 'tools/call',
232+
id: 'req-ok-1',
233+
params: { name: 'echo' },
234+
};
235+
mockTransport.onmessage?.(jsonRpcRequest, {});
236+
237+
const jsonRpcSuccessResponse = {
238+
jsonrpc: '2.0',
239+
id: 'req-ok-1',
240+
result: { content: [{ type: 'text', text: 'hello' }] },
241+
};
242+
await mockTransport.send?.(jsonRpcSuccessResponse as any);
243+
244+
expect(mockSpan.setStatus).not.toHaveBeenCalled();
245+
expect(mockSpan.end).toHaveBeenCalled();
246+
});
185247
});
186248

187249
describe('Stdio Transport Tests', () => {

0 commit comments

Comments
 (0)