From 49a365e5b75b8091b533833dd47ce97166ea0475 Mon Sep 17 00:00:00 2001 From: Savio Dias Date: Mon, 21 Sep 2026 20:07:13 +0530 Subject: [PATCH 1/8] feat(lib): emit a per-call MCPToolCompleted event with duration and outcome Every tool today writes its MCPInstrumentation row at entry, before any work happens, so nothing in telemetry carries a duration and a tool that returns `isError: true` without throwing counts as a success. Add one wrapper in server-factory around every registered tool's handler. It times the call and emits a separate `MCPToolCompleted` event when the handler settles, with `duration_ms` and `outcome` (ok / error_result / threw). The existing entry and catch rows are untouched, and the new event uses its own event_type so no query or dashboard keyed on MCPInstrumentation changes. The wrapper is transparent (result passed through, throws rethrown), idempotent (a second call on the same map is a no-op), skips task-style handlers, and never lets a telemetry failure affect the tool call. Handler is assigned directly rather than via tool.update() to avoid a tools/list_changed notification at registration time. Exported as `instrumentToolLatency` so the remote wrapper can apply the same timing to the tools it registers itself (uploadAsset, TFA plugin). Refs AIMCP-225 Co-Authored-By: Claude Fable 5.1 --- src/index.ts | 3 +- src/lib/instrumentation.ts | 106 ++++++++++++++----- src/lib/tool-latency.ts | 87 ++++++++++++++++ src/server-factory.ts | 11 ++ tests/lib/instrumentation-completed.test.ts | 65 ++++++++++++ tests/lib/tool-latency.test.ts | 108 ++++++++++++++++++++ 6 files changed, 354 insertions(+), 26 deletions(-) create mode 100644 src/lib/tool-latency.ts create mode 100644 tests/lib/instrumentation-completed.test.ts create mode 100644 tests/lib/tool-latency.test.ts diff --git a/src/index.ts b/src/index.ts index 5d020007..31c91e6d 100644 --- a/src/index.ts +++ b/src/index.ts @@ -53,6 +53,7 @@ process.on("exit", () => { export { setLogger } from "./logger.js"; export { BrowserStackMcpServer } from "./server-factory.js"; -export { trackMCP } from "./lib/instrumentation.js"; +export { trackMCP, trackMCPCompleted } from "./lib/instrumentation.js"; +export { instrumentToolLatency } from "./lib/tool-latency.js"; export { default as addTfaRcaCollaborationTools } from "./tools/tfa-rca-collaboration.js"; export const PackageJsonVersion = packageJson.version; diff --git a/src/lib/instrumentation.ts b/src/lib/instrumentation.ts index 51027e51..85d0f27f 100644 --- a/src/lib/instrumentation.ts +++ b/src/lib/instrumentation.ts @@ -6,6 +6,13 @@ const packageJson = require("../../package.json"); import { apiClient } from "./apiClient.js"; import globalConfig from "../config.js"; +const INSTRUMENTATION_ENDPOINT = "https://api.browserstack.com/sdk/v1/event"; + +export type ClientInfo = { name?: string; version?: string }; + +/** How a tool call ended, as seen by the completion wrapper. */ +export type ToolOutcome = "ok" | "error_result" | "threw"; + interface MCPEventPayload { event_type: string; event_properties: { @@ -17,18 +24,55 @@ interface MCPEventPayload { error_message?: string; error_type?: string; is_remote?: boolean; + duration_ms?: number; + outcome?: ToolOutcome; + }; +} + +function baseProperties(toolName: string, clientInfo: ClientInfo) { + return { + mcp_version: packageJson.version as string, + tool_name: toolName, + mcp_client: clientInfo?.name || "unknown", + node_version: process.versions.node, + is_remote: globalConfig.REMOTE_MCP, }; } +/** Fire-and-forget POST. Never throws, never delays the caller. */ +function sendEvent(event: MCPEventPayload, config?: any): void { + let authHeader: string | undefined; + if (config) { + const authString = getBrowserStackAuth(config); + authHeader = `Basic ${Buffer.from(authString).toString("base64")}`; + } + + apiClient + .post({ + url: INSTRUMENTATION_ENDPOINT, + body: event, + headers: { + "Content-Type": "application/json", + ...(authHeader ? { Authorization: authHeader } : {}), + }, + timeout: 2000, + raise_error: false, + }) + .catch(() => {}); +} + +/** + * The per-invocation event. Fired at tool entry with `success: true` (meaning + * "invoked"), and again from the catch block with `success: false` when the + * handler throws. A failing call therefore produces two rows. + */ export function trackMCP( toolName: string, - clientInfo: { name?: string; version?: string }, + clientInfo: ClientInfo, error?: unknown, config?: any, ): void { - const instrumentationEndpoint = "https://api.browserstack.com/sdk/v1/event"; const isSuccess = !error; - const mcpClient = clientInfo?.name || "unknown"; // Log client information if (clientInfo?.name) { @@ -42,12 +86,8 @@ export function trackMCP( const event: MCPEventPayload = { event_type: "MCPInstrumentation", event_properties: { - mcp_version: packageJson.version, - tool_name: toolName, - mcp_client: mcpClient, - node_version: process.versions.node, + ...baseProperties(toolName, clientInfo), success: isSuccess, - is_remote: globalConfig.REMOTE_MCP, }, }; @@ -59,22 +99,38 @@ export function trackMCP( error instanceof Error ? error.constructor.name : "Unknown"; } - let authHeader = undefined; - if (config) { - const authString = getBrowserStackAuth(config); - authHeader = `Basic ${Buffer.from(authString).toString("base64")}`; - } + sendEvent(event, config); +} - apiClient - .post({ - url: instrumentationEndpoint, - body: event, - headers: { - "Content-Type": "application/json", - ...(authHeader ? { Authorization: authHeader } : {}), - }, - timeout: 2000, - raise_error: false, - }) - .catch(() => {}); +/** + * The per-completion event: one row per tool call, written AFTER the handler + * settles, carrying wall-clock duration and how it ended. + * + * Deliberately a separate `event_type` from `MCPInstrumentation`, so every + * existing query and dashboard keyed on that name keeps its row counts. + * + * outcome = "ok" handler returned a result without `isError` + * outcome = "error_result" handler returned `{ isError: true }` (a failure the + * entry/catch rows never see today) + * outcome = "threw" handler threw; the catch row also exists + * + * A call with an entry row and no completion row was killed before it finished + * (client closed the IDE, process exit), which is the closest thing to a + * timeout signal this event can give. + */ +export function trackMCPCompleted( + toolName: string, + clientInfo: ClientInfo, + completion: { durationMs: number; outcome: ToolOutcome }, + config?: any, +): void { + const event: MCPEventPayload = { + event_type: "MCPToolCompleted", + event_properties: { + ...baseProperties(toolName, clientInfo), + duration_ms: Math.max(0, Math.round(completion.durationMs)), + outcome: completion.outcome, + }, + }; + sendEvent(event, config); } diff --git a/src/lib/tool-latency.ts b/src/lib/tool-latency.ts new file mode 100644 index 00000000..2923e385 --- /dev/null +++ b/src/lib/tool-latency.ts @@ -0,0 +1,87 @@ +import { RegisteredTool } from "@modelcontextprotocol/sdk/server/mcp.js"; +import { + ClientInfo, + ToolOutcome, + trackMCPCompleted, +} from "./instrumentation.js"; + +/** + * Marks a handler we have already wrapped, so calling `instrumentToolLatency` + * twice on the same tool map (library + remote wrapper) emits one event, not two. + */ +const WRAPPED = Symbol.for("browserstack.mcp.latencyWrapped"); + +type AnyHandler = (...args: unknown[]) => unknown; + +function outcomeOf(result: unknown): ToolOutcome { + const isError = + typeof result === "object" && + result !== null && + (result as { isError?: unknown }).isError === true; + return isError ? "error_result" : "ok"; +} + +/** + * Wrap every registered tool's handler with a stopwatch. + * + * Why here and not in each tool: the 51 call sites write their own `trackMCP` + * rows by hand at entry, before any work happens, so none of them can carry a + * duration. Wrapping at the registry gives every tool, current and future, the + * same completion row from one place, and leaves the existing rows untouched. + * + * The wrapper is transparent: the result is returned as-is and a throw is + * rethrown, so tool behaviour and the SDK's own error handling do not change. + * The event is fire-and-forget; a telemetry failure never affects the call. + * + * Task-style handlers (objects with `createTask`) are left alone — none of our + * tools use them, and the SDK dispatches them differently. + */ +export function instrumentToolLatency( + tools: Record, + getClientInfo: () => ClientInfo, + config?: unknown, +): void { + for (const [name, tool] of Object.entries(tools)) { + const inner = tool.handler as unknown; + if (typeof inner !== "function") continue; + if ((inner as AnyHandler & { [WRAPPED]?: true })[WRAPPED]) continue; + + const wrapped: AnyHandler & { [WRAPPED]?: true } = async ( + ...args: unknown[] + ) => { + const startedAt = performance.now(); + try { + const result = await (inner as AnyHandler)(...args); + emit(name, getClientInfo, config, startedAt, outcomeOf(result)); + return result; + } catch (error) { + emit(name, getClientInfo, config, startedAt, "threw"); + throw error; + } + }; + wrapped[WRAPPED] = true; + + // Assign directly rather than via `tool.update()`: update() also fires a + // tools/list_changed notification, which is noise at registration time. + (tool as { handler: unknown }).handler = wrapped; + } +} + +function emit( + name: string, + getClientInfo: () => ClientInfo, + config: unknown, + startedAt: number, + outcome: ToolOutcome, +): void { + try { + trackMCPCompleted( + name, + getClientInfo() ?? {}, + { durationMs: performance.now() - startedAt, outcome }, + config, + ); + } catch { + // Telemetry must never decide whether a tool call succeeds. + } +} diff --git a/src/server-factory.ts b/src/server-factory.ts index c34f688b..e2f42384 100644 --- a/src/server-factory.ts +++ b/src/server-factory.ts @@ -21,6 +21,7 @@ import { setupOnInitialized } from "./oninitialized.js"; import { BrowserStackConfig } from "./lib/types.js"; import addRCATools from "./tools/rca-agent.js"; import addAskBrowserStackAITool from "./tools/ask-browserstack/register.js"; +import { instrumentToolLatency } from "./lib/tool-latency.js"; /** * Wrapper class for BrowserStack MCP Server @@ -76,6 +77,16 @@ export class BrowserStackMcpServer { ); Object.assign(this.tools, added); }); + + // One completion row per tool call (duration + outcome), for every tool + // registered above. The per-tool trackMCP entry/catch rows are unchanged. + // getClientVersion() is empty until the client's initialize arrives, hence + // the thunk: it is read at call time, not now. + instrumentToolLatency( + this.tools, + () => this.server.server.getClientVersion() ?? {}, + this.config, + ); } /** diff --git a/tests/lib/instrumentation-completed.test.ts b/tests/lib/instrumentation-completed.test.ts new file mode 100644 index 00000000..d99f3bbd --- /dev/null +++ b/tests/lib/instrumentation-completed.test.ts @@ -0,0 +1,65 @@ +import { describe, it, expect, vi, beforeEach } from "vitest"; +import { trackMCP, trackMCPCompleted } from "../../src/lib/instrumentation"; +import { apiClient } from "../../src/lib/apiClient"; + +vi.mock("../../src/lib/apiClient", () => ({ + apiClient: { post: vi.fn().mockResolvedValue({ status: 200, data: {} }) }, +})); +vi.mock("../../src/logger", () => ({ + default: { info: vi.fn(), error: vi.fn(), debug: vi.fn() }, +})); +vi.mock("../../src/config", () => ({ + default: { REMOTE_MCP: false }, +})); + +const clientInfo = { name: "claude-code", version: "1.2.3" }; +const config = { + "browserstack-username": "user", + "browserstack-access-key": "key", +}; + +describe("trackMCPCompleted", () => { + beforeEach(() => vi.clearAllMocks()); + + it("posts a separate MCPToolCompleted event with duration and outcome", () => { + trackMCPCompleted( + "listTestCases", + clientInfo, + { durationMs: 1234.6, outcome: "ok" }, + config, + ); + + expect(apiClient.post).toHaveBeenCalledTimes(1); + const call = (apiClient.post as any).mock.calls[0][0]; + expect(call.url).toBe("https://api.browserstack.com/sdk/v1/event"); + expect(call.body.event_type).toBe("MCPToolCompleted"); + expect(call.body.event_properties).toMatchObject({ + tool_name: "listTestCases", + mcp_client: "claude-code", + is_remote: false, + duration_ms: 1235, + outcome: "ok", + }); + // Not an invocation row: must not carry `success`, or it would be double-counted. + expect(call.body.event_properties).not.toHaveProperty("success"); + expect(call.headers.Authorization).toMatch(/^Basic /); + expect(call.timeout).toBe(2000); + expect(call.raise_error).toBe(false); + }); + + it("clamps negative and fractional durations to a non-negative integer", () => { + trackMCPCompleted("t", clientInfo, { durationMs: -3.2, outcome: "threw" }, config); + expect( + (apiClient.post as any).mock.calls[0][0].body.event_properties.duration_ms, + ).toBe(0); + }); + + it("leaves the MCPInstrumentation entry row unchanged", () => { + trackMCP("listTestCases", clientInfo, undefined, config); + const body = (apiClient.post as any).mock.calls[0][0].body; + expect(body.event_type).toBe("MCPInstrumentation"); + expect(body.event_properties.success).toBe(true); + expect(body.event_properties).not.toHaveProperty("duration_ms"); + expect(body.event_properties).not.toHaveProperty("outcome"); + }); +}); diff --git a/tests/lib/tool-latency.test.ts b/tests/lib/tool-latency.test.ts new file mode 100644 index 00000000..7847a9e6 --- /dev/null +++ b/tests/lib/tool-latency.test.ts @@ -0,0 +1,108 @@ +import { describe, it, expect, vi, beforeEach } from "vitest"; +import { instrumentToolLatency } from "../../src/lib/tool-latency"; +import { trackMCPCompleted } from "../../src/lib/instrumentation"; + +vi.mock("../../src/lib/instrumentation", () => ({ + trackMCPCompleted: vi.fn(), +})); + +const clientInfo = { name: "test-client", version: "1.0" }; +const config = { "browserstack-username": "u", "browserstack-access-key": "k" }; + +function fakeTool(handler: unknown) { + // Only the fields the wrapper touches; the real RegisteredTool has more. + return { handler, enabled: true } as any; +} + +describe("instrumentToolLatency", () => { + beforeEach(() => vi.clearAllMocks()); + + it("emits ok with a duration and returns the result untouched", async () => { + const result = { content: [{ type: "text", text: "done" }] }; + const tools = { listTestCases: fakeTool(vi.fn().mockResolvedValue(result)) }; + + instrumentToolLatency(tools, () => clientInfo, config); + const out = await tools.listTestCases.handler({ projectId: "PR-1" }, {}); + + expect(out).toBe(result); + expect(trackMCPCompleted).toHaveBeenCalledTimes(1); + const [name, ci, completion, cfg] = (trackMCPCompleted as any).mock.calls[0]; + expect(name).toBe("listTestCases"); + expect(ci).toBe(clientInfo); + expect(completion.outcome).toBe("ok"); + expect(completion.durationMs).toBeGreaterThanOrEqual(0); + expect(cfg).toBe(config); + }); + + it("classifies an isError result as error_result", async () => { + const tools = { + fetchBuildInsights: fakeTool( + vi.fn().mockResolvedValue({ content: [], isError: true }), + ), + }; + instrumentToolLatency(tools, () => clientInfo, config); + await tools.fetchBuildInsights.handler({}, {}); + expect((trackMCPCompleted as any).mock.calls[0][2].outcome).toBe( + "error_result", + ); + }); + + it("emits threw and rethrows when the handler throws", async () => { + const boom = new Error("upstream 500"); + const tools = { getFailureLogs: fakeTool(vi.fn().mockRejectedValue(boom)) }; + instrumentToolLatency(tools, () => clientInfo, config); + + await expect(tools.getFailureLogs.handler({}, {})).rejects.toBe(boom); + expect((trackMCPCompleted as any).mock.calls[0][2].outcome).toBe("threw"); + }); + + it("passes every handler argument through (schema-less tools get only extra)", async () => { + const inner = vi.fn().mockResolvedValue({ content: [] }); + const tools = { ping: fakeTool(inner) }; + instrumentToolLatency(tools, () => clientInfo, config); + + const extra = { signal: new AbortController().signal }; + await tools.ping.handler(extra); + expect(inner).toHaveBeenCalledWith(extra); + }); + + it("reads client info at call time, not at wrap time", async () => { + let current: any = {}; + const tools = { t: fakeTool(vi.fn().mockResolvedValue({ content: [] })) }; + instrumentToolLatency(tools, () => current, config); + + current = { name: "cursor", version: "2" }; + await tools.t.handler({}, {}); + expect((trackMCPCompleted as any).mock.calls[0][1]).toEqual({ + name: "cursor", + version: "2", + }); + }); + + it("is idempotent: wrapping twice emits one event per call", async () => { + const tools = { t: fakeTool(vi.fn().mockResolvedValue({ content: [] })) }; + instrumentToolLatency(tools, () => clientInfo, config); + instrumentToolLatency(tools, () => clientInfo, config); + + await tools.t.handler({}, {}); + expect(trackMCPCompleted).toHaveBeenCalledTimes(1); + }); + + it("skips task-style handlers that are not functions", () => { + const taskHandler = { createTask: vi.fn() }; + const tools = { t: fakeTool(taskHandler) }; + instrumentToolLatency(tools, () => clientInfo, config); + expect(tools.t.handler).toBe(taskHandler); + }); + + it("never lets a telemetry failure affect the tool call", async () => { + (trackMCPCompleted as any).mockImplementation(() => { + throw new Error("telemetry down"); + }); + const result = { content: [] }; + const tools = { t: fakeTool(vi.fn().mockResolvedValue(result)) }; + instrumentToolLatency(tools, () => clientInfo, config); + + await expect(tools.t.handler({}, {})).resolves.toBe(result); + }); +}); From 9c69af4c4feb4f7d351a66635db62848a7dda947 Mon Sep 17 00:00:00 2001 From: Savio Dias Date: Mon, 21 Sep 2026 20:42:47 +0530 Subject: [PATCH 2/8] refactor(lib): trim comments in the latency wrapper to the non-obvious ones Co-Authored-By: Claude Fable 5.1 --- src/lib/instrumentation.ts | 24 +++--------------------- src/lib/tool-latency.ts | 25 +++++-------------------- src/server-factory.ts | 5 +---- tests/lib/tool-latency.test.ts | 1 - 4 files changed, 9 insertions(+), 46 deletions(-) diff --git a/src/lib/instrumentation.ts b/src/lib/instrumentation.ts index 85d0f27f..a3c88c45 100644 --- a/src/lib/instrumentation.ts +++ b/src/lib/instrumentation.ts @@ -10,7 +10,6 @@ const INSTRUMENTATION_ENDPOINT = "https://api.browserstack.com/sdk/v1/event"; export type ClientInfo = { name?: string; version?: string }; -/** How a tool call ended, as seen by the completion wrapper. */ export type ToolOutcome = "ok" | "error_result" | "threw"; interface MCPEventPayload { @@ -39,7 +38,6 @@ function baseProperties(toolName: string, clientInfo: ClientInfo) { }; } -/** Fire-and-forget POST. Never throws, never delays the caller. */ function sendEvent(event: MCPEventPayload, config?: any): void { let authHeader: string | undefined; if (config) { @@ -61,11 +59,7 @@ function sendEvent(event: MCPEventPayload, config?: any): void { .catch(() => {}); } -/** - * The per-invocation event. Fired at tool entry with `success: true` (meaning - * "invoked"), and again from the catch block with `success: false` when the - * handler throws. A failing call therefore produces two rows. - */ +/** Per-invocation row: fired at tool entry (success) and from the catch block (failure). */ export function trackMCP( toolName: string, clientInfo: ClientInfo, @@ -103,20 +97,8 @@ export function trackMCP( } /** - * The per-completion event: one row per tool call, written AFTER the handler - * settles, carrying wall-clock duration and how it ended. - * - * Deliberately a separate `event_type` from `MCPInstrumentation`, so every - * existing query and dashboard keyed on that name keeps its row counts. - * - * outcome = "ok" handler returned a result without `isError` - * outcome = "error_result" handler returned `{ isError: true }` (a failure the - * entry/catch rows never see today) - * outcome = "threw" handler threw; the catch row also exists - * - * A call with an entry row and no completion row was killed before it finished - * (client closed the IDE, process exit), which is the closest thing to a - * timeout signal this event can give. + * Per-completion row, written after the handler settles, with duration and + * outcome. Separate event_type so existing MCPInstrumentation counts do not change. */ export function trackMCPCompleted( toolName: string, diff --git a/src/lib/tool-latency.ts b/src/lib/tool-latency.ts index 2923e385..a5bb2c94 100644 --- a/src/lib/tool-latency.ts +++ b/src/lib/tool-latency.ts @@ -5,10 +5,6 @@ import { trackMCPCompleted, } from "./instrumentation.js"; -/** - * Marks a handler we have already wrapped, so calling `instrumentToolLatency` - * twice on the same tool map (library + remote wrapper) emits one event, not two. - */ const WRAPPED = Symbol.for("browserstack.mcp.latencyWrapped"); type AnyHandler = (...args: unknown[]) => unknown; @@ -22,19 +18,9 @@ function outcomeOf(result: unknown): ToolOutcome { } /** - * Wrap every registered tool's handler with a stopwatch. - * - * Why here and not in each tool: the 51 call sites write their own `trackMCP` - * rows by hand at entry, before any work happens, so none of them can carry a - * duration. Wrapping at the registry gives every tool, current and future, the - * same completion row from one place, and leaves the existing rows untouched. - * - * The wrapper is transparent: the result is returned as-is and a throw is - * rethrown, so tool behaviour and the SDK's own error handling do not change. - * The event is fire-and-forget; a telemetry failure never affects the call. - * - * Task-style handlers (objects with `createTask`) are left alone — none of our - * tools use them, and the SDK dispatches them differently. + * Wraps every registered tool handler with a stopwatch and emits one + * `MCPToolCompleted` event per call. Transparent (result passed through, + * throws rethrown) and idempotent. Skips task-style (non-function) handlers. */ export function instrumentToolLatency( tools: Record, @@ -61,8 +47,7 @@ export function instrumentToolLatency( }; wrapped[WRAPPED] = true; - // Assign directly rather than via `tool.update()`: update() also fires a - // tools/list_changed notification, which is noise at registration time. + // Direct assignment: tool.update() would also fire tools/list_changed. (tool as { handler: unknown }).handler = wrapped; } } @@ -82,6 +67,6 @@ function emit( config, ); } catch { - // Telemetry must never decide whether a tool call succeeds. + // Telemetry must never affect the tool call. } } diff --git a/src/server-factory.ts b/src/server-factory.ts index e2f42384..8c0c43e7 100644 --- a/src/server-factory.ts +++ b/src/server-factory.ts @@ -78,10 +78,7 @@ export class BrowserStackMcpServer { Object.assign(this.tools, added); }); - // One completion row per tool call (duration + outcome), for every tool - // registered above. The per-tool trackMCP entry/catch rows are unchanged. - // getClientVersion() is empty until the client's initialize arrives, hence - // the thunk: it is read at call time, not now. + // Client info is read at call time; it is empty until initialize arrives. instrumentToolLatency( this.tools, () => this.server.server.getClientVersion() ?? {}, diff --git a/tests/lib/tool-latency.test.ts b/tests/lib/tool-latency.test.ts index 7847a9e6..47598185 100644 --- a/tests/lib/tool-latency.test.ts +++ b/tests/lib/tool-latency.test.ts @@ -10,7 +10,6 @@ const clientInfo = { name: "test-client", version: "1.0" }; const config = { "browserstack-username": "u", "browserstack-access-key": "k" }; function fakeTool(handler: unknown) { - // Only the fields the wrapper touches; the real RegisteredTool has more. return { handler, enabled: true } as any; } From 07763e0ac3f607eede2465817f1633afc903638b Mon Sep 17 00:00:00 2001 From: Savio Dias Date: Tue, 22 Sep 2026 11:52:41 +0530 Subject: [PATCH 3/8] fix(lib): send the completion row as MCPInstrumentation with phase=completed The Rails endpoint (railsApp sdk_controller#event) allowlists event types and rejects anything else with 400 INVALID_EVENT_TYPE; a probe confirmed the MCPToolCompleted rows never reached BigQuery while the entry/catch rows did. Reuse the accepted event type and mark the row with `phase: "completed"`, omitting `success`, so queries counting success='true' / 'false' are unaffected. Verified in BigQuery: completion rows land with phase, outcome and duration_ms. Co-Authored-By: Claude Fable 5.1 --- src/lib/instrumentation.ts | 8 ++++++-- src/lib/tool-latency.ts | 2 +- tests/lib/instrumentation-completed.test.ts | 6 ++++-- 3 files changed, 11 insertions(+), 5 deletions(-) diff --git a/src/lib/instrumentation.ts b/src/lib/instrumentation.ts index a3c88c45..21fdc8b2 100644 --- a/src/lib/instrumentation.ts +++ b/src/lib/instrumentation.ts @@ -23,6 +23,7 @@ interface MCPEventPayload { error_message?: string; error_type?: string; is_remote?: boolean; + phase?: "completed"; duration_ms?: number; outcome?: ToolOutcome; }; @@ -98,7 +99,9 @@ export function trackMCP( /** * Per-completion row, written after the handler settles, with duration and - * outcome. Separate event_type so existing MCPInstrumentation counts do not change. + * outcome. Same event_type as the entry row because the Rails endpoint + * allowlists event types; `phase: "completed"` and the absence of `success` + * keep it out of existing success/failure counts. */ export function trackMCPCompleted( toolName: string, @@ -107,9 +110,10 @@ export function trackMCPCompleted( config?: any, ): void { const event: MCPEventPayload = { - event_type: "MCPToolCompleted", + event_type: "MCPInstrumentation", event_properties: { ...baseProperties(toolName, clientInfo), + phase: "completed", duration_ms: Math.max(0, Math.round(completion.durationMs)), outcome: completion.outcome, }, diff --git a/src/lib/tool-latency.ts b/src/lib/tool-latency.ts index a5bb2c94..30758457 100644 --- a/src/lib/tool-latency.ts +++ b/src/lib/tool-latency.ts @@ -19,7 +19,7 @@ function outcomeOf(result: unknown): ToolOutcome { /** * Wraps every registered tool handler with a stopwatch and emits one - * `MCPToolCompleted` event per call. Transparent (result passed through, + * completion row (`phase: "completed"`) per call. Transparent (result passed through, * throws rethrown) and idempotent. Skips task-style (non-function) handlers. */ export function instrumentToolLatency( diff --git a/tests/lib/instrumentation-completed.test.ts b/tests/lib/instrumentation-completed.test.ts index d99f3bbd..ddcd2222 100644 --- a/tests/lib/instrumentation-completed.test.ts +++ b/tests/lib/instrumentation-completed.test.ts @@ -21,7 +21,7 @@ const config = { describe("trackMCPCompleted", () => { beforeEach(() => vi.clearAllMocks()); - it("posts a separate MCPToolCompleted event with duration and outcome", () => { + it("posts a completion row with phase, duration and outcome", () => { trackMCPCompleted( "listTestCases", clientInfo, @@ -32,11 +32,12 @@ describe("trackMCPCompleted", () => { expect(apiClient.post).toHaveBeenCalledTimes(1); const call = (apiClient.post as any).mock.calls[0][0]; expect(call.url).toBe("https://api.browserstack.com/sdk/v1/event"); - expect(call.body.event_type).toBe("MCPToolCompleted"); + expect(call.body.event_type).toBe("MCPInstrumentation"); expect(call.body.event_properties).toMatchObject({ tool_name: "listTestCases", mcp_client: "claude-code", is_remote: false, + phase: "completed", duration_ms: 1235, outcome: "ok", }); @@ -59,6 +60,7 @@ describe("trackMCPCompleted", () => { const body = (apiClient.post as any).mock.calls[0][0].body; expect(body.event_type).toBe("MCPInstrumentation"); expect(body.event_properties.success).toBe(true); + expect(body.event_properties).not.toHaveProperty("phase"); expect(body.event_properties).not.toHaveProperty("duration_ms"); expect(body.event_properties).not.toHaveProperty("outcome"); }); From 51dfcfa1054d19396c1bc94626d640ef6c724ddf Mon Sep 17 00:00:00 2001 From: Savio Dias Date: Tue, 22 Sep 2026 12:45:51 +0530 Subject: [PATCH 4/8] feat(lib): classify failures into error_class on the failure row MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Failure rows carried only the free-text error_message, so grouping failures by cause meant regex over strings. Add `classifyError` and stamp `error_class` on the catch-path row: auth_error, not_found, rate_limited, validation, timeout, network, server_error, unknown. Reads the HTTP status from the axios error object, falls back to the status embedded in plain Error messages thrown by utils ("…: 404 Not Found", "status code 401"), maps transport codes (ECONNABORTED, ECONNREFUSED…), Zod errors, and the entitlement refusal message. The success row is unchanged. Refs AIMCP-225 Co-Authored-By: Claude Fable 5.1 --- src/lib/instrumentation.ts | 61 +++++++++++++++++++ tests/lib/error-class.test.ts | 107 ++++++++++++++++++++++++++++++++++ 2 files changed, 168 insertions(+) create mode 100644 tests/lib/error-class.test.ts diff --git a/src/lib/instrumentation.ts b/src/lib/instrumentation.ts index 21fdc8b2..d0ea1283 100644 --- a/src/lib/instrumentation.ts +++ b/src/lib/instrumentation.ts @@ -12,6 +12,16 @@ export type ClientInfo = { name?: string; version?: string }; export type ToolOutcome = "ok" | "error_result" | "threw"; +export type ErrorClass = + | "auth_error" + | "not_found" + | "rate_limited" + | "validation" + | "timeout" + | "network" + | "server_error" + | "unknown"; + interface MCPEventPayload { event_type: string; event_properties: { @@ -22,6 +32,7 @@ interface MCPEventPayload { success?: boolean; error_message?: string; error_type?: string; + error_class?: ErrorClass; is_remote?: boolean; phase?: "completed"; duration_ms?: number; @@ -29,6 +40,55 @@ interface MCPEventPayload { }; } +const TIMEOUT_CODES = new Set(["ECONNABORTED", "ETIMEDOUT"]); +const NETWORK_CODES = new Set([ + "ECONNREFUSED", + "ECONNRESET", + "ENOTFOUND", + "EAI_AGAIN", + "EPIPE", +]); + +function httpStatusOf(error: unknown): number | undefined { + const e = error as { response?: { status?: unknown }; status?: unknown }; + const direct = e?.response?.status ?? e?.status; + if (typeof direct === "number") return direct; + // Plain Errors from utils carry the status only in the message: + // "Request failed with status code 404", "Failed to fetch from …: 404 Not Found" + const message = error instanceof Error ? error.message : String(error ?? ""); + const m = message.match( + /status code (\d{3})|: (\d{3}) [A-Z]|\bHTTP (\d{3})\b/, + ); + const found = m && (m[1] || m[2] || m[3]); + return found ? Number(found) : undefined; +} + +/** Bucket a thrown error into a fixed set of causes, so failures group by class. */ +export function classifyError(error: unknown): ErrorClass { + const e = error as { code?: unknown; name?: unknown; issues?: unknown }; + if (typeof e?.code === "string") { + if (TIMEOUT_CODES.has(e.code)) return "timeout"; + if (NETWORK_CODES.has(e.code)) return "network"; + } + if (e?.name === "ZodError" || Array.isArray(e?.issues)) return "validation"; + + const status = httpStatusOf(error); + if (status === 401 || status === 403) return "auth_error"; + if (status === 404) return "not_found"; + if (status === 429) return "rate_limited"; + if (status === 408 || status === 504) return "timeout"; + if (status === 400 || status === 422) return "validation"; + if (status !== undefined && status >= 500) return "server_error"; + + const message = ( + error instanceof Error ? error.message : String(error ?? "") + ).toLowerCase(); + if (/timed? ?out/.test(message)) return "timeout"; + if (/not enabled for|unauthori[sz]ed|forbidden/.test(message)) + return "auth_error"; + return "unknown"; +} + function baseProperties(toolName: string, clientInfo: ClientInfo) { return { mcp_version: packageJson.version as string, @@ -92,6 +152,7 @@ export function trackMCP( error instanceof Error ? error.message : String(error); event.event_properties.error_type = error instanceof Error ? error.constructor.name : "Unknown"; + event.event_properties.error_class = classifyError(error); } sendEvent(event, config); diff --git a/tests/lib/error-class.test.ts b/tests/lib/error-class.test.ts new file mode 100644 index 00000000..06c6568c --- /dev/null +++ b/tests/lib/error-class.test.ts @@ -0,0 +1,107 @@ +import { describe, it, expect, vi, beforeEach } from "vitest"; +import { classifyError, trackMCP } from "../../src/lib/instrumentation"; +import { apiClient } from "../../src/lib/apiClient"; + +vi.mock("../../src/lib/apiClient", () => ({ + apiClient: { post: vi.fn().mockResolvedValue({ status: 200, data: {} }) }, +})); +vi.mock("../../src/logger", () => ({ + default: { info: vi.fn(), error: vi.fn(), debug: vi.fn() }, +})); +vi.mock("../../src/config", () => ({ default: { REMOTE_MCP: false } })); + +function axiosLike(status: number, code?: string) { + const err: any = new Error(`Request failed with status code ${status}`); + err.response = { status }; + if (code) err.code = code; + return err; +} + +describe("classifyError", () => { + it.each([ + [axiosLike(401), "auth_error"], + [axiosLike(403), "auth_error"], + [axiosLike(404), "not_found"], + [axiosLike(429), "rate_limited"], + [axiosLike(400), "validation"], + [axiosLike(422), "validation"], + [axiosLike(500), "server_error"], + [axiosLike(503), "server_error"], + [axiosLike(504), "timeout"], + ])("maps HTTP status on the error object (%#)", (err, expected) => { + expect(classifyError(err)).toBe(expected); + }); + + it("reads the status out of plain Error messages thrown by utils", () => { + expect( + classifyError( + new Error( + "Failed to fetch from https://api-automation.browserstack.com/ext/v1/builds/junk: 404 Not Found", + ), + ), + ).toBe("not_found"); + expect( + classifyError(new Error("Request failed with status code 401")), + ).toBe("auth_error"); + }); + + it("prefers the transport code over any status", () => { + const err: any = new Error("timeout of 2000ms exceeded"); + err.code = "ECONNABORTED"; + expect(classifyError(err)).toBe("timeout"); + const refused: any = new Error("connect ECONNREFUSED 10.0.0.1:443"); + refused.code = "ECONNREFUSED"; + expect(classifyError(refused)).toBe("network"); + }); + + it("treats Zod errors as validation", () => { + const zod: any = new Error("Invalid input"); + zod.name = "ZodError"; + zod.issues = []; + expect(classifyError(zod)).toBe("validation"); + }); + + it("classifies entitlement refusals and message-only timeouts", () => { + expect( + classifyError( + new Error("BrowserStack AI is not enabled for `tm` on your account."), + ), + ).toBe("auth_error"); + expect(classifyError(new Error("Scan timed out after 300s"))).toBe( + "timeout", + ); + }); + + it("falls back to unknown for anything else, including non-Errors", () => { + expect(classifyError(new Error("Converting circular structure to JSON"))).toBe( + "unknown", + ); + expect(classifyError("some string")).toBe("unknown"); + expect(classifyError(undefined)).toBe("unknown"); + }); +}); + +describe("trackMCP failure row", () => { + beforeEach(() => vi.clearAllMocks()); + + it("adds error_class next to the existing error fields", () => { + trackMCP("fetchBuildInsights", { name: "c" }, axiosLike(404), { + "browserstack-username": "u", + "browserstack-access-key": "k", + }); + const props = (apiClient.post as any).mock.calls[0][0].body.event_properties; + expect(props.success).toBe(false); + expect(props.error_type).toBe("Error"); + expect(props.error_message).toBe("Request failed with status code 404"); + expect(props.error_class).toBe("not_found"); + }); + + it("does not add error_class to the success row", () => { + trackMCP("listTestCases", { name: "c" }, undefined, { + "browserstack-username": "u", + "browserstack-access-key": "k", + }); + const props = (apiClient.post as any).mock.calls[0][0].body.event_properties; + expect(props).not.toHaveProperty("error_class"); + }); +}); From ed3348178057f94265bcda981833503a21ac51fa Mon Sep 17 00:00:00 2001 From: Savio Dias Date: Wed, 23 Sep 2026 12:49:44 +0530 Subject: [PATCH 5/8] refactor(lib): one MCPInstrumentation row per tool call, written when the call settles Replace the entry row + completion row pair with a single row per call. The tools' existing trackMCP calls (entry and catch block) no longer post; inside an instrumented call they record into an AsyncLocalStorage context and the wrapper writes one row when the handler settles, carrying success, duration_ms, outcome (ok / error_result / threw) and, on failure, error_message / error_type / error_class. - success=false when the handler reported an error via trackMCP or threw; failure counts stay identical to today, but a failed call is now one row instead of two. - Invocation count is COUNT(*) per tool (excluding `started`), no phase filter needed. - trackMCP outside an instrumented call (the started heartbeat, tools a host registers without wrapping) behaves exactly as before. - Context is request-scoped (AsyncLocalStorage), so concurrent calls in the remote wrapper never share state; verified by test. - trackMCPCompleted and the phase field are gone; withToolCall is exported for hosts. Co-Authored-By: Claude Fable 5.1 --- node_modules | 1 + src/index.ts | 2 +- src/lib/instrumentation.ts | 125 +++++++++++----- src/lib/tool-latency.ts | 58 ++------ tests/lib/instrumentation-completed.test.ts | 67 --------- tests/lib/instrumentation-single-row.test.ts | 135 +++++++++++++++++ tests/lib/tool-latency.test.ts | 147 +++++++++++++++---- 7 files changed, 354 insertions(+), 181 deletions(-) create mode 120000 node_modules delete mode 100644 tests/lib/instrumentation-completed.test.ts create mode 100644 tests/lib/instrumentation-single-row.test.ts diff --git a/node_modules b/node_modules new file mode 120000 index 00000000..6ebb6850 --- /dev/null +++ b/node_modules @@ -0,0 +1 @@ +/Users/saviodias/Projects/mcp-server/node_modules \ No newline at end of file diff --git a/src/index.ts b/src/index.ts index 31c91e6d..caa8d79b 100644 --- a/src/index.ts +++ b/src/index.ts @@ -53,7 +53,7 @@ process.on("exit", () => { export { setLogger } from "./logger.js"; export { BrowserStackMcpServer } from "./server-factory.js"; -export { trackMCP, trackMCPCompleted } from "./lib/instrumentation.js"; +export { trackMCP, withToolCall } from "./lib/instrumentation.js"; export { instrumentToolLatency } from "./lib/tool-latency.js"; export { default as addTfaRcaCollaborationTools } from "./tools/tfa-rca-collaboration.js"; export const PackageJsonVersion = packageJson.version; diff --git a/src/lib/instrumentation.ts b/src/lib/instrumentation.ts index d0ea1283..4f87bf24 100644 --- a/src/lib/instrumentation.ts +++ b/src/lib/instrumentation.ts @@ -1,3 +1,4 @@ +import { AsyncLocalStorage } from "node:async_hooks"; import logger from "../logger.js"; import { getBrowserStackAuth } from "./get-auth.js"; import { createRequire } from "module"; @@ -34,7 +35,6 @@ interface MCPEventPayload { error_type?: string; error_class?: ErrorClass; is_remote?: boolean; - phase?: "completed"; duration_ms?: number; outcome?: ToolOutcome; }; @@ -99,6 +99,14 @@ function baseProperties(toolName: string, clientInfo: ClientInfo) { }; } +function errorProperties(error: unknown) { + return { + error_message: error instanceof Error ? error.message : String(error), + error_type: error instanceof Error ? error.constructor.name : "Unknown", + error_class: classifyError(error), + }; +} + function sendEvent(event: MCPEventPayload, config?: any): void { let authHeader: string | undefined; if (config) { @@ -120,16 +128,41 @@ function sendEvent(event: MCPEventPayload, config?: any): void { .catch(() => {}); } -/** Per-invocation row: fired at tool entry (success) and from the catch block (failure). */ +/** + * State of one tool call while its handler runs. Lives in AsyncLocalStorage, so it is + * request-scoped: concurrent calls in the multi-tenant remote wrapper never share it. + */ +interface CallContext { + toolName: string; + clientInfo: ClientInfo; + config?: any; + error?: unknown; +} + +const callContext = new AsyncLocalStorage(); + +/** + * Records a tool invocation or failure. + * + * Inside an instrumented call (see `withToolCall`) nothing is sent: the entry call and + * the catch-block call fold into the single row written when the handler settles. + * Outside one (the `started` heartbeat, tools a host registers without wrapping) it + * behaves as before and posts a row immediately. + */ export function trackMCP( toolName: string, clientInfo: ClientInfo, error?: unknown, config?: any, ): void { - const isSuccess = !error; + const ctx = callContext.getStore(); + if (ctx) { + if (clientInfo?.name && !ctx.clientInfo?.name) ctx.clientInfo = clientInfo; + if (config && !ctx.config) ctx.config = config; + if (error) ctx.error = error; + return; + } - // Log client information if (clientInfo?.name) { logger.info( `Client connected: ${clientInfo.name} (version: ${clientInfo.version})`, @@ -142,42 +175,66 @@ export function trackMCP( event_type: "MCPInstrumentation", event_properties: { ...baseProperties(toolName, clientInfo), - success: isSuccess, + success: !error, + ...(error ? errorProperties(error) : {}), }, }; - - // Add error details if applicable - if (error) { - event.event_properties.error_message = - error instanceof Error ? error.message : String(error); - event.event_properties.error_type = - error instanceof Error ? error.constructor.name : "Unknown"; - event.event_properties.error_class = classifyError(error); - } - sendEvent(event, config); } +function isErrorResult(result: unknown): boolean { + return ( + typeof result === "object" && + result !== null && + (result as { isError?: unknown }).isError === true + ); +} + /** - * Per-completion row, written after the handler settles, with duration and - * outcome. Same event_type as the entry row because the Rails endpoint - * allowlists event types; `phase: "completed"` and the absence of `success` - * keep it out of existing success/failure counts. + * Runs a tool handler and writes exactly one MCPInstrumentation row when it settles: + * `success` (false when the handler reported or threw an error), `duration_ms`, + * `outcome` (ok / error_result / threw) and the error fields on failures. + * Telemetry never affects the call: the result is passed through, throws are rethrown. */ -export function trackMCPCompleted( +export async function withToolCall( toolName: string, - clientInfo: ClientInfo, - completion: { durationMs: number; outcome: ToolOutcome }, - config?: any, -): void { - const event: MCPEventPayload = { - event_type: "MCPInstrumentation", - event_properties: { - ...baseProperties(toolName, clientInfo), - phase: "completed", - duration_ms: Math.max(0, Math.round(completion.durationMs)), - outcome: completion.outcome, - }, - }; - sendEvent(event, config); + getClientInfo: () => ClientInfo, + config: any, + fn: () => Promise | T, +): Promise { + const ctx: CallContext = { toolName, clientInfo: {}, config }; + const startedAt = performance.now(); + let outcome: ToolOutcome = "ok"; + try { + const result = await callContext.run(ctx, fn); + if (isErrorResult(result)) outcome = "error_result"; + return result; + } catch (error) { + outcome = "threw"; + ctx.error ??= error; + throw error; + } finally { + try { + let clientInfo = ctx.clientInfo; + try { + const live = getClientInfo(); + if (live?.name) clientInfo = live; + } catch { + // client info is optional + } + const event: MCPEventPayload = { + event_type: "MCPInstrumentation", + event_properties: { + ...baseProperties(toolName, clientInfo), + success: ctx.error === undefined && outcome !== "threw", + duration_ms: Math.max(0, Math.round(performance.now() - startedAt)), + outcome, + ...(ctx.error !== undefined ? errorProperties(ctx.error) : {}), + }, + }; + sendEvent(event, ctx.config ?? config); + } catch { + // Telemetry must never affect the tool call. + } + } } diff --git a/src/lib/tool-latency.ts b/src/lib/tool-latency.ts index 30758457..94798d70 100644 --- a/src/lib/tool-latency.ts +++ b/src/lib/tool-latency.ts @@ -1,26 +1,16 @@ import { RegisteredTool } from "@modelcontextprotocol/sdk/server/mcp.js"; -import { - ClientInfo, - ToolOutcome, - trackMCPCompleted, -} from "./instrumentation.js"; +import { ClientInfo, withToolCall } from "./instrumentation.js"; const WRAPPED = Symbol.for("browserstack.mcp.latencyWrapped"); type AnyHandler = (...args: unknown[]) => unknown; -function outcomeOf(result: unknown): ToolOutcome { - const isError = - typeof result === "object" && - result !== null && - (result as { isError?: unknown }).isError === true; - return isError ? "error_result" : "ok"; -} - /** - * Wraps every registered tool handler with a stopwatch and emits one - * completion row (`phase: "completed"`) per call. Transparent (result passed through, - * throws rethrown) and idempotent. Skips task-style (non-function) handlers. + * Wraps every registered tool handler in `withToolCall`, so each call writes exactly + * one MCPInstrumentation row when it settles (success, duration_ms, outcome, error + * fields). The tools' own entry and catch-block `trackMCP` calls fold into that row. + * Transparent (result passed through, throws rethrown), idempotent, skips task-style + * (non-function) handlers. */ export function instrumentToolLatency( tools: Record, @@ -32,41 +22,13 @@ export function instrumentToolLatency( if (typeof inner !== "function") continue; if ((inner as AnyHandler & { [WRAPPED]?: true })[WRAPPED]) continue; - const wrapped: AnyHandler & { [WRAPPED]?: true } = async ( - ...args: unknown[] - ) => { - const startedAt = performance.now(); - try { - const result = await (inner as AnyHandler)(...args); - emit(name, getClientInfo, config, startedAt, outcomeOf(result)); - return result; - } catch (error) { - emit(name, getClientInfo, config, startedAt, "threw"); - throw error; - } - }; + const wrapped: AnyHandler & { [WRAPPED]?: true } = (...args: unknown[]) => + withToolCall(name, getClientInfo, config, () => + (inner as AnyHandler)(...args), + ); wrapped[WRAPPED] = true; // Direct assignment: tool.update() would also fire tools/list_changed. (tool as { handler: unknown }).handler = wrapped; } } - -function emit( - name: string, - getClientInfo: () => ClientInfo, - config: unknown, - startedAt: number, - outcome: ToolOutcome, -): void { - try { - trackMCPCompleted( - name, - getClientInfo() ?? {}, - { durationMs: performance.now() - startedAt, outcome }, - config, - ); - } catch { - // Telemetry must never affect the tool call. - } -} diff --git a/tests/lib/instrumentation-completed.test.ts b/tests/lib/instrumentation-completed.test.ts deleted file mode 100644 index ddcd2222..00000000 --- a/tests/lib/instrumentation-completed.test.ts +++ /dev/null @@ -1,67 +0,0 @@ -import { describe, it, expect, vi, beforeEach } from "vitest"; -import { trackMCP, trackMCPCompleted } from "../../src/lib/instrumentation"; -import { apiClient } from "../../src/lib/apiClient"; - -vi.mock("../../src/lib/apiClient", () => ({ - apiClient: { post: vi.fn().mockResolvedValue({ status: 200, data: {} }) }, -})); -vi.mock("../../src/logger", () => ({ - default: { info: vi.fn(), error: vi.fn(), debug: vi.fn() }, -})); -vi.mock("../../src/config", () => ({ - default: { REMOTE_MCP: false }, -})); - -const clientInfo = { name: "claude-code", version: "1.2.3" }; -const config = { - "browserstack-username": "user", - "browserstack-access-key": "key", -}; - -describe("trackMCPCompleted", () => { - beforeEach(() => vi.clearAllMocks()); - - it("posts a completion row with phase, duration and outcome", () => { - trackMCPCompleted( - "listTestCases", - clientInfo, - { durationMs: 1234.6, outcome: "ok" }, - config, - ); - - expect(apiClient.post).toHaveBeenCalledTimes(1); - const call = (apiClient.post as any).mock.calls[0][0]; - expect(call.url).toBe("https://api.browserstack.com/sdk/v1/event"); - expect(call.body.event_type).toBe("MCPInstrumentation"); - expect(call.body.event_properties).toMatchObject({ - tool_name: "listTestCases", - mcp_client: "claude-code", - is_remote: false, - phase: "completed", - duration_ms: 1235, - outcome: "ok", - }); - // Not an invocation row: must not carry `success`, or it would be double-counted. - expect(call.body.event_properties).not.toHaveProperty("success"); - expect(call.headers.Authorization).toMatch(/^Basic /); - expect(call.timeout).toBe(2000); - expect(call.raise_error).toBe(false); - }); - - it("clamps negative and fractional durations to a non-negative integer", () => { - trackMCPCompleted("t", clientInfo, { durationMs: -3.2, outcome: "threw" }, config); - expect( - (apiClient.post as any).mock.calls[0][0].body.event_properties.duration_ms, - ).toBe(0); - }); - - it("leaves the MCPInstrumentation entry row unchanged", () => { - trackMCP("listTestCases", clientInfo, undefined, config); - const body = (apiClient.post as any).mock.calls[0][0].body; - expect(body.event_type).toBe("MCPInstrumentation"); - expect(body.event_properties.success).toBe(true); - expect(body.event_properties).not.toHaveProperty("phase"); - expect(body.event_properties).not.toHaveProperty("duration_ms"); - expect(body.event_properties).not.toHaveProperty("outcome"); - }); -}); diff --git a/tests/lib/instrumentation-single-row.test.ts b/tests/lib/instrumentation-single-row.test.ts new file mode 100644 index 00000000..3be8abe2 --- /dev/null +++ b/tests/lib/instrumentation-single-row.test.ts @@ -0,0 +1,135 @@ +import { describe, it, expect, vi, beforeEach } from "vitest"; +import { trackMCP, withToolCall } from "../../src/lib/instrumentation"; +import { apiClient } from "../../src/lib/apiClient"; + +vi.mock("../../src/lib/apiClient", () => ({ + apiClient: { post: vi.fn().mockResolvedValue({ status: 200, data: {} }) }, +})); +vi.mock("../../src/logger", () => ({ + default: { info: vi.fn(), error: vi.fn(), debug: vi.fn() }, +})); +vi.mock("../../src/config", () => ({ + default: { REMOTE_MCP: false }, +})); + +const clientInfo = { name: "claude-code", version: "1.2.3" }; +const config = { + "browserstack-username": "user", + "browserstack-access-key": "key", +}; +const rows = () => + (apiClient.post as any).mock.calls.map( + (c: any) => c[0].body.event_properties, + ); + +describe("withToolCall", () => { + beforeEach(() => vi.clearAllMocks()); + + it("posts one MCPInstrumentation row with success, duration and outcome", async () => { + const out = await withToolCall( + "listTestCases", + () => clientInfo, + config, + async () => { + trackMCP("listTestCases", clientInfo, undefined, config); + return { content: [] }; + }, + ); + + expect(out).toEqual({ content: [] }); + expect(apiClient.post).toHaveBeenCalledTimes(1); + const call = (apiClient.post as any).mock.calls[0][0]; + expect(call.url).toBe("https://api.browserstack.com/sdk/v1/event"); + expect(call.body.event_type).toBe("MCPInstrumentation"); + expect(call.body.event_properties).toMatchObject({ + tool_name: "listTestCases", + mcp_client: "claude-code", + is_remote: false, + success: true, + outcome: "ok", + }); + expect(call.body.event_properties.duration_ms).toBeGreaterThanOrEqual(0); + expect(call.body.event_properties).not.toHaveProperty("phase"); + expect(call.timeout).toBe(2000); + expect(call.raise_error).toBe(false); + }); + + it("folds the handler's catch-block trackMCP into the same row as a failure", async () => { + await withToolCall( + "fetchRCA", + () => clientInfo, + config, + async () => { + trackMCP("fetchRCA", clientInfo, undefined, config); + trackMCP( + "fetchRCA", + clientInfo, + new Error("Request failed with status code 401"), + config, + ); + return { content: [], isError: true }; + }, + ); + + expect(apiClient.post).toHaveBeenCalledTimes(1); + expect(rows()[0]).toMatchObject({ + success: false, + outcome: "error_result", + error_class: "auth_error", + error_message: "Request failed with status code 401", + }); + }); + + it("uses the config and client the handler passed when the wrapper had none", async () => { + await withToolCall( + "t", + () => ({}), + undefined, + async () => { + trackMCP("t", { name: "cursor" }, undefined, config); + return { content: [] }; + }, + ); + const call = (apiClient.post as any).mock.calls[0][0]; + expect(call.body.event_properties.mcp_client).toBe("cursor"); + expect(call.headers.Authorization).toMatch(/^Basic /); + }); + + it("rounds and clamps the duration", async () => { + await withToolCall( + "t", + () => clientInfo, + config, + () => ({ content: [] }), + ); + const d = rows()[0].duration_ms; + expect(Number.isInteger(d)).toBe(true); + expect(d).toBeGreaterThanOrEqual(0); + }); +}); + +describe("trackMCP outside an instrumented call", () => { + beforeEach(() => vi.clearAllMocks()); + + it("still posts the entry row immediately (heartbeat and unwrapped tools)", () => { + trackMCP("started", clientInfo, undefined, config); + expect(apiClient.post).toHaveBeenCalledTimes(1); + expect(rows()[0]).toMatchObject({ tool_name: "started", success: true }); + expect(rows()[0]).not.toHaveProperty("duration_ms"); + expect(rows()[0]).not.toHaveProperty("outcome"); + }); + + it("still posts the failure row immediately, with error_class", () => { + trackMCP( + "uploadAsset", + clientInfo, + new Error("x: 503 Service Unavailable"), + config, + ); + expect(rows()[0]).toMatchObject({ + success: false, + error_class: "server_error", + error_type: "Error", + }); + }); +}); diff --git a/tests/lib/tool-latency.test.ts b/tests/lib/tool-latency.test.ts index 47598185..3c5447a8 100644 --- a/tests/lib/tool-latency.test.ts +++ b/tests/lib/tool-latency.test.ts @@ -1,58 +1,146 @@ import { describe, it, expect, vi, beforeEach } from "vitest"; import { instrumentToolLatency } from "../../src/lib/tool-latency"; -import { trackMCPCompleted } from "../../src/lib/instrumentation"; +import { trackMCP } from "../../src/lib/instrumentation"; +import { apiClient } from "../../src/lib/apiClient"; -vi.mock("../../src/lib/instrumentation", () => ({ - trackMCPCompleted: vi.fn(), +vi.mock("../../src/lib/apiClient", () => ({ + apiClient: { post: vi.fn().mockResolvedValue({ status: 200, data: {} }) }, +})); +vi.mock("../../src/logger", () => ({ + default: { info: vi.fn(), error: vi.fn(), debug: vi.fn() }, +})); +vi.mock("../../src/config", () => ({ + default: { REMOTE_MCP: false }, })); const clientInfo = { name: "test-client", version: "1.0" }; const config = { "browserstack-username": "u", "browserstack-access-key": "k" }; +const posts = () => (apiClient.post as any).mock.calls.map((c: any) => c[0]); +const rows = () => posts().map((p: any) => p.body.event_properties); + function fakeTool(handler: unknown) { return { handler, enabled: true } as any; } -describe("instrumentToolLatency", () => { +/** A handler written the way every tool in src/tools is: trackMCP at entry, trackMCP in catch. */ +function toolLike(name: string, work: () => Promise) { + return async () => { + try { + trackMCP(name, clientInfo, undefined, config); + return await work(); + } catch (error) { + trackMCP(name, clientInfo, error, config); + return { + content: [{ type: "text", text: `Failed: ${error}` }], + isError: true, + }; + } + }; +} + +describe("instrumentToolLatency: one row per call", () => { beforeEach(() => vi.clearAllMocks()); - it("emits ok with a duration and returns the result untouched", async () => { + it("a successful call writes exactly one row, after the handler, with duration and outcome", async () => { const result = { content: [{ type: "text", text: "done" }] }; - const tools = { listTestCases: fakeTool(vi.fn().mockResolvedValue(result)) }; - + const tools = { + listTestCases: fakeTool(toolLike("listTestCases", async () => result)), + }; instrumentToolLatency(tools, () => clientInfo, config); + const out = await tools.listTestCases.handler({ projectId: "PR-1" }, {}); expect(out).toBe(result); - expect(trackMCPCompleted).toHaveBeenCalledTimes(1); - const [name, ci, completion, cfg] = (trackMCPCompleted as any).mock.calls[0]; - expect(name).toBe("listTestCases"); - expect(ci).toBe(clientInfo); - expect(completion.outcome).toBe("ok"); - expect(completion.durationMs).toBeGreaterThanOrEqual(0); - expect(cfg).toBe(config); + expect(apiClient.post).toHaveBeenCalledTimes(1); + const row = rows()[0]; + expect(row).toMatchObject({ + tool_name: "listTestCases", + mcp_client: "test-client", + success: true, + outcome: "ok", + is_remote: false, + }); + expect(row.duration_ms).toBeGreaterThanOrEqual(0); + expect(row).not.toHaveProperty("error_message"); + expect(row).not.toHaveProperty("phase"); + expect(posts()[0].headers.Authorization).toMatch(/^Basic /); }); - it("classifies an isError result as error_result", async () => { + it("a handler that caught an error writes one row with success=false, the error fields and error_result", async () => { const tools = { fetchBuildInsights: fakeTool( - vi.fn().mockResolvedValue({ content: [], isError: true }), + toolLike("fetchBuildInsights", async () => { + throw new Error( + "Failed to fetch from https://x/builds/1: 404 Not Found", + ); + }), ), }; instrumentToolLatency(tools, () => clientInfo, config); - await tools.fetchBuildInsights.handler({}, {}); - expect((trackMCPCompleted as any).mock.calls[0][2].outcome).toBe( - "error_result", - ); + + const out = await tools.fetchBuildInsights.handler({}, {}); + + expect(out.isError).toBe(true); + expect(apiClient.post).toHaveBeenCalledTimes(1); + expect(rows()[0]).toMatchObject({ + tool_name: "fetchBuildInsights", + success: false, + outcome: "error_result", + error_class: "not_found", + error_type: "Error", + }); + expect(rows()[0].error_message).toContain("404 Not Found"); + }); + + it("an isError result without a recorded error keeps success=true but marks outcome error_result", async () => { + const tools = { + invokeCapability: fakeTool(async () => ({ content: [], isError: true })), + }; + instrumentToolLatency(tools, () => clientInfo, config); + await tools.invokeCapability.handler({}, {}); + expect(rows()[0]).toMatchObject({ success: true, outcome: "error_result" }); }); - it("emits threw and rethrows when the handler throws", async () => { - const boom = new Error("upstream 500"); + it("a handler that throws writes one failure row with outcome threw and rethrows", async () => { + const boom = Object.assign(new Error("connect ECONNREFUSED"), { + code: "ECONNREFUSED", + }); const tools = { getFailureLogs: fakeTool(vi.fn().mockRejectedValue(boom)) }; instrumentToolLatency(tools, () => clientInfo, config); await expect(tools.getFailureLogs.handler({}, {})).rejects.toBe(boom); - expect((trackMCPCompleted as any).mock.calls[0][2].outcome).toBe("threw"); + expect(apiClient.post).toHaveBeenCalledTimes(1); + expect(rows()[0]).toMatchObject({ + success: false, + outcome: "threw", + error_class: "network", + }); + }); + + it("concurrent calls keep separate contexts", async () => { + let releaseA!: () => void; + const gateA = new Promise((r) => (releaseA = r)); + const tools = { + a: fakeTool( + toolLike("a", async () => { + await gateA; + throw new Error("a failed"); + }), + ), + b: fakeTool(toolLike("b", async () => ({ content: [] }))), + }; + instrumentToolLatency(tools, () => clientInfo, config); + + const pa = tools.a.handler({}, {}); + await tools.b.handler({}, {}); + releaseA(); + await pa; + + const byTool = Object.fromEntries(rows().map((r: any) => [r.tool_name, r])); + expect(apiClient.post).toHaveBeenCalledTimes(2); + expect(byTool.b).toMatchObject({ success: true, outcome: "ok" }); + expect(byTool.a).toMatchObject({ success: false, outcome: "error_result" }); }); it("passes every handler argument through (schema-less tools get only extra)", async () => { @@ -72,19 +160,16 @@ describe("instrumentToolLatency", () => { current = { name: "cursor", version: "2" }; await tools.t.handler({}, {}); - expect((trackMCPCompleted as any).mock.calls[0][1]).toEqual({ - name: "cursor", - version: "2", - }); + expect(rows()[0].mcp_client).toBe("cursor"); }); - it("is idempotent: wrapping twice emits one event per call", async () => { - const tools = { t: fakeTool(vi.fn().mockResolvedValue({ content: [] })) }; + it("is idempotent: wrapping twice still writes one row per call", async () => { + const tools = { t: fakeTool(toolLike("t", async () => ({ content: [] }))) }; instrumentToolLatency(tools, () => clientInfo, config); instrumentToolLatency(tools, () => clientInfo, config); await tools.t.handler({}, {}); - expect(trackMCPCompleted).toHaveBeenCalledTimes(1); + expect(apiClient.post).toHaveBeenCalledTimes(1); }); it("skips task-style handlers that are not functions", () => { @@ -95,7 +180,7 @@ describe("instrumentToolLatency", () => { }); it("never lets a telemetry failure affect the tool call", async () => { - (trackMCPCompleted as any).mockImplementation(() => { + (apiClient.post as any).mockImplementation(() => { throw new Error("telemetry down"); }); const result = { content: [] }; From 215a7fb3cd660fde2859548ba83572e4d0d6a436 Mon Sep 17 00:00:00 2001 From: Savio Dias Date: Wed, 23 Sep 2026 12:50:01 +0530 Subject: [PATCH 6/8] chore: drop node_modules symlink that slipped into the previous commit --- node_modules | 1 - 1 file changed, 1 deletion(-) delete mode 120000 node_modules diff --git a/node_modules b/node_modules deleted file mode 120000 index 6ebb6850..00000000 --- a/node_modules +++ /dev/null @@ -1 +0,0 @@ -/Users/saviodias/Projects/mcp-server/node_modules \ No newline at end of file From 4135d38d3607d37c2e9e7175a94f4ee29a363c73 Mon Sep 17 00:00:00 2001 From: Savio Dias Date: Wed, 23 Sep 2026 13:05:28 +0530 Subject: [PATCH 7/8] refactor(lib): trim comments in the single-row telemetry to the non-obvious ones --- src/lib/instrumentation.ts | 29 ++++++++++------------------- src/lib/tool-latency.ts | 8 +------- 2 files changed, 11 insertions(+), 26 deletions(-) diff --git a/src/lib/instrumentation.ts b/src/lib/instrumentation.ts index 4f87bf24..121dc389 100644 --- a/src/lib/instrumentation.ts +++ b/src/lib/instrumentation.ts @@ -53,8 +53,7 @@ function httpStatusOf(error: unknown): number | undefined { const e = error as { response?: { status?: unknown }; status?: unknown }; const direct = e?.response?.status ?? e?.status; if (typeof direct === "number") return direct; - // Plain Errors from utils carry the status only in the message: - // "Request failed with status code 404", "Failed to fetch from …: 404 Not Found" + // Errors from utils carry the status only in the message ("…: 404 Not Found"). const message = error instanceof Error ? error.message : String(error ?? ""); const m = message.match( /status code (\d{3})|: (\d{3}) [A-Z]|\bHTTP (\d{3})\b/, @@ -63,7 +62,7 @@ function httpStatusOf(error: unknown): number | undefined { return found ? Number(found) : undefined; } -/** Bucket a thrown error into a fixed set of causes, so failures group by class. */ +/** Fixed set of causes, so failures group by class instead of by message text. */ export function classifyError(error: unknown): ErrorClass { const e = error as { code?: unknown; name?: unknown; issues?: unknown }; if (typeof e?.code === "string") { @@ -128,10 +127,7 @@ function sendEvent(event: MCPEventPayload, config?: any): void { .catch(() => {}); } -/** - * State of one tool call while its handler runs. Lives in AsyncLocalStorage, so it is - * request-scoped: concurrent calls in the multi-tenant remote wrapper never share it. - */ +/** Per-call state; AsyncLocalStorage keeps concurrent (multi-tenant) calls apart. */ interface CallContext { toolName: string; clientInfo: ClientInfo; @@ -142,12 +138,9 @@ interface CallContext { const callContext = new AsyncLocalStorage(); /** - * Records a tool invocation or failure. - * - * Inside an instrumented call (see `withToolCall`) nothing is sent: the entry call and - * the catch-block call fold into the single row written when the handler settles. - * Outside one (the `started` heartbeat, tools a host registers without wrapping) it - * behaves as before and posts a row immediately. + * Inside `withToolCall` this only records into the call's context; the single row is + * written when the handler settles. Outside one (`started` heartbeat, unwrapped host + * tools) it posts a row immediately, as before. */ export function trackMCP( toolName: string, @@ -191,10 +184,8 @@ function isErrorResult(result: unknown): boolean { } /** - * Runs a tool handler and writes exactly one MCPInstrumentation row when it settles: - * `success` (false when the handler reported or threw an error), `duration_ms`, - * `outcome` (ok / error_result / threw) and the error fields on failures. - * Telemetry never affects the call: the result is passed through, throws are rethrown. + * Runs a tool handler and writes one MCPInstrumentation row when it settles: success, + * duration_ms, outcome (ok / error_result / threw), error fields on failure. */ export async function withToolCall( toolName: string, @@ -220,7 +211,7 @@ export async function withToolCall( const live = getClientInfo(); if (live?.name) clientInfo = live; } catch { - // client info is optional + /* client info is optional */ } const event: MCPEventPayload = { event_type: "MCPInstrumentation", @@ -234,7 +225,7 @@ export async function withToolCall( }; sendEvent(event, ctx.config ?? config); } catch { - // Telemetry must never affect the tool call. + /* telemetry must never affect the call */ } } } diff --git a/src/lib/tool-latency.ts b/src/lib/tool-latency.ts index 94798d70..54bb8b3c 100644 --- a/src/lib/tool-latency.ts +++ b/src/lib/tool-latency.ts @@ -5,13 +5,7 @@ const WRAPPED = Symbol.for("browserstack.mcp.latencyWrapped"); type AnyHandler = (...args: unknown[]) => unknown; -/** - * Wraps every registered tool handler in `withToolCall`, so each call writes exactly - * one MCPInstrumentation row when it settles (success, duration_ms, outcome, error - * fields). The tools' own entry and catch-block `trackMCP` calls fold into that row. - * Transparent (result passed through, throws rethrown), idempotent, skips task-style - * (non-function) handlers. - */ +/** Wraps every function handler in `withToolCall`. Idempotent; skips task-style handlers. */ export function instrumentToolLatency( tools: Record, getClientInfo: () => ClientInfo, From e8f94c5ec0598dd940ea5aad7c3bea512e4697f5 Mon Sep 17 00:00:00 2001 From: Savio Dias Date: Wed, 23 Sep 2026 19:02:57 +0530 Subject: [PATCH 8/8] =?UTF-8?q?refactor(lib):=20drop=20error=5Fclass=20?= =?UTF-8?q?=E2=80=94=20it=20classified=2098.8%=20of=20real=20failures=20as?= =?UTF-8?q?=20"unknown"?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Checked the classifier against 30 days of production failure messages rather than the synthetic errors it was tested with. Of the 13 most common failure messages (1,718 failures), exactly one classified into a real bucket; everything else fell to "unknown". The cause is upstream of the classifier: utilities catch the API error and re-throw a human-readable string, discarding `code` and `response.status`. So classification falls to message matching, and the patterns ("unauthorized", "forbidden", "timed out") do not match the product's actual vocabulary ("is required", "you must provide", "you do not have access to"). Shipping a field that is 99% one value is worse than not shipping it. The row keeps error_message and error_type, which is what readers use today. Revisit either by tuning patterns to the real vocabulary, or by preserving the status at the throw sites so the classifier gets structured data instead of prose. Co-Authored-By: Claude Opus 5 (1M context) --- src/lib/instrumentation.ts | 60 ----------- tests/lib/error-class.test.ts | 107 ------------------- tests/lib/instrumentation-single-row.test.ts | 4 +- tests/lib/tool-latency.test.ts | 2 - 4 files changed, 1 insertion(+), 172 deletions(-) delete mode 100644 tests/lib/error-class.test.ts diff --git a/src/lib/instrumentation.ts b/src/lib/instrumentation.ts index 121dc389..f47b7261 100644 --- a/src/lib/instrumentation.ts +++ b/src/lib/instrumentation.ts @@ -13,16 +13,6 @@ export type ClientInfo = { name?: string; version?: string }; export type ToolOutcome = "ok" | "error_result" | "threw"; -export type ErrorClass = - | "auth_error" - | "not_found" - | "rate_limited" - | "validation" - | "timeout" - | "network" - | "server_error" - | "unknown"; - interface MCPEventPayload { event_type: string; event_properties: { @@ -33,61 +23,12 @@ interface MCPEventPayload { success?: boolean; error_message?: string; error_type?: string; - error_class?: ErrorClass; is_remote?: boolean; duration_ms?: number; outcome?: ToolOutcome; }; } -const TIMEOUT_CODES = new Set(["ECONNABORTED", "ETIMEDOUT"]); -const NETWORK_CODES = new Set([ - "ECONNREFUSED", - "ECONNRESET", - "ENOTFOUND", - "EAI_AGAIN", - "EPIPE", -]); - -function httpStatusOf(error: unknown): number | undefined { - const e = error as { response?: { status?: unknown }; status?: unknown }; - const direct = e?.response?.status ?? e?.status; - if (typeof direct === "number") return direct; - // Errors from utils carry the status only in the message ("…: 404 Not Found"). - const message = error instanceof Error ? error.message : String(error ?? ""); - const m = message.match( - /status code (\d{3})|: (\d{3}) [A-Z]|\bHTTP (\d{3})\b/, - ); - const found = m && (m[1] || m[2] || m[3]); - return found ? Number(found) : undefined; -} - -/** Fixed set of causes, so failures group by class instead of by message text. */ -export function classifyError(error: unknown): ErrorClass { - const e = error as { code?: unknown; name?: unknown; issues?: unknown }; - if (typeof e?.code === "string") { - if (TIMEOUT_CODES.has(e.code)) return "timeout"; - if (NETWORK_CODES.has(e.code)) return "network"; - } - if (e?.name === "ZodError" || Array.isArray(e?.issues)) return "validation"; - - const status = httpStatusOf(error); - if (status === 401 || status === 403) return "auth_error"; - if (status === 404) return "not_found"; - if (status === 429) return "rate_limited"; - if (status === 408 || status === 504) return "timeout"; - if (status === 400 || status === 422) return "validation"; - if (status !== undefined && status >= 500) return "server_error"; - - const message = ( - error instanceof Error ? error.message : String(error ?? "") - ).toLowerCase(); - if (/timed? ?out/.test(message)) return "timeout"; - if (/not enabled for|unauthori[sz]ed|forbidden/.test(message)) - return "auth_error"; - return "unknown"; -} - function baseProperties(toolName: string, clientInfo: ClientInfo) { return { mcp_version: packageJson.version as string, @@ -102,7 +43,6 @@ function errorProperties(error: unknown) { return { error_message: error instanceof Error ? error.message : String(error), error_type: error instanceof Error ? error.constructor.name : "Unknown", - error_class: classifyError(error), }; } diff --git a/tests/lib/error-class.test.ts b/tests/lib/error-class.test.ts deleted file mode 100644 index 06c6568c..00000000 --- a/tests/lib/error-class.test.ts +++ /dev/null @@ -1,107 +0,0 @@ -import { describe, it, expect, vi, beforeEach } from "vitest"; -import { classifyError, trackMCP } from "../../src/lib/instrumentation"; -import { apiClient } from "../../src/lib/apiClient"; - -vi.mock("../../src/lib/apiClient", () => ({ - apiClient: { post: vi.fn().mockResolvedValue({ status: 200, data: {} }) }, -})); -vi.mock("../../src/logger", () => ({ - default: { info: vi.fn(), error: vi.fn(), debug: vi.fn() }, -})); -vi.mock("../../src/config", () => ({ default: { REMOTE_MCP: false } })); - -function axiosLike(status: number, code?: string) { - const err: any = new Error(`Request failed with status code ${status}`); - err.response = { status }; - if (code) err.code = code; - return err; -} - -describe("classifyError", () => { - it.each([ - [axiosLike(401), "auth_error"], - [axiosLike(403), "auth_error"], - [axiosLike(404), "not_found"], - [axiosLike(429), "rate_limited"], - [axiosLike(400), "validation"], - [axiosLike(422), "validation"], - [axiosLike(500), "server_error"], - [axiosLike(503), "server_error"], - [axiosLike(504), "timeout"], - ])("maps HTTP status on the error object (%#)", (err, expected) => { - expect(classifyError(err)).toBe(expected); - }); - - it("reads the status out of plain Error messages thrown by utils", () => { - expect( - classifyError( - new Error( - "Failed to fetch from https://api-automation.browserstack.com/ext/v1/builds/junk: 404 Not Found", - ), - ), - ).toBe("not_found"); - expect( - classifyError(new Error("Request failed with status code 401")), - ).toBe("auth_error"); - }); - - it("prefers the transport code over any status", () => { - const err: any = new Error("timeout of 2000ms exceeded"); - err.code = "ECONNABORTED"; - expect(classifyError(err)).toBe("timeout"); - const refused: any = new Error("connect ECONNREFUSED 10.0.0.1:443"); - refused.code = "ECONNREFUSED"; - expect(classifyError(refused)).toBe("network"); - }); - - it("treats Zod errors as validation", () => { - const zod: any = new Error("Invalid input"); - zod.name = "ZodError"; - zod.issues = []; - expect(classifyError(zod)).toBe("validation"); - }); - - it("classifies entitlement refusals and message-only timeouts", () => { - expect( - classifyError( - new Error("BrowserStack AI is not enabled for `tm` on your account."), - ), - ).toBe("auth_error"); - expect(classifyError(new Error("Scan timed out after 300s"))).toBe( - "timeout", - ); - }); - - it("falls back to unknown for anything else, including non-Errors", () => { - expect(classifyError(new Error("Converting circular structure to JSON"))).toBe( - "unknown", - ); - expect(classifyError("some string")).toBe("unknown"); - expect(classifyError(undefined)).toBe("unknown"); - }); -}); - -describe("trackMCP failure row", () => { - beforeEach(() => vi.clearAllMocks()); - - it("adds error_class next to the existing error fields", () => { - trackMCP("fetchBuildInsights", { name: "c" }, axiosLike(404), { - "browserstack-username": "u", - "browserstack-access-key": "k", - }); - const props = (apiClient.post as any).mock.calls[0][0].body.event_properties; - expect(props.success).toBe(false); - expect(props.error_type).toBe("Error"); - expect(props.error_message).toBe("Request failed with status code 404"); - expect(props.error_class).toBe("not_found"); - }); - - it("does not add error_class to the success row", () => { - trackMCP("listTestCases", { name: "c" }, undefined, { - "browserstack-username": "u", - "browserstack-access-key": "k", - }); - const props = (apiClient.post as any).mock.calls[0][0].body.event_properties; - expect(props).not.toHaveProperty("error_class"); - }); -}); diff --git a/tests/lib/instrumentation-single-row.test.ts b/tests/lib/instrumentation-single-row.test.ts index 3be8abe2..8354517a 100644 --- a/tests/lib/instrumentation-single-row.test.ts +++ b/tests/lib/instrumentation-single-row.test.ts @@ -75,7 +75,6 @@ describe("withToolCall", () => { expect(rows()[0]).toMatchObject({ success: false, outcome: "error_result", - error_class: "auth_error", error_message: "Request failed with status code 401", }); }); @@ -119,7 +118,7 @@ describe("trackMCP outside an instrumented call", () => { expect(rows()[0]).not.toHaveProperty("outcome"); }); - it("still posts the failure row immediately, with error_class", () => { + it("still posts the failure row immediately", () => { trackMCP( "uploadAsset", clientInfo, @@ -128,7 +127,6 @@ describe("trackMCP outside an instrumented call", () => { ); expect(rows()[0]).toMatchObject({ success: false, - error_class: "server_error", error_type: "Error", }); }); diff --git a/tests/lib/tool-latency.test.ts b/tests/lib/tool-latency.test.ts index 3c5447a8..b0ef4e15 100644 --- a/tests/lib/tool-latency.test.ts +++ b/tests/lib/tool-latency.test.ts @@ -87,7 +87,6 @@ describe("instrumentToolLatency: one row per call", () => { tool_name: "fetchBuildInsights", success: false, outcome: "error_result", - error_class: "not_found", error_type: "Error", }); expect(rows()[0].error_message).toContain("404 Not Found"); @@ -114,7 +113,6 @@ describe("instrumentToolLatency: one row per call", () => { expect(rows()[0]).toMatchObject({ success: false, outcome: "threw", - error_class: "network", }); });