fix(analytics): judge collector health against its own baseline instead of a fixed 0.5 (#6026) - #6041
Conversation
|
@Yigtwxx is attempting to deploy a commit to the World Monitor Team on Vercel. A member of the Team first needs to authorize it. |
…ion copy and crash filters (#6129) * fix(payments): resolve a payment webhook's user from its subscription before failing closed `handlePaymentOrRefundEvent` resolved identity only from HMAC-signed checkout metadata or the `customers` table, while its sibling `handleDisputeEvent` — handling the identical `DodoPaymentData` shape — already fell back to the `subscriptions` row keyed by `dodoSubscriptionId`. Dodo routinely drops checkout-session metadata from payment payloads, and `customers` rows are written only by the subscription handlers, so a renewal charge or a refund on a subscription we already track was resolvable from our own row all along. Instead it threw, rolling back `processWebhookEvent`, dead-lettering the event and leaving Dodo to retry it (observed at attemptCount 4 and 5 in `paymentWebhookFailures`). Also makes the fail-closed message diagnostic. It asserted "no dodoCustomerId" unconditionally, which is false for the events that actually hit it — the dead-letter rows carry `cus_…` — and that is the only signal an operator gets, since the projection is deliberately payload-free. It now names which of the three resolution inputs were present. Only presence is reported for the metadata fields: `wm_user_id` is our internal user id and must not be copied into a string Convex auto-Sentry forwards. A genuinely unattributable event still throws, so the dead-letter and provider retry are unchanged. Tests: every other case in webhook.test.ts routes through `processEvent`, which pre-seeds a `customers` row, so the production shape was never exercised. Both new tests dispatch the mutation directly and were confirmed red first — reverting the fallback turns the resolution test red again. WORLDMONITOR-YA (9 events / 4 users) Claude-Session: https://claude.ai/code/session_01P9SZGzqRV8ov6kWwZNpxYs * fix(sentry): stop two known third-party crash classes re-escaping beforeSend WORLDMONITOR-Y4 is the third build-rename of the DebugBear RUM wrapper already handled as VC and VQ. The gate matched the trampoline's minified function name (`window.fetch`, then `Rt.window.fetch`), and Vite emitted one hop of the same trampoline as a bare `t` — a name no fetch-anchored pattern can match. Bare names are now admitted, but only at <=2 chars and only inside the two chunks already on the allowlist. `fetchContent` (SG) and `apiClient.fetch` both stay above that bound and still surface; their existing regression tests caught a first attempt at this that dropped the name check entirely, which would have turned the gate into a blanket chunk allowlist and hidden real fetch failures. What keeps the tolerance honest is that neither module backing those chunks issues a fetch of its own — previously a comment asserting a grep. The new tests/debugbear-trampoline-chunks.test.mjs fails if either ever gains one, so the gate's premise cannot rot silently. WORLDMONITOR-WK is `Maximum call stack size exceeded` with an empty stack. Zero frames prove nothing here — a blown stack is exactly when the SDK cannot collect them — so the OS census is the load-bearing half: 23 events across 20 users, 100% iOS, 21 of them inside the Google app's in-app WebView, zero desktop, zero Android, one release. Our bundle is the same code everywhere, so a first-party recursion cannot be confined to one iOS WebView family. Triple-gated on frames, first-party frames, and OS, so a real recursion regression still reaches Sentry. Both filters were mutation-tested: removing the bare-name tolerance turns the Y4 test red, and dropping the iOS gate turns the desktop-must-surface test red. WORLDMONITOR-Y4 (10 events / 9 users), WORLDMONITOR-WK (23 events / 20 users) Claude-Session: https://claude.ai/code/session_01P9SZGzqRV8ov6kWwZNpxYs * fix(analytics): give the collector queue the depth its upstream buffer delivers `src/services/analytics.ts` buffers up to `UMAMI_QUEUE_LIMIT` (50) tracker calls made before the Umami script loads, and `flushPendingUmamiCalls` splices and dispatches all of them in one synchronous loop. The transport queue in front of the single in-flight slot held 25, so a full flush shed roughly half its events as `queue-overflow` before they reached the network. That is not backpressure against a slow collector — it is two limits that were never reconciled, and the events are simply lost for the affected page. Raising the queue depth does not raise concurrency: writes still drain one at a time, which is what keeps umami#4183 session_data contention off the collector. tests/analytics-queue-capacity.test.mjs fails if the two constants drift apart again, and asserts the constants still parse so a rename cannot pass silently. Also fingerprints the failure report. Tags do not split Sentry issues, and without a fingerprint the fixed message grouped all five failure kinds into one 2230-event issue — burying our own dropped writes inside the ad-blocker population, which is unactionable by design. Cardinality stays bounded at five kinds times the small status set. WORLDMONITOR-Y3 (2230 events / 1747 users) Note: the separate aggregate-floor false alarm on the same subsystem (WORLDMONITOR-Y6/Y7) is #6026, addressed by PR #6041 against api/analytics-health.js. No overlap with this change. Claude-Session: https://claude.ai/code/session_01P9SZGzqRV8ov6kWwZNpxYs * fix(wm-session): tell the user the remedy that matches why the session died The degraded-session toast said "Check your cookie settings, then reload" for all three causes. Only `cookie_not_persisted` is about cookies. `mint_failed` means /api/wm-session never answered — offline, a content blocker, or the 10s timeout — and `retry_401` means the cookie was delivered and the server rejected it, so the session is stale rather than missing. Now that WORLDMONITOR-WG has decayed from ~10k episodes/day to single digits after #5674/#5683/#5798, `mint_failed` is roughly two thirds of what remains, so the majority of users who still see this toast are being sent to fix a setting that was never at fault. `markWmSessionDead` already knows the reason; it just dispatched a bare `Event`. It now carries the reason on a CustomEvent and the copy is chosen from it. A pre-#6120 bundle in a long-lived tab still dispatches an Event with no detail, so the handler keeps the old wording as its fallback. The mapping lives in a dependency-free leaf rather than wm-session.ts, which reaches the runtime fetch patch through `@/` aliases and therefore cannot be imported from the tsx test suites — the same reason premium-intent.ts exists. The test asserts the strings themselves, since wrong copy is exactly the defect no type or wiring guard can catch; collapsing the mapping back to one message turns three of its five cases red. WORLDMONITOR-WG (residual after the 07-27/07-29 fixes) Claude-Session: https://claude.ai/code/session_01P9SZGzqRV8ov6kWwZNpxYs * chore(docs): refresh the service-module count for the new wm-session copy leaf `src/services/wm-session-copy.ts` moves `serviceTopLevelEntries` from 221 to 222, which the docs-stats gate tracks in both directions: the generated snapshot must be regenerated, and AGENTS.md quotes the number in its tree listing, so `docs-stats --check` fails until the prose matches too. Both gate steps verified locally: the snapshot is clean and all 121 doc claims match code. Claude-Session: https://claude.ai/code/session_01P9SZGzqRV8ov6kWwZNpxYs * fix(analytics): size the overflow test from the queue bound instead of a literal Raising COLLECTOR_QUEUE_LIMIT to 50 broke `drops a non-critical write before a queued conversion when the queue overflows`. The test filled a literal 30 slots against a comment asserting the bound was 25, so at 50 it filled 60% of the queue and the eviction under test simply stopped happening. Export the constant and size the loop from it (+5). A literal is the wrong shape for this: it does not fail loudly when the bound moves, it silently stops exercising the branch — the failure here was visible only because 30 fell below the new bound. Had it landed above, the test would have kept passing while asserting nothing. Verified the assertion is real, not just green: filling to LIMIT-10 turns it red again. Also widens the capacity guard's source pattern to accept the new `export` prefix. That guard caught this change itself, failing with the exact "renamed or made computed" message it carries for the case — and it still fails on a reverted queue depth afterwards, so accepting the prefix did not defang it. Full test:data run locally: 20484 pass, 2 fail — both are tests/dashboard-critical-css.test.mjs cases that read dist/dashboard.html, which this worktree has no build for. CI builds first and passed both in the run that caught this bug. Claude-Session: https://claude.ai/code/session_01P9SZGzqRV8ov6kWwZNpxYs
|
Thanks for this @Yigtwxx — the diagnosis in the description is right, and the move from a raw quotient to a sample-size-aware interval is the correct shape. The Wilson helper, the derived floor, and the per-cohort baseline are all good ideas. Reviewed it in depth; there's one blocker and one design question, plus some test gaps that hid both. Blocker: a saturated baseline makes the alarm permanently unsatisfiable
return observed > wilsonBounds(baseline.failures, baseline.writes).upper;
There are two ways to reach that state, and the first is self-inflicted:
The same lever works in reverse, incidentally: three requests/day of Suggested handling: give the baseline a veto ceiling. Above some upper-bound threshold (0.9 works) the baseline is no longer describing "normal", so it should lose its veto and let conditions (1) and (2) decide alone. That's strictly the safe direction — a saturated baseline can only ever have suppressed alerts. Worth pairing with excluding already-breached windows from the day accumulator, so the outage stops feeding the thing that later exonerates it. Design question: does the baseline gate actually fix #6026?A full day of counters collapses the baseline's Wilson interval onto its point estimate: So condition (3) degenerates into The perverse part is that higher traffic tightens the window CI, so the gate gets more trigger-happy as volume rises — weakest during the busiest hour, which is exactly the #6026 incident condition. The 3-consecutive-window requirement doesn't help either, since a peak hour spans ~60 windows. This isn't necessarily wrong — it depends on whether this deployment's peak-hour ad-block rate actually runs above its daily mean, which is your call and needs the measured data. But the current test only pins the 60%-vs-61% case, where peak sits below the mean, so it doesn't exercise the situation that caused the incident. Options: compare against the same hour-of-day from the baseline period, or widen the baseline interval by the between-hour variance rather than treating a whole day as one binomial sample. The tests can't see either problemRunning mutants against the suite as it stands (23/23 green baseline):
To be clear, the suite is not toothless — the decision-function mutants your description lists really are killed (point-estimate instead of Wilson, streak 3→1, dropping the Smaller items
Also worth saying: wiring Getting the fixesI have all of the above except the two design items implemented and verified — 30 tests passing (was 23), with all nine mutants above confirmed killed, biome clean, and the file still bundling for the edge runtime. I couldn't push it here because "Allow edits by maintainers" is unchecked on this PR, so the branch is only writable by you. If you tick that box I'll push the branch directly. Otherwise happy to attach the patch or open it as a separate branch you can pull from — whichever you prefer. |
…a73#6026) The aggregate alert compared a raw quotient to a fixed 0.5 on a denominator that could be as small as 5, so ordinary ad-blocker attrition and a dead collector produced the same verdict. On 2026-08-01 it pegged at the once-per-window ceiling through the day's busiest, outage-free hour. Three changes, none of which invent a calibration: - rate judgements now read the low end of a Wilson score interval, so a window's sample size is part of its claim rather than invisible; - MIN_WRITES is derived from the interval half-width at p = 0.5 instead of picked, which puts it at 31; - a window is compared against the previous day's observed rate for the same cohort, so the alert fires on a departure from normal rather than on normal, and three consecutive breached windows are required before it emits. The absolute MIN_FAILURE_RATE floor is left at 0.5 on purpose: raising it needs the measured ad-block baseline, which is an operator input. Also wires api/analytics-health.test.mjs into test:sidecar. The file existed but no script ran it.
46c46b4 to
ff39fcf
Compare
|
Box is ticked — the branch is writable by you now, push whenever suits you. I rebased onto The blockerConfirmed, and it's not just "can never be true" — it's exactly unsatisfiable, independent of The 0.9 veto ceiling is the right shape. Worth noting the pairing you suggest isn't optional: the ceiling alone only rescues the fully saturated case. A collector failing at 85% still publishes an 85% baseline the next day, which sits under the ceiling and keeps its veto, so the outage still exonerates itself — just quietly. Excluding already-breached windows from the day accumulator is the part that actually closes that path. The derivation commentConfirmed, and your n is right. The design questionThis is the one I can't settle from the code, and I think the framing points somewhere slightly different from either option you listed. The day aggregate's Wilson interval measures sampling error only. It treats a day as one homogeneous binomial, when the quantity that actually moves is the audience mix by hour — so the interval collapses toward ~0.001 while the real between-hour dispersion stays exactly where it was. That's the mechanism behind the perverse part you spotted: more samples buy a tighter estimate of a quantity that was never the source of the variation, so the gate tightens as volume rises. Which suggests the statistical machinery on the baseline side isn't earning its place at day scale however it's sliced. The cheapest honest version is probably to stop treating (3) as an interval comparison at all and give it an explicit effect size — All three want the same input from you that |
Summary
Fixes #6026.
api/analytics-health.js:47-49decided the whole thing on a raw quotient:Two separate reasons that cannot separate a dead collector from ordinary attrition.
The denominator carried no weight.
failures / writesreads identically for 5/5 and 5000/5000, and those are not the same claim. At theMIN_WRITES = 5floor the 95% Wilson half-width at the worst case p = 0.5 is 0.368 — the rate was not resolved at all, so the floor was not measuring anything.The threshold was absolute, and the quantity it measures is not. 0.5 is a property of this audience's ad-blocker baseline, not of collector health. The issue's own evidence is the proof: 21:00 UTC on 2026-08-01 was the busiest hour of the day with zero gap, 67,394 events landed, and the alert still pegged at the once-per-window ceiling of 60. No sample-size fix reaches that case — the window was large and its rate really was above 0.5. It was simply normal.
What this changes
1. Rate judgements read the low end of a Wilson score interval, not the point estimate. Sample size becomes part of the claim instead of being invisible. 17/31 is 54.8% and clears the old gate; its lower bound is 0.4035 and it no longer does.
2.
MIN_WRITESis derived rather than picked. The half-width of the interval at p = 0.5 isz * sqrt(0.25 / n), so n = 31 is the smallest denominator that resolves the rate to within ±0.15 (0.1477 — n = 30 gives 0.1502). That is the resolution needed to tell an ad-blocker baseline from a collector that has stopped accepting writes. The comment records the derivation so the next person can re-derive it rather than inherit another round number.3. A window is compared against that cohort's own observed baseline. Per-cohort day-scoped counters accumulate alongside the window counters, and the previous — complete — day supplies the baseline. The alert fires only when the window's lower bound clears the baseline's upper bound, which is the issue's second suggested direction ("calibrate it from observed healthy-hour rates rather than a round 0.5") with no operator number required. Against the 21:00 hour:
0.5886 > 0.6108is false, so it stays silent. A genuine collector death — 190/200 — has a lower bound of 0.9181 and clears it immediately.4. Three consecutive breached windows are required before Sentry hears about it. The stored value is
count:bucket, so a gap of two or more windows resets the run rather than letting alternating breached/healthy windows accumulate into a false streak, and re-entering the same bucket is idempotent (every isolate in a window reads the same prior value and computes the same successor). Per the issue's own timings this costs at most two extra minutes — the real outage was surfaced in 9.5. The Sentry payload now carries what the floors would have to be tuned against:
failureRateLowerBound,baselineFailureRate,baselineFailureRateUpperBound,baselineWriteCount,consecutiveBreachedWindows, and the two constants in force.Cost is one Redis round trip, unchanged. The window counters, the day accumulator, the previous day's baseline and the streak all ride the existing pipeline (6 commands to 13). A healthy window still makes exactly one call — there is a test for that, because the endpoint is on the hot path.
Design decisions left for you
MIN_FAILURE_RATEis deliberately still 0.5. It is now a backstop rather than the primary gate — it only stops an alert on a deployment whose baseline is so low that a statistically real excursion is still operationally uninteresting. Raising it is the one part of fix(analytics): collector aggregate-health alert fires during healthy peak traffic (MIN_WRITES=5 / MIN_FAILURE_RATE=0.5) #6026 that needs the measured ad-block baseline, and picking a number for it here would be inventing a calibration from data I do not have. Happy to set it if you have the figure.MIN_CONSECUTIVE_BREACHED_WINDOWS = 3trades two minutes of detection latency for immunity to a single noisy window. If you would rather have the minute back, 2 still kills the single-window case.EXPIRE ... NXor hourly sub-buckets; a day-scoped key means re-arming its TTL on every write cannot extend the window it measures, which seemed worth more than the boundary alignment. A long outage moves a full day's rate by a few points, so it does not blind the alert; the alternative — using the previous hour — would, since an outage hour would become the baseline for the next one.fingerprintis untouched, so existing grouping is preserved.Verification
Every guard is mutation-proven:
MIN_WRITESback to the pre-fix 5No survivors.
The fixtures are built through a
pipelineResults()helper shaped exactly like the reply the endpoint reads, so a test cannot assert against a Redis state the real call could not produce. Two of the cases are the incident itself rather than synthetic values: an hour of baseline-matching windows that must stay silent for all 60 of them, and a three-window run that must stay silent for the first two.Other gates:
npm run test:data: identical failure set toorigin/main— 47 failing test names on both,commdiff empty in both directions.One thing found along the way
api/analytics-health.test.mjswas not run by anything. It is in neithertest:data(which globstests/plus a fixed list) nortest:sidecar, and no workflow picks it up, so the four assertions it already carried had never gated a merge. This PR adds it totest:sidecarnext to the otherapi/*.test.mjsentries — that is the 306 to 329 above.Out of scope
src/services/analytics-collector-transport.ts:396(ENVIRONMENT_NOISE_MIN_WRITES = 5) is a separate per-page fallback floor on the client, not the cross-user aggregate this issue is about. Left alone deliberately; changing it would alter what the browser reports rather than how the server judges it.queue-overflowkind being downstream ofnetwork/timeoutrather than independent, noted at the end of fix(analytics): collector aggregate-health alert fires during healthy peak traffic (MIN_WRITES=5 / MIN_FAILURE_RATE=0.5) #6026. That is a taxonomy change in the transport, with its own blast radius.Type of change
Affected areas
/api/*) —/api/analytics-health; request and response contracts are unchanged, only the internal alert decisionpackage.jsontest wiringChecklist
api/rss-proxy.jsallowlist (if adding feeds) — N/A, no feeds added.npm run typecheck)Documentation Alignment Checklist
N/A — this PR does not publish or change a documentation claim. It changes when an internal Sentry warning is emitted; no methodology, API/MCP contract, generated doc or example changes. Listed for completeness:
analytics:collector-health:v1:{env}:prefix, neither of which is documented anywhere:…:day:{dayIndex}:{cohort}:{writes,failures}(written and read here only, TTL 48h) and…:{cohort}:streak(written and read here only, TTL 180s). The existing…:{bucket}:{cohort}:{writes,failures,reported}keys are unchanged in shape and TTL.