Conversation
|
@MathurAditya724 is attempting to deploy a commit to the Sentry Team on Vercel. A member of the Team first needs to authorize it. |
BYK
left a comment
There was a problem hiding this comment.
Good start!
We'd also need some tests, even if they are cursory like just making sure we can connect to the /mcp endpoint. Happy to help with that once we are further down the road with this PR.
BYK
left a comment
There was a problem hiding this comment.
Looking much better. I think we can clear out some of my questions in a meeting and see how we can get this merged ASAP.
packages/sidecar/src/mcp/index.ts
Outdated
| import { MessageBuffer } from "./messageBuffer.js"; | ||
| import type { Payload } from "./utils.js"; | ||
| import type { TextContent } from "@modelcontextprotocol/sdk/types.js"; | ||
| import packageJson from "../../package.json" with { type: "json" }; |
There was a problem hiding this comment.
Cannot do this due to fossilized binaries.
is a way (which requires embedding that value during the build process)| .map(([contentType, data]) => processEnvelope({ contentType, data })) | ||
| .sort((a, b) => { |
There was a problem hiding this comment.
We can probably move that event.type === "event" check here and save some memory and CPU time. It wold also allow us to do
const content = errors.map(payload => ({...}));which should also be more efficient than someArray.push().
There was a problem hiding this comment.
i think instead of event.type === "event" we should check something like this
because we only need error events here.
and why not check this in the errorBuffer itself?
| @@ -0,0 +1,66 @@ | |||
| type RawEventContext = { | |||
There was a problem hiding this comment.
Might be good to add a top-level comment about where this code came from and our future plans of splitting this out.
packages/sidecar/src/main.ts
Outdated
| return; | ||
| } | ||
| return handler(req, res, pathname, searchParams); | ||
| return (await handler)(req, res, pathname, searchParams); |
There was a problem hiding this comment.
I do not like awaiting a handler every single time we get a request. It adds at least one tick to everything. Can we find a better way to do this? I'm happy doing await mcp.connect() at the start of the server and delay that once rather than doing this.
| @@ -68,10 +68,16 @@ export class MessageBuffer<T> { | |||
| } | |||
|
|
|||
| read(): T[] { | |||
There was a problem hiding this comment.
This does something very similar to .stream() method above so we should align them or make them share some code. Also it might be wise to make start an argument but defaulting to this.head for different clients being at different places?
| } | ||
|
|
||
| // MCP Setup | ||
| const transport = new StreamableHTTPServerTransport({ |
There was a problem hiding this comment.
I think we should not create an MCP server instance every time. Instead, we should move the initialization logic to handleMCPRequest.
also in handleMCPRequest, we should initiate MCP server only in first API call
something like-
let mcpServerInstance: McpServer;
let mcpTransport: StreamableHTTPServerTransport;
async function handleMcpRequest(): Promise<RequestHandler> {
if (!mcpServerInstance) {
mcpTransport = new StreamableHTTPServerTransport({
sessionIdGenerator: undefined,
});
mcpServerInstance = createMcpInstance(buffer);
await mcpServerInstance.connect(mcpTransport);
}
return (req, res) => mcpTransport.handleRequest(req, res);
}
however i think this approach also can cause an issue with concurrent requests - if multiple requests arrive simultaneously before initialization is complete, they could all try to initialize the server at the same time.
BYK
left a comment
There was a problem hiding this comment.
This addresses all my blockers so unblocking. Great work!
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
There was a problem hiding this comment.
Pull Request Overview
This PR adds Model Context Protocol (MCP) support to the Spotlight sidecar, enabling AI assistants to fetch recent error data for debugging purposes.
- Implements MCP server with error retrieval tools
- Adds new HTTP endpoint
/mcpfor MCP communication - Extracts shared types and adds envelope parsing utilities
Reviewed Changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/sidecar/tsdown.config.ts | Removes external dependencies configuration from build config |
| packages/sidecar/src/utils.ts | Defines shared Payload type |
| packages/sidecar/src/messageBuffer.ts | Adds read() method to MessageBuffer class |
| packages/sidecar/src/mcp/parsing.ts | Implements Sentry envelope parsing utilities |
| packages/sidecar/src/mcp/index.ts | Creates MCP server instance with error retrieval tool |
| packages/sidecar/src/main.ts | Integrates MCP server and adds /mcp endpoint |
| packages/sidecar/package.json | Adds MCP SDK dependencies and fixes export paths |
| // @ts-expect-error ts(2339) -- We should really stop adding type to payloads | ||
| itemPayload.type = itemHeader.type; |
There was a problem hiding this comment.
The @ts-expect-error comment indicates a known type safety issue. Consider defining proper types for itemPayload or using a type assertion instead of suppressing the error.
| // @ts-expect-error ts(2339) -- We should really stop adding type to payloads | |
| itemPayload.type = itemHeader.type; | |
| (itemPayload as Record<string, unknown>).type = itemHeader.type; |
| export function parseJSONFromBuffer<T = unknown>(data: Uint8Array): T { | ||
| try { | ||
| return JSON.parse(new TextDecoder().decode(data)) as T; | ||
| } catch (err) { | ||
| console.error(err); | ||
| return {} as T; |
There was a problem hiding this comment.
Returning an empty object when JSON parsing fails could cause runtime errors if the caller expects specific properties. Consider returning null or throwing a more specific error instead of masking the failure.
| export function parseJSONFromBuffer<T = unknown>(data: Uint8Array): T { | |
| try { | |
| return JSON.parse(new TextDecoder().decode(data)) as T; | |
| } catch (err) { | |
| console.error(err); | |
| return {} as T; | |
| export function parseJSONFromBuffer<T = unknown>(data: Uint8Array): T | null { | |
| try { | |
| return JSON.parse(new TextDecoder().decode(data)) as T; | |
| } catch (err) { | |
| console.error(err); | |
| return null; |
| "Fetches the most recent errors from Spotlight debugger. Returns error details, stack traces, and request details for immediate debugging context.", | ||
| async () => { | ||
| const envelopes = errorsBuffer.read(); | ||
| errorsBuffer.clear(); |
There was a problem hiding this comment.
Clearing the errors buffer after reading means subsequent calls to get_errors will return empty results until new errors occur. This could lead to data loss if multiple consumers try to access errors or if the tool is called multiple times.
| errorsBuffer.clear(); |
| for (let i = start; i < end; i++) { | ||
| const item = this.items[i % this.size]; | ||
| if (item !== undefined) { | ||
| result.push(item[1]); |
There was a problem hiding this comment.
The read() method accesses item[1] but the MessageBuffer stores items of type T directly, not as tuples. This will cause a runtime error since T items don't have an index [1].
No description provided.