Repository navigation
Serve a sweep_pressure verdict from get_collection_health so half-rate collection is visible (#2296) - #2308
Conversation
…e collection is visible (#2296)
|
Reviewed the diff ( One parity gap, not touched by this PR's diff so I can't leave it inline:
Darling's equivalent ( Lite's tool itself ( No correctness, security, or performance issues found otherwise. |
…e-visible # Conflicts: # CHANGELOG.md
ReviewWent through the Logic checks out. Walked every branch by hand against the pinned test table (motivating-server 60,712 ms/min ≈ 101%, band edges at 44,999/45,000/59,999/60,000, on-load + zero-duration exclusion, cadence amortization, empty window) — all match Verified the 60,000 ms budget assumption, since it's hardcoded rather than derived: confirmed the min non-zero Lite/Darling parity: the classifier, both MCP tool bodies, One parity gap, in a file this PR doesn't touch: This is the markdown resource the MCP client surfaces as usage guidance (separate from the No security, correctness, or performance concerns found otherwise. Not flagging missing-index DMV recommendations per repo convention. |
|
Both passes' one finding fixed in 59eebce — Lite's McpInstructions row now carries the same sweep_pressure hint as Darling's, so the session-init guidance and the per-tool Description agree in both SKUs. Good catch on a file the diff didn't touch. |
ReviewReviewed against Parity: Full file-coverage parity check passed — every surface touched on the Darling side ( Correctness:
Security/perf: No new SQL, no new I/O, no untrusted input handled differently. The added LINQ work is O(n) over a small per-server collector list — no regression risk. No missing-index DMV suggestions here, per the project's standing rule. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| by cadence) against the minute the fastest cadence holds; heaviest_collectors names where | ||
| the budget goes, which is the actionable half of the answer. */ | ||
| var pressure = SweepPressureClassifier.Compute( | ||
| rows.Select(r => (r.CollectorName, r.AvgDurationMs, r.FrequencyMinutes))); |
There was a problem hiding this comment.
Minor correctness edge case, Lite-only: r.FrequencyMinutes (from CollectorHealthRow.FrequencyMinutes, Lite/Services/LocalDataService.CollectionHealth.cs:364-365) resolves the collector's cadence from the shipped CollectorScheduleDefaults, not from ScheduleManager's per-install, user-editable schedule (Lite/Services/ScheduleManager.cs:146 shows the setter that lets a user actually change FrequencyMinutes per install).
That's a deliberate, documented choice inherited from the existing health-banding code ("shipped default...so all three surfaces stay in parity"), and it's fine for staleness banding, which is inherently approximate. But sweep_pressure is a precise quantitative signal with a hard 75%/100% threshold, and on Lite specifically the real body composition can diverge from the shipped defaults:
- A user who slows down a heavy collector (e.g. bumps
index_object_statsfrom 1440→4320 min because it was too heavy) would still get it amortized at the old, faster shipped cadence — possibly reporting AT_RISK/SATURATED on a body that now comfortably fits. - A user who speeds up a collector's cadence would get it amortized at the old, slower shipped cadence — understating real demand, and potentially reporting OK on a server that's actually saturated. This is the false-negative case that defeats the point of the feature (Darling has no schedule knobs, so this gap doesn't apply there — Lite-only).
Not necessarily blocking — just flagging since CollectorHealthRow currently has no resolved-per-install-cadence property to plug in instead, so fixing this would need a small plumbing change (passing the ScheduleManager-resolved frequency through). Worth a conscious call on whether the parity/simplicity tradeoff is acceptable here, or whether sweep pressure specifically should use the real cadence.
1 participant
Implements option 3 of #2296 — the "worth doing regardless" one. Options 1 (move the two servers in-region, gated on the use1
darling_monitorcredential) and 2 (longer cadence for saturated servers, a data-grain decision) stay open on the issue; this PR does not close it.The gap
Two servers collect at half their configured cadence and nothing but a service-log warning says so. The mechanism:
RunDueCollectorsAsyncruns collectors serially and re-stampsNextDuefrom onenow; when the body's own execution outlives the 60s cadence (multi-01's four heaviest collectors average ~60.7s combined), it finishes after the next due time, the watchdog skips the relaunch (~50 warnings/hour), and the delivered interval is a multiple of the configured one. Meanwhileget_collection_healthreports every collector HEALTHY — correctly, because from each collector's own seat nothing is wrong. No counter of skipped relaunches exists anywhere, in memory or the store.The signal, and why this one
get_collection_health(both SKUs) now serves a top-levelsweep_pressureblock:busy_ms_per_minute,busy_percent, a verdict (OK/AT_RISK≥ 75% /SATURATED≥ 100%), the three heaviest contributors (attribution — "which collectors spend the budget" is the actionable half), and anotenaming the consequence and the levers when the verdict isn't OK.Built from execution demand, not delivered-gap statistics, deliberately: at fleet scale the delivered cadence stretches benignly — bounded sweep concurrency queues bodies, and the fleet-wide median gap already runs a multiple of the configured minute — so gap-based detection would flag all 52 servers and drown the two that matter. Execution demand (each collector's
avg_duration_msamortized by its own cadence) is the arithmetic behind the watchdog's own "has not completed after Ns of EXECUTION" distinction, and queueing cannot inflate it. This is also why the watchdog itself classifies Hang (running) apart from Queued (capacity).No SQL change, no migration, no schema change — the roll-up is computed from the rows the tool already reads plus the shared
CollectorScheduleDefaults, resolved exactly the way the health banding already resolves cadence (shipped defaults, for three-surface parity).Shape
SweepPressureClassifier(PerformanceMonitor.Common), pure and static likeCollectorHealthClassifier— callers resolve cadence, only the verdict lives there. Thresholds carry their rationale: 75% because the average smooths variance (intermittent overrun is how saturation looks before it's constant), 100% because the body mathematically cannot fit. The motivating server computes 101.2%.CollectorHealth/CollectorHealthRowexpose their existing cadence resolution internally rather than duplicating it.Tests
SweepPressureClassifierTests— the same decision table pinned in Darling.Tests and Lite.Tests (theCollectorHealthClassifiertwin-suite pattern): the motivating profile verbatim (22,141 + 16,590 + 13,544 + 8,437 ms → SATURATED at ~101%), a healthy profile, both inclusive band edges, on-load/zero-duration exclusion, own-cadence amortization (the pin that forbids charging a heavy daily collector at the fast cadence), and the empty window.Verification scope
Both test projects build clean on macOS (
EnableWindowsTargeting). The Windows suites can't run locally, so the full decision table was executed against the realPerformanceMonitor.Commonbuild via a throwaway net10.0 harness: 12/12 pass, including the motivating server's exact numbers. Not covered locally: the xUnit twins executing (CI's build job), and the tool end-to-end against a live store — the number it rolls up (avg_duration_ms) is already live-pinned by the existing collection-health tests.🤖 Generated with Claude Code