diff --git a/src/lib/telemetry.ts b/src/lib/telemetry.ts index acfbe516..e74bd6d9 100644 --- a/src/lib/telemetry.ts +++ b/src/lib/telemetry.ts @@ -13,7 +13,7 @@ import * as Sentry from "@sentry/bun"; import { CLI_VERSION, SENTRY_CLI_DSN } from "./constants.js"; import { tryRepairAndRetry } from "./db/schema.js"; -import { AuthError } from "./errors.js"; +import { ApiError, AuthError } from "./errors.js"; import { getSentryBaseUrl, isSentrySaasUrl } from "./sentry-urls.js"; export type { Span } from "@sentry/bun"; @@ -102,17 +102,25 @@ export async function withTelemetry( async (span) => { try { return await callback(span); + } catch (e) { + // Record 4xx API errors as span attributes instead of exceptions. + // These are user errors (wrong ID, no access) not CLI bugs, but + // recording on the span lets us detect volume spikes in Discover. + if (isClientApiError(e)) { + recordApiErrorOnSpan(span, e as ApiError); + } + throw e; } finally { span.end(); } } ); } catch (e) { - // Don't capture or mark as crashed for expected auth state - // AuthError("not_authenticated") is re-thrown from app.ts for auto-login flow const isExpectedAuthState = e instanceof AuthError && e.reason === "not_authenticated"; - if (!isExpectedAuthState) { + // 4xx API errors are user errors (wrong ID, no access), not CLI bugs. + // They're recorded as span attributes above for volume-spike detection. + if (!(isExpectedAuthState || isClientApiError(e))) { Sentry.captureException(e); markSessionCrashed(); } @@ -190,6 +198,35 @@ export function isEpipeError(event: Sentry.ErrorEvent): boolean { return false; } +/** + * Check if an error is a client-side (4xx) API error. + * + * 4xx errors are user errors — wrong issue IDs, no access, invalid input — + * not CLI bugs. These should be recorded as span attributes for volume-spike + * detection in Discover, but should NOT be captured as Sentry exceptions. + * + * @internal Exported for testing + */ +export function isClientApiError(error: unknown): boolean { + return error instanceof ApiError && error.status >= 400 && error.status < 500; +} + +/** + * Record a client API error as span attributes for Discover queryability. + * + * Sets `api_error.status`, `api_error.message`, and optionally `api_error.detail` + * on the span. Must be called before `span.end()`. + * + * @internal Exported for testing + */ +export function recordApiErrorOnSpan(span: Span, error: ApiError): void { + span.setAttribute("api_error.status", error.status); + span.setAttribute("api_error.message", error.message); + if (error.detail) { + span.setAttribute("api_error.detail", error.detail); + } +} + /** * Integrations to exclude for CLI. * These add overhead without benefit for short-lived CLI processes. diff --git a/test/lib/telemetry.test.ts b/test/lib/telemetry.test.ts index e75f2b4b..00279d05 100644 --- a/test/lib/telemetry.test.ts +++ b/test/lib/telemetry.test.ts @@ -8,9 +8,12 @@ import { Database } from "bun:sqlite"; import { afterEach, beforeEach, describe, expect, spyOn, test } from "bun:test"; // biome-ignore lint/performance/noNamespaceImport: needed for spyOn mocking import * as Sentry from "@sentry/bun"; +import { ApiError, AuthError } from "../../src/lib/errors.js"; import { createTracedDatabase, initSentry, + isClientApiError, + recordApiErrorOnSpan, setArgsContext, setCommandSpanName, setFlagContext, @@ -126,6 +129,158 @@ describe("withTelemetry", () => { }); expect(executed).toBe(true); }); + + test("propagates 4xx ApiError to caller", async () => { + const error = new ApiError("Not found", 404, "Issue not found"); + await expect( + withTelemetry(() => { + throw error; + }) + ).rejects.toThrow(error); + }); + + describe("with telemetry enabled", () => { + beforeEach(() => { + delete process.env[ENV_VAR]; + }); + + afterEach(() => { + // Re-init with enabled=false to reset global SDK state. + // Without this, Sentry.isEnabled() returns true for all + // subsequent test files (e.g. feedbackCommand checks it). + initSentry(false); + }); + + test("propagates 4xx ApiError through enabled SDK path", async () => { + const error = new ApiError("Not found", 404, "Issue not found"); + await expect( + withTelemetry(() => { + throw error; + }) + ).rejects.toThrow(error); + }); + + test("propagates 5xx ApiError through enabled SDK path", async () => { + const error = new ApiError("Server error", 500, "Internal error"); + await expect( + withTelemetry(() => { + throw error; + }) + ).rejects.toThrow(error); + }); + + test("propagates generic Error through enabled SDK path", async () => { + await expect( + withTelemetry(() => { + throw new Error("unexpected bug"); + }) + ).rejects.toThrow("unexpected bug"); + }); + + test("returns result through enabled SDK path", async () => { + const result = await withTelemetry(() => 42); + expect(result).toBe(42); + }); + }); +}); + +describe("isClientApiError", () => { + test("returns true for 400 Bad Request", () => { + expect(isClientApiError(new ApiError("Bad request", 400))).toBe(true); + }); + + test("returns true for 403 Forbidden", () => { + expect(isClientApiError(new ApiError("Forbidden", 403, "No access"))).toBe( + true + ); + }); + + test("returns true for 404 Not Found", () => { + expect( + isClientApiError(new ApiError("Not found", 404, "Issue not found")) + ).toBe(true); + }); + + test("returns true for 429 Too Many Requests", () => { + expect(isClientApiError(new ApiError("Rate limited", 429))).toBe(true); + }); + + test("returns false for 500 Internal Server Error", () => { + expect(isClientApiError(new ApiError("Server error", 500))).toBe(false); + }); + + test("returns false for 502 Bad Gateway", () => { + expect(isClientApiError(new ApiError("Bad gateway", 502))).toBe(false); + }); + + test("returns false for non-ApiError", () => { + expect(isClientApiError(new Error("generic error"))).toBe(false); + }); + + test("returns false for AuthError", () => { + expect(isClientApiError(new AuthError("not_authenticated"))).toBe(false); + }); + + test("returns false for null/undefined", () => { + expect(isClientApiError(null)).toBe(false); + expect(isClientApiError(undefined)).toBe(false); + }); + + test("returns false for non-Error objects", () => { + expect(isClientApiError({ status: 404 })).toBe(false); + expect(isClientApiError("404")).toBe(false); + }); +}); + +describe("recordApiErrorOnSpan", () => { + function createMockSpan() { + const attributes: Record = {}; + return { + attributes, + setAttribute(key: string, value: string | number) { + attributes[key] = value; + }, + }; + } + + test("sets status and message attributes", () => { + const span = createMockSpan(); + const error = new ApiError("Not found", 404); + recordApiErrorOnSpan(span as never, error); + + expect(span.attributes["api_error.status"]).toBe(404); + expect(span.attributes["api_error.message"]).toBe("Not found"); + expect(span.attributes["api_error.detail"]).toBeUndefined(); + }); + + test("sets detail attribute when present", () => { + const span = createMockSpan(); + const error = new ApiError("Not found", 404, "Issue not found"); + recordApiErrorOnSpan(span as never, error); + + expect(span.attributes["api_error.status"]).toBe(404); + expect(span.attributes["api_error.message"]).toBe("Not found"); + expect(span.attributes["api_error.detail"]).toBe("Issue not found"); + }); + + test("omits detail attribute when empty string", () => { + const span = createMockSpan(); + const error = new ApiError("Bad request", 400, ""); + recordApiErrorOnSpan(span as never, error); + + expect(span.attributes["api_error.status"]).toBe(400); + expect(span.attributes["api_error.detail"]).toBeUndefined(); + }); + + test("handles different 4xx status codes", () => { + const span = createMockSpan(); + const error = new ApiError("Forbidden", 403, "No access"); + recordApiErrorOnSpan(span as never, error); + + expect(span.attributes["api_error.status"]).toBe(403); + expect(span.attributes["api_error.message"]).toBe("Forbidden"); + expect(span.attributes["api_error.detail"]).toBe("No access"); + }); }); describe("setCommandSpanName", () => {