Skip to content

Commit f3a6c61

Browse files
committed
fix(telemetry): skip Sentry reporting for 4xx API errors
4xx API errors (404 Not Found, 403 Forbidden, etc.) are user errors — wrong IDs, missing access — not CLI bugs. Recording them as exceptions creates noise in the Sentry issue stream without being actionable. Instead of capturing these as exceptions, record them as span attributes (api_error.status, api_error.message, api_error.detail) so they remain queryable in Discover for volume-spike detection without polluting the error feed. Extract isClientApiError() helper for classifying 4xx errors, following the same pattern as the existing isEpipeError() helper. Fixes CLI-5M Fixes CLI-46
1 parent 2b25152 commit f3a6c61

File tree

2 files changed

+89
-4
lines changed

2 files changed

+89
-4
lines changed

src/lib/telemetry.ts

Lines changed: 30 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,7 @@
1313
import * as Sentry from "@sentry/bun";
1414
import { CLI_VERSION, SENTRY_CLI_DSN } from "./constants.js";
1515
import { tryRepairAndRetry } from "./db/schema.js";
16-
import { AuthError } from "./errors.js";
16+
import { ApiError, AuthError } from "./errors.js";
1717
import { getSentryBaseUrl, isSentrySaasUrl } from "./sentry-urls.js";
1818

1919
export type { Span } from "@sentry/bun";
@@ -102,17 +102,30 @@ export async function withTelemetry<T>(
102102
async (span) => {
103103
try {
104104
return await callback(span);
105+
} catch (e) {
106+
// Record 4xx API errors as span attributes instead of exceptions.
107+
// These are user errors (wrong ID, no access) not CLI bugs, but
108+
// recording on the span lets us detect volume spikes in Discover.
109+
if (isClientApiError(e)) {
110+
span.setAttribute("api_error.status", (e as ApiError).status);
111+
span.setAttribute("api_error.message", (e as ApiError).message);
112+
const detail = (e as ApiError).detail;
113+
if (detail) {
114+
span.setAttribute("api_error.detail", detail);
115+
}
116+
}
117+
throw e;
105118
} finally {
106119
span.end();
107120
}
108121
}
109122
);
110123
} catch (e) {
111-
// Don't capture or mark as crashed for expected auth state
112-
// AuthError("not_authenticated") is re-thrown from app.ts for auto-login flow
113124
const isExpectedAuthState =
114125
e instanceof AuthError && e.reason === "not_authenticated";
115-
if (!isExpectedAuthState) {
126+
// 4xx API errors are user errors (wrong ID, no access), not CLI bugs.
127+
// They're recorded as span attributes above for volume-spike detection.
128+
if (!(isExpectedAuthState || isClientApiError(e))) {
116129
Sentry.captureException(e);
117130
markSessionCrashed();
118131
}
@@ -190,6 +203,19 @@ export function isEpipeError(event: Sentry.ErrorEvent): boolean {
190203
return false;
191204
}
192205

206+
/**
207+
* Check if an error is a client-side (4xx) API error.
208+
*
209+
* 4xx errors are user errors — wrong issue IDs, no access, invalid input —
210+
* not CLI bugs. These should be recorded as span attributes for volume-spike
211+
* detection in Discover, but should NOT be captured as Sentry exceptions.
212+
*
213+
* @internal Exported for testing
214+
*/
215+
export function isClientApiError(error: unknown): boolean {
216+
return error instanceof ApiError && error.status >= 400 && error.status < 500;
217+
}
218+
193219
/**
194220
* Integrations to exclude for CLI.
195221
* These add overhead without benefit for short-lived CLI processes.

test/lib/telemetry.test.ts

Lines changed: 59 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,9 +8,11 @@ import { Database } from "bun:sqlite";
88
import { afterEach, beforeEach, describe, expect, spyOn, test } from "bun:test";
99
// biome-ignore lint/performance/noNamespaceImport: needed for spyOn mocking
1010
import * as Sentry from "@sentry/bun";
11+
import { ApiError, AuthError } from "../../src/lib/errors.js";
1112
import {
1213
createTracedDatabase,
1314
initSentry,
15+
isClientApiError,
1416
setArgsContext,
1517
setCommandSpanName,
1618
setFlagContext,
@@ -126,6 +128,63 @@ describe("withTelemetry", () => {
126128
});
127129
expect(executed).toBe(true);
128130
});
131+
132+
test("propagates 4xx ApiError to caller", async () => {
133+
const error = new ApiError("Not found", 404, "Issue not found");
134+
await expect(
135+
withTelemetry(() => {
136+
throw error;
137+
})
138+
).rejects.toThrow(error);
139+
});
140+
});
141+
142+
describe("isClientApiError", () => {
143+
test("returns true for 400 Bad Request", () => {
144+
expect(isClientApiError(new ApiError("Bad request", 400))).toBe(true);
145+
});
146+
147+
test("returns true for 403 Forbidden", () => {
148+
expect(isClientApiError(new ApiError("Forbidden", 403, "No access"))).toBe(
149+
true
150+
);
151+
});
152+
153+
test("returns true for 404 Not Found", () => {
154+
expect(
155+
isClientApiError(new ApiError("Not found", 404, "Issue not found"))
156+
).toBe(true);
157+
});
158+
159+
test("returns true for 429 Too Many Requests", () => {
160+
expect(isClientApiError(new ApiError("Rate limited", 429))).toBe(true);
161+
});
162+
163+
test("returns false for 500 Internal Server Error", () => {
164+
expect(isClientApiError(new ApiError("Server error", 500))).toBe(false);
165+
});
166+
167+
test("returns false for 502 Bad Gateway", () => {
168+
expect(isClientApiError(new ApiError("Bad gateway", 502))).toBe(false);
169+
});
170+
171+
test("returns false for non-ApiError", () => {
172+
expect(isClientApiError(new Error("generic error"))).toBe(false);
173+
});
174+
175+
test("returns false for AuthError", () => {
176+
expect(isClientApiError(new AuthError("not_authenticated"))).toBe(false);
177+
});
178+
179+
test("returns false for null/undefined", () => {
180+
expect(isClientApiError(null)).toBe(false);
181+
expect(isClientApiError(undefined)).toBe(false);
182+
});
183+
184+
test("returns false for non-Error objects", () => {
185+
expect(isClientApiError({ status: 404 })).toBe(false);
186+
expect(isClientApiError("404")).toBe(false);
187+
});
129188
});
130189

131190
describe("setCommandSpanName", () => {

0 commit comments

Comments
 (0)