-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
fix(mcp-server): Add defensive patches for Transport edge cases #17291
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Changes from 12 commits
7f68a56
a8b39b7
a4f7b8c
ff7d1af
00cd668
2678e9f
fb40d64
9fcece1
3e137ac
b590352
46b6907
ba7435f
ede7daf
b0bf0b9
bbfd2a4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -45,21 +45,23 @@ import type { | |
* @returns Transport type mapping for span attributes | ||
*/ | ||
export function getTransportTypes(transport: MCPTransport): { mcpTransport: string; networkTransport: string } { | ||
const transportName = transport.constructor?.name?.toLowerCase() || ''; | ||
|
||
if (transportName.includes('stdio')) { | ||
return { mcpTransport: 'stdio', networkTransport: 'pipe' }; | ||
if (!transport?.constructor) { | ||
return { mcpTransport: 'unknown', networkTransport: 'unknown' }; | ||
} | ||
const transportName = transport.constructor.name?.toLowerCase() || 'unknown'; | ||
|
||
if (transportName.includes('streamablehttp') || transportName.includes('streamable')) { | ||
return { mcpTransport: 'http', networkTransport: 'tcp' }; | ||
} | ||
let networkTransport = 'unknown'; | ||
|
||
if (transportName.includes('sse')) { | ||
return { mcpTransport: 'sse', networkTransport: 'tcp' }; | ||
if (transportName.includes('stdio')) { | ||
networkTransport = 'pipe'; | ||
} else if (transportName.includes('http') || transportName.includes('sse')) { | ||
networkTransport = 'tcp'; | ||
} | ||
|
||
return { mcpTransport: 'unknown', networkTransport: 'unknown' }; | ||
return { | ||
mcpTransport: transportName, | ||
networkTransport, | ||
}; | ||
betegon marked this conversation as resolved.
Show resolved
Hide resolved
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Bug: API Breaks: Incorrect Transport Type HandlingThe There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is this OK/desired, that the transportName is now arbitrary and not from a defined list? 🤔 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I did it that way as we can have several MCP transports:
So I believe is a good idea to leave the constructor name as devs will know exactly what the transport is. That said, maybe we tend to do things differently and I'll be happy to change it as a set of values. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. sounds good to me, just wanted to double check ;) |
||
} | ||
|
||
/** | ||
|
@@ -260,12 +262,13 @@ export function extractClientInfo(extra: ExtraHandlerData): { | |
* @param transport - MCP transport instance | ||
* @param extra - Optional extra handler data | ||
* @returns Transport attributes for span instrumentation | ||
* @note sessionId may be undefined during initial setup - session should be established by client during initialize flow | ||
*/ | ||
export function buildTransportAttributes( | ||
transport: MCPTransport, | ||
extra?: ExtraHandlerData, | ||
): Record<string, string | number> { | ||
const sessionId = transport.sessionId; | ||
const sessionId = transport && 'sessionId' in transport ? transport.sessionId : undefined; | ||
const clientInfo = extra ? extractClientInfo(extra) : {}; | ||
const { mcpTransport, networkTransport } = getTransportTypes(transport); | ||
const clientAttributes = getClientAttributes(transport); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -61,7 +61,7 @@ describe('MCP Server Semantic Conventions', () => { | |
'mcp.session.id': 'test-session-123', | ||
'client.address': '192.168.1.100', | ||
'client.port': 54321, | ||
'mcp.transport': 'http', | ||
'mcp.transport': 'streamablehttpservertransport', | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. hmm, should we maybe no lower-case the transport? I suppose this would be easier to read in camel case? 😅 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. totally right. fixing it |
||
'network.transport': 'tcp', | ||
'network.protocol.version': '2.0', | ||
'mcp.request.argument.location': '"Seattle, WA"', | ||
|
@@ -93,7 +93,7 @@ describe('MCP Server Semantic Conventions', () => { | |
'mcp.resource.uri': 'file:///docs/api.md', | ||
'mcp.request.id': 'req-2', | ||
'mcp.session.id': 'test-session-123', | ||
'mcp.transport': 'http', | ||
'mcp.transport': 'streamablehttpservertransport', | ||
'network.transport': 'tcp', | ||
'network.protocol.version': '2.0', | ||
'mcp.request.argument.uri': '"file:///docs/api.md"', | ||
|
@@ -125,7 +125,7 @@ describe('MCP Server Semantic Conventions', () => { | |
'mcp.prompt.name': 'analyze-code', | ||
'mcp.request.id': 'req-3', | ||
'mcp.session.id': 'test-session-123', | ||
'mcp.transport': 'http', | ||
'mcp.transport': 'streamablehttpservertransport', | ||
'network.transport': 'tcp', | ||
'network.protocol.version': '2.0', | ||
'mcp.request.argument.name': '"analyze-code"', | ||
|
@@ -154,7 +154,7 @@ describe('MCP Server Semantic Conventions', () => { | |
attributes: { | ||
'mcp.method.name': 'notifications/tools/list_changed', | ||
'mcp.session.id': 'test-session-123', | ||
'mcp.transport': 'http', | ||
'mcp.transport': 'streamablehttpservertransport', | ||
'network.transport': 'tcp', | ||
'network.protocol.version': '2.0', | ||
'sentry.op': 'mcp.notification.client_to_server', | ||
|
@@ -193,7 +193,7 @@ describe('MCP Server Semantic Conventions', () => { | |
'mcp.request.id': 'req-4', | ||
'mcp.session.id': 'test-session-123', | ||
// Transport attributes | ||
'mcp.transport': 'http', | ||
'mcp.transport': 'streamablehttpservertransport', | ||
'network.transport': 'tcp', | ||
'network.protocol.version': '2.0', | ||
// Sentry-specific | ||
|
@@ -227,7 +227,7 @@ describe('MCP Server Semantic Conventions', () => { | |
attributes: { | ||
'mcp.method.name': 'notifications/message', | ||
'mcp.session.id': 'test-session-123', | ||
'mcp.transport': 'http', | ||
'mcp.transport': 'streamablehttpservertransport', | ||
'network.transport': 'tcp', | ||
'network.protocol.version': '2.0', | ||
'mcp.logging.level': 'info', | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Transport Mapping and Runtime Error
The
getTransportTypes
function introduces two issues:networkTransport
was changed. Previously, transport names including 'streamable' were mapped to 'tcp'. Now, only those including 'http' or 'sse' are mapped to 'tcp'. This causes transports containing 'streamable' but not 'http' to incorrectly be classified as 'unknown' instead of 'tcp', potentially breaking compatibility.transportName
assignment (transport.constructor.name?.toLowerCase()
) can throw a runtime error iftransport.constructor.name
is a non-string value, astoLowerCase()
would be called on an invalid type.