-
-
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
Changes from 10 commits
7f68a56
a8b39b7
a4f7b8c
ff7d1af
00cd668
2678e9f
fb40d64
9fcece1
3e137ac
b590352
46b6907
ba7435f
ede7daf
b0bf0b9
bbfd2a4
aecd8c6
c121658
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,27 @@ 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' }; | ||
if (transportName === 'unknown') { | ||
return { mcpTransport: 'unknown', networkTransport: 'unknown' }; | ||
betegon marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
|
||
if (transportName.includes('sse')) { | ||
return { mcpTransport: 'sse', networkTransport: 'tcp' }; | ||
let networkTransport = 'unknown'; | ||
|
||
if (transportName.includes('stdio')) { | ||
networkTransport = 'pipe'; | ||
} else if (transportName.includes('http') || transportName.includes('sse')) { | ||
networkTransport = 'tcp'; | ||
betegon marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
|
||
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 +266,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); | ||
|
Uh oh!
There was an error while loading. Please reload this page.