-
Notifications
You must be signed in to change notification settings - Fork 84
CPU and I/O latency reach robust-baseline parity in Darling (#1743 follow-up) #1995
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: dev
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -112,13 +112,19 @@ all the way out or the same overflow returns by a different route. */ | |
| Assert.Contains("delta_stall_read_ms::DOUBLE PRECISION", TimescaleSupport.CreateFileIoBaselineSql, StringComparison.Ordinal); | ||
| Assert.DoesNotContain("delta_stall_read_ms * 1.0", TimescaleSupport.CreateFileIoBaselineSql, StringComparison.Ordinal); | ||
|
|
||
| /* #1743 follow-up moved the arm off the rollup and onto the raw hypertable (the rollup | ||
| cannot produce a median) — the cast pin moves WITH it: the ratio must still be computed | ||
| in float arithmetic at the source. The old SUM(row_count) pin's SEMANTIC survives as the | ||
| nullable-v design: the arm's WHERE keeps write-only rows (delta_reads > 0 OR | ||
| delta_writes > 0) and must NOT filter the NULL ratios out — the scaffold's COUNT(*) | ||
| counts them (the row_count behavior) while AVG/STDDEV/median/mad ignore them (the | ||
| ratio_count behavior), exactly the retired rollup's two-count distinction. */ | ||
| var sql = PgBaselineProvider.GetBaselineQuery(MetricNames.IoLatency)!; | ||
| Assert.Contains("file_io_baseline", sql, StringComparison.Ordinal); | ||
| Assert.Contains("SQRT(", sql, StringComparison.Ordinal); | ||
| /* sample_count must come from row_count, NOT ratio_count: the raw path counted rows whose ratio was | ||
| NULL (writes but no reads pass the filter and average to nothing), so counting only the non-null | ||
| ratios would silently under-report the sample size the baseline gate reads. */ | ||
| Assert.Contains("SUM(row_count) AS sample_count", sql, StringComparison.Ordinal); | ||
| Assert.Contains("FROM file_io_stats", sql, StringComparison.Ordinal); | ||
| Assert.Contains("delta_stall_read_ms::DOUBLE PRECISION / NULLIF(delta_reads, 0)", sql, StringComparison.Ordinal); | ||
| Assert.DoesNotContain("delta_stall_read_ms * 1.0", sql, StringComparison.Ordinal); | ||
| Assert.Contains("(delta_reads > 0 OR delta_writes > 0)", sql, StringComparison.Ordinal); | ||
| Assert.DoesNotContain("v IS NOT NULL", sql, StringComparison.Ordinal); | ||
|
Comment on lines
+115
to
+127
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This class is That leaves the one genuinely new piece of SQL behavior in this PR — whether Postgres's |
||
| } | ||
|
|
||
| /* ---------------- ungated: method-surface pins vs Lite ---------------- */ | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -145,9 +145,9 @@ public void InvalidateCache(int serverId) | |
|
|
||
| using var reader = await cmd.ExecuteReaderAsync(); | ||
| /* #1743: the robust-scaffold metrics return eight columns (…, median_val, mad_val) | ||
| and carry sentinel tier rows; the two rollup-bound metrics (CPU, I/O) still return | ||
| the six-column classical shape — detected by column count, so their buckets read | ||
| Median=0/Mad=0 and the robust path degrades to the classical one for them. */ | ||
| and carry sentinel tier rows; the two event-family metrics (blocking, deadlock) | ||
| keep the six-column classical shape — detected by column count, so their buckets | ||
| read Median=0/Mad=0 and the robust path degrades for them. */ | ||
| var hasRobustColumns = reader.FieldCount >= 8; | ||
| while (await reader.ReadAsync()) | ||
| { | ||
|
|
@@ -241,12 +241,13 @@ JOIN tier_mads AS m | |
| /// The eleven per-metric baseline queries — Lite's, verbatim, except the four QUALIFY | ||
| /// sites rewritten for Postgres (no QUALIFY support). Internal (not private like Lite's) | ||
| /// so Darling.Tests can pin every query's dialect and the rewrites' structure ungated. | ||
| /// <para>#1743: the seven raw-grain metrics route their cleaned rowsets through | ||
| /// <see cref="RobustTierScaffold"/> and return EIGHT columns (…, median_val, mad_val). | ||
| /// CPU and I/O latency read pre-aggregated sum/sumsq rollups that cannot produce a median — | ||
| /// they keep the six-column classical shape until their raw-window variants land, and the | ||
| /// reader detects the shape by column count. Blocking/deadlock are event-family (events/day, | ||
| /// stddev 0) evaluated on the event-ratio path, deliberately untouched.</para> | ||
| /// <para>#1743: the nine non-event metrics route their cleaned rowsets through | ||
| /// <see cref="RobustTierScaffold"/> and return EIGHT columns (…, median_val, mad_val) — CPU | ||
| /// and I/O latency included, reading their RAW hypertables at Lite's grain (their retired | ||
| /// sum/sumsq rollups could not produce a median; both tables carry their own 30-day | ||
| /// service-side retention, so this does not reopen #1757 — see the arms' notes). | ||
| /// Blocking/deadlock are event-family (events/day, stddev 0) evaluated on the event-ratio | ||
| /// path, deliberately untouched; the reader detects their six-column shape by count.</para> | ||
| /// </summary> | ||
| internal static string? GetBaselineQuery(string metricName) | ||
| { | ||
|
|
@@ -258,19 +259,22 @@ JOIN tier_mads AS m | |
| // collection_time first, then bucket by hour+dow. | ||
| return metricName switch | ||
| { | ||
| // Point-in-time metric — no restart exclusion needed | ||
| /* #1743 follow-up: CPU reads the RAW hypertable, at Lite's exact per-sample grain, so | ||
| the robust scaffold applies — the old sum/sumsq rollup could reconstruct mean/stddev | ||
| but structurally cannot produce a median. Reading raw here does NOT reopen #1757: | ||
| that finding was 4 days of supply under a 30-day window, and cpu_utilization carries | ||
| its own 30-DAY service-side retention (CollectorScheduleDefaults: 1-minute cadence, | ||
| 30-day retention; verified on a production store — no TimescaleDB retention policy | ||
| on the table, service-side purge at 30d, compressed after 1 day). The mean/stddev | ||
| this computes are the SAME per-sample statistics the rollup reconstruction produced. | ||
| The now-unused cpu_utilization_baseline aggregate remains registered for upgrade | ||
| compatibility; retiring it is separate cleanup. */ | ||
| MetricNames.Cpu => @" | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Retention guarantee weakened for CPU/IO, with no runtime guard — only a build-time default pin. Before this PR, CPU and I/O baselines were served by After this PR, CPU/IO read the raw So a Darling operator who shortens
Worth either wiring a runtime warning analogous to Lite's for these two collectors, or enforcing a >=30-day floor specifically for |
||
| SELECT EXTRACT(HOUR FROM collection_time)::INT AS hour_of_day, | ||
| EXTRACT(DOW FROM collection_time)::INT AS day_of_week, | ||
| SUM(cpu_sum) / NULLIF(SUM(cpu_count), 0) AS mean_val, | ||
| SQRT(GREATEST( | ||
| (SUM(cpu_sumsq) - POWER(SUM(cpu_sum), 2) / NULLIF(SUM(cpu_count), 0)) | ||
| / NULLIF(SUM(cpu_count) - 1, 0), 0)) AS stddev_val, | ||
| SUM(cpu_count) AS sample_count, | ||
| COUNT(DISTINCT collection_time::DATE) AS distinct_days | ||
| FROM cpu_utilization_baseline | ||
| WHERE server_id = $1 AND collection_time >= $2 AND collection_time < $3 | ||
| GROUP BY hour_of_day, day_of_week", | ||
| WITH clean AS ( | ||
| SELECT collection_time, sqlserver_cpu_utilization::DOUBLE PRECISION AS v | ||
| FROM cpu_utilization_stats | ||
| WHERE server_id = $1 AND collection_time >= $2 AND collection_time < $3 | ||
| )," + RobustTierScaffold, | ||
|
|
||
| /* QUALIFY rewrite 1 of 4 — cumulative counter, restart exclusion. | ||
| Excludes samples where the delta drops to 0 when the prior sample was > 1000 | ||
|
|
@@ -399,22 +403,21 @@ FROM with_lag | |
| WHERE NOT (total_elapsed = 0 AND prior_total_elapsed > 100000) | ||
| )," + RobustTierScaffold, | ||
|
|
||
| // Point-in-time metric — no restart exclusion needed. The stall/reads ratio is cast to | ||
| // DOUBLE PRECISION (as the memory / wait-rate metrics are) so a spurious large delta can't | ||
| // make STDDEV_SAMP produce a numeric that overflows System.Decimal when Npgsql materializes | ||
| // the aggregate (it does with `* 1.0`, which yields numeric, not float8). | ||
| /* #1743 follow-up: same move as CPU — raw hypertable at Lite's per-file-row grain so | ||
| the robust scaffold applies (file_io_stats also carries its own 30-day service-side | ||
| retention; see the CPU arm's note). The stall/reads ratio keeps its DOUBLE PRECISION | ||
| cast so a spurious large delta can't make STDDEV_SAMP produce a numeric that | ||
| overflows System.Decimal when Npgsql materializes the aggregate. v stays NULLABLE | ||
| (a write-only file row has no read latency): AVG/STDDEV/median/mad all ignore those | ||
| rows while COUNT(*) keeps counting them — exactly the row_count-vs-ratio_count | ||
| distinction the retired rollup documented, preserved at the raw grain. */ | ||
| MetricNames.IoLatency => @" | ||
| SELECT EXTRACT(HOUR FROM collection_time)::INT AS hour_of_day, | ||
| EXTRACT(DOW FROM collection_time)::INT AS day_of_week, | ||
| SUM(ratio_sum) / NULLIF(SUM(ratio_count), 0) AS mean_val, | ||
| SQRT(GREATEST( | ||
| (SUM(ratio_sumsq) - POWER(SUM(ratio_sum), 2) / NULLIF(SUM(ratio_count), 0)) | ||
| / NULLIF(SUM(ratio_count) - 1, 0), 0)) AS stddev_val, | ||
| SUM(row_count) AS sample_count, | ||
| COUNT(DISTINCT collection_time::DATE) AS distinct_days | ||
| FROM file_io_baseline | ||
| WHERE server_id = $1 AND collection_time >= $2 AND collection_time < $3 | ||
| GROUP BY hour_of_day, day_of_week", | ||
| WITH clean AS ( | ||
| SELECT collection_time, delta_stall_read_ms::DOUBLE PRECISION / NULLIF(delta_reads, 0) AS v | ||
| FROM file_io_stats | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Performance measurement gap: the PR description says the composed CPU arm was prod-validated at 227 ms, but I don't see an equivalent measurement mentioned for this I/O arm. That matters because |
||
| WHERE server_id = $1 AND collection_time >= $2 AND collection_time < $3 | ||
| AND (delta_reads > 0 OR delta_writes > 0) | ||
| )," + RobustTierScaffold, | ||
|
|
||
| // Event-based — mean = events per day for this bucket, sample_count = distinct days observed. | ||
| // No restart exclusion needed (event counts, not cumulative). | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This pin only protects the shipped default in
CollectorScheduleDefaults, not a deployed store's actual runtime retention — and retention forcpu_utilization/file_io_statsis user-editable (CollectorScheduleEditorWindowin the Darling Viewer).Before this PR, Darling's CPU/IO baselines read the
cpu_utilization_baseline/file_io_baselinecontinuous aggregates, which carry their own fixed 35-dayBaselineRetentionInterval(TimescaleSupport.cs) independent of the raw hypertable's retention — so lowering raw retention couldn't silently degrade these two families. This PR moves them onto the raw tables directly, which removes that insulation: a Darling user who lowerscpu_utilization/file_io_statsretention below 30 days now gets the exact #1757 silent-degradation failure mode, with nothing surfacing it at runtime.Lite already has a targeted fix for this class of problem —
BaselineProvider.WarnIfRetentionUndercutsBaselineWindow, which logs once per family per process-run when a source table's actual configured retention undercuts the baseline window. Darling has no equivalent anywhere (PgBaselineProvider's constructor doesn't even take a retention-lookup callback). Should this PR port that runtime warning for the two families it just made retention-sensitive, rather than relying solely on a build-time pin against the packaged default?