Skip to content

feat(core): MCP server instrumentation without breaking Miniflare #16817

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

Open
wants to merge 41 commits into
base: develop
Choose a base branch
from

Conversation

betegon
Copy link
Member

@betegon betegon commented Jul 4, 2025

Closes #16826, #16654, #16666

Different approach from #16807 .

Using Proxy was causing issues in cloudflare #16182.

Now using fill we shouldn't have those problems as fill doesn't create a new wrapper object with a different identity, so now:

  1. fill just replaces the method on the existing object
  2. The transport object keeps its original identity
  3. When transport.start() runs and accesses private fields, this is still the original transport object
  4. The WeakMap recognizes it as the same object that owns the private fields

What's inside

  • Support for new MCP SDK methods (mcpServerInstance.tool(), mcpServerInstance.resource(), etc.)
  • Tracing instrumentation
  • Error handling

Tracing

It follows OTEL semantic conventions for MCP and adds more attributes we thought are useful.

It also handles PII based on user setting of sendDefaultPii.

Tracing flow

  1. Transport receives tools/call request (id: 2)
  2. Create INACTIVE mcp.server span
  3. Store span inrequestIdToSpanMap[2] = { span, method: "tools/call", startTime }
  4. Execute handler with span context (handler gets requestId: 2)
  5. Handler finds span using requestId: 2
  6. Tool execution happens within span context
  7. Response sent with tool results
  8. completeSpanWithToolResults(2, result) enriches and completes span

Error handling

  1. error capture - errorCapture.ts
  • Non-invasive error reporting that never interferes with MCP service operation
  • Error context with MCP-specific metadata
  • PII filtering respects sendDefaultPii settings
  • Resilient to Sentry failures (wrapped in try-catch)
  1. Tool execution error capturing - handlers.ts
  • Captures exceptions thrown during tool execution
  • Preserves normal MCP behaviour (errors converted to isError: true)
  • Includes tool name, arguments, and request context
  • Handles both sync and async tool handlers
  1. Transport error instrumentation - transport.ts
  • Captures connection errors and network failures
  • Intercepts JSON-RPC error responses
  • Includes transport type and session information
  • Handles error responses being sent back to clients

betegon added 24 commits June 27, 2025 20:46
…ibute names to match OTEL draft semantic convention
@betegon betegon self-assigned this Jul 4, 2025
@betegon betegon requested a review from AbhiPrasad July 4, 2025 18:28
@betegon betegon changed the title Bete/mcp server semantic convention fill feat(core): MCP server instrumentation without breaking Miniflare Jul 4, 2025
Copy link
Member

@AbhiPrasad AbhiPrasad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice, all good from my eyes in overall direction!

@betegon
Copy link
Member Author

betegon commented Jul 7, 2025

Thanks @AbhiPrasad! Will keep working on this and close the other PR using Proxy. Will apply your suggestions here!

betegon added 17 commits July 8, 2025 14:04
…duration. adds configuration, extraction, and transport utilities. Introduce span creation functions and improves method wrapping for improved telemetry.
… files for attribute extraction, correlation, and handler wrapping, removing deprecated utilities and configuration files. Improve transport instrumentation for better telemetry and span handling.
…n handling functions. Update type definitions and separate method wrapping for transport handlers.
… to remove sensitive data based on the sendDefaultPii setting.
- Introduced new attributes for tool result content count and error status.
- Updated attribute extraction functions to utilize new constants for better maintainability.
- Added error capturing utilities to handle tool execution errors and transport errors gracefully.
@betegon betegon marked this pull request as ready for review July 14, 2025 16:17
Copy link

@cursor cursor bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: Incorrect `this` Context in Handler Calls

The this context is incorrectly handled when calling the originalHandler. The originalHandler.apply(originalHandler, handlerArgs) call sets this to the handler function itself, instead of preserving the original context from the wrapper. This also affects the fallback path in createWrappedHandler. The wrapper's this context must be explicitly passed to createErrorCapturingHandler to ensure the original MCP handler executes with its intended context.

