Repository navigation
Route collector cost regression to a daily digest except when it is fleet-wide (#3443) - #3448
Conversation
…ap lives CollectorCostDigestTests covers the routing partition (single-server demotion however large, the fleet-wide twin that still pages, the majority boundary at floor two), the digest document's own identities (each line's ratio and s/day reconcile against the figures printed beside them, ranked as MoverSql delivers), and the #2862 census and #2150 regression figures reconstructed as fixtures. Three expected values are corrected from the interrupted draft: the reconcile fixture arrives pre-ranked because the renderer trusts its reader's ORDER BY and does not sort; -30555.0 s/day is what (456.2 - 1,150.6) x 44,002 actually renders; and 5,489 ms/run is the summary row's integer division (#2862 quoted the rounded 5,490). The Lite.Tests pin the digest tests' doc comment cites now exists: CollectorCostDigest_IsInfoBlueOnPurpose holds the one deliberate INFO arm against the next #2090-style sweep reading it as a miss.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| var regressed = regressedServers[regression.CollectorName].Count; | ||
| var collected = collectedServers.TryGetValue(regression.CollectorName, out var c) ? c : 0; | ||
|
|
||
| if (regressed >= CostRegressionFleetWideMinServers && regressed * 2 > collected) |
There was a problem hiding this comment.
Correctness bug: a collector missing from the census pages instead of failing safe.
When a collector has no entry in census at all (e.g. brand new, or absent from the trailing 24h window), collected is 0. The majority test then becomes regressed * 2 > 0, which is trivially true for any regressed >= 1. Combined with the regressed >= CostRegressionFleetWideMinServers (2) floor, any collector with 2+ regressed servers and zero census rows will page, which is the exact opposite of the documented intent:
"A collector absent from it has no denominator and is report-only" / "this routing failing toward SILENCE"
This also contradicts the PR's own test ACollectorMissingFromTheCensus_IsReportOnly_NotPaged, which uses exactly 2 regressed servers for a collector absent from the census and asserts routing.Paging is empty. Tracing the code by hand: regressedServers["brand_new_collector"].Count == 2, collectedServers has no entry for it so collected == 0, and 2 >= 2 && 4 > 0 evaluates true — so this specific test should fail as written.
Suggested fix: require a known, non-zero denominator before applying the majority rule, e.g.
if (collected > 0 && regressed >= CostRegressionFleetWideMinServers && regressed * 2 > collected)
|
|
||
| if (census.Count > 0) | ||
| { | ||
| sb.Append("\nHeaviest collectors over the same window, every server together - a collector can be" |
There was a problem hiding this comment.
Minor accuracy issue: this text says the heaviest-collector census is "over the same window" as the "Moved most" movers section printed just above it, but they're actually computed over two different windows:
- Movers (
GetCostMoversAsync) usesCostRegressionBaselineWindow= 14 days (_utcNow() - CostRegressionBaselineWindow). censusreused here isGetTopAsync(postgres, _utcNow() - CollectorCostDenominatorWindow, ...)= a trailing 24 hours (fetched earlier inEvaluateCollectorCostAsyncfor the routing denominator, then reused for the digest render).
So a reader is told the heaviest-collectors ranking spans the same baseline as the movers, when it's actually a much shorter (24h) window. Worth either wording this more precisely (e.g. "over the trailing 24 hours") or fetching the census over the same 14-day window if that's what was intended for the digest's heaviest-first section.
There was a problem hiding this comment.
Confirmed still shipped: the census denominator is the trailing 24 hours (CollectorCostDenominatorWindow) while the movers compare against the 14-day baseline, so "over the same window" is wrong in exactly the way an operator would act on. One-string reword riding the same follow-up PR as the severity carve-out above.
|
Reviewed the routing/digest change. Overall the design is sound and well-tested, but I found one correctness bug that looks like it should make the PR's own test suite fail, plus a minor doc/text accuracy nit. Left both as inline comments:
No Lite/Darling parity concerns — this condition is explicitly Darling-only (no |
… stop an unknown denominator paging #3443's evaluator pass grew two swallowed store reads beside the regression read: the collector-cost census (get_collector_cost's own ranked read, supplying the fan-out denominator the routing decides on) and the collector-cost digest read (the daily report's movers). Both were already instrumented to the census's own discipline - a distinct read name, a clock started on the line immediately above their try, the elapsed rendered in the log line and handed to the counter under the null key - so every scan found them and every inventory total came up two short. They are COUNTED rather than exempt, because each is the evidence an alerting decision is judged on: a blind census read means a tick routes nothing rather than guessing a channel, and a blind digest read costs a day's report. No exemption category fits - neither is a write, a delivery, a non-store read, or a block handed its evidence as parameters. The registrations, not the numerals, are the change: the evaluator's whole-file scope goes 5 counted to 7, CountedSites 28 to 30 with the two sites named in its cross-check note, Lite's fleet-scoped derivation goes 4 null-key records to 6 with each read asserted apart (a shared 'collector-cost' match would leave last_failure_read ambiguous exactly when one of the three stages went quiet), and FleetScopedReads names all three collector-cost reads - which carries them into both SKUs' get_collection_health descriptions and the WindowNote by the existing concatenation. Two prose claims that counted the set ('the other two are Darling store self-alerts') are corrected where they stand. One routing defect rode along, and it is the whole-tree guards' fifth failure: RouteCostRegressions folded a census miss into a denominator of ZERO, which satisfies regressed * 2 > collected for any numerator at all - paging exactly the collector with the least evidence, against the routing's own remarks and ACollectorMissingFromTheCensus_IsReportOnly. The TryGetValue is now part of the paging condition, so an unknown denominator fails toward the digest, which carries the finding in full. The suites cannot run on this host; both census test classes and CollectorCostDigestTests were compiled against their real sources and executed here through a shim runner - 61 of 61 green, including every routing pin in both directions - and Darling.Tests and Lite.Tests both build clean.
| "Blocking Detected" or "Deadlocks Detected" or "Failed Agent Job" | ||
| or "Custom Alert Rules Unhealthy" or "Stale Mute Rules" => $"{value:F0}", | ||
| or "Custom Alert Rules Unhealthy" or "Stale Mute Rules" | ||
| or "Collector Cost Digest" => $"{value:F0}", |
There was a problem hiding this comment.
IsWarning will still render this row as a warning in both Alert History grids.
AlertSeverity.ForMetric("Collector Cost Digest") is deliberately pinned to INFO/blue in this PR (PerformanceMonitor.Notifications/AlertSeverity.cs), with a test (CollectorCostDigest_IsInfoBlueOnPurpose) guarding against a future sweep "promoting" it to WARNING — that's the surface email and webhooks read.
But AlertMetricClassifier.IsWarning (used here, a few lines up) is the complement of IsResolution/IsCritical — anything that isn't a resolution or a deadlock/poison-wait name defaults to true (see Darling/Darling.Tests/AlertMetricClassifierTests.cs's own IsWarning_True_ForOrdinaryActionableAlerts, which documents even null/"" as warning by default). "Collector Cost Digest" doesn't contain "Cleared/Resolved/Restored/.../Reconnected" or "Deadlock/Poison", so IsWarning returns true for it.
That classifier feeds the Alert History grid's row styling in both apps identically:
Lite/Controls/AlertsHistoryTab.xaml/Darling/PerformanceMonitor.Darling.Viewer/AlertsHistoryTab.xaml:DataTrigger Binding="{Binding IsWarning}" Value="True"→Background="#33D97706"(amber)- via
Lite/Services/LocalDataService.AlertHistory.csandDarling/PerformanceMonitor.Darling.Viewer/ViewerDataService.AlertHistory.cs, both of which deriveIsWarningfromAlertMetricClassifier.IsWarning(MetricName)
So every digest row a user actually looks at in the desktop history grid will get the same amber "this needs attention" highlight as a real actionable alert — precisely the outcome the extensive commentary and the dedicated AlertSeverityTests pin were written to prevent, just reached through a different, untouched classifier. Since this is the first metric in the codebase meant to be genuinely non-actionable INFO (everything else hitting the INFO fallback is treated as an accident to be fixed), IsWarning/IsCritical may need an explicit carve-out for it (or some other way to keep the grid consistent with the email/webhook treatment).
There was a problem hiding this comment.
Confirmed unaddressed on dev — IsWarning is still !IsResolution && !IsCritical with no informational carve-out, so both Alert History grids render the digest amber against its own deliberately-pinned INFO/blue delivery severity. Queued as a small follow-up PR together with the window-text reword below (one commit each, after the #3466 train lands, since the evaluator file is in flight there).
|
Reviewed the diff (DarlingSelfAlertEvaluator's cost-regression routing/digest, the new Overall: the routing logic, SQL, and rendering all check out against the test suite — I traced One correctness gap (left as an inline comment on No Lite/Darling parity issues otherwise — this feature is correctly Darling-only (per |
…#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).
… delivery path, and census the paths from source (#3464) (#3465) An operator flipped the master switch off on both SQL Server stores at 13:23:40Z and was paged twice in the next hour anyway: an Analysis INFO card at 38 minutes, a Collector Cost Regression at 60 - while the engine sweep's own server_alert_passes counters sat byte-identical across consecutive hourly reads, proving the switch was honored by the sweep and bypassed by its siblings. Silencing the fleet ultimately took three independent actions discovered serially, each found only because the previous one demonstrably failed to silence a live channel. The switch's own documentation (Darling/README.md, the alerts section) has promised all along that `enabled: false` turns off all alert evaluation and scheduled-analysis finding notifications; this change makes the code keep that promise, in the AND'd idiom the connection-change gate established (`!_settings.AlertsEnabled || !_notifyConnectionChanges()`). * Bypass 1, measured: analysis notifications. Both entry points (the scheduled tick, analyze_now) handed the notification service config.Analysis.NotificationsEnabled ALONE - the D0 split's "delivery gated on analysis_notifications_enabled" had quietly REPLACED the master consult rather than adding to it. Both now route through DarlingWorker.ShouldNotifyAnalysisFindings(config): master AND family, one predicate, so a third entry point has one right thing to call. The D0 split itself stands, because the contract says it should: the pass still runs and persists findings; only the channels go quiet. Lite had the identical bypass in RunAnalysisIfDueAsync and gets the identical twin (CollectionBackgroundService.ShouldNotifyAnalysisFindings over the App statics). * Bypass 2, measured, and the call site the issue deliberately left unpinned: the collector-cost paging path. The fire path is the worker's hourly store-metrics tick (DarlingWorker.cs:2098) -> DarlingSelfAlertEvaluator.EvaluateCollectorCostAsync -> ApplyCostRegressionsAsync(routing.Paging) -> FireAsync -> IAlertDeliverer.DeliverAsync -> the shared send core. The ONLY AlertsEnabled consult on that method guarded the DIGEST branch, which runs AFTER the paging apply - and a comment beside it claimed "AlertsEnabled is checked inside the apply, like every sibling" while ApplyCostRegressionsAsync alone among the fifteen firing members had no such check. The gate now sits in both places the siblings put it: the top of the evaluate (before the first store read, the engine's early-return shape) and the top of the apply (the direct-test seam). Master-off means what the frozen counters prove it means for the engine: no evaluation, no history rows, fire/resolve state frozen where it stands - chosen over the connection edge's track-always split because there is no cross-sweep state machine here to corrupt; the edge rebuilds from stored rows the moment the switch returns. * Two more bypasses the census surfaced, same shape, not yet measured only because nothing on the muted stores happened to trip them. The PostgreSQL Tier 0 predictors: the #3013 comment had named this gap outright ("this path has no master-switch gate at all") and left it as an open question about deliberate exemption; the answer arrived as an incident, so the group entry now gates on config.Alerts.Enabled before RecordPass - the denominator stays truthful by not counting passes that never ran, which is the parity with both sibling sites the old comment had to disclaim. And the user-authored custom alert rules: CustomAlertEvaluator.EvaluateServerAsync fired into the same channels with no consult anywhere; it now takes a REQUIRED alertsEnabled seam (a construction that cannot say where its master switch lives must not compile) and stops evaluating under master-off exactly as the engine its rules ride beside does. ReconcileStateAsync stays ungated on purpose: state hygiene writes history rows, never a channel, and a mute that could orphan rule state would make flipping the switch back on unsafe. * The census (AlertMasterSwitchSurfaceTests), which is the actual fix: the read-failure surface discipline applied to delivery. The file set is DERIVED - every production .cs whose stripped source calls a delivery seam (DeliverAsync, NotifyAsync, TrySendAlertEmailAsync, TrySendAsync, SendFindingAlertAsync, SendTest*) must appear in the census - and every call site within those files is attributed to its enclosing member and classified: consults the master switch inline (asserted BEFORE the first send), or is reached only through a verified gated caller (the gate asserted, and sole-caller status swept across the whole production tree so a second path in fails naming its file), or carries a stated exemption (the shared funnels below the gates; the settings windows' Send Test buttons, which must work mid-mute or they cannot verify the channel an un-mute will need). A separate sweep holds every FireAsync-calling member of the self-alert evaluator to the same rule, so the sixteenth member cannot ship what the fifteenth did. Run against the pre-fix tree, the census goes red naming ApplyCostRegressionsAsync, EvaluatePostgresAlertsAsync, the custom-rule sweep and both analysis sites; against this tree it is green, and the digest path (#3448) proves inside the gate. * Fixtures both ways: master off suppresses the cost-regression fire, the resolution row, and the state advance (re-enabling fires as a clean first entry with no phantom Cleared row owed); the analysis predicates get their truth tables in both SKUs, pinning that the family toggles remain narrower knobs under master-on rather than synonyms. The AND'd idiom precedent and the README contract sentence are pinned verbatim, so a change to what the gates mean has to land where the meaning is stated. Operationally, master-off is again sufficient: the analysis toggle and the stopgap fleet-wide mute rules that today's incident required as compensating controls are now redundant with the switch, and their removal is the operator's unwind, not this change's. Verification: full solution build clean (0 errors); tests cannot run on this workstation, so the census suite plus the gap-2 fixture were executed out-of-band against the compiled assemblies and the live tree (all green), and re-executed against the pre-fix sources (census red on all four bypasses, fixture unable to compile without the predicate). Closes #3464
…ed degrade, and the deterministic name (#3476) * Carve the deliberate INFO reports out of IsWarning, so the history grids agree with the severity map (#3448 review) * Name the census window the digest actually reads: the trailing 24 hours, not the movers' baseline (#3448 review) * Hand get_sweep_reports' child reads the host's logger, so a degraded section leaves a trace (#3473 review) * Order the rollup's names read so a renamed server's newest name wins deterministically (#3473 review)
1 participant
Fixes #3443 — the delivery half of what #3440 raised; #3441 fixed its threshold half.
Collector Cost Regressionhas needed two gates (#3316's materiality floor, #3441's dispersion bound) to stop it paging on unactionable things, and the issue's diagnosis is that the remaining problem is delivery, not threshold: a seconds-per-day trend in the monitoring tool's own overhead is a report to read, not a condition to interrupt somebody for — except when it is fleet-wide, the one shape that means something changed rather than some server did.RouteCostRegressions— pure, testable without a host — demotes every selected regression to report-only unless the collector regressed on at least two servers and on more than half the servers it actually ran on in the 24h census (get_collector_cost's own ranked read supplies the denominator; distinct servers, not row count, on both sides of the fraction). The paging half still fires per (server, collector). A census read failure fails safe: nothing routed, nothing paged on a fraction whose denominator is missing.The digest itself (
Collector Cost Digest, daily, fleet-keyed) is inclusive where the paging predicate is exclusive: no materiality floor, no dispersion bound, movers in both directions ranked by |added s/day| — a collector that got cheaper is the half of the series the paging condition structurally cannot report — each line carrying the baseline's own p95 and worst day so the ratio is judgeable, plus the heaviest-first census that would have surfaced #2862. Its severity arm is the map's one deliberate INFO: declared rather than left to the fall-through it renders identically to, so the next #1136/#2090-style sweep does not promote it to WARNING, and pinned in Lite.Tests so that promotion fails red.Digest tests assert the routing partition (single-server demotion however large, the fleet-wide twin that still pages, the majority boundary at floor two, dedup on both sides of the fraction) and the rendered document's own identities: each line's ratio and s/day reconcile against the figures printed beside it, in the order MoverSql delivers. Three expected values are corrected from the interrupted draft this lane continues — the reconcile fixture arrives pre-ranked because the renderer trusts its reader's ORDER BY and does not sort,
-30555.0 s/dayis what (456.2 − 1,150.6) × 44,002 actually renders, and5,489 ms/runis the summary row's integer division (#2862 quoted the rounded 5,490). The #2150 and #2862 figures are reconstructed as fixtures so the worst regression and the census catch this metric exists for stay named.No migration rung and no schema change.
collect.collector_costand this condition are Darling-only.