Conversation
Contributor
size-limit report 📦
|
mydea
added this pull request to stack #24534
September 21, 2026 08:30
mydea
force-pushed
the
feat/mcp-server-auto-instrumentation-e2e
branch
2 times, most recently
from
September 21, 2026 08:58
ea8d457 to
59cc528
Compare
mydea
changed the title
mydea
marked this pull request as ready for review
September 21, 2026 09:05
betegon
reviewed
Sep 21, 2026
betegon
left a comment
betegon
left a comment
Member
There was a problem hiding this comment.
great one!
could we keep the existing legacy coverage and add a no-wrapper HTTP scenario pinned to protocol 2026-07-28, using createMcpHandler?
The current SDK 2 scenario still uses the legacy initialization/session path (there's no initialization in v2), so it doesn't verify auto-instrumentation through the v2 entry point
Contributor
|
👋 @nicohrubec, @s1gr1d — Please review this PR when you get a chance! |
JPeer264
approved these changes
Sep 24, 2026
mydea
force-pushed
the
feat/mcp-server-auto-instrumentation-e2e
branch
from
September 25, 2026 11:51
59cc528 to
d4eaf18
Compare
s1gr1d
approved these changes
Sep 25, 2026
Comment on lines
-10
to
+14
| const server = wrapMcpServerWithSentry( | ||
| new McpServer({ | ||
| name: 'Echo-V2', | ||
| version: '2.0.0', | ||
| }), | ||
| ); | ||
| // Intentionally NOT wrapped with `wrapMcpServerWithSentry`: the `mcpServer` integration | ||
| // auto-instruments the `McpServer` constructor, so spans must be produced anyway. | ||
| const server = new McpServer({ | ||
| name: 'Echo-V2', | ||
| version: '2.0.0', | ||
| }); |
Member
There was a problem hiding this comment.
Is this pr still just test, even though there are actual changes? I'll let you decide
Member
There was a problem hiding this comment.
I think this is just changed based on the previous PR in the stack
Add two Express e2e apps that construct an `McpServer` WITHOUT calling `wrapMcpServerWithSentry`, exercising the `mcpServer` integration's auto-wrapping over a real streamable-HTTP transport: - `node-express-mcp-v1-auto` (`@modelcontextprotocol/sdk` v1) - `node-express-mcp-v2-auto` (`@modelcontextprotocol/server` v2) Both assert the expected `mcp.server` spans (initialize, tool call, resource read, error status) still appear without a manual wrap. Bun/Deno/Cloudflare variants are intentionally left as a follow-up. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Drop the `-auto` suffix now that auto-instrumentation is the default path: - `node-express-mcp-v1-auto` -> `node-express-mcp-v1` - `node-express-mcp-v2-auto` -> `node-express-mcp-v2`, replacing the previous manual-wrap app of that name (same assertions, now exercising the default auto path). Manual-wrap coverage remains via the cloudflare-mcp apps and the v1 `node-express*` apps. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
mydea
force-pushed
the
feat/mcp-server-auto-instrumentation-e2e
branch
from
September 28, 2026 07:59
d4eaf18 to
c029964
Compare
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
Comment on lines
+9
to
+14
| // Intentionally NOT wrapped with `wrapMcpServerWithSentry`: the `mcpServer` integration | ||
| // auto-instruments the `McpServer` constructor, so spans must be produced anyway. | ||
| const server = new McpServer({ | ||
| name: 'Echo-V2', | ||
| version: '2.0.0', | ||
| }); |
Contributor
There was a problem hiding this comment.
Bug: The E2E test's custom instrumentation script (instrument.mjs) calls Sentry.init() but fails to register the orchestrion runtime hook, preventing auto-instrumentation and causing the test to fail.
Severity: MEDIUM
Suggested Fix
Modify the test application's startup command to correctly register the orchestrion runtime hook. Instead of using a custom instrument.mjs file, the package.json should be updated to use --import @sentry/node/import. This ensures the hook is active before the application code runs, allowing mcpServerIntegration to function as expected.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: dev-packages/e2e-tests/test-applications/node-express-mcp-v2/src/mcp.ts#L9-L14
Potential issue: The E2E test application `node-express-mcp-v2` is configured to run
with a custom entry point (`--import ./instrument.mjs`) that calls `Sentry.init()`.
However, calling `Sentry.init()` alone does not register the orchestrion runtime hook,
which is required for the `mcpServerIntegration` to perform auto-instrumentation. The
hook is typically registered by using `--import @sentry/node/import`. Without the hook,
the `@modelcontextprotocol/server` module will not be instrumented, no spans will be
produced, and the test will fail because it explicitly waits for and asserts the
presence of these spans.
Did we get this right? 👍 / 👎 to inform future reviews.
The MCP server suites were restructured into `mcp-server/manual-instrumentation`, `mcp-server/v1`, and `mcp-server/v2`, where v1/v2 rely on orchestrion auto-instrumentation. `bun run` cannot inject the diagnostics channels, so those suites produce no spans and fail. Replace the stale `mcp-server-streamed` exclude with a `mcp-server/**` glob covering all three. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
mydea
requested review from
andreiborza and
isaacs
and removed request for
a team
September 28, 2026 08:34
…egration The default `mcpServer` integration pushed the main `@sentry/node` entry to 141.21 KB, over its 139 KB limit. Bump to 142 KB. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stacked on #24529.
Adds two Express e2e apps that construct an
McpServerwithout callingwrapMcpServerWithSentry, so themcpServerintegration from #24529 is the only thing producing spans — over a real streamable-HTTP transport rather than the in-memory one used by the node-integration-tests:node-express-mcp-v1—@modelcontextprotocol/sdk(v1)node-express-mcp-v2—@modelcontextprotocol/server(v2)Each asserts the expected
mcp.serverspans (initialize, tool call, resource read, error status) still appear with no manual wrap. Express is the existing convention for the sibling MCP e2e apps; MCP itself doesn't require it.node-express-mcp-v2replaces the previous manual-wrap app of that name — now that auto-instrumentation is the default path, these apps drop the interim-autosuffix and exercise the default. Manual-wrap e2e coverage remains viacloudflare-mcp(v2),cloudflare-mcp-agent(v1), and the v1node-express*apps.Bun/Deno/Cloudflare variants (hono-4 multi-entry style) are intentionally deferred to a follow-up — this PR keeps the auto-instrumentation e2e coverage node-only.
🤖 Generated with Claude Code