feat(api): instrument the internal sweep rail end to end - #64
Merged
Conversation
… backlog H.1 records what a clone actually owes its user: every backend capability either reachable from the UI or declared UI-less, plus the two gaps a first pass already found. The sweep-instrumentation row moves to D.6 — the id D.5 is already taken by the shipped email delivery queue milestone.
SpanOptions is open-time only, so a span could never record what it learned - how many rows a sweep pass deleted, why it stopped. Sentry writes onto getActiveSpan(); NoOp keeps ignoring it. Also aligns sentry-init.test.ts's @sentry/bun mock with the new getActiveSpan export - bun's mock.module registry is process-global, so a mock missing an export used by another test file breaks that file when both run in the same test process.
Five more object-literal fakes cast with `as never` were missing setSpanAttributes, invisible to tsc because the cast hides the gap from the compiler - a TypeError waiting for the callers Tasks 2-6 add. Also documents on the port that the write is silently dropped when no span is active.
The rail is functional, so rule #8's constructor injection has no constructor to use. `sweepSpans()` is built per request from `di.IInstrumentation` and threaded down, with a per-instance budget so a 1000-batch run cannot push the spans that matter out of the transaction.
… helper Six routes carried byte-identical purgeBatch bodies. Promoting it is what makes instrumenting the batches a one-place change; the delete now runs inside a db.query span carrying its SQL, and the dry-run count does too. The shared drizzle test-mock factory (extracted from sweep-notifications.test.ts, now reused for the purge tests) also gained a sweepSchema entry and a sql tag that reconstructs its literal text: sweep-lock.ts's sweepSchema import was missing from the mock, breaking sweep-notifications.test.ts even before this change, and the new purge test asserts on the three SET LOCAL guards by stringifying the sql-tagged statements passed to tx.execute.
…rrors A pass span now carries deleted/batch_count/stop_reason, so a 40s pass in the trace says whether it finished or hit the budget. The two catches that logged and moved on — a broken batch under onBatchError=break, a failed lease release — now reach telemetry; the rethrowing branch stays silent because app.onError already captures it.
The lease is the reason a sweep can report "skipped"; without a span the trace shows the skip with no evidence of the contention that caused it.
… purge Each route now builds one span facade per request and hands it to the runner, the lease and both query helpers, so a sweep is a single coherent trace instead of six dark ones. The six local purgeBatch copies are gone. Also extends the shared @packages/drizzle test mock (ilike, getRateLimitDbClient, migrate, ssoSchema) since sweep-notifications' test mocks the whole module and now transitively pulls in the full DI container through the new di.IInstrumentation import.
Task 7 of the D.6 sweep-instrumentation plan: verifies the finished rail against a real Postgres (all six /internal/sweep-* routes 2xx, deleted:0, exit 0) and records the as-built in docs/HISTORY.md, docs/CRON.md and ROADMAP.md. No production code changes. Corrects the plan's jscpd claim: promoting the six purgeBatch bodies does not lower duplication (484/3042/35 before, 496/3107/35 after, worktree-measured) because jscpd's exact-token matcher never flagged those bodies in the first place - only their import blocks, which grew. Still under the 3% gate (~1.24%), and still justified by rule #2 and by making batch instrumentation a one-place change.
The batch span budget was shared with the lease's acquire/release spans, and the release runs last in the runner's finally — so any run past ~48 batches silently dropped the lease-release span from the trace, the exact span the budget's rationale exists to protect. Add SweepSpans.lease(), backed by a shared dbSpan helper, that always opens a span regardless of the batch budget. Also make both db() and lease() take a lazy sql thunk instead of a pre-computed string, so a skipped db span no longer pays for a toSQL() serialisation it never uses. Claude-Session: https://claude.ai/code/session_01CMYjy6MearjUeEkE3CT3ju
The run span opened by runRetentionSweep carried no attributes of its own, so a run refused because another run held the lease was indistinguishable in Sentry from a run that executed and found no work — the exact scenario the rail was built to make visible. Write sweep.skipped: true before the lost-lease early return, and sweep.deleted / sweep.batch_count / sweep.truncated before the final return, both while the run span is still active. Claude-Session: https://claude.ai/code/session_01CMYjy6MearjUeEkE3CT3ju
The script shared one module-level SweepSpans instance across all 23 checks and 6 route invocations, which no longer models the per-request property it exercises (every route builds its own facade per request). Replace it with a freshSpans() factory called at each acquire/release/ sweepLockFor call site. Claude-Session: https://claude.ai/code/session_01CMYjy6MearjUeEkE3CT3ju
The docstring said the mock factory was "exhaustive on purpose", but nothing enforces that against the real module — this branch alone added four exports (sweepSchema, ilike, getRateLimitDbClient, migrate, ssoSchema) that a test needed and the factory was missing. State it as what it actually is: aims at the full surface, grows on demand. Claude-Session: https://claude.ai/code/session_01CMYjy6MearjUeEkE3CT3ju
…uncation Update docs/CRON.md's trace-shape section to cover the lease release child, the dry-run count child, the run-span attributes on both the skipped and completed paths, and the lease's exemption from the batch budget. Fix docs/FEATURES.md: sweepLockFor's signature was stale (dropped the spans argument), the email sweep's "both cutoffs measured from created_at" claim was false (the sent pass uses sentAt), and sweep-span.ts / sweep-purge.ts were missing from the inventory. Add a paragraph to docs/HISTORY.md recording that the ~1000-span Sentry truncation-from-the-end claim — asserted as fact in the original as-built text despite the plan's own caveat that it was unconfirmed — was subsequently measured live (1200 emitted, 1000 retained, tail dropped), so the record now rests on evidence. Claude-Session: https://claude.ai/code/session_01CMYjy6MearjUeEkE3CT3ju
- purgeBatchWithTimeout: narrow `where` to SQL and fail closed on an undefined predicate, instead of letting and() collapse to an unfiltered full-table delete typecheck; every route's filterFor now asserts its and() result as SQL for the same reason - sweepSpans: wrap capture/attributes bodies in try/catch so telemetry can never alter a sweep's outcome (swallow branch, finally, post-commit) - runRetentionSweep: write sweep.skipped: false on the completed path so the discriminator works as a query, not just a presence check - runRetentionSweep: use op: "function" for the run and pass spans, matching the repo's existing op convention instead of introducing sweep/sweep.pass as a new family - check-sweep-lock: replace the SET LOCAL test's SQL-text assertion (banned pattern) with a live check against current_setting() run inside purgeBatchWithTimeout's own transaction - document IInstrumentation's fourth method (setSpanAttributes) and its silent-drop-with-no-active-span caveat in apps/api/CLAUDE.md and docs/OBSERVABILITY.md Claude-Session: https://claude.ai/code/session_01CMYjy6MearjUeEkE3CT3ju
…guard Seven identical `and(...) as SQL` assertions across the retention sweep routes silently trusted that their predicate arguments were always defined. Promote them into one requireFilter helper next to purgeBatchWithTimeout that throws with a call-site label instead of letting the cast lie the day a predicate goes conditional.
|
🎉 This PR is included in version 1.24.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes the last declared debt on the retention rail (ROADMAP row D.6): the six
/internal/sweep-*routes ran with no telemetry, and two of the runner'scatchblocks logged an error and moved on without ever reporting it.What lands
sweep-span.ts). The rail is functional code — its purge and count helpers are module-scoped functions with no constructor to injectIInstrumentationinto. The façade is built per request fromdi.IInstrumentationand threaded down explicitly, which keeps what repo rule Fix base repository type #8 protects: no module-level singleton, and two labels sweeping concurrently never share a budget.sweep-purge.ts). Six routes carried byte-identicalpurgeBatchbodies; promoting them is what makes instrumenting the batches a one-place change instead of six.sweep.deleted/sweep.batch_count/sweep.stop_reasonas it closes, and the run recordingsweep.skippedor its totals — so a sweep refused because another run holds the lease is no longer indistinguishable from one that found nothing.onBatchError: "break", and a failed lease release. The rethrowing branch stays uncaptured on purpose —app.onErroralready reports it, and capturing twice would double-report every thrown batch error.IInstrumentation.setSpanAttributes—SpanOptionsis open-time only, and what is worth recording about a span is known as it closes.No new events; the catalog stays 81 / 35 public / 46 internal. No behaviour change on the rail: response shapes, the three nested deadlines, lease semantics, the
SET LOCALguards, filters, order columns and label strings are all unchanged, verified route by route against the pre-image.Verification
pnpm --filter api type-check0 errors ·bun test798/798 ·pnpm check:sweep-lock23/23 against real Postgres ·pnpm ci:checkgreen.deleted: 0, every passexhausted, exit 0. Every unit test here runs on a mocked Drizzle, so this is the only step that proves the refactor did not break the SQL.@sentry/bunwith a stub transport:getActiveSpan()after awaiting a nestedstartSpanreturns the parent under Bun, so pass attributes land on the pass span.Three things worth knowing
purgeBatchbodies — each route names different tables and columns — only the routes' near-identical import blocks, which grew. The promotion still stands on rule feat: Define the ddd-kit packages #2; the measurement never supported it.devwas found and fixed:sweep-notifications.test.tsfails in isolation withExport named 'sweepSchema' not found(since88e3e92). Full-suite runs hid it because bun'smock.moduleregistry is process-global — another file's factory supplied the export. The shared mock factory extracted here removes that class of trap.Known residual
spans.capturefills onlymetadata;ErrorContextalso exposesrequestIdandpath. A batch error reaches Sentry identifiable by label and phase, but without request correlation. Deferred rather than re-opening six freshly reviewed route files.https://claude.ai/code/session_01CMYjy6MearjUeEkE3CT3ju