From cc96f207f8d13a2bac24269d1ea5b30172e9c07b Mon Sep 17 00:00:00 2001 From: Clayton Date: Wed, 15 Jul 2026 23:13:10 -0500 Subject: [PATCH] feat(mcp): instrument the remote tool-dispatch chokepoint with PostHog events handleMcpRequest is the single point every remote MCP tool invocation passes through, so it now calls the #6235 PostHog wrapper once per tools/call request with the tool name, callerType "remote", success/ failure, and coarse latency -- reusing the same startedAt/toolName signals the existing product-usage telemetry already derives here. The call is wrapped in its own try/catch so a telemetry failure can never affect the tool response, on top of the wrapper's own no-op guarantee. Closes #6237 --- src/mcp/server.ts | 19 +++++++ test/unit/mcp-server-telemetry.test.ts | 75 ++++++++++++++++++++++++++ 2 files changed, 94 insertions(+) diff --git a/src/mcp/server.ts b/src/mcp/server.ts index b03b03d302..ab0133605e 100644 --- a/src/mcp/server.ts +++ b/src/mcp/server.ts @@ -20,6 +20,7 @@ import { runIssueRagRetrieval, validateIssueRagInput, } from "./issue-rag"; +import { recordMcpToolCall } from "./telemetry"; import { authenticatePrivateToken, extractBearerToken, @@ -1631,6 +1632,9 @@ export async function handleMcpRequest(c: AppContext): Promise { const server = new LoopoverMcp(c.env, identity).createServer(); try { const response = await createMcpHandler(server, { route: "/mcp", enableJsonResponse: true })(c.req.raw, c.env, getExecutionContext(c)); + if (typeof usageMetadata.toolName === "string") { + recordMcpToolTelemetry(c.env, usageMetadata.toolName, response.status < 400, Date.now() - startedAt); + } await recordProductUsageEvent(c.env, { surface: "mcp", role: "miner", @@ -1646,6 +1650,9 @@ export async function handleMcpRequest(c: AppContext): Promise { }).catch(() => undefined); return response; } catch (error) { + if (typeof usageMetadata.toolName === "string") { + recordMcpToolTelemetry(c.env, usageMetadata.toolName, false, Date.now() - startedAt); + } await recordProductUsageEvent(c.env, { surface: "mcp", role: "miner", @@ -1663,6 +1670,18 @@ export async function handleMcpRequest(c: AppContext): Promise { } } +// Single chokepoint for the #6228 PostHog tool-call telemetry (#6237): every `tools/call` request that +// reaches handleMcpRequest routes through here exactly once, whether it succeeds or throws. Pure +// observability -- never lets a telemetry failure reach the caller, matching recordMcpToolCall's own +// no-op guarantee (#6235) with a second, defensive layer at the actual call site. +function recordMcpToolTelemetry(env: Env, tool: string, ok: boolean, durationMs: number): void { + try { + recordMcpToolCall(env, { tool, callerType: "remote", ok, durationMs }); + } catch { + // Telemetry must never affect the tool response (#6237). + } +} + async function describeMcpUsageRequest(request: Request, telemetryMetadata: Record | undefined): Promise> { const body = await request.clone().json().catch(() => null); if (!body || typeof body !== "object") return { transport: "http", method: request.method, ...telemetryMetadata }; diff --git a/test/unit/mcp-server-telemetry.test.ts b/test/unit/mcp-server-telemetry.test.ts index ab2a5a4638..db5f8b8b2b 100644 --- a/test/unit/mcp-server-telemetry.test.ts +++ b/test/unit/mcp-server-telemetry.test.ts @@ -6,6 +6,7 @@ import { createTestEnv } from "../helpers/d1"; describe("MCP server telemetry", () => { afterEach(() => { vi.doUnmock("agents/mcp"); + vi.doUnmock("../../src/mcp/telemetry"); vi.resetModules(); }); @@ -147,4 +148,78 @@ describe("MCP server telemetry", () => { }), ]); }); + + it("records exactly one recordMcpToolCall, tagged callerType remote, on a successful tool invocation (#6237)", async () => { + vi.resetModules(); + vi.doMock("agents/mcp", () => ({ + createMcpHandler: () => () => Response.json({ ok: true }), + })); + const recordMcpToolCall = vi.fn(); + vi.doMock("../../src/mcp/telemetry", () => ({ recordMcpToolCall })); + const { handleMcpRequest } = await import("../../src/mcp/server"); + const env = createTestEnv({ PRODUCT_USAGE_HASH_SALT: "mcp-remote-telemetry-salt" }); + const request = new Request("https://api.test/mcp", { + method: "POST", + headers: { + authorization: `Bearer ${env.LOOPOVER_MCP_TOKEN}`, + "content-type": "application/json", + }, + body: JSON.stringify({ jsonrpc: "2.0", id: "tool-call", method: "tools/call", params: { name: "loopover_local_status" } }), + }); + + await expect( + handleMcpRequest({ + env, + executionCtx: { waitUntil() {}, passThroughOnException() {} }, + req: { + method: "POST", + raw: request, + header: (name: string) => request.headers.get(name) ?? undefined, + }, + json: (body: unknown, status?: number) => Response.json(body, status === undefined ? undefined : { status }), + } as never), + ).resolves.toMatchObject({ status: 200 }); + + expect(recordMcpToolCall).toHaveBeenCalledTimes(1); + expect(recordMcpToolCall).toHaveBeenCalledWith( + env, + expect.objectContaining({ tool: "loopover_local_status", callerType: "remote", ok: true, durationMs: expect.any(Number) }), + ); + }); + + it("does not let a throwing recordMcpToolCall affect the tool response (#6237)", async () => { + vi.resetModules(); + vi.doMock("agents/mcp", () => ({ + createMcpHandler: () => () => Response.json({ ok: true, result: "unchanged" }), + })); + vi.doMock("../../src/mcp/telemetry", () => ({ + recordMcpToolCall: () => { + throw new Error("posthog_unreachable"); + }, + })); + const { handleMcpRequest } = await import("../../src/mcp/server"); + const env = createTestEnv({ PRODUCT_USAGE_HASH_SALT: "mcp-remote-telemetry-throws-salt" }); + const request = new Request("https://api.test/mcp", { + method: "POST", + headers: { + authorization: `Bearer ${env.LOOPOVER_MCP_TOKEN}`, + "content-type": "application/json", + }, + body: JSON.stringify({ jsonrpc: "2.0", id: "tool-call", method: "tools/call", params: { name: "loopover_local_status" } }), + }); + + const response = await handleMcpRequest({ + env, + executionCtx: { waitUntil() {}, passThroughOnException() {} }, + req: { + method: "POST", + raw: request, + header: (name: string) => request.headers.get(name) ?? undefined, + }, + json: (body: unknown, status?: number) => Response.json(body, status === undefined ? undefined : { status }), + } as never); + + expect(response.status).toBe(200); + await expect(response.clone().json()).resolves.toEqual({ ok: true, result: "unchanged" }); + }); });