Repository navigation
Measure Collector Cost Regression against the baseline's upper edge, not its mean - #3441
Conversation
…not its mean A heavy collector's own per-run spread already exceeds CostRegressionFactor, so against a mean baseline a normal upper-mode day cleared the 2.0x ratio by construction and the alert could not tell "this collector got slower" from "this collector had a normal slow day". RegressionSql now also computes percentile_disc(0.95) of the PRIOR days' per-run cost and requires the factor on that as well as on the run-weighted mean. The two are a conjunction, so the predicate can only select a subset of what it selected before. CostRegression.ThresholdMsPerRun derives the bound the fired alert reports from the same two baselines the query gates on.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
ReviewWent through the SQL predicate change, the derived-threshold record, the evaluator's alert text, and all three test files ( Correctness: The conjunction is genuinely narrowing-only — Left one inline comment on Lite/Darling parity: No drift. Style: N/A for the T-SQL conventions ( Tests: The dispersion fixture is well-constructed — the count-preserving permutation pair ( 🤖 Generated with Claude Code |
| AND sc.latest_ms_per_run > sc.baseline_ms_per_run * $3 | ||
| AND sc.latest_ms_per_run > sc.baseline_p95_ms_per_run * $3 | ||
| AND (sc.latest_ms_per_run - sc.baseline_ms_per_run) * sc.latest_runs >= $4 | ||
| ORDER BY (sc.latest_ms_per_run - sc.baseline_ms_per_run) DESC"; |
There was a problem hiding this comment.
Minor, non-blocking: now that a row can be selected because it cleared the p95 conjunct rather than the mean one, ranking by latest_ms_per_run - baseline_ms_per_run (the mean gap) no longer reflects "how far past the bound that actually gated it" this row is — a row that barely clears its p95 bound but has a large mean gap ranks above one that blows through a much tighter mean-based bound. Not a correctness bug (both are still CostRegressions, both still get reported), just a ranking nit if the ordering is meant to reflect "most regressed by the bound that matters."
|
Reviewed the SQL, the C# evaluator changes, and both new/modified test files. Correctness
Lite/Darling parity
Style
Minor nit (left inline, non-blocking): the final Overall: correct, narrowing-only by construction, well covered by the new dispersion test and the two supplementary self-alert tests. No security, parity, or performance concerns. |
|
Reviewed the diff (new SQL correctness (
Lite/Darling parity
Style
Security
Nice test design on the dispersion fixture — the count-preserving permutation ( |
…ngle-line anchor does not belong in
|
Reviewed the diff ( Correctness — checked out. Traced the Lite/Darling parity — no drift. Security — no external input surfaces here; this is an internal self-monitoring query with parameterized values ($1–$4). Style — matches the Postgres conventions in CONTRIBUTING.md (naive UTC timestamps via One minor process note: every comparable prior self-alert threshold change in this area (#3316, #2846) got a No inline findings to post; didn't find any correctness, security, or parity bugs. |
…3456) (#3459) * Make the one-line change CollectorCostDigestTests reserved for the #3441 merge The digest tests' Regression() factory carried a remark saying #3441 would add a member to CostRegression's positional constructor and that keeping every fixture behind one factory made that merge a one-line change here. The two lanes crossed and the line was never written, so Darling.Tests has not compiled on dev since the second of them landed. BaselineP95MsPerRun is pinned to the fixture's mean, which makes ThresholdMsPerRun resolve to the pre-#3441 bound exactly - every fixture keeps asking the routing question it was written to ask (single-server demotion, fleet-wide paging, the majority boundary) rather than silently gaining a dispersion conjunct that belongs to CollectorCostRegressionDispersionTests. * Decline the cooldown seed for server keys the integer column cannot hold, instead of answering with the fleet-wide bucket (#3456) Both history stores mapped the string server key onto config_alert_log's integer server_id with int.TryParse(...) ? sid : 0, on both sides of the table. On the write side that is a tolerable collapse - the row must be recorded somewhere, and 0 is where every non-integer key lands. On the read side it was the defect: the restart seed asked 'when did THIS key last deliver?' and got MAX(alert_time) over EVERY collapsed key's rows - the fleet-wide last send of the metric - so for the whole self-alert family (cost:<id>:<collector>, the AG composites, the sentinels) the per-fingerprint cooldown #1154 built degraded back into a fleet-wide per-metric mutex, one layer down. Measured twice on one day: same-metric pairs 3.7 ms apart where the second seeded against the first's just-written row and rendered throttled despite its own last delivery being two hours old. The mapping now lives in one shared type, AlertHistoryServerIdentity, the way AlertValueParser already does for the value columns and for the same drift reason. StorageId keeps the write-side collapse unchanged; SeedScope declines (null - no seed - first notice) any key whose rows are not distinguishable by server_id alone: the unparseable keys, and the literal 0, because the bucket's rows are mixed by construction and an answer from it is exactly the cross-key history the invariant forbids. The invariant: the seed never answers one key's question with another key's history. Declining fails toward posting, the direction the delivery doctrine already names. What this deliberately does not do: restore cross-restart cooldown continuity for the self-alert family, or make #3436's roster engage for their slow recurrences - both need seed-and-stamp identity that survives the integer column, which is a schema-shaped follow-up, not a parse fix. StorageId is the one line per direction that follow-up changes. Pinned four ways: the pure mapping in AlertHistoryServerIdentityTests; the real DuckDB store's refusal (and the write-side collapse it guards) in StoreRoundTripTests; the service-level consequence - a first notice on an unparseable key attempted, never throttled by another key's send, with the integer-key seed as its control - in WebhookCooldownSeedTests; and the banked shape end to end in RepeatDeliveryBudgetTests, two self-alert siblings milliseconds apart through the real deliverer against a column-faithful store double, both posting, with the no-roster consequence asserted rather than left silent.
…#3462) (#3463) * Give the cost regression an absolute materiality floor, derived from the fleet's own daily-cost distribution, on both delivery surfaces (#3462) A 5.5x regression of a 122 ms/run collector paged hourly on 2026-09-15 while costing 8.7 seconds of collection time all day, and the operator's reaction - "it's a few hundred ms. what's the point?" - is the whole issue: since #3441 the predicate is purely relative, so a regression of something tiny fires identically to one of something expensive. The same day's genuine exhibits added 224 and 377 s/day. Both classes clear the ratio; only one is worth a human. * Recalibrate CostRegressionAddedMsFloor from the fleet distribution (#3316's constant, raised 5,000 -> 15,000 ms/day) The card that produced this issue broke #3316's calibration honestly: its 41-hour sample put the truthful-unactionable mass at 0.16-3.6 s/day and claimed 5 s held the empty band with 4x headroom, and an 8.7 s/day unactionable firing shows that mass reaches at least 8.7, so 5 s sits BELOW the band, not inside it. Per the issue, the new value comes from the measured fleet distribution of per-collector daily cost rather than from that one card: over 7 days on two stores of one production store class (42 and 43 servers), the median (server, collector) pair costs 13.7 and 13.1 s/day to exist at all, and the cheapest 17 of 39 collectors each cost under 4 s/day. A regression whose whole added footprint is under the median collector's daily bill is lost in the fleet's own operating mass. 15 s sits just above both medians - independently, at the geometric midpoint (13.8) of the measured empty band (8.7, 21.8) - and the constant's remarks now state the thinner headroom instead of the disproved 4x claim. It stays one named constant with one unit, evaluated in the reads themselves, so every surface derives one definition of material and nothing recomputes it at delivery time. * AND it beside #3441's conjunction, which it cannot loosen The floor was already a conjunct of RegressionSql ($4), so the paging predicate's shape is unchanged and #3441's subset property is undisturbed: a raised floor ANDed beside the ratio-and-dispersion gate can only shrink the selected set. The fired alert's threshold text now states the floor beside the per-run bound - #3441's rule extended across units: the reported threshold is the conjunction that actually selected the row, or a reader falsifying the arithmetic reconstructs a looser predicate than the one that fired. * Narrow the digest's inclusivity, which #3448 shipped as load-bearing prose, because #3462 says to and says why The movers read carried "no materiality floor and no dispersion bound" as a design claim: ranking by |added s/day| made a threshold unnecessary because the quantity that would have been thresholded was the sort key. The row cap is a presentation bound, though, and on a quiet fleet the list filled its remainder with rows worth a few hundred milliseconds a day - the issue's words are that a 5.5x-of-nothing row in a daily digest is the same noise in a quieter channel, and that the evaluation belongs in the metric so both delivery surfaces inherit it. So MoverSql now gates on the SAME constant, taken on abs() of the same expression it ranks by and CostMover.AddedMsPerDay derives, so the gate, the order and the printed figure cannot disagree: a collector that got materially CHEAPER keeps its line (the half of the series the paging condition structurally cannot report), an immaterial move in either direction loses it, and eligible_pairs becomes the denominator that survived the floor so the digest's "N of M" still counts from the answer the rows came out of. What the digest still refuses to carry is the part that decides - no ratio factor, no dispersion bound - and the census section keeps no floor at all, because expensive-without-moving is that section's whole catch (#2862). The demotion's fail-toward-the- report guarantee is not narrowed: every selected regression clears the shared floor by construction, so every row the routing receives stays digest-visible. * Fixtures: the issue's exhibits bracket the shipped value, and the discriminating pair is a ratio-preserving rescale CollectorCostMaterialityFloorTests runs the SHIPPED constant (now internal) against the shipped queries on a live store, unlike its siblings, which pin query mechanics at explicit values: the noise exhibit (668.5 vs ~123 ms/run over 13 runs, past the mean ratio, the p95 bound and the old 5,000 ms floor, worth 7,088 ms/day) must not return, its x32 rescale - same twelve days, same 13 runs, identical mean and p95 ratios by construction, 226,824 ms/day - must, so the exclusion is attributable to absolute magnitude and nothing else, and the recorded plan_correction (223 s/day) and query_stats (263 s/day at a chosen baseline; 189 s/day at the loosest baseline the ratio admits) exhibits must keep firing. The movers read gets the same treatment in both directions, with eligible_pairs counting survivors. A no-store pin brackets the constant inside the measured band and computes that the #2150 reconstruction (26,582,400 ms/day) clears it a thousandfold, so a retune red-lines instead of silently re-admitting the noise or eating the catches. The dispersion suite keeps its explicit 5,000 ms floor deliberately - its fixtures' 10,500 ms added cost sits UNDER the new production value, and passing that would turn a dispersion test into a materiality one - and its "runs at the PRODUCTION floor" remark is rewritten to say so, since it stopped being true here. The digest reconcile fixture's rows are re-scaled to figures the floored read can actually return. No migration rung and no schema change: both bounds are computed from columns collect.collector_cost already carries, and get_collector_cost serves exactly what it served before - only the alert/digest evaluation gained a condition, which is the issue's stated scope. Closes #3462 * State the calibration in the floor's own unit The recalibration remark measured the noise exhibit's whole-day total (8.7 s) where the floor gates its ADDED cost - 7,088 ms/day, 1.42x the old floor - so the band's lower edge, the headroom figures, and the band pin all moved to the unit the constant actually compares against. A geometric-midpoint claim that was false even on its own numbers is gone; the floor's justification is the medians and the band, stated with the corrected margins (2.1x above noise, 1.45x under the smallest real catch).
1 participant
Fixes the threshold half of #3440. The delivery question that issue also raised — whether this metric belongs on a paging channel at all — is #3443 and is not addressed here.
Collector Cost Regressioncompared one day's per-run cost against a mean 14-day baseline at a factor of 2.0. Every heavy collector's own per-run spread already exceeds that factor — measuredp95/avgon one production fleet: 2.03x and 2.95x forindex_object_statson two servers, 4.74x forprocedure_stats, 5.04x forquery_store— so a perfectly normal upper-mode day cleared the ratio by construction. The alert could not tell "this collector got slower" from "this collector had a normal slow day", which is the only question it exists to answer. The firing that produced the issue was 17,548 ms/run against a 6,477 ms mean, on a once-daily collector whose own worst run in the same week was 17,935 ms.The spread is structural on the daily-cadence collectors, which is why no choice of factor fixes it: a once-daily collector contributes one run per day, so a 14-day baseline is the mean of about fourteen single observations and the quantity tested against it is one more single observation from the same distribution. There is no within-day averaging to damp either side, and widening the window adds observations to the mean without narrowing the thing being tested.
#3316's
CostRegressionAddedMsFloorcannot screen these, because the expense that makes a collector bimodal also makes its upper mode's excess large: the measuredquery_storespread adds 16,534 ms on a single p95 run, 3.3x the 5,000 ms floor.The change
DarlingCollectorCostReader.RegressionSqlnow also computespercentile_disc(0.95)of the prior days' per-run cost, and requires the latest day to clear the factor on that as well as on the run-weighted mean.The two are a conjunction rather than a replacement, so the predicate can only ever select a subset of what it selected before — nothing that was silent can start firing, and no distributional assumption about p95 versus mean is load-bearing for that property.
That matters more than it first looks, because at the shipped window the mean conjunct is entailed and cannot independently exclude anything.
percentile_disc(0.95)over N rows returns rankceil(0.95 * N), which equals N for every N up to 19, so on a baseline of at most 13 days the p95 is exactly the maximum of the daily per-run costs; andbaseline_ms_per_runissum(sql_ms) / sum(runs), the run-weighted mean of that same population, which cannot exceed its maximum. SoMath.Maxalways resolves to the p95 side on any row the query can return, and no fixture can exercise the mean side. It is kept anyway, deliberately: the subset property then holds by construction — a conjunct added, none removed — rather than by an entailment whose only precondition is a window constant in another file. Past 19 baseline days DISC stops returning the maximum, the entailment ends, and a predicate that had dropped the mean conjunct as dead would loosen at that moment with nothing red to say so. Both conjuncts and that window precondition are pinned.CostRegression.ThresholdMsPerRun(factor)derives the bound from the higher of the two baselines and is what the fired alert now reports, so the threshold a reader falsifies against is the one that actually selected the row; it was the mean-derived figure, which after this change is the non-binding conjunct. It is derived rather than carried, for the same reasonAddedMsPerDayis, and theMath.Maxis what makes an incoherent construction degrade to the pre-change bound rather than to a looser one — there is no member holding a bound for awithexpression to overwrite.Two deliberate choices, both reusing reasoning the product already applies to collector cost on an adjacent surface:
percentile_disc, notpercentile_cont— the same choice and the same reason as the per-run p95get_collection_healthserves (avg_duration_ms averages across a bimodal population: query_store's empty runs cost 36 ms and its productive ones ~80 s #2460): the baseline holds at most 13 prior days, and an interpolation between a bimodal series' two modes is a figure no day ever cost.No migration rung and no schema change:
collect.collector_costalready carriesrun_countandtotal_sql_msper hour, which is everything the percentile is computed from.What this stops detecting
A regression that raises a collector's daily per-run cost above the factor on its mean but not above the factor on its own worst prior day. On a single-population collector that band is narrow, because
p95 ≈ mean. On a wide one it is wide — a collector whose worst day is 3x its mean now needs a 6x day. Those are precisely the regressions indistinguishable from the collector's own bad days, which the alert could not honestly claim to detect against a mean baseline either; the difference is that it now stays silent instead of reporting them as 2.7x. It also inherits the mean baseline's existing exposure to a slow drift that the 14-day window absorbs.The alternative considered was making a collector whose own
p95/avgexceeds the factor ineligible for the test. That is smaller, and it suppresses the measured firing, but it would remove the four collectors above from detection entirely and permanently — including the shape that found a collector running 37-100 minutes in #2150 — and the exclusion is self-reinforcing, since a real regression widens the spread that excludes it.Tests
CollectorCostRegressionDispersionTestsruns the shipped query against a real store, like its two siblings, because the bound is a line of SQL and a text assertion on it would pass against any query containing the right words.The fixture's first two collectors are a count-preserving permutation of each other: the same twelve baseline days, the same twelve runs, the same 84,000 ms baseline total and therefore the same 7,000 ms/run mean, and the same 17,500 ms latest day over one run — identical ratio, identical
AddedMsPerDayof 10,500 ms, identical everything the old predicate consumed. They differ only in how the baseline's 84,000 ms is distributed across its twelve days. One is reported and the other is not, so the exclusion is attributable to dispersion and to nothing else, and flattening the bimodal baseline to the even one turns the test red.Both directions are asserted.
bimodal_regressedcarries the identical bimodal baseline with a latest day just past the bound and must still be reported, so the change cannot be satisfied by disabling the alert;tight_regressedis the single-population case where the new bound must be inert.interpolation_would_reportpins the instrument: its latest day sits above the interpolated bound and below the discrete one, so swappingpercentile_discforpercentile_contin the shipped query turns that exclusion into a firing.BothConjunctsStayShipped_AndTheEntailmentsWindowPreconditionStillHoldsneeds no store. It asserts both conjuncts are present in the shipped SQL, and readsCostRegressionBaselineWindowout of the evaluator's source to assert that DISC's rank still equals the population size at that window — so dropping the mean conjunct as dead code fails, and widening the window past the point where the entailment holds also fails, which is the moment the mean conjunct becomes load-bearing.Also pinned in
DarlingSelfAlertTests: the fired alert'sNumericThresholdValueis the bound that selected it rather than the mean-derived one, and the derived bound never falls below the pre-change bound across a grid of p95 values including zero and values under the mean. That grid asserts its own loop bound, so a grid mutated to zero rows cannot pass having checked nothing.collect.collector_costand this condition are Darling-only — Lite has no twin of the series and noCollector Cost Regressioncondition — so there is no second store implementation to add and no Lite value that could read permanently empty.