packages/core/src/integrations/mcp-server/handlers.ts#L58-L61

): unknown {
try {
const result = originalHandler.apply(originalHandler, handlerArgs);

packages/core/src/integrations/mcp-server/handlers.ts#L42-L44

} catch (error) {
DEBUG_BUILD && logger.warn('MCP handler wrapping failed:', error);
return originalHandler.apply(this, handlerArgs);

Fix in CursorFix in Web


Bug: Transport Methods Not Instrumented After Initialization

The wrapTransportOnMessage, wrapTransportSend, wrapTransportOnClose, and wrapTransportError functions only instrument transport methods if they are defined at connect time. This fill-based approach fails to instrument methods set after initial connection, unlike the previous Proxy-based implementation which intercepted later assignments. As a result, lazily initialized transport handlers will not be instrumented.

packages/core/src/integrations/mcp-server/transport.ts#L17-L53

*/
export function wrapTransportOnMessage(transport: MCPTransport): void {
if (transport.onmessage) {
fill(transport, 'onmessage', originalOnMessage => {
return function (this: MCPTransport, jsonRpcMessage: unknown, extra?: unknown) {
if (isJsonRpcRequest(jsonRpcMessage)) {
const messageTyped = jsonRpcMessage as { method: string; id: string | number };
// Create isolation scope for this request (standard Sentry pattern)
const isolationScope = getIsolationScope().clone();
return withIsolationScope(isolationScope, () => {
// Create manual span that stays open until response
const spanConfig = buildMcpServerSpanConfig(jsonRpcMessage, this, extra as ExtraHandlerData);
const span = startInactiveSpan(spanConfig);
// Store span context for handler correlation using requestId
storeSpanForRequest(messageTyped.id, span, messageTyped.method);
// Execute handler within span context
return withActiveSpan(span, () => {
return (originalOnMessage as (...args: unknown[]) => unknown).call(this, jsonRpcMessage, extra);
});
});
}
if (isJsonRpcNotification(jsonRpcMessage)) {
return createMcpNotificationSpan(jsonRpcMessage, this, extra as ExtraHandlerData, () => {
return (originalOnMessage as (...args: unknown[]) => unknown).call(this, jsonRpcMessage, extra);
});
}
return (originalOnMessage as (...args: unknown[]) => unknown).call(this, jsonRpcMessage, extra);
};
});
}
}

Fix in CursorFix in Web


Was this report helpful? Give feedback by reacting with 👍 or 👎

Copy link
Member

@AbhiPrasad AbhiPrasad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1st pass.

Before I review further, I'd like to see the develop docs PR so that I can make sure all the attributes are being added as we expect. Right now it's a bit hard to know if an attribute is missing or not.

I assume much of this is AI generated, which is fine, but let's make sure we clean the comments up (and expand the jsdoc string appropriately).

@@ -0,0 +1,166 @@
/**
* types for MCP server instrumentation
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are these vendored in from the mcp package? If so we need to put the mcp library license + sha we grabbed the types from in this file.

if (isJsonRpcRequest(jsonRpcMessage)) {
const messageTyped = jsonRpcMessage as { method: string; id: string | number };

// Create isolation scope for this request (standard Sentry pattern)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

m: Instead of inline comments like this, I would prefer if we added notes in the jsdoc to document behaviour. Right now this comment is a bit redundant with the code.

Ditto with other instances of this.


// Use Object.fromEntries with filter for a more functional approach
return Object.fromEntries(
Object.entries(spanData).filter(([key]) => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

l: I would prefer if we called reduce on Object.entries(spanData) to do this operation instead of calling filter + Object.fromEntries on the constructed array.


captureException(error, {
tags: {
mcp_error_type: errorType || 'handler_execution',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The sdk should not be setting tags by default. Tags are meant to be only set by users.

What we can instead do is set a mechanism: https://develop.sentry.dev/sdk/data-model/event-payloads/exception/#exception-mechanism.

*
* Compatible with versions `^1.9.0` of the `@modelcontextprotocol/sdk` package.
*/
export function wrapMcpServerWithSentry<S extends object>(mcpServerInstance: S): S {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's add an example usage snippet to the jsdoc here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

MCP Server tracing instrumentation
2 participants