Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 15 additions & 2 deletions src/api/worker-posthog.ts
Original file line number Diff line number Diff line change
Expand Up @@ -99,16 +99,29 @@ export interface WorkerErrorRequestContext {
/** Capture one exception from the hosted Worker path. Never throws -- a PostHog init/capture/flush failure
* degrades to recording nothing, matching every other capture* function in this codebase's identical
* best-effort guarantee. Resolves once the event has actually been flushed (or given up on), so the caller
* should schedule this via ctx.waitUntil rather than await it inline on the request's hot path. */
export async function capturePostHogWorkerError(env: WorkerPostHogEnv, error: unknown, context: WorkerErrorRequestContext): Promise<void> {
* should schedule this via ctx.waitUntil rather than await it inline on the request's hot path.
*
* `extraProperties` lets a non-HTTP caller (the MCP dispatch sink, #10037) attach its own grouping
* properties (e.g. `mcp_tool`/`error_code`) alongside the fixed `environment`/`request_path`/
* `request_method` shape -- scrubbed the same way those are, then merged in. Omitting it leaves
* `createWorkerPostHogErrorMiddleware`'s HTTP callers unaffected. */
export async function capturePostHogWorkerError(
env: WorkerPostHogEnv,
error: unknown,
context: WorkerErrorRequestContext,
extraProperties?: Record<string, unknown>,
): Promise<void> {
try {
const client = await buildClient(env);
if (!client) return;
const err = error instanceof Error ? error : new Error(String(error));
const scrubbedExtra = extraProperties ? { ...extraProperties } : undefined;
if (scrubbedExtra) scrubRecord(scrubbedExtra, 0);
client.captureException(err, WORKER_ERROR_DISTINCT_ID, {
environment: trimmedOrUndefined(env.WORKER_POSTHOG_ENVIRONMENT) ?? "production",
request_path: scrubString(context.path),
request_method: context.method,
...scrubbedExtra,
});
await client.flush();
} catch {
Expand Down
5 changes: 4 additions & 1 deletion src/mcp/dispatch-telemetry-sink.ts
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,10 @@ export function createDispatchTelemetrySink(
if (!isWorkerPostHogConfigured(env)) return;
// `mcp_tool` + `error_code` are the grouping properties: an exception dashboard broken down by
// tool and cause is the thing an operator can act on, unlike a stack-only view.
defer(capturePostHogWorkerError(env, error, { path: `mcp.tool/${call.tool}`, method: call.errorCode ?? "unknown_error" }));
const errorCode = call.errorCode ?? "unknown_error";
defer(
capturePostHogWorkerError(env, error, { path: `mcp.tool/${call.tool}`, method: errorCode }, { mcp_tool: call.tool, error_code: errorCode }),
);
},
// The registry is consulted per call rather than captured at construction so a self-host boot
// that fills the slot after the first request still traces.
Expand Down
54 changes: 54 additions & 0 deletions test/unit/mcp-dispatch-telemetry-sink.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -138,6 +138,60 @@ describe("MCP dispatch telemetry sink (#9525)", () => {
});
});

// The exception properties tests below mock posthog-node so they can read the properties the sink
// actually hands to captureException, instead of only observing whether the deferred promise resolves.
describe("MCP dispatch telemetry sink exception properties (#10037)", () => {
afterEach(() => {
vi.doUnmock("posthog-node");
vi.resetModules();
});

it("attaches mcp_tool and error_code to the captured exception, matching the stdio/miner sinks", async () => {
vi.resetModules();
const captureException = vi.fn();
const flush = vi.fn().mockResolvedValue(undefined);
vi.doMock("posthog-node", () => ({
PostHog: vi.fn(function (this: { captureException: typeof captureException; flush: typeof flush }) {
this.captureException = captureException;
this.flush = flush;
}),
}));
const { createDispatchTelemetrySink: freshCreateDispatchTelemetrySink } = await import("../../src/mcp/dispatch-telemetry-sink");
const deferred: Promise<unknown>[] = [];
const sink = freshCreateDispatchTelemetrySink(env({ WORKER_POSTHOG_API_KEY: "phc_worker" }), (work) => deferred.push(work));
const forbiddenCall: McpToolCallTelemetry = { ...call, ok: false, errorCode: "forbidden" };

sink.captureException(new Error("boom"), forbiddenCall);
expect(deferred).toHaveLength(1);
await deferred[0];

const properties = captureException.mock.calls.at(-1)?.[2] as Record<string, unknown>;
expect(properties).toMatchObject({ mcp_tool: forbiddenCall.tool, error_code: "forbidden" });
});

it("defaults error_code to unknown_error when the call carries none", async () => {
vi.resetModules();
const captureException = vi.fn();
const flush = vi.fn().mockResolvedValue(undefined);
vi.doMock("posthog-node", () => ({
PostHog: vi.fn(function (this: { captureException: typeof captureException; flush: typeof flush }) {
this.captureException = captureException;
this.flush = flush;
}),
}));
const { createDispatchTelemetrySink: freshCreateDispatchTelemetrySink } = await import("../../src/mcp/dispatch-telemetry-sink");
const deferred: Promise<unknown>[] = [];
const sink = freshCreateDispatchTelemetrySink(env({ WORKER_POSTHOG_API_KEY: "phc_worker" }), (work) => deferred.push(work));
const noCodeCall: McpToolCallTelemetry = { ...call, ok: false };

sink.captureException(new Error("boom"), noCodeCall);
await deferred[0];

const properties = captureException.mock.calls.at(-1)?.[2] as Record<string, unknown>;
expect(properties).toMatchObject({ mcp_tool: noCodeCall.tool, error_code: "unknown_error" });
});
});

describe("LoopoverMcp telemetry-sink injection (#9525)", () => {
it("routes a real tool call through the injected sink", async () => {
const { Client } = await import("@modelcontextprotocol/sdk/client/index.js");
Expand Down
34 changes: 34 additions & 0 deletions test/unit/worker-posthog.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -149,6 +149,30 @@ describe("capturePostHogWorkerError", () => {
expect(flushed).toBe(true);
});

it("merges extraProperties into the captured exception alongside the fixed request shape (#10037)", async () => {
await capturePostHogWorkerError({ WORKER_POSTHOG_API_KEY: "phc_test" } as WorkerPostHogEnv, new Error("boom"), { path: "mcp.tool/loopover_x", method: "forbidden" }, {
mcp_tool: "loopover_x",
error_code: "forbidden",
});
const properties = mocks.captureException.mock.calls.at(-1)?.[2] as Record<string, unknown>;
expect(properties).toMatchObject({
environment: "production",
request_path: "mcp.tool/loopover_x",
request_method: "forbidden",
mcp_tool: "loopover_x",
error_code: "forbidden",
});
});

it("scrubs a secret-shaped extraProperties value the same way request_path is scrubbed (#10037)", async () => {
await capturePostHogWorkerError({ WORKER_POSTHOG_API_KEY: "phc_test" } as WorkerPostHogEnv, new Error("boom"), { path: "/x", method: "GET" }, {
leaked_token: `${"github" + "_pat_"}${"a".repeat(24)}`,
});
const properties = mocks.captureException.mock.calls.at(-1)?.[2] as Record<string, unknown>;
expect(properties.leaked_token).not.toContain("github_pat_");
expect(properties.leaked_token).toContain("[redacted]");
});

it("never throws when the PostHog client construction fails", async () => {
mocks.PostHog.mockImplementationOnce(() => {
throw new Error("client construction failed");
Expand Down Expand Up @@ -223,6 +247,16 @@ describe("createWorkerPostHogErrorMiddleware", () => {
expect((properties as Record<string, unknown>).request_method).toBe("GET");
});

it("still carries only request_path/request_method, never mcp_tool, when no extraProperties argument is passed (#10037)", async () => {
const { app, executionCtx, getWaited } = buildTestApp();
const res = await app.fetch(new Request("https://loopover.test/boom"), { WORKER_POSTHOG_API_KEY: "phc_test" } as WorkerPostHogEnv, executionCtx);
expect(res.status).toBe(500);
await getWaited();
const properties = mocks.captureException.mock.calls.at(-1)?.[2] as Record<string, unknown>;
expect(properties).toMatchObject({ request_path: "/boom", request_method: "GET" });
expect(properties).not.toHaveProperty("mcp_tool");
});

it("falls back to a no-op executionCtx when c.executionCtx throws (self-host calling the same Worker fetch handler outside a real isolate)", async () => {
const app = new Hono<{ Bindings: WorkerPostHogEnv }>();
app.use(createWorkerPostHogErrorMiddleware());
Expand Down