diff --git a/docs/decisions/adr-014-instrumentation-sentry.md b/docs/decisions/adr-014-instrumentation-sentry.md index 5adba53..dfef6de 100644 --- a/docs/decisions/adr-014-instrumentation-sentry.md +++ b/docs/decisions/adr-014-instrumentation-sentry.md @@ -68,6 +68,20 @@ The Lazar Nikolov reference repo (`nextjs-clean-architecture`) demonstrates a Se - **Pre-existing lint nits surfaced when Task 28's restricted-imports rule lit up `pnpm lint`:** added `argsIgnorePattern: "^_"` to the shared eslint config (matches the underscore convention used throughout the repo); added `globals.node` for `*.{mjs,cjs,js}` and `*.config.{ts,tsx}` so `next.config.mjs`'s `process.env` lints clean; cleared two unused-import / unused-disable nits in marketing-pages and core-testing that were unrelated to instrumentation but blocked the lint gate. - **Direct `@repo/core-shared` deps:** `apps/cms` and `apps/web-tanstack` previously had only transitive access to `@repo/core-shared`; both gained explicit `workspace:*` deps so the deep `./instrumentation/sentry/*` subpath imports resolve. +## Post-merge follow-up — closing the R44 gap + +Plan 10 as merged shipped repository-side capture (R43) but **not** use-case or controller capture (R44). The ADR/AGENTS docs described the intended capture-rules table as if it were the as-shipped state; in fact every `captureException` call site lived in `infrastructure/repositories/*.repository.ts`. A grep proved it: zero call sites in any controller or use-case body. The user spotted the gap. + +**Fix (post-merge commit):** + +1. Extracted the `__sentryReported` flag helpers into `core-shared/instrumentation/reported-flag.ts` (`markReported`, `isReported`). `SentryLogger` now imports them; `RecordingLogger` carries an inlined copy (tooling → core import is disallowed by the boundary rule, so duplication is the right tradeoff). +2. Added `withCapture(logger, tags, fn)` higher-order wrapper at `core-shared/instrumentation/with-capture.ts`, parallel to `withSpan`. On error: capture-with-tags, mark, re-throw — but bail if the flag is already set (covers the bubbled-from-repo case). +3. Applied `withSpan(withCapture(factory))` to every use case and controller in every feature's `bind-production.ts` and `bind-dev-seed.ts`. Span is outermost so the errored span's timing reflects the capture-and-rethrow. +4. `RecordingLogger.captureException` now also honours the flag, so test assertions about capture counts stay honest. +5. Added `packages/blog/tests/r44-no-double-capture.test.ts` to lock the contract: an error originated in the repo is captured once with repo tags; an error originated in the controller (parse failure) is captured once with controller tags; success paths capture nothing. + +**Why use cases also wrap, even though current bodies mostly delegate to the repo:** R44's intent is that *any* throw originated locally — output-schema validation, business-rule errors like `AuthenticationError` in `signInUseCase` — gets captured with use-case tags. The wrapper makes the rule uniform; the flag makes it safe. + ## Related - ADR-008 — per-feature DI containers diff --git a/docs/superpowers/refactor-logs/2026-05-06-instrumentation-sentry.md b/docs/superpowers/refactor-logs/2026-05-06-instrumentation-sentry.md index faed6ae..d9571e2 100644 --- a/docs/superpowers/refactor-logs/2026-05-06-instrumentation-sentry.md +++ b/docs/superpowers/refactor-logs/2026-05-06-instrumentation-sentry.md @@ -70,3 +70,19 @@ - **Vite/TanStack Start typing not yet present in web-tanstack.** The client entry uses `import.meta.env.VITE_*`, but no Vite types are installed since the build infra is a placeholder. A small `src/vite-env.d.ts` declares the env shape for now; replace with `/// ` when Vite is wired. - **Spec deviations are usually a sign of a mis-aligned spec, not a wrong implementation.** Three of the deviations above (ipaddress, vertical-feature-spec section number, di-explainer section number) are minor; the vite.config skip and vite-env shim are real architecture decisions worth recording in ADR-014 alongside the major decisions. + +## Post-merge follow-up — R44 capture leg was missing + +After Plan 10 merged to `main`, the user pushed back on a description I gave of how capture works. A `grep captureException packages/*/src` showed zero call sites in any controller or use-case body — the R44 leg of "throw-site capture" had been **documented but never implemented**. Only repos (R43) actually called `logger.captureException`. + +I had described intent as reality both during execution and in the ADR-014 capture-rules table. The lesson is now in `feedback_verify_before_claiming.md`: when describing what code does — especially in the same session as writing it — grep before asserting. + +**The fix (one commit on `main`):** + +1. Extracted the `__sentryReported` flag helpers into `core-shared/instrumentation/reported-flag.ts`. `SentryLogger` switched to importing them; `RecordingLogger` got an inlined copy (boundary rule disallows tooling → core). +2. Added `withCapture(logger, tags, fn)` at `core-shared/instrumentation/with-capture.ts` — parallel to `withSpan`. On throw: capture, mark, re-throw — but bail if the flag was already set. +3. Wrapped every use case and controller in every feature's `bind-production.ts` and `bind-dev-seed.ts` with `withSpan(withCapture(factory))`. Span outermost so the errored span's timing reflects the capture-and-rethrow. +4. `RecordingLogger` now also honours the flag, so test capture counts are honest across the chain. +5. Added `packages/blog/tests/r44-no-double-capture.test.ts` proving: (a) repo-originated error → 1 capture with repo tags; (b) controller parse failure → 1 capture with controller tags; (c) success → 0 captures. + +Verification: `pnpm test` 26/26, `pnpm lint` 15/15 (warnings only), `pnpm typecheck` 14/14. diff --git a/packages/auth/src/di/bind-dev-seed.ts b/packages/auth/src/di/bind-dev-seed.ts index 6a17844..011b78f 100644 --- a/packages/auth/src/di/bind-dev-seed.ts +++ b/packages/auth/src/di/bind-dev-seed.ts @@ -1,5 +1,6 @@ import { withSpan, + withCapture, INSTRUMENTATION_SYMBOLS, type ITracer, type ILogger, @@ -61,17 +62,29 @@ export async function bindDevSeedAuth(tracer: ITracer, logger: ILogger): Promise const wrappedSignIn = withSpan( tracer, { name: "auth.signIn", op: "use-case" }, - signInUseCase(repo, authService), + withCapture( + logger, + { feature: "auth", layer: "use-case", name: "auth.signIn" }, + signInUseCase(repo, authService), + ), ); const wrappedSignUp = withSpan( tracer, { name: "auth.signUp", op: "use-case" }, - signUpUseCase(repo, authService), + withCapture( + logger, + { feature: "auth", layer: "use-case", name: "auth.signUp" }, + signUpUseCase(repo, authService), + ), ); const wrappedSignOut = withSpan( tracer, { name: "auth.signOut", op: "use-case" }, - signOutUseCase(authService), + withCapture( + logger, + { feature: "auth", layer: "use-case", name: "auth.signOut" }, + signOutUseCase(authService), + ), ); for (const sym of [ @@ -94,7 +107,11 @@ export async function bindDevSeedAuth(tracer: ITracer, logger: ILogger): Promise withSpan( tracer, { name: "auth.signIn", op: "controller" }, - signInController(wrappedSignIn), + withCapture( + logger, + { feature: "auth", layer: "controller", name: "auth.signIn" }, + signInController(wrappedSignIn), + ), ), ); authContainer @@ -103,7 +120,11 @@ export async function bindDevSeedAuth(tracer: ITracer, logger: ILogger): Promise withSpan( tracer, { name: "auth.signUp", op: "controller" }, - signUpController(wrappedSignUp), + withCapture( + logger, + { feature: "auth", layer: "controller", name: "auth.signUp" }, + signUpController(wrappedSignUp), + ), ), ); authContainer @@ -112,7 +133,11 @@ export async function bindDevSeedAuth(tracer: ITracer, logger: ILogger): Promise withSpan( tracer, { name: "auth.signOut", op: "controller" }, - signOutController(wrappedSignOut), + withCapture( + logger, + { feature: "auth", layer: "controller", name: "auth.signOut" }, + signOutController(wrappedSignOut), + ), ), ); } diff --git a/packages/auth/src/di/bind-production.ts b/packages/auth/src/di/bind-production.ts index 35a1ab6..a96c6e5 100644 --- a/packages/auth/src/di/bind-production.ts +++ b/packages/auth/src/di/bind-production.ts @@ -1,6 +1,7 @@ import type { SanitizedConfig } from "payload"; import { withSpan, + withCapture, INSTRUMENTATION_SYMBOLS, type ITracer, type ILogger, @@ -53,21 +54,33 @@ export function bindProductionAuth( .bind(AUTH_SYMBOLS.IAuthenticationService) .toConstantValue(authService); - // Use cases — wrapped with span at bind time + // Use cases — wrapped with span + capture at bind time const wrappedSignIn = withSpan( tracer, { name: "auth.signIn", op: "use-case" }, - signInUseCase(repo, authService), + withCapture( + logger, + { feature: "auth", layer: "use-case", name: "auth.signIn" }, + signInUseCase(repo, authService), + ), ); const wrappedSignUp = withSpan( tracer, { name: "auth.signUp", op: "use-case" }, - signUpUseCase(repo, authService), + withCapture( + logger, + { feature: "auth", layer: "use-case", name: "auth.signUp" }, + signUpUseCase(repo, authService), + ), ); const wrappedSignOut = withSpan( tracer, { name: "auth.signOut", op: "use-case" }, - signOutUseCase(authService), + withCapture( + logger, + { feature: "auth", layer: "use-case", name: "auth.signOut" }, + signOutUseCase(authService), + ), ); for (const sym of [ @@ -95,7 +108,11 @@ export function bindProductionAuth( withSpan( tracer, { name: "auth.signIn", op: "controller" }, - signInController(wrappedSignIn), + withCapture( + logger, + { feature: "auth", layer: "controller", name: "auth.signIn" }, + signInController(wrappedSignIn), + ), ), ); authContainer @@ -104,7 +121,11 @@ export function bindProductionAuth( withSpan( tracer, { name: "auth.signUp", op: "controller" }, - signUpController(wrappedSignUp), + withCapture( + logger, + { feature: "auth", layer: "controller", name: "auth.signUp" }, + signUpController(wrappedSignUp), + ), ), ); authContainer @@ -113,7 +134,11 @@ export function bindProductionAuth( withSpan( tracer, { name: "auth.signOut", op: "controller" }, - signOutController(wrappedSignOut), + withCapture( + logger, + { feature: "auth", layer: "controller", name: "auth.signOut" }, + signOutController(wrappedSignOut), + ), ), ); } diff --git a/packages/blog/src/di/bind-dev-seed.ts b/packages/blog/src/di/bind-dev-seed.ts index 7fa5696..d7c1904 100644 --- a/packages/blog/src/di/bind-dev-seed.ts +++ b/packages/blog/src/di/bind-dev-seed.ts @@ -1,5 +1,6 @@ import { withSpan, + withCapture, INSTRUMENTATION_SYMBOLS, type ITracer, type ILogger, @@ -52,17 +53,29 @@ export async function bindDevSeedBlog(tracer: ITracer, logger: ILogger): Promise const wrappedGetArticles = withSpan( tracer, { name: "blog.getArticles", op: "use-case" }, - getArticlesUseCase(repo), + withCapture( + logger, + { feature: "blog", layer: "use-case", name: "blog.getArticles" }, + getArticlesUseCase(repo), + ), ); const wrappedGetArticleBySlug = withSpan( tracer, { name: "blog.getArticleBySlug", op: "use-case" }, - getArticleBySlugUseCase(repo), + withCapture( + logger, + { feature: "blog", layer: "use-case", name: "blog.getArticleBySlug" }, + getArticleBySlugUseCase(repo), + ), ); const wrappedCreateArticle = withSpan( tracer, { name: "blog.createArticle", op: "use-case" }, - createArticleUseCase(repo), + withCapture( + logger, + { feature: "blog", layer: "use-case", name: "blog.createArticle" }, + createArticleUseCase(repo), + ), ); for (const sym of [ @@ -87,7 +100,11 @@ export async function bindDevSeedBlog(tracer: ITracer, logger: ILogger): Promise withSpan( tracer, { name: "blog.getArticles", op: "controller" }, - getArticlesController(wrappedGetArticles), + withCapture( + logger, + { feature: "blog", layer: "controller", name: "blog.getArticles" }, + getArticlesController(wrappedGetArticles), + ), ), ); blogContainer @@ -96,7 +113,11 @@ export async function bindDevSeedBlog(tracer: ITracer, logger: ILogger): Promise withSpan( tracer, { name: "blog.getArticleBySlug", op: "controller" }, - getArticleBySlugController(wrappedGetArticleBySlug), + withCapture( + logger, + { feature: "blog", layer: "controller", name: "blog.getArticleBySlug" }, + getArticleBySlugController(wrappedGetArticleBySlug), + ), ), ); blogContainer @@ -105,7 +126,11 @@ export async function bindDevSeedBlog(tracer: ITracer, logger: ILogger): Promise withSpan( tracer, { name: "blog.createArticle", op: "controller" }, - createArticleController(wrappedCreateArticle), + withCapture( + logger, + { feature: "blog", layer: "controller", name: "blog.createArticle" }, + createArticleController(wrappedCreateArticle), + ), ), ); } diff --git a/packages/blog/src/di/bind-production.ts b/packages/blog/src/di/bind-production.ts index ba35943..d3a3296 100644 --- a/packages/blog/src/di/bind-production.ts +++ b/packages/blog/src/di/bind-production.ts @@ -1,6 +1,7 @@ import type { SanitizedConfig } from "payload"; import { withSpan, + withCapture, INSTRUMENTATION_SYMBOLS, type ITracer, type ILogger, @@ -37,21 +38,33 @@ export function bindProductionBlog( const repo = new ArticlesRepository(config, tracer, logger); blogContainer.bind(BLOG_SYMBOLS.IArticlesRepository).toConstantValue(repo); - // Use cases — wrapped with span at bind time (R41) + // Use cases — wrapped with span + capture at bind time (R41, R44) const wrappedGetArticles = withSpan( tracer, { name: "blog.getArticles", op: "use-case" }, - getArticlesUseCase(repo), + withCapture( + logger, + { feature: "blog", layer: "use-case", name: "blog.getArticles" }, + getArticlesUseCase(repo), + ), ); const wrappedGetArticleBySlug = withSpan( tracer, { name: "blog.getArticleBySlug", op: "use-case" }, - getArticleBySlugUseCase(repo), + withCapture( + logger, + { feature: "blog", layer: "use-case", name: "blog.getArticleBySlug" }, + getArticleBySlugUseCase(repo), + ), ); const wrappedCreateArticle = withSpan( tracer, { name: "blog.createArticle", op: "use-case" }, - createArticleUseCase(repo), + withCapture( + logger, + { feature: "blog", layer: "use-case", name: "blog.createArticle" }, + createArticleUseCase(repo), + ), ); if (blogContainer.isBound(BLOG_SYMBOLS.IGetArticlesUseCase)) { @@ -85,7 +98,11 @@ export function bindProductionBlog( withSpan( tracer, { name: "blog.getArticles", op: "controller" }, - getArticlesController(wrappedGetArticles), + withCapture( + logger, + { feature: "blog", layer: "controller", name: "blog.getArticles" }, + getArticlesController(wrappedGetArticles), + ), ), ); blogContainer @@ -94,7 +111,11 @@ export function bindProductionBlog( withSpan( tracer, { name: "blog.getArticleBySlug", op: "controller" }, - getArticleBySlugController(wrappedGetArticleBySlug), + withCapture( + logger, + { feature: "blog", layer: "controller", name: "blog.getArticleBySlug" }, + getArticleBySlugController(wrappedGetArticleBySlug), + ), ), ); blogContainer @@ -103,7 +124,11 @@ export function bindProductionBlog( withSpan( tracer, { name: "blog.createArticle", op: "controller" }, - createArticleController(wrappedCreateArticle), + withCapture( + logger, + { feature: "blog", layer: "controller", name: "blog.createArticle" }, + createArticleController(wrappedCreateArticle), + ), ), ); } diff --git a/packages/blog/tests/r44-no-double-capture.test.ts b/packages/blog/tests/r44-no-double-capture.test.ts new file mode 100644 index 0000000..f7c765f --- /dev/null +++ b/packages/blog/tests/r44-no-double-capture.test.ts @@ -0,0 +1,141 @@ +// R44 — verify the full chain (controller → use case → repo) wraps with +// withSpan + withCapture and never double-captures the same error. +// +// Each layer's withCapture catch checks the __sentryReported flag (set by +// the inner-most layer that captured first) and skips. End result: one +// error → one logger.captureException call, with the inner-most tags. + +import { describe, it, expect } from "vitest"; +import { + RecordingTracer, + RecordingLogger, +} from "@repo/core-testing/instrumentation"; +import { withSpan, withCapture } from "@repo/core-shared/instrumentation"; + +import { MockArticlesRepository } from "../src/infrastructure/repositories/articles.repository.mock"; +import { getArticleBySlugUseCase } from "../src/application/use-cases/get-article-by-slug.use-case"; +import { getArticleBySlugController } from "../src/interface-adapters/controllers/get-article-by-slug.controller"; + +describe("R44 — no double-capture across span/capture-wrapped layers", () => { + it("an error originated in the repo is captured exactly once with repo tags", async () => { + const tracer = new RecordingTracer(); + const logger = new RecordingLogger(); + + // Repo throws on getArticleBySlug for a special slug. We simulate the + // throw via a subclass — the production repos call captureException + // inside their own try/catch as part of the repo span body. + class ThrowingRepo extends MockArticlesRepository { + override getArticleBySlug = async (_slug: string) => { + const err = new Error("boom from repo"); + // Mirror what the real repo does: capture with repo tags, mark the + // flag (RecordingLogger.captureException does this for us now). + logger.captureException(err, { + tags: { feature: "blog", repo: "articles", method: "getArticleBySlug" }, + }); + throw err; + }; + } + + const repo = new ThrowingRepo(tracer, logger); + + // Wire the use case + controller exactly the way bind-production does. + const wrappedUC = withSpan( + tracer, + { name: "blog.getArticleBySlug", op: "use-case" }, + withCapture( + logger, + { feature: "blog", layer: "use-case", name: "blog.getArticleBySlug" }, + getArticleBySlugUseCase(repo), + ), + ); + const wrappedCtrl = withSpan( + tracer, + { name: "blog.getArticleBySlug", op: "controller" }, + withCapture( + logger, + { feature: "blog", layer: "controller", name: "blog.getArticleBySlug" }, + getArticleBySlugController(wrappedUC), + ), + ); + + await expect(wrappedCtrl({ slug: "anything" })).rejects.toThrow("boom from repo"); + + // R44: exactly one capture. Outer wrappers saw the flag and skipped. + expect(logger.captures).toHaveLength(1); + const only = logger.captures[0]; + expect(only?.kind).toBe("exception"); + if (only?.kind !== "exception") return; + expect(only.ctx?.tags).toEqual({ + feature: "blog", + repo: "articles", + method: "getArticleBySlug", + }); + }); + + it("an error originated in the controller (parse failure) is captured once with controller tags", async () => { + const tracer = new RecordingTracer(); + const logger = new RecordingLogger(); + const repo = new MockArticlesRepository(); + + const wrappedUC = withSpan( + tracer, + { name: "blog.getArticleBySlug", op: "use-case" }, + withCapture( + logger, + { feature: "blog", layer: "use-case", name: "blog.getArticleBySlug" }, + getArticleBySlugUseCase(repo), + ), + ); + const wrappedCtrl = withSpan( + tracer, + { name: "blog.getArticleBySlug", op: "controller" }, + withCapture( + logger, + { feature: "blog", layer: "controller", name: "blog.getArticleBySlug" }, + getArticleBySlugController(wrappedUC), + ), + ); + + // safeParse fails because slug is missing. + await expect(wrappedCtrl({})).rejects.toThrow(); + + expect(logger.captures).toHaveLength(1); + const only = logger.captures[0]; + expect(only?.kind).toBe("exception"); + if (only?.kind !== "exception") return; + expect(only.ctx?.tags).toEqual({ + feature: "blog", + layer: "controller", + name: "blog.getArticleBySlug", + }); + }); + + it("a successful call captures nothing", async () => { + const tracer = new RecordingTracer(); + const logger = new RecordingLogger(); + const repo = new MockArticlesRepository(); + + const wrappedUC = withSpan( + tracer, + { name: "blog.getArticles", op: "use-case" }, + withCapture( + logger, + { feature: "blog", layer: "use-case", name: "blog.getArticles" }, + async () => [], + ), + ); + const wrappedCtrl = withSpan( + tracer, + { name: "blog.getArticles", op: "controller" }, + withCapture( + logger, + { feature: "blog", layer: "controller", name: "blog.getArticles" }, + async () => wrappedUC(), + ), + ); + + await expect(wrappedCtrl()).resolves.toEqual([]); + expect(logger.captures).toHaveLength(0); + void repo; + }); +}); diff --git a/packages/core-shared/src/instrumentation/index.ts b/packages/core-shared/src/instrumentation/index.ts index 54e0b6c..dbe7b13 100644 --- a/packages/core-shared/src/instrumentation/index.ts +++ b/packages/core-shared/src/instrumentation/index.ts @@ -12,6 +12,8 @@ export type { export { NoopTracer } from "./noop-tracer"; export { NoopLogger } from "./noop-logger"; export { withSpan } from "./with-span"; +export { withCapture } from "./with-capture"; +export { isReported, markReported } from "./reported-flag"; export { INSTRUMENTATION_SYMBOLS } from "./symbols"; export { bindNoopInstrumentation } from "./di/bind-noop-instrumentation"; export { diff --git a/packages/core-shared/src/instrumentation/reported-flag.ts b/packages/core-shared/src/instrumentation/reported-flag.ts new file mode 100644 index 0000000..74a4718 --- /dev/null +++ b/packages/core-shared/src/instrumentation/reported-flag.ts @@ -0,0 +1,24 @@ +// Non-enumerable flag used by every ILogger implementation to skip +// already-reported errors. The flag is non-enumerable so JSON.stringify +// and {...err} spread won't surface it. + +const REPORTED = "__sentryReported" as const; + +export function isReported(err: unknown): boolean { + return ( + err !== null && + typeof err === "object" && + Boolean((err as Record)[REPORTED]) + ); +} + +export function markReported(err: unknown): void { + if (err !== null && typeof err === "object" && !isReported(err)) { + Object.defineProperty(err, REPORTED, { + value: true, + enumerable: false, + configurable: false, + writable: false, + }); + } +} diff --git a/packages/core-shared/src/instrumentation/sentry/sentry-logger.ts b/packages/core-shared/src/instrumentation/sentry/sentry-logger.ts index d14c393..69ec58a 100644 --- a/packages/core-shared/src/instrumentation/sentry/sentry-logger.ts +++ b/packages/core-shared/src/instrumentation/sentry/sentry-logger.ts @@ -1,27 +1,7 @@ // packages/core-shared/src/instrumentation/sentry/sentry-logger.ts import * as Sentry from "@sentry/nextjs"; import type { ILogger, Breadcrumb, CaptureContext } from "../logger.interface"; - -const REPORTED = "__sentryReported" as const; - -function isReported(err: unknown): boolean { - return ( - err !== null && - typeof err === "object" && - Boolean((err as Record)[REPORTED]) - ); -} - -function markReported(err: unknown): void { - if (err !== null && typeof err === "object") { - Object.defineProperty(err, REPORTED, { - value: true, - enumerable: false, - configurable: false, - writable: false, - }); - } -} +import { isReported, markReported } from "../reported-flag"; export class SentryLogger implements ILogger { captureException(err: unknown, ctx?: CaptureContext): void { diff --git a/packages/core-shared/src/instrumentation/with-capture.test.ts b/packages/core-shared/src/instrumentation/with-capture.test.ts new file mode 100644 index 0000000..8b056d3 --- /dev/null +++ b/packages/core-shared/src/instrumentation/with-capture.test.ts @@ -0,0 +1,62 @@ +import { describe, it, expect, vi } from "vitest"; +import { withCapture } from "@/instrumentation/with-capture"; +import type { ILogger } from "@/instrumentation/logger.interface"; +import { isReported } from "@/instrumentation/reported-flag"; + +function makeLogger(): ILogger & { captureException: ReturnType } { + return { + captureException: vi.fn(), + captureMessage: vi.fn(), + addBreadcrumb: vi.fn(), + setUser: vi.fn(), + }; +} + +describe("withCapture", () => { + it("does not capture on success", async () => { + const logger = makeLogger(); + const wrapped = withCapture(logger, { layer: "use-case" }, async (x: number) => x + 1); + await expect(wrapped(1)).resolves.toBe(2); + expect(logger.captureException).not.toHaveBeenCalled(); + }); + + it("captures with tags and re-throws on failure", async () => { + const logger = makeLogger(); + const err = new Error("boom"); + const wrapped = withCapture(logger, { layer: "use-case", name: "blog.x" }, async () => { + throw err; + }); + await expect(wrapped()).rejects.toBe(err); + expect(logger.captureException).toHaveBeenCalledTimes(1); + expect(logger.captureException).toHaveBeenCalledWith(err, { + tags: { layer: "use-case", name: "blog.x" }, + }); + }); + + it("marks the error as reported after first capture", async () => { + const logger = makeLogger(); + const err = new Error("boom"); + const wrapped = withCapture(logger, { layer: "use-case" }, async () => { + throw err; + }); + await expect(wrapped()).rejects.toBe(err); + expect(isReported(err)).toBe(true); + }); + + it("does NOT capture again when the same error already carries the flag", async () => { + const logger = makeLogger(); + const err = new Error("boom"); + // Simulate an inner layer (repo) having already captured + marked. + const inner = withCapture(logger, { layer: "repo" }, async () => { + throw err; + }); + const outer = withCapture(logger, { layer: "use-case" }, () => inner()); + + await expect(outer()).rejects.toBe(err); + // Only the inner layer captured it; outer saw the flag and bailed. + expect(logger.captureException).toHaveBeenCalledTimes(1); + expect(logger.captureException).toHaveBeenCalledWith(err, { + tags: { layer: "repo" }, + }); + }); +}); diff --git a/packages/core-shared/src/instrumentation/with-capture.ts b/packages/core-shared/src/instrumentation/with-capture.ts new file mode 100644 index 0000000..f9fbeb9 --- /dev/null +++ b/packages/core-shared/src/instrumentation/with-capture.ts @@ -0,0 +1,40 @@ +import type { ILogger } from "./logger.interface"; +import { isReported, markReported } from "./reported-flag"; + +/** + * Higher-order wrapper applied at DI bind time. Mirrors `withSpan`: takes a + * factory result `(args) => Promise` and returns the same shape, but any + * thrown error is captured via `logger.captureException(err, { tags })` before + * being re-thrown. + * + * Skips capture if the error already carries the `__sentryReported` flag — + * this is what prevents double-capture when the same error bubbles through + * a wrapped repo → use case → controller chain (the repo's catch site + * captures first; outer wrappers see the flag and bail). + * + * Usage at bind time: + * + * const captured = withCapture(logger, { feature: "blog", layer: "use-case", name: "blog.getArticles" }, factory(deps)); + * const wrapped = withSpan(tracer, opts, captured); + * + * Span wraps capture: the span timing reflects the captured-and-rethrown + * failure (errored span gets a duration), and the capture has accurate + * tags by the time it fires. + */ +export function withCapture( + logger: ILogger, + tags: Record, + fn: (...args: Args) => Promise, +): (...args: Args) => Promise { + return async (...args) => { + try { + return await fn(...args); + } catch (err) { + if (!isReported(err)) { + logger.captureException(err, { tags }); + markReported(err); + } + throw err; + } + }; +} diff --git a/packages/core-testing/src/instrumentation/recording-logger.ts b/packages/core-testing/src/instrumentation/recording-logger.ts index bb6e7c2..7e36691 100644 --- a/packages/core-testing/src/instrumentation/recording-logger.ts +++ b/packages/core-testing/src/instrumentation/recording-logger.ts @@ -24,13 +24,38 @@ export type RecordedCapture = | { kind: "exception"; err: unknown; ctx?: CaptureContext } | { kind: "message"; message: string; level?: "info" | "warning" | "error"; ctx?: CaptureContext }; +// Inlined to avoid a tooling → core import (boundary rule). Mirrors the +// implementation in @repo/core-shared/instrumentation/reported-flag.ts. +const REPORTED = "__sentryReported" as const; + +function isReported(err: unknown): boolean { + return ( + err !== null && + typeof err === "object" && + Boolean((err as Record)[REPORTED]) + ); +} + +function markReported(err: unknown): void { + if (err !== null && typeof err === "object" && !isReported(err)) { + Object.defineProperty(err, REPORTED, { + value: true, + enumerable: false, + configurable: false, + writable: false, + }); + } +} + export class RecordingLogger implements ILogger { captures: RecordedCapture[] = []; breadcrumbs: Breadcrumb[] = []; users: Array<{ id: string } | null> = []; captureException(err: unknown, ctx?: CaptureContext): void { + if (isReported(err)) return; this.captures.push({ kind: "exception", err, ctx }); + markReported(err); } captureMessage( diff --git a/packages/marketing-pages/src/di/bind-dev-seed.ts b/packages/marketing-pages/src/di/bind-dev-seed.ts index b7a87cd..8a556fa 100644 --- a/packages/marketing-pages/src/di/bind-dev-seed.ts +++ b/packages/marketing-pages/src/di/bind-dev-seed.ts @@ -1,5 +1,6 @@ import { withSpan, + withCapture, INSTRUMENTATION_SYMBOLS, type ITracer, type ILogger, @@ -62,12 +63,20 @@ export async function bindDevSeedMarketingPages(tracer: ITracer, logger: ILogger const wrappedGetSiteSettings = withSpan( tracer, { name: "marketing-pages.getSiteSettings", op: "use-case" }, - getSiteSettingsUseCase(siteSettingsRepo), + withCapture( + logger, + { feature: "marketing-pages", layer: "use-case", name: "marketing-pages.getSiteSettings" }, + getSiteSettingsUseCase(siteSettingsRepo), + ), ); const wrappedGetPageBySlug = withSpan( tracer, { name: "marketing-pages.getPageBySlug", op: "use-case" }, - getPageBySlugUseCase(pagesRepo), + withCapture( + logger, + { feature: "marketing-pages", layer: "use-case", name: "marketing-pages.getPageBySlug" }, + getPageBySlugUseCase(pagesRepo), + ), ); for (const sym of [ @@ -91,7 +100,11 @@ export async function bindDevSeedMarketingPages(tracer: ITracer, logger: ILogger withSpan( tracer, { name: "marketing-pages.getSiteSettings", op: "controller" }, - getSiteSettingsController(wrappedGetSiteSettings), + withCapture( + logger, + { feature: "marketing-pages", layer: "controller", name: "marketing-pages.getSiteSettings" }, + getSiteSettingsController(wrappedGetSiteSettings), + ), ), ); marketingPagesContainer @@ -100,7 +113,11 @@ export async function bindDevSeedMarketingPages(tracer: ITracer, logger: ILogger withSpan( tracer, { name: "marketing-pages.getPageBySlug", op: "controller" }, - getPageBySlugController(wrappedGetPageBySlug), + withCapture( + logger, + { feature: "marketing-pages", layer: "controller", name: "marketing-pages.getPageBySlug" }, + getPageBySlugController(wrappedGetPageBySlug), + ), ), ); } diff --git a/packages/marketing-pages/src/di/bind-production.ts b/packages/marketing-pages/src/di/bind-production.ts index db71408..11cd4e8 100644 --- a/packages/marketing-pages/src/di/bind-production.ts +++ b/packages/marketing-pages/src/di/bind-production.ts @@ -1,6 +1,7 @@ import type { SanitizedConfig } from "payload"; import { withSpan, + withCapture, INSTRUMENTATION_SYMBOLS, type ITracer, type ILogger, @@ -46,16 +47,24 @@ export function bindProductionMarketingPages( .bind(MARKETING_PAGES_SYMBOLS.ISiteSettingsRepository) .toConstantValue(siteSettingsRepo); - // Use cases — wrapped with span at bind time + // Use cases — wrapped with span + capture at bind time const wrappedGetSiteSettings = withSpan( tracer, { name: "marketing-pages.getSiteSettings", op: "use-case" }, - getSiteSettingsUseCase(siteSettingsRepo), + withCapture( + logger, + { feature: "marketing-pages", layer: "use-case", name: "marketing-pages.getSiteSettings" }, + getSiteSettingsUseCase(siteSettingsRepo), + ), ); const wrappedGetPageBySlug = withSpan( tracer, { name: "marketing-pages.getPageBySlug", op: "use-case" }, - getPageBySlugUseCase(pagesRepo), + withCapture( + logger, + { feature: "marketing-pages", layer: "use-case", name: "marketing-pages.getPageBySlug" }, + getPageBySlugUseCase(pagesRepo), + ), ); for (const sym of [ @@ -84,7 +93,11 @@ export function bindProductionMarketingPages( withSpan( tracer, { name: "marketing-pages.getSiteSettings", op: "controller" }, - getSiteSettingsController(wrappedGetSiteSettings), + withCapture( + logger, + { feature: "marketing-pages", layer: "controller", name: "marketing-pages.getSiteSettings" }, + getSiteSettingsController(wrappedGetSiteSettings), + ), ), ); marketingPagesContainer @@ -93,7 +106,11 @@ export function bindProductionMarketingPages( withSpan( tracer, { name: "marketing-pages.getPageBySlug", op: "controller" }, - getPageBySlugController(wrappedGetPageBySlug), + withCapture( + logger, + { feature: "marketing-pages", layer: "controller", name: "marketing-pages.getPageBySlug" }, + getPageBySlugController(wrappedGetPageBySlug), + ), ), ); } diff --git a/packages/media/src/di/bind-dev-seed.ts b/packages/media/src/di/bind-dev-seed.ts index 77610fb..05b7a8f 100644 --- a/packages/media/src/di/bind-dev-seed.ts +++ b/packages/media/src/di/bind-dev-seed.ts @@ -1,5 +1,6 @@ import { withSpan, + withCapture, INSTRUMENTATION_SYMBOLS, type ITracer, type ILogger, @@ -54,17 +55,29 @@ export async function bindDevSeedMedia(tracer: ITracer, logger: ILogger): Promis const wrappedGetMedia = withSpan( tracer, { name: "media.getMedia", op: "use-case" }, - getMediaUseCase(repo), + withCapture( + logger, + { feature: "media", layer: "use-case", name: "media.getMedia" }, + getMediaUseCase(repo), + ), ); const wrappedListMedia = withSpan( tracer, { name: "media.listMedia", op: "use-case" }, - listMediaUseCase(repo), + withCapture( + logger, + { feature: "media", layer: "use-case", name: "media.listMedia" }, + listMediaUseCase(repo), + ), ); const wrappedDeleteMedia = withSpan( tracer, { name: "media.deleteMedia", op: "use-case" }, - deleteMediaUseCase(repo), + withCapture( + logger, + { feature: "media", layer: "use-case", name: "media.deleteMedia" }, + deleteMediaUseCase(repo), + ), ); for (const sym of [ @@ -87,7 +100,11 @@ export async function bindDevSeedMedia(tracer: ITracer, logger: ILogger): Promis withSpan( tracer, { name: "media.getMedia", op: "controller" }, - getMediaController(wrappedGetMedia), + withCapture( + logger, + { feature: "media", layer: "controller", name: "media.getMedia" }, + getMediaController(wrappedGetMedia), + ), ), ); mediaContainer @@ -96,7 +113,11 @@ export async function bindDevSeedMedia(tracer: ITracer, logger: ILogger): Promis withSpan( tracer, { name: "media.listMedia", op: "controller" }, - listMediaController(wrappedListMedia), + withCapture( + logger, + { feature: "media", layer: "controller", name: "media.listMedia" }, + listMediaController(wrappedListMedia), + ), ), ); mediaContainer @@ -105,7 +126,11 @@ export async function bindDevSeedMedia(tracer: ITracer, logger: ILogger): Promis withSpan( tracer, { name: "media.deleteMedia", op: "controller" }, - deleteMediaController(wrappedDeleteMedia), + withCapture( + logger, + { feature: "media", layer: "controller", name: "media.deleteMedia" }, + deleteMediaController(wrappedDeleteMedia), + ), ), ); } diff --git a/packages/media/src/di/bind-production.ts b/packages/media/src/di/bind-production.ts index 1178bcc..21a1c90 100644 --- a/packages/media/src/di/bind-production.ts +++ b/packages/media/src/di/bind-production.ts @@ -1,6 +1,7 @@ import type { SanitizedConfig } from "payload"; import { withSpan, + withCapture, INSTRUMENTATION_SYMBOLS, type ITracer, type ILogger, @@ -39,21 +40,33 @@ export function bindProductionMedia( .bind(MEDIA_SYMBOLS.IMediaRepository) .toConstantValue(repo); - // Use cases — wrapped with span at bind time + // Use cases — wrapped with span + capture at bind time const wrappedGetMedia = withSpan( tracer, { name: "media.getMedia", op: "use-case" }, - getMediaUseCase(repo), + withCapture( + logger, + { feature: "media", layer: "use-case", name: "media.getMedia" }, + getMediaUseCase(repo), + ), ); const wrappedListMedia = withSpan( tracer, { name: "media.listMedia", op: "use-case" }, - listMediaUseCase(repo), + withCapture( + logger, + { feature: "media", layer: "use-case", name: "media.listMedia" }, + listMediaUseCase(repo), + ), ); const wrappedDeleteMedia = withSpan( tracer, { name: "media.deleteMedia", op: "use-case" }, - deleteMediaUseCase(repo), + withCapture( + logger, + { feature: "media", layer: "use-case", name: "media.deleteMedia" }, + deleteMediaUseCase(repo), + ), ); for (const sym of [ @@ -81,7 +94,11 @@ export function bindProductionMedia( withSpan( tracer, { name: "media.getMedia", op: "controller" }, - getMediaController(wrappedGetMedia), + withCapture( + logger, + { feature: "media", layer: "controller", name: "media.getMedia" }, + getMediaController(wrappedGetMedia), + ), ), ); mediaContainer @@ -90,7 +107,11 @@ export function bindProductionMedia( withSpan( tracer, { name: "media.listMedia", op: "controller" }, - listMediaController(wrappedListMedia), + withCapture( + logger, + { feature: "media", layer: "controller", name: "media.listMedia" }, + listMediaController(wrappedListMedia), + ), ), ); mediaContainer @@ -99,7 +120,11 @@ export function bindProductionMedia( withSpan( tracer, { name: "media.deleteMedia", op: "controller" }, - deleteMediaController(wrappedDeleteMedia), + withCapture( + logger, + { feature: "media", layer: "controller", name: "media.deleteMedia" }, + deleteMediaController(wrappedDeleteMedia), + ), ), ); } diff --git a/packages/navigation/src/di/bind-dev-seed.ts b/packages/navigation/src/di/bind-dev-seed.ts index 933be81..19cdcba 100644 --- a/packages/navigation/src/di/bind-dev-seed.ts +++ b/packages/navigation/src/di/bind-dev-seed.ts @@ -1,5 +1,6 @@ import { withSpan, + withCapture, INSTRUMENTATION_SYMBOLS, type ITracer, type ILogger, @@ -46,7 +47,11 @@ export async function bindDevSeedNavigation(tracer: ITracer, logger: ILogger): P const wrappedGetHeader = withSpan( tracer, { name: "navigation.getHeader", op: "use-case" }, - getHeaderUseCase(repo), + withCapture( + logger, + { feature: "navigation", layer: "use-case", name: "navigation.getHeader" }, + getHeaderUseCase(repo), + ), ); for (const sym of [ @@ -65,7 +70,11 @@ export async function bindDevSeedNavigation(tracer: ITracer, logger: ILogger): P withSpan( tracer, { name: "navigation.getHeader", op: "controller" }, - getHeaderController(wrappedGetHeader), + withCapture( + logger, + { feature: "navigation", layer: "controller", name: "navigation.getHeader" }, + getHeaderController(wrappedGetHeader), + ), ), ); } diff --git a/packages/navigation/src/di/bind-production.ts b/packages/navigation/src/di/bind-production.ts index 322fbff..19a0d15 100644 --- a/packages/navigation/src/di/bind-production.ts +++ b/packages/navigation/src/di/bind-production.ts @@ -1,6 +1,7 @@ import type { SanitizedConfig } from "payload"; import { withSpan, + withCapture, INSTRUMENTATION_SYMBOLS, type ITracer, type ILogger, @@ -35,11 +36,15 @@ export function bindProductionNavigation( .bind(NAVIGATION_SYMBOLS.IHeaderRepository) .toConstantValue(repo); - // Use case — wrapped with span at bind time + // Use case — wrapped with span + capture at bind time const wrappedGetHeader = withSpan( tracer, { name: "navigation.getHeader", op: "use-case" }, - getHeaderUseCase(repo), + withCapture( + logger, + { feature: "navigation", layer: "use-case", name: "navigation.getHeader" }, + getHeaderUseCase(repo), + ), ); if (navigationContainer.isBound(NAVIGATION_SYMBOLS.IGetHeaderUseCase)) { @@ -59,7 +64,11 @@ export function bindProductionNavigation( withSpan( tracer, { name: "navigation.getHeader", op: "controller" }, - getHeaderController(wrappedGetHeader), + withCapture( + logger, + { feature: "navigation", layer: "controller", name: "navigation.getHeader" }, + getHeaderController(wrappedGetHeader), + ), ), ); }