Skip to content

Commit 477f1f2

Browse files
betegonbetegon
committed
Set span status to error when capturing errors and added MCP method name to exception mechanism
1 parent 8524991 commit 477f1f2

File tree

5 files changed

+109
-102
lines changed

5 files changed

+109
-102
lines changed

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

Lines changed: 5 additions & 46 deletions
Original file line numberDiff line numberDiff line change
@@ -5,15 +5,10 @@
55

66
import { getClient } from '../../currentScopes';
77
import { SPAN_STATUS_ERROR, withActiveSpan } from '../../tracing';
8-
import type { Span, SpanAttributeValue } from '../../types-hoist/span';
9-
import {
10-
MCP_TOOL_RESULT_CONTENT_ATTRIBUTE,
11-
MCP_TOOL_RESULT_CONTENT_COUNT_ATTRIBUTE,
12-
MCP_TOOL_RESULT_IS_ERROR_ATTRIBUTE,
13-
} from './attributes';
14-
import { captureError } from './errorCapture';
8+
import type { Span } from '../../types-hoist/span';
9+
import { extractToolResultAttributes } from './attributeExtraction';
1510
import { filterMcpPiiFromSpanData } from './piiFiltering';
16-
import type { RequestId, RequestSpanMapValue, SessionId } from './types';
11+
import type { RequestId, RequestSpanMapValue } from './types';
1712

