Danijel Martinek
f0775d6ecc
Plan 10 documented R44 (capture at originating-throw layer) but only the R43 repo leg was wired. captureException had zero call sites in any controller or use-case body. This commit closes the gap. Mechanism: - Extract __sentryReported flag helpers into core-shared/instrumentation/ reported-flag.ts. SentryLogger switches to importing them; RecordingLogger carries an inlined copy (tooling → core boundary disallows the import). - Add withCapture(logger, tags, fn) higher-order wrapper paralleling withSpan. On throw: capture-with-tags, mark, re-throw. Bail if the flag was already set — covers the bubbled-from-repo case so each error surfaces in the logger exactly once with the inner-most layer's tags. - Apply withSpan(withCapture(factory)) in every feature's bind-production and bind-dev-seed: auth (3 use cases × 3 controllers), blog (3×3), marketing-pages (2×2), navigation (1×1), media (3×3). Span is outermost so the errored span timing reflects the capture-and-rethrow. - RecordingLogger.captureException now also honours the flag — test capture counts stay honest when both repo and outer layer wrap. Tests: - packages/core-shared/src/instrumentation/with-capture.test.ts — 4 cases covering success, capture-on-throw, mark-on-capture, no-double via the flag. - packages/blog/tests/r44-no-double-capture.test.ts — 3 cases: repo throw → 1 capture with repo tags; controller parse fail → 1 capture with controller tags; success → 0 captures. Verification: pnpm test 26/26, pnpm lint 15/15, pnpm typecheck 14/14. Docs: ADR-014 and the refactor log gain a "Post-merge follow-up" section recording the gap, the fix, and the underlying lesson (don't describe intent as shipped state — grep first). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
89 lines
8.9 KiB
Markdown
89 lines
8.9 KiB
Markdown
# Refactor Log — Instrumentation + Sentry Logging (Plan 10)
|
||
|
||
**Date:** 2026-05-06
|
||
**Spec:** docs/superpowers/specs/2026-05-06-instrumentation-sentry-design.md
|
||
**Plan:** docs/superpowers/plans/2026-05-06-plan-10-instrumentation-sentry.md
|
||
**Branch:** feature/instrumentation-sentry
|
||
|
||
## Tasks
|
||
|
||
- [x] Task 1 — Scaffold refactor log + ADR-014 stub
|
||
- [x] Task 2 — Tracer interface + ISpan + AttributeValue + SpanOpts
|
||
- [x] Task 3 — NoopTracer
|
||
- [x] Task 4 — Logger interface + NoopLogger + Breadcrumb + CaptureContext
|
||
- [x] Task 5 — withSpan helper
|
||
- [x] Task 6 — Symbols + index barrel
|
||
- [x] Task 7 — SentryTracer adapter
|
||
- [x] Task 8 — SentryLogger adapter (with double-report guard)
|
||
- [x] Task 9 — pii-fields constants + scrub.beforeSend / scrub.beforeSendTransaction
|
||
- [x] Task 10 — init-server helper
|
||
- [x] Task 11 — init-client helper (browser-only)
|
||
- [x] Task 12 — bindNoopInstrumentation + bindSentryInstrumentation
|
||
- [x] Task 13 — apps/web-next bindAll() Rule 0 dispatcher
|
||
- [x] Task 14 — Tests for bindAll() orthogonality (R47)
|
||
- [x] Task 15 — RecordingTracer in core-testing
|
||
- [x] Task 16 — RecordingLogger in core-testing
|
||
- [x] Task 17 — vitest.setup.ts binds Noop by default
|
||
- [x] Task 18 — Blog feature wiring (pilot)
|
||
- [x] Task 19 — Auth feature wiring
|
||
- [x] Task 20 — Marketing-pages feature wiring
|
||
- [x] Task 21 — Navigation feature wiring
|
||
- [x] Task 22 — Media feature wiring
|
||
- [x] Task 23 — defineContractSuite expectSpan helper
|
||
- [x] Task 24 — Update repo contract suites to assert span shape
|
||
- [x] Task 25 — apps/web-next instrumentation files + scrubber test
|
||
- [x] Task 26 — apps/cms instrumentation files + scrubber test
|
||
- [x] Task 27 — apps/web-tanstack instrumentation files + scrubber test
|
||
- [x] Task 28 — ESLint boundary rule (R40) + CI grep gate (R31)
|
||
- [x] Task 29 — turbo.json globalEnv updates
|
||
- [x] Task 30 — Doc updates (CLAUDE.md, AGENTS.md, vertical-feature-spec.md)
|
||
- [x] Task 31 — Doc updates (tdd-workflow.md, testing-strategy.md, dependency-flow.md, core-shared/AGENTS.md)
|
||
- [x] Task 32 — HTML updates (data-flow-explainer §06, di-explainer §08)
|
||
- [x] Task 33 — ADR-014 final + refactor log final
|
||
|
||
## Decisions deviated from spec
|
||
|
||
- **PII key-substring list extended (Task 25).** Added `"ipaddress"` to `PII_KEY_SUBSTRINGS` so keys like `ipAddress` trigger full key-level redaction in addition to the existing IPv4/IPv6 string-pattern redaction. The plan's R38 PII-scrubber test expected `result.extra.ipAddress === "[redacted]"`, which the original substring list (without "ipaddress") could not satisfy. Tighter posture than the spec; existing scrub.test.ts continues to pass.
|
||
|
||
- **vertical-feature-spec.md numbering (Task 30).** The plan instructed to add the instrumentation section as "§10". The spec already had 15 sections, so the section was appended as **§16** (rather than renumbering §10–§15) to avoid invalidating every cross-reference in the spec.
|
||
|
||
- **Web-tanstack `vite.config.ts` modifications skipped (Task 27, Step 6).** The app is a placeholder — its `package.json` has `"build": "echo 'placeholder — TanStack Start build configured in later plan'"` and there's no `vite.config.ts`. The `@sentry/vite-plugin` dep is installed but dormant; it will be wired into the build when TanStack Start build is configured. A minimal `src/vite-env.d.ts` shims `ImportMetaEnv` for the client entry's `import.meta.env` reads.
|
||
|
||
- **data-flow-explainer.html section numbering (Task 32).** The plan asked to renumber the verdict from §06 to §07 and slot the new tracing section in as §06 — done. The `<style>` block uses dark-paper variables (`--paper`, `--ink-2`, `--rule`) tuned to the existing palette rather than the plan's snippet which used `--paper-tone, #f4ede3` (light-mode token). Same visual intent, correct theme.
|
||
|
||
- **di-explainer.html numbering (Task 32).** The plan suggested numbering the new instrumentation section as "§ 0X" (placeholder) and "renumber subsequent sections accordingly". The page's existing sections went up to §07; appending the new instrumentation section as **§08** required no renumbering and matches the plan's intent.
|
||
|
||
## Notable surprises
|
||
|
||
- **`@repo/core-shared` was not a direct dep of `apps/cms` or `apps/web-tanstack`** before Plan 10. Both apps had transitive access via feature packages. Adding the deep subpath imports `@repo/core-shared/instrumentation/sentry/init-server` (etc.) required adding `@repo/core-shared` as a direct `workspace:*` dependency of each app — pnpm's strict-peer mode wouldn't resolve the subpath through transitive paths.
|
||
|
||
- **Subpath exports needed for the dynamic-import pattern.** Apps' `instrumentation.ts` uses `await import("@repo/core-shared/instrumentation/sentry/init-server")` to keep the Sentry code out of the cold-start boot path. The barrel re-export was sufficient for static imports but not for the dynamic deep paths — added explicit `package.json#exports` entries for each `sentry/init-*` and `sentry/scrub` file.
|
||
|
||
- **`core-shared`'s `peerDependenciesMeta.optional`** was needed for `@sentry/node` and `@sentry/react` so feature packages installing `core-shared` don't pull in the React/Node SDK transitively. The same packages stay in `devDependencies` of `core-shared` so its own typecheck and test runs resolve them locally.
|
||
|
||
- **Pre-existing lint debt blocked the Task 28 R40 rule from going green.** Several issues unrelated to instrumentation surfaced once the rule was active: unused variables not covered by an `argsIgnorePattern: "^_"` config, `process` undefined in `*.mjs` config files (Node globals not declared), and stale `eslint-disable` directives in `marketing-pages`. These were each a one-line fix and were bundled into the Task 28 commit since they were the only thing blocking the lint gate.
|
||
|
||
- **`core-testing/setup/no-sentry.test.ts` was not in the original allowlist.** The file imports `@sentry/nextjs` to assert that the mock is in place. Added `**/setup/no-sentry.test.{ts,js}` to the allowlist alongside the source file. Same for `**/instrumentation/di/bind-sentry-instrumentation.test.{ts,js}` — Task 12's test file lives in `instrumentation/di/`, not `instrumentation/sentry/`, so the `**/instrumentation/sentry/**` allowlist alone wouldn't catch it.
|
||
|
||
- **ESLint flat-config glob patterns are relative to the eslint command's cwd.** When `pnpm --filter <pkg> lint` runs, ESLint sees file paths relative to the package root, not the repo root. The plan's allowlist patterns (`packages/core-shared/src/instrumentation/sentry/**`) wouldn't match. Switched to `**/`-prefixed patterns (`**/instrumentation/sentry/**`) which match regardless of where ESLint is invoked.
|
||
|
||
- **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 `/// <reference types="vite/client" />` 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.
|