Repository navigation
The File Growth threshold means one thing — megabytes per hour — at every surface that shows it, so a 10 GB bar stops meaning 10 GB per five minutes on one store and per day on another (#3539 A8c) - #3631
Conversation
…very surface that shows it (#3539 A8c) FileGrowthRiseMb was compared against the raw growth inside the lookback window while the lookback was a second knob clamped 5..1440 minutes, so the same 10240 meant 10 GB per five minutes on one store and 10 GB per day on another. The knob is now a rate: GetBreachedFiles holds the in-window growth to FileGrowthRiseBarMb (rate × lookback/60), which is byte-identical on the shipped 60-minute lookback and the same sustained rate on every other. Scaled to the CONFIGURED window rather than read off the measured span, because the measured span on a freshly-collecting server is one collection interval and one autogrowth in it would extrapolate to twelve an hour. Both Settings rows, both preview lines, the alert's threshold line (rate + window + the bar in MB), the card's Growth field, IAlertEngineSettings, both MCP descriptions and the settings carriers now say MB/hr averaged over the lookback, in one shared phrase (AlertContextBuilders.FileGrowthRiseUnit) that FileGrowthRiseUnitCensusTests holds across nine surfaces. Key, column and integer are unchanged.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| @@ -613,6 +613,8 @@ public sealed class AlertsConfig | |||
| /* #2349: the database file-growth alert. Ships OFF -- a new alert that starts firing on upgrade is a bad | |||
| citizen, and the right thresholds are a property of the fleet rather than of the product. */ | |||
| public bool FileGrowthEnabled { get; set; } | |||
There was a problem hiding this comment.
Nit: FileGrowthRiseMb's doc comment here was updated to note it's now MB/hr, but the adjacent FileGrowthLookbackMinutes property (line 615) didn't get the same "this is the window the rate is averaged over" note that App.xaml.cs's AlertFileGrowthLookbackMinutes and IAlertEngineSettings.FileGrowthLookbackMinutes received. Not a functional issue — DarlingAlertSettings.FileGrowthLookbackMinutes (the property actually consumed) does carry the updated framing — just a small doc-comment gap on this config POCO for anyone reading it in isolation.
There was a problem hiding this comment.
Fixed in 1ef34a1 — the config POCO's FileGrowthLookbackMinutes now carries the same one-line note ("the window the rise RATE is averaged over — it does not rescale the threshold") that App.AlertFileGrowthLookbackMinutes and IAlertEngineSettings.FileGrowthLookbackMinutes got. Comment-only.
|
Reviewed the diff (rate-vs-window logic in Core arithmetic checks out. Lite/Darling parity is intact. Both No correctness bugs, security issues, or missed Lite/Darling counterparts found. Left one non-blocking inline nit about a doc-comment gap on |
There was a problem hiding this comment.
LGTM — verified the rate-scaling arithmetic (FileGrowthRiseBarMb), the read-adapter invariant that measured growth window never exceeds the configured lookback, all GetBreachedFiles call sites, and Lite/Darling parity across Settings XAML, code-behinds, MCP tool descriptions, and settings contracts. No correctness, security, or parity issues found; one non-blocking doc-comment nit left inline.
…ndow note as its twins (#3539 A8c, review nit)
There was a problem hiding this comment.
LGTM — reviewed the file-growth rate change (AlertContextBuilders.FileGrowthRiseBarMb, GetBreachedFiles, AlertEngine.CheckFileGrowthAsync) and the parity across Lite/Darling settings, XAML, code-behind, and MCP tool descriptions. The math checks out (rate × configured-window/60, byte-identical on the shipped 60-min default), the single production call site was updated correctly, and the new FileGrowthRiseUnitCensusTests roster enforces both-SKU parity going forward. No SQL changed in this PR, so the T-SQL conventions don't apply here. No correctness, security, or performance issues found.
…ob says so on the wire, get_pg_server_config says when it was captured, Lite's latch/spinlock pages bind to the caller's limit, the ORDER-BY-integer fold gets a census, and two rung docs stop asserting superseded facts (#3653) (#3659) * Six small honesty riders from the brains-review residue: a retired knob says so on the wire, get_pg_server_config says when it was captured, Lite's latch/spinlock pages bind to the caller's limit, the ORDER-BY-integer fold gets a census, and two rung docs stop asserting superseded facts (#3653) Partial for #3653; six checklist items, none of them closing the issue. - get_alert_settings.poison_wait.threshold_ms (both SKUs) keeps its key and gains threshold_ms_note beside it; Darling's update_alert_settings accepts the value with a warnings[] entry and accepts the note back silently so a whole-payload round-trip still writes. The premise (nothing reads the member) is pinned against stripped source of the two engines and the PostgreSQL host. - McpLatestSnapshotStampTests' reader-call regex now names GetCurrent*Async, which puts get_pg_server_config inside the census; the tool carries the snapshot's own collection_time as captured_at (on the row statement) and joins DarlingOnlyStamped. - Lite's GetLatchStatsSnapshotAsync / GetSpinlockStatsSnapshotAsync take a limit defaulting to LatchSpinlockGridRowCap = 20; get_latch_stats / get_spinlock_stats fetch limit + 1, publish *_returned / truncated / order, and join Lite's McpPageContractTests roster with an observed-truncation pair each. - DarlingPgReadSqlParsesLiveTests gains a no-server fact sweeping every Storage *Reader string field and every Lite/Services/*.cs for ORDER BY <int> <op> <int>, with floors and a witnessed defect/fixed pair. Zero hits today. - PgMigrations V79 / V122 doc clauses carry "(superseded by #3631 / #3638 — …)". - Lite ToolRecommendations gains HADR_SYNC_COMMIT; a new ToolRecommendationsTests holds every recommended tool and parameter to the Lite surface by reflection. * Move PoisonWaitThresholdMsNote above BuildAlertSettingsPayload's doc block, which its insertion had stacked a second summary onto (DocCommentHygieneTests.NoMemberCarriesTwoStackedSummaryBlocks)
1 participant
What does this PR do?
The File Growth rise threshold is now a rate — megabytes per hour, averaged over the lookback — and every surface that shows the number says so in the same words. Partial delivery of #3539 (checklist item A8c); the issue stays open by design.
FileGrowthRiseMb(default 10240) was compared against the raw growth inside the lookback window (AlertContextBuilders.GetBreachedFiles:f.GrowthMb >= riseMb), while the lookback was a second, independent knob clamped 5..1440 minutes. Nothing related the two. The same 10 GB bar therefore meant "10 GB in five minutes" on a store whose operator had shortened the lookback and "10 GB in a day" on one who had lengthened it — a 288× swing in what the threshold asked for, driven by a knob whose label ("min") never mentioned that it rescaled its neighbour. Each surface described the number in its own vocabulary: "MB within N min" on the Settings rows,10240MB/60mon the preview line,rise ≥ 10240 MBon the alert, a barerise_mbon the wire. None was wrong alone; none could be compared with another.Why this shape, and what was rejected
The comparison now holds the in-window growth to
AlertContextBuilders.FileGrowthRiseBarMb(riseMbPerHour, lookbackMinutes)=rate × lookback / 60. On the shipped 60-minute lookback that israte × 60 / 60— the exact number that was always compared, so every store on the defaults sees no behavioural change at all. On a 5-minute lookback the bar is 853 MB inside the window; on a 24-hour one, 240 GB; both are 10 GB/hr sustained.The brief's recommended shape (b′) — compare
DatabaseFileGrowthInfo.GrowthMbPerHour >= riseMbdirectly — lies in one case, so it was not taken.GrowthMbPerHourdivides by the span the samples actually cover (the read measures the window rather than assuming it, on purpose, so a collection gap cannot make a slow rise look fast). On a server that started collecting five minutes ago, or one just out of a collection gap, that span is one collection interval: a single 1 GB autogrowth inside it reads as 12,288 MB/hr, pages the default bar, and resolves at the next sample when the span widens. That is a flap, and it is a behavioural change for operators on the defaults — the population the fleet is on. Scaling the bar to the configured window instead reads unobserved time as no growth, the same conservative stance the single-sample window already takes ("no rise observed", not "the whole file appeared"), and is silent for the defaults.Option (a) — reinterpret stored values at read with a semantics marker — needs a column and therefore a rung, which this wave forbids; and the shape above makes it unnecessary, because the stored integer keeps its meaning on the shipped lookback and gains a sane one everywhere else.
Key, column and integer are unchanged.
config_alert_settings.file_growth_rise_mb, the MCP keyfile_growth.rise_mb, the Lite preferences key,IAlertEngineSettings.FileGrowthRiseMb— all keep their spelling. A rename would break every client that reads or writes the setting to say something the descriptions say just as well; the unit is stated where the number is shown.What now says "MB/hr averaged over the lookback"
One shared phrase,
AlertContextBuilders.FileGrowthRiseUnit = "MB/hr", referenced from code and spelled literally in XAML:Database file growing over [10240] MB/hr averaged over [60] min, or is over [60]% of its volume. WasDatabase file grew over [10240] MB within [60] min.file growth > 10240 MB/hr over 60m or volume > 60%. Was10240MB/60m.rise ≥ 10240 MB/hr averaged over 60 min (≥ 10240 MB in the window) or file ≥ 60% of volume. States the rate, the window it was averaged over, and the megabytes that rate amounts to inside the window — because the card's own "Growth" figure is over the measured span, which can be narrower than the window, and naming the in-window bar is what lets an operator compare the two without knowing that.39.1 GB in 60 min (40000 MB/hr)).get_alert_settingsdescriptions and Darling'supdate_alert_settings: "rise_mb is megabytes per HOUR, averaged over file_growth.lookback_minutes — a rate, not a total for the window", with the 5-minute / 24-hour arithmetic spelled out. Payload shape untouched.IAlertEngineSettings(both knobs' docs),DatabaseFileGrowthInfo.GrowthMbPerHour(now says it is display-only and why), and the settings carriers on both SKUs (DarlingConfig,DarlingAlertSettings, the viewer's row model, Lite'sAppstatic andAppAlertEngineSettings) carry the column's meaning in comments.What it does NOT do
StorageVersionbump. No CHANGELOG edit (entry proposed to the coordinator).FileGrowthVolumePercent), the read SQL, the fingerprinting, the ordering, or any other alert condition.rise_mb_unit(or similar) wire field: Lite'sMcpAlertSettingsKeyTestsderives the payload shape from Darling's source, and the unit belongs in the description, which is where an agent reads it.PgMigrations.csV79 doc comment ("this file grew N MB inside the window") is left as the historical description of what the rung shipped; the semantics now live at every surface that reads the column. Flagged in the lane report.Test plan
New pins (all compile-verified here; the pure-logic and engine ones were also executed on this Mac via a throwaway net10.0 xunit harness referencing the built shared libraries — 26/26
FileGrowthAlertTests, 117/117AlertEngineTests+ census, and the MCP round-trip pin green; plusDocCommentHygieneTests,RepoFileAdoptionTests,CommentFilterAdoptionTests,AsOfWindowAnchorTests112/112 against this tree):Lite.Tests/FileGrowthAlertTests.cs:TheSameGrowthRate_GivesTheSameVerdict_OnEveryLookback(5/30/60/240/1440 — on-bar fires, a tenth under is silent);ThePerWindowReading_GaveTheOppositeVerdict_AtBothEndsOfTheClamp(2 GB in 5 min fires, 12 GB in a day does not; asserts the old reading would have said the reverse);TheBarIsTheRateTimesTheWindowInHours(10240@60 = 10240 exactly, @5 = 853.333, @1440 = 245760, 0 = 0);GrowthObservedOverLessThanTheWindow_IsHeldToTheWholeWindowsBar(the fresh-server case: 12,288 MB/hr on the card, silent on a 60-min lookback, fires on a 5-min one);TheCardsRate_UsesTheSharedUnitPhrase. Existing calls updated to the new named parameters.Darling/Darling.Tests/AlertEngineTests.cs: the fake read adapter gains plantableFilesand records the lookback asked;FileGrowth_TheRiseIsPerHour_SoTheSameRatePagesOnAnyLookback_AndTheThresholdSaysSopins the exact threshold string at 5 and 1440 minutes through the engine;FileGrowth_ARateUnderTheBar_IsSilentOnAnyLookback.Darling/Darling.Tests/FileGrowthRiseUnitCensusTests.cs(new): nine surfaces — two XAML, two code-behinds, engine, builder, settings contract, both MCP files — each asserted to carry the shared phrase beside the averaging language AND to have lost the per-window phrase, so a revert of one twin is a red build rather than a quiet regression. Floor on the roster count so it cannot be edited down to nothing.Darling/Darling.Tests/DarlingMcpAlertToolsTests.cs:FileGrowthRise_KeepsItsKeyAndColumn_AndBothDescriptionsSayItIsPerHour— payload emitsrise_mbfrom the row,{"file_growth":{"rise_mb":2048}}targetsfile_growth_rise_mb, both descriptions state the unit (read off the attributes by reflection, the house idiom).Builds (zero warnings, all with
-p:EnableWindowsTargeting=truewhere needed):PerformanceMonitor.Alerting,PerformanceMonitor.Darling.Service,PerformanceMonitor.Darling.Viewer,PerformanceMonitorLite,Darling.Tests,Lite.Tests.CI is the first execution of the WPF-hosted Lite.Tests classes that touch
App; nothing in this change reaches them beyond the label text, which is pinned from the Darling side.Which component(s) does this affect?
How was this tested?
See the test plan. Pure-logic and engine pins executed locally against the real built assemblies (values in the tests are the values the code produced, including the two exact threshold strings); the twinned label text is pinned by source census. No SQL Server involved — the change is threshold arithmetic and label text over an existing store read.
Checklist
dotnet build -c Debug)