1813
// Simplified correlation system that works with or without sessionId
1914
// Maps requestId directly to span data for stateless operation
@@ -34,7 +29,7 @@ export function storeSpanForRequest(requestId: RequestId, span: Span, method: st
3429
* Associates handler execution with the corresponding request span
3530
*/
3631
export function associateContextWithRequestSpan<T>(
37-
extraHandlerData: { sessionId?: SessionId; requestId: RequestId } | undefined,
32+
extraHandlerData: { requestId: RequestId } | undefined,
3833
cb: () => T,
3934
): T {
4035
if (extraHandlerData) {
@@ -70,17 +65,6 @@ export function completeSpanWithResults(requestId: RequestId, result: unknown):
7065
const toolAttributes = filterMcpPiiFromSpanData(rawToolAttributes, sendDefaultPii);
7166

7267
span.setAttributes(toolAttributes);
73-
74-
const isToolError = rawToolAttributes[MCP_TOOL_RESULT_IS_ERROR_ATTRIBUTE] === true;
75-
76-
if (isToolError) {
77-
span.setStatus({
78-
code: SPAN_STATUS_ERROR,
79-
message: 'Tool returned error result',
80-
});
81-
82-
captureError(new Error('Tool returned error result'), 'tool_execution');
83-
}
8468
}
8569

8670
span.end();
@@ -97,36 +81,11 @@ export function cleanupAllPendingSpans(): number {
9781
for (const [, spanData] of requestIdToSpanMap) {
9882
spanData.span.setStatus({
9983
code: SPAN_STATUS_ERROR,
100-
message: 'Transport closed before request completion',
84+
message: 'cancelled',
10185
});
10286
spanData.span.end();
10387
}
10488

10589
requestIdToSpanMap.clear();
10690
return pendingCount;
10791
}
108-
109-
/**
110-
* Simplified tool result attribute extraction
111-
*/
112-
function extractToolResultAttributes(result: unknown): Record<string, SpanAttributeValue | undefined> {
113-
const attributes: Record<string, SpanAttributeValue | undefined> = {};
114-
115-
if (typeof result === 'object' && result !== null) {
116-
const resultObj = result as Record<string, unknown>;
117-
118-
if (typeof resultObj.isError === 'boolean') {
119-
attributes[MCP_TOOL_RESULT_IS_ERROR_ATTRIBUTE] = resultObj.isError;
120-
}
121-
122-
if (Array.isArray(resultObj.content)) {
123-
attributes[MCP_TOOL_RESULT_CONTENT_COUNT_ATTRIBUTE] = resultObj.content.length;
124-
125-
const serializedContent = JSON.stringify(resultObj.content);
126-
attributes[MCP_TOOL_RESULT_CONTENT_ATTRIBUTE] =
127-
serializedContent.length > 5000 ? `${serializedContent.substring(0, 4997)}...` : serializedContent;
128-
}
129-
}
130-
131-
return attributes;
132-
}

‎packages/core/src/integrations/mcp-server/errorCapture.ts‎

Lines changed: 18 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -5,22 +5,37 @@
55

66
import { getClient } from '../../currentScopes';
77
import { captureException } from '../../exports';
8+
import { SPAN_STATUS_ERROR } from '../../tracing';
9+
import { getActiveSpan } from '../../utils/spanUtils';
810

911
/**
1012
* Safely captures an error to Sentry without affecting MCP service operation
1113
* The active span already contains all MCP context (method, tool, arguments, etc.)
1214
* Sentry automatically associates the error with the active span
1315
*/
14-
export function captureError(error: Error, errorType?: string): void {
16+
export function captureError(error: Error, errorType?: string, extraData?: Record<string, unknown>): void {
1517
try {
1618
const client = getClient();
1719
if (!client) {
1820
return;
1921
}
2022

23+
const activeSpan = getActiveSpan();
24+
if (activeSpan?.isRecording()) {
25+
activeSpan.setStatus({
26+
code: SPAN_STATUS_ERROR,
27+
message: 'internal_error',
28+
});
29+
}
30+
2131
captureException(error, {
22-
tags: {
23-
mcp_error_type: errorType || 'handler_execution',
32+
mechanism: {
33+
type: 'mcp_server',
34+
handled: false,
35+
data: {
36+
error_type: errorType || 'handler_execution',
37+
...extraData,
38+
},
2439
},
2540
});
2641
} catch {

‎packages/core/src/integrations/mcp-server/handlers.ts‎

Lines changed: 12 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -91,32 +91,37 @@ function createErrorCapturingHandler(
9191
function captureHandlerError(
9292
error: Error,
9393
methodName: keyof MCPServerInstance,
94-
_handlerName: string,
94+
handlerName: string,
9595
_handlerArgs: unknown[],
9696
_extraHandlerData?: HandlerExtraData,
9797
): void {
9898
try {
99+
const extraData: Record<string, unknown> = {};
100+
99101
if (methodName === 'tool') {
100-
// Check if this is a validation/protocol error
102+
extraData.tool_name = handlerName;
103+
101104
if (
102105
error.name === 'ProtocolValidationError' ||
103106
error.message.includes('validation') ||
104107
error.message.includes('protocol')
105108
) {
106-
captureError(error, 'validation');
109+
captureError(error, 'validation', extraData);
107110
} else if (
108111
error.name === 'ServerTimeoutError' ||
109112
error.message.includes('timed out') ||
110113
error.message.includes('timeout')
111114
) {
112-
captureError(error, 'timeout');
115+
captureError(error, 'timeout', extraData);
113116
} else {
114-
captureError(error, 'tool_execution');
117+
captureError(error, 'tool_execution', extraData);
115118
}
116119
} else if (methodName === 'resource') {
117-
captureError(error, 'resource_operation');
120+
extraData.resource_uri = handlerName;
121+
captureError(error, 'resource_operation', extraData);
118122
} else if (methodName === 'prompt') {
119-
captureError(error, 'prompt_execution');
123+
extraData.prompt_name = handlerName;
124+
captureError(error, 'prompt_execution', extraData);
120125
}
121126
} catch (captureErr) {
122127
// silently ignore capture errors to not affect MCP operation

‎packages/core/test/lib/integrations/mcp-server/mcpServerErrorCapture.test.ts‎

Lines changed: 48 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -17,26 +17,18 @@ describe('MCP Server Error Capture', () => {
1717
});
1818

1919
describe('captureError', () => {
20-
it('should capture errors with default error type', () => {
21-
const error = new Error('Test error');
22-
23-
captureError(error);
24-
25-
expect(captureExceptionSpy).toHaveBeenCalledWith(error, {
26-
tags: {
27-
mcp_error_type: 'handler_execution',
28-
},
29-
});
30-
});
31-
32-
it('should capture errors with custom error type', () => {
20+
it('should capture errors with error type', () => {
3321
const error = new Error('Tool execution failed');
3422

3523
captureError(error, 'tool_execution');
3624

3725
expect(captureExceptionSpy).toHaveBeenCalledWith(error, {
38-
tags: {
39-
mcp_error_type: 'tool_execution',
26+
mechanism: {
27+
type: 'mcp_server',
28+
handled: false,
29+
data: {
30+
error_type: 'tool_execution',
31+
},
4032
},
4133
});
4234
});
@@ -47,8 +39,12 @@ describe('MCP Server Error Capture', () => {
4739
captureError(error, 'transport');
4840

4941
expect(captureExceptionSpy).toHaveBeenCalledWith(error, {
50-
tags: {
51-
mcp_error_type: 'transport',
42+
mechanism: {
43+
type: 'mcp_server',
44+
handled: false,
45+
data: {
46+
error_type: 'transport',
47+
},
5248
},
5349
});
5450
});
@@ -59,8 +55,12 @@ describe('MCP Server Error Capture', () => {
5955
captureError(error, 'protocol');
6056

6157
expect(captureExceptionSpy).toHaveBeenCalledWith(error, {
62-
tags: {
63-
mcp_error_type: 'protocol',
58+
mechanism: {
59+
type: 'mcp_server',
60+
handled: false,
61+
data: {
62+
error_type: 'protocol',
63+
},
6464
},
6565
});
6666
});
@@ -71,8 +71,12 @@ describe('MCP Server Error Capture', () => {
7171
captureError(error, 'validation');
7272

7373
expect(captureExceptionSpy).toHaveBeenCalledWith(error, {
74-
tags: {
75-
mcp_error_type: 'validation',
74+
mechanism: {
75+
type: 'mcp_server',
76+
handled: false,
77+
data: {
78+
error_type: 'validation',
79+
},
7680
},
7781
});
7882
});
@@ -83,8 +87,29 @@ describe('MCP Server Error Capture', () => {
8387
captureError(error, 'timeout');
8488

8589
expect(captureExceptionSpy).toHaveBeenCalledWith(error, {
86-
tags: {
87-
mcp_error_type: 'timeout',
90+
mechanism: {
91+
type: 'mcp_server',
92+
handled: false,
93+
data: {
94+
error_type: 'timeout',
95+
},
96+
},
97+
});
98+
});
99+
100+
it('should capture errors with MCP data for filtering', () => {
101+
const error = new Error('Tool failed');
102+
103+
captureError(error, 'tool_execution', { tool_name: 'my-tool' });
104+
105+
expect(captureExceptionSpy).toHaveBeenCalledWith(error, {
106+
mechanism: {
107+
type: 'mcp_server',
108+
handled: false,
109+
data: {
110+
error_type: 'tool_execution',
111+
tool_name: 'my-tool',
112+
},
88113
},
89114
});
90115
});

‎packages/core/test/lib/integrations/mcp-server/semanticConventions.test.ts‎

Lines changed: 26 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,7 @@ describe('MCP Server Semantic Conventions', () => {
1616
getOptions: () => ({ sendDefaultPii: true }),
1717
getDsn: () => ({ publicKey: 'test-key', host: 'test-host' }),
1818
emit: vi.fn(),
19-
} as any);
19+
} as unknown as ReturnType<typeof currentScopes.getClient>);
2020
});
2121

2222
describe('Span Creation & Semantic Conventions', () => {
@@ -166,7 +166,7 @@ describe('MCP Server Semantic Conventions', () => {
166166
);
167167

168168
// Should not include mcp.request.id for notifications
169-
const callArgs = vi.mocked(tracingModule.startSpan).mock.calls[0];
169+
const callArgs = startSpanSpy.mock.calls[0];
170170
expect(callArgs).toBeDefined();
171171
const attributes = callArgs?.[0]?.attributes;
172172
expect(attributes).not.toHaveProperty('mcp.request.id');
@@ -363,12 +363,15 @@ describe('MCP Server Semantic Conventions', () => {
363363
it('should instrument tool call results and complete span with enriched attributes', async () => {
364364
await wrappedMcpServer.connect(mockTransport);
365365

366+
const setAttributesSpy = vi.fn();
367+
const setStatusSpy = vi.fn();
368+
const endSpy = vi.fn();
366369
const mockSpan = {
367-
setAttributes: vi.fn(),
368-
setStatus: vi.fn(),
369-
end: vi.fn(),
370+
setAttributes: setAttributesSpy,
371+
setStatus: setStatusSpy,
372+
end: endSpy,
370373
};
371-
startInactiveSpanSpy.mockReturnValueOnce(mockSpan as any);
374+
startInactiveSpanSpy.mockReturnValueOnce(mockSpan as unknown as ReturnType<typeof tracingModule.startInactiveSpan>);
372375

373376
const toolCallRequest = {
374377
jsonrpc: '2.0',
@@ -416,28 +419,31 @@ describe('MCP Server Semantic Conventions', () => {
416419
mockTransport.send?.(toolResponse);
417420

418421
// Verify that the span was enriched with tool result attributes
419-
expect(mockSpan.setAttributes).toHaveBeenCalledWith(
422+
expect(setAttributesSpy).toHaveBeenCalledWith(
420423
expect.objectContaining({
421424
'mcp.tool.result.is_error': false,
422425
'mcp.tool.result.content_count': 1,
423-
'mcp.tool.result.content':
424-
'[{"type":"text","text":"The weather in San Francisco is 18°C with partly cloudy skies."}]',
426+
'mcp.tool.result.content_type': 'text',
427+
'mcp.tool.result.content': 'The weather in San Francisco is 18°C with partly cloudy skies.',
425428
}),
426429
);
427430

428431
// Verify span was completed successfully (no error status set)
429-
expect(mockSpan.setStatus).not.toHaveBeenCalled();
430-
expect(mockSpan.end).toHaveBeenCalled();
432+
expect(setStatusSpy).not.toHaveBeenCalled();
433+
expect(endSpy).toHaveBeenCalled();
431434
});
432435

433436
it('should set span status to ERROR when tool result has isError: true', async () => {
434437
await wrappedMcpServer.connect(mockTransport);
435438

439+
const setAttributesSpy = vi.fn();
440+
const setStatusSpy = vi.fn();
441+
const endSpy = vi.fn();
436442
const mockSpan = {
437-
setAttributes: vi.fn(),
438-
setStatus: vi.fn(),
439-
end: vi.fn(),
440-
} as any;
443+
setAttributes: setAttributesSpy,
444+
setStatus: setStatusSpy,
445+
end: endSpy,
446+
} as unknown as ReturnType<typeof tracingModule.startInactiveSpan>;
441447
startInactiveSpanSpy.mockReturnValueOnce(mockSpan);
442448

443449
const toolCallRequest = {
@@ -472,20 +478,17 @@ describe('MCP Server Semantic Conventions', () => {
472478
mockTransport.send?.(toolErrorResponse);
473479

474480
// Verify that the span was enriched with tool result attributes including error
475-
expect(mockSpan.setAttributes).toHaveBeenCalledWith(
481+
expect(setAttributesSpy).toHaveBeenCalledWith(
476482
expect.objectContaining({
477483
'mcp.tool.result.is_error': true,
478484
'mcp.tool.result.content_count': 1,
479-
'mcp.tool.result.content': '[{"type":"text","text":"Tool execution failed"}]',
485+
'mcp.tool.result.content_type': 'text',
486+
'mcp.tool.result.content': 'Tool execution failed',
480487
}),
481488
);
482489

483-
// Verify span status was set to ERROR
484-
expect(mockSpan.setStatus).toHaveBeenCalledWith({
485-
code: 2, // ERROR
486-
message: 'Tool execution failed',
487-
});
488-
expect(mockSpan.end).toHaveBeenCalled();
490+
expect(setStatusSpy).not.toHaveBeenCalled();
491+
expect(endSpy).toHaveBeenCalled();
489492
});
490493
});
491494
});

0 commit comments

Comments
 (0)