diff --git a/review-enrichment/src/sentry.ts b/review-enrichment/src/sentry.ts index 4904ea0658..10ebed8200 100644 --- a/review-enrichment/src/sentry.ts +++ b/review-enrichment/src/sentry.ts @@ -309,7 +309,14 @@ export function captureAnalyzerDegradation(error: unknown, context: AnalyzerDegr release: activeRelease, environment: activeEnvironment, }, - fingerprint: ["rees-analyzer-degraded", context.analyzer], + // Group by WHY (partialReason, e.g. "analyzer_timeout"), not WHICH analyzer hit it (#5010): the generic + // reasons genuinely share one root cause (the shared, dynamically-shrinking per-analyzer time budget) + // regardless of which analyzer's turn it was, so grouping by analyzer name fragmented one condition into + // N issues (one per analyzer) that each individually looked small. A reason that IS inherently + // analyzer-specific (e.g. "bundlephobia-size_http_error") stays its own issue either way, since the + // reason string itself already encodes that specificity -- falls back to analyzer name only on the + // defensive case where partialReason is somehow absent. + fingerprint: ["rees-analyzer-degraded", context.partialReason ?? context.analyzer], tags: { event: "rees_analyzer_degraded", analyzer: context.analyzer, diff --git a/review-enrichment/test/sentry-degradation.test.ts b/review-enrichment/test/sentry-degradation.test.ts index 2b6605fd5c..e32685fccd 100644 --- a/review-enrichment/test/sentry-degradation.test.ts +++ b/review-enrichment/test/sentry-degradation.test.ts @@ -100,6 +100,49 @@ test("captureAnalyzerDegradation tags and fingerprints sanitized analyzer failur assert.equal(serializedContext.includes("Bearer should_never_be_attached"), false); }); +test("captureAnalyzerDegradation groups by partialReason (WHY), not analyzer name (WHICH), so the same reason from different analyzers is one issue (#5010)", () => { + const sentry = sentryHarness(); + + captureAnalyzerDegradation(new Error("analyzer_timeout"), { + analyzer: "installScript", + repoFullName: "JSONbored/gittensory", + prNumber: 7, + headSha: "abc123", + timeoutMs: 1400, + partialReason: "analyzer_timeout", + } as never); + captureAnalyzerDegradation(new Error("analyzer_timeout"), { + analyzer: "nativeBuild", + repoFullName: "JSONbored/gittensory", + prNumber: 8, + headSha: "def456", + timeoutMs: 1400, + partialReason: "analyzer_timeout", + } as never); + + // Same fingerprint from two DIFFERENT analyzers: both group into one Sentry issue. + assert.deepEqual(sentry.fingerprints, [ + ["rees-analyzer-degraded", "analyzer_timeout"], + ["rees-analyzer-degraded", "analyzer_timeout"], + ]); + // The specific analyzer is still fully visible via the tag -- only the GROUPING changed. + assert.equal(sentry.tags.analyzer, "nativeBuild"); +}); + +test("captureAnalyzerDegradation falls back to analyzer name when partialReason is absent", () => { + const sentry = sentryHarness(); + + captureAnalyzerDegradation(new Error("boom"), { + analyzer: "dependency", + repoFullName: "JSONbored/gittensory", + prNumber: 7, + headSha: "abc123", + timeoutMs: 8000, + }); + + assert.deepEqual(sentry.fingerprints, [["rees-analyzer-degraded", "dependency"]]); +}); + test("captureAnalyzerDegradation filters tag values before sending them", () => { const sentry = sentryHarness(); const secretLikeValue = ["ghp", "abcdefghijklmnopqrstuvwxyz1234567890"].join("_"); diff --git a/src/selfhost/sentry.ts b/src/selfhost/sentry.ts index 31114452b3..cffa023dbd 100644 --- a/src/selfhost/sentry.ts +++ b/src/selfhost/sentry.ts @@ -444,7 +444,11 @@ function namedCaptureError(error: unknown, eventName?: string): Error { } /** Capture an error with optional structured context. No-op when Sentry is off. `eventName`, when given, becomes - * the Sentry issue title's prefix (see {@link namedCaptureError}) instead of the generic "Error". */ + * the Sentry issue title's prefix (see {@link namedCaptureError}) AND the grouping fingerprint (#5010) -- + * Sentry's default stack-trace-based grouping fragments the SAME logical failure into separate issues whenever + * it is captured from more than one call site (e.g. two different functions each constructing the identical + * `new Error("...")` message), which is exactly what happened to GITTENSORY-5/10 and GITTENSORY-C/W before this. + * Mirrors forwardStructuredLogToSentry's identical `scope.setFingerprint(["gittensory-log", event])` discipline. */ export function captureError( error: unknown, context?: Record, @@ -454,13 +458,15 @@ export function captureError( Sentry.withScope((scope) => { setOtelTraceScope(scope); if (context) { const safeContext = hashedInstallationContext(context); scope.setContext("gittensory", safeContext); applyOperationalTags(scope, safeContext); } + if (eventName) scope.setFingerprint(["gittensory-error", eventName]); Sentry!.captureException(namedCaptureError(error, eventName)); }); } /** Capture a failed review at ERROR level, tagged by repo/PR/SHA for triage. A review that cannot be produced is a * real failure the maintainer must SEE — not a warning that hides in the noise. No-op when off. `eventName`, when - * given, becomes the Sentry issue title's prefix (see {@link namedCaptureError}) instead of the generic "Error". */ + * given, becomes the Sentry issue title's prefix AND the grouping fingerprint -- see {@link captureError}'s + * identical discipline and #5010. */ export function captureReviewFailure( error: unknown, context?: Record, @@ -475,6 +481,7 @@ export function captureReviewFailure( scope.setContext("review", safeContext); applyOperationalTags(scope, safeContext); } + if (eventName) scope.setFingerprint(["gittensory-review-failure", eventName]); Sentry!.captureException(namedCaptureError(error, eventName)); }); } diff --git a/test/unit/selfhost-sentry.test.ts b/test/unit/selfhost-sentry.test.ts index 323becf4fe..b5f9a9a405 100644 --- a/test/unit/selfhost-sentry.test.ts +++ b/test/unit/selfhost-sentry.test.ts @@ -578,14 +578,15 @@ describe("enabled when SENTRY_DSN is set", () => { expect(mocks.captureException).toHaveBeenCalledTimes(2); }); - it("captureError with an eventName renames the captured Error so the Sentry title isn't the generic 'Error'", async () => { + it("captureError with an eventName renames the captured Error so the Sentry title isn't the generic 'Error', and groups it by that same name (#5010)", async () => { await initSentry({ SENTRY_DSN: "d" } as unknown as NodeJS.ProcessEnv); captureError(new Error("self-host queue processing lease expired"), { kind: "job_dead" }, "processing_timeout"); expect(lastCapturedError().name).toBe("processing_timeout"); expect(lastCapturedError().message).toBe("self-host queue processing lease expired"); + expect(mocks.scope.setFingerprint).toHaveBeenCalledWith(["gittensory-error", "processing_timeout"]); }); - it("captureError without an eventName leaves a caught exception's own name untouched", async () => { + it("captureError without an eventName leaves a caught exception's own name untouched, and never overrides Sentry's default grouping", async () => { await initSentry({ SENTRY_DSN: "d" } as unknown as NodeJS.ProcessEnv); class HttpError extends Error { constructor(message: string) { @@ -595,12 +596,20 @@ describe("enabled when SENTRY_DSN is set", () => { } captureError(new HttpError("merge already in progress"), { kind: "agent_merge_blocked" }); expect(lastCapturedError().name).toBe("HttpError"); + expect(mocks.scope.setFingerprint).not.toHaveBeenCalled(); }); - it("captureReviewFailure with an eventName renames the captured Error the same way captureError does", async () => { + it("captureReviewFailure with an eventName renames the captured Error and groups it the same way captureError does (#5010) -- this is what consolidates the same failure captured from two different call sites (GITTENSORY-5/10, GITTENSORY-C/W) into one issue", async () => { await initSentry({ SENTRY_DSN: "d" } as unknown as NodeJS.ProcessEnv); captureReviewFailure(new Error("AI review inconclusive — no usable verdict for the PR head"), { repo: "o/r" }, "ai_review_inconclusive"); expect(lastCapturedError().name).toBe("ai_review_inconclusive"); + expect(mocks.scope.setFingerprint).toHaveBeenCalledWith(["gittensory-review-failure", "ai_review_inconclusive"]); + }); + + it("captureReviewFailure without an eventName never overrides Sentry's default grouping", async () => { + await initSentry({ SENTRY_DSN: "d" } as unknown as NodeJS.ProcessEnv); + captureReviewFailure(new Error("rev"), { repo: "o/r" }); + expect(mocks.scope.setFingerprint).not.toHaveBeenCalled(); }); it("captureError/captureReviewFailure with an eventName still names a non-Error value's synthesized Error", async () => {