Break chart lines at collection gaps, cadence-derived per series (#1944)

[erikdarlingdata]

From discussion #1936 (markallisongit): offline windows rendered as continuous lines. Mechanism: TimeSeriesGaps.BreakAtGaps - median positive inter-point spacing per series (robust: the outage gaps cannot inflate the yardstick), NaN break injected mid-gap past 3x, ScottPlot 5.1.59 renders NaN as the documented gap. Applied through the Add.TimeSeries extension at 104 time-series sites in both apps symmetrically; the ~20 remaining Add.Scatter sites are synthetic zero-lines/placeholders, audited individually. Ten unit tests pin both directions (outages break; jitter, duplicate timestamps, short series, and slow-cadence series stay connected). Both apps rebuild 0 warnings; Lite 1947/1947.

Note for Erik: this changes how every time chart renders - worth a visual pass on the dev build, and the client briefing tells the site to report any break that does not match a real outage.

🤖 Generated with Claude Code

[claude]

This series isn't a real per-collection-cadence time series — it's the synthetic "spike effect" baseline (rangeStart, time-0.0001, time, time+0.0001, ..., rangeEnd). Feeding it through Add.TimeSeries means TimeSeriesGaps.BreakAtGaps computes its "cadence" from the artificial 0.0001-day (~8.6s) offsets around each spike, which dominate the delta list whenever there are 2+ events. The median (and thus the 3x threshold) collapses toward ~0.0001 day, so any real inter-event gap — which is normally minutes to hours — exceeds the threshold and gets a NaN break inserted.

Concretely, with even two blocking events in the window, this shatters the flat "no blocking" zero-baseline into disconnected segments around each spike (I traced through the math: a 2-event example over a 6-hour window produces 3 spurious breaks — before the first event, between the two events, and after the last one). That's the exact visual regression this PR is trying to fix elsewhere: a quiet period (value 0, genuinely collected) now renders as a gap, which reads as "monitor was offline" when it wasn't.

This — and the identical pattern in RenderDeadlockTrendChart below, plus the Lite equivalents in ServerTab.Charts.cs — should probably stay on plain Add.Scatter, same as the other synthetic zero-line/placeholder sites that were deliberately left unconverted.

[claude]

Same issue as RenderBlockingTrendChart above: expandedTimes/expandedCounts is the synthetic zero-baseline "spike effect" array (artificial ±0.0001-day offsets around each deadlock event), not a real collection-cadence series. Running it through Add.TimeSeries lets those tiny synthetic deltas dominate the median, so genuine multi-minute/hour gaps between deadlock events get misclassified as "collection gaps" and the flat zero baseline between events gets broken up. Recommend reverting this one (and the Blocking one) to Add.Scatter.

[claude]

Same synthetic-spike-array issue flagged in the Darling counterpart (ViewerServerTab.Blocking.cs, RenderBlockingTrendChart): expandedTimes/expandedCounts interleaves artificial ±0.0001-day offsets around each event purely for the "spike" rendering effect — it's not a real collection cadence. Routing it through Add.TimeSeries lets those tiny synthetic deltas dominate the median spacing, so real gaps between blocking events (typically minutes+) get flagged as "collection gaps" and the flat zero baseline shatters between spikes. This should stay on Add.Scatter like the other synthetic zero-line sites.

[claude]

Same issue as the Blocking chart above — synthetic spike-baseline array, not a real cadence series. Recommend reverting to Add.Scatter here too.

[claude]

Review: #1947 (chart gap-breaking)

What it does: Adds TimeSeriesGaps.BreakAtGaps (median positive inter-point spacing x3, NaN-break injection) plus a drop-in Add.TimeSeries extension, and swaps ~104 real time-series Add.Scatter call sites to Add.TimeSeries across both apps. New Lite.Tests/TimeSeriesGapsTests.cs pins both directions well (outage breaks; jitter/duplicates/short/slow series stay connected).

Correctness bug (flagged inline, 4 locations)

RenderBlockingTrendChart / RenderDeadlockTrendChart in Darling/PerformanceMonitor.Darling.Viewer/ViewerServerTab.Blocking.cs, and their Lite counterparts in Lite/Controls/ServerTab.Charts.cs, build a synthetic "spike effect" array (rangeStart, time-0.0001, time, time+0.0001, ..., rangeEnd) to draw a flat zero baseline with spikes at event timestamps. These four call sites were switched to Add.TimeSeries, but the array's spacing has nothing to do with real collection cadence — it's dominated by the artificial ±0.0001-day offsets around each event. With 2+ events in the window, the median (and thus the 3x threshold) collapses toward that tiny synthetic value, so genuine multi-minute/hour gaps between events get misclassified as "collection gaps" and the flat zero baseline shatters into disconnected segments around each spike. That's the exact regression this PR fixes elsewhere, reintroduced on the one chart type where 0 is a real, collected value rather than "nothing collected." These should stay on plain Add.Scatter, same as the other synthetic zero-line/placeholder sites the PR explicitly left alone.

Everything else checked out

• Core algorithm (TimeSeriesGaps.cs): guards (< 3 points, < 2 positive deltas) are sound; median-over-mean avoids the yardstick-inflation problem; NaN insertion at gap midpoint doesn't perturb real point positions; no-gap case correctly returns the original arrays by reference (allocation-free, pinned by test).
• Hover integration: ChartHoverHelper.GetNearestSeries/OnMouseMove already skip !nearest.IsReal, so the injected NaN points are correctly invisible to hit-testing — no tooltip/crosshair glitches expected.
• Lite/Darling parity: every genuine time-series Add.Scatter site got converted symmetrically in both apps; the remaining Add.Scatter sites (zero-line placeholders in ServerTab.BlockingStats.cs/ViewerServerTab.Blocking.cs, ChartRenderHelper.cs, GroupedTrendChartRenderer.cs) are consistent between apps and correctly left alone. Test coverage for the shared PerformanceMonitor.Ui logic lives only in Lite.Tests, not Darling.Tests — acceptable since the code itself is shared and not duplicated, just noting for anyone looking for it later.
• CHANGELOG entry accurately describes the mechanism (3x median, per-series derivation) and matches the code.
• Anomaly-marker scatters (LineWidth = 0, no connecting line) were also converted to Add.TimeSeries in both apps; harmless since there's no line to break, just slightly unnecessary computation — not worth changing.

No security, injection, or missing-index concerns applicable here (pure C#/WPF charting change, no SQL).

[claude]

Applying Add.TimeSeries here (and at the DeadlockTrendChart site at line 524, plus the Lite mirrors in ServerTab.Charts.cs:878/957) breaks the spike-chart rendering it's added to.

expandedTimes/expandedCounts isn't a real collection-cadence series — it's a synthetic construct: for every incident it inserts (t-0.0001, 0), (t, count), (t+0.0001, 0) to draw a zero-baseline spike. Those ±0.0001-day (≈8.6s) offsets are by far the smallest deltas in the array, so TimeSeriesGaps.GapThreshold's median-of-positive-deltas locks onto ~0.0001 as soon as there are 2+ incidents (2 tiny deltas per incident vs. 1 real inter-incident gap), giving a break threshold of ~26 seconds.

Since real incidents are essentially never <26s apart, every "back to zero after incident A → zero before incident B" segment now exceeds the threshold and gets a NaN break injected — fragmenting the exact "zero baseline between spikes" this code explicitly builds (see the comment two lines up: "Build arrays with zero baseline between data points for spike effect"). The chart will render disconnected floating spikes instead of a continuous zero line between them.

This is precisely the case TimeSeriesPlotExtensions.cs's own doc comment says to exclude: "Non-time scatters (histograms, synthetic zero-lines, pixel-space annotations) keep plain Add.Scatter — a cadence is a property of collection timestamps, and deriving one from arbitrary X data would be noise." expandedTimes is exactly this kind of pixel-space/synthetic construct, not a real per-collection timestamp series, and should have stayed on plain Add.Scatter like the zeroLine fallback a few lines above it.

[claude]

Same issue as the Darling counterpart (ViewerServerTab.Blocking.cs:454): expandedTimes/expandedCounts is a synthetic zero-baseline/spike construct (±0.0001-day offsets around each incident), not a real collection-cadence series. Feeding it through Add.TimeSeries makes the median-derived gap threshold lock onto the tiny spike-offset spacing (~26s), so essentially every real inter-incident gap now gets a spurious NaN break — fragmenting the "zero baseline between spikes" this code builds. Same problem repeats at UpdateDeadlockTrendChart (line 957). Per TimeSeriesPlotExtensions.cs's own doc comment, synthetic/pixel-space series like this should stay on plain Add.Scatter.

[claude]

Two other pre-existing consumers of scatter.Data.GetScatterPoints() also see the synthetic NaN break-points this injects, beyond what's exercised by the new unit tests (which only test TimeSeriesGaps.BreakAtGaps in isolation, never the interaction with existing downstream consumers):

1. Gradient area-fill is silently disabled for any series with a gap. ChartStyle.StyleScatter (PerformanceMonitor.Ui/ChartStyle.cs:240-241, called right after Add.TimeSeries at nearly every one of the ~90 converted call sites) computes minY = pts.Min(p => p.Y) / maxY = pts.Max(p => p.Y) over the full point set. .NET's Enumerable.Min/Max<double> propagate NaN — per the documented behavior, the result is NaN if any element is NaN — so as soon as a series contains one gap, minY/maxY become NaN, canFill (which requires !IsNaN(minY) && !IsNaN(maxY)) goes false, and FillY is disabled for the entire line, not just around the gap. The exact scenario this PR targets (a monitor going offline) is the one that now also silently kills that series' gradient fill for its whole rendered range.

2. CSV chart export gets fabricated rows. Both Lite/Helpers/ContextMenuHelper.cs:291 and Darling/PerformanceMonitor.Darling.Viewer/ViewerServerTab.ChartContextMenu.cs:305 iterate scatter.Data.GetScatterPoints() to write one CSV row per point for every Scatter plottable on the chart. The injected gap-break point (a fabricated mid-gap timestamp with Y = NaN) is a real entry in that collection, so exporting data from any chart with a detected gap now writes out a row with a timestamp that was never actually collected and a literal "NaN" value string — polluting exported data for a tool whose purpose is accurate historical reporting.

Neither of these is caught by the new TimeSeriesGapsTests, since those only exercise BreakAtGaps directly and never touch ChartStyle or the CSV export path.

[claude]

Review: #1947 — break chart lines at collection gaps

What it does: Adds TimeSeriesGaps.BreakAtGaps (median-of-positive-inter-point-spacing × 3, NaN-injected mid-gap) and a PlottableAdder.TimeSeries extension in the shared PerformanceMonitor.Ui project, then migrates ~104 Add.Scatter call sites in both Lite and Darling to Add.TimeSeries. Ten new unit tests pin the core algorithm's behavior in isolation.

Parity

No drift found. The algorithm and extension live in the shared PerformanceMonitor.Ui project, so both apps get identical behavior for free — no duplicated logic to keep in sync. The ~18 remaining Add.Scatter sites (9 per app) are all zeroLine reference-line placeholders, correctly left unconverted and symmetric between apps. Conversion counts (49 Lite vs. 48 Darling) reconcile exactly against the pre-existing "compare range" ghost-line feature that only exists in Lite — not something this PR introduced asymmetrically.

Correctness — 3 issues found, all left as inline comments

1. Blocking/Deadlock trend "spike" charts get spurious breaks (ViewerServerTab.Blocking.cs:454/524, ServerTab.Charts.cs:878/957): these build a synthetic zero-baseline/spike array (±0.0001-day offsets per incident), not a real collection-cadence series. The median-based cadence detector locks onto that tiny synthetic spacing, so the break threshold ends up ~26 seconds — meaning essentially every real inter-incident gap now triggers a break, fragmenting the "zero baseline between spikes" the code explicitly builds. This is exactly the kind of synthetic/pixel-space series the new extension's own doc comment says to exclude.
2. Gradient area-fill is silently disabled on any series with a gap (ChartStyle.StyleScatter, PerformanceMonitor.Ui/ChartStyle.cs:240-241): Enumerable.Min/Max<double> propagate NaN, so once a series contains a gap-break point, minY/maxY become NaN and canFill goes false for the entire series — not just around the gap. The scenario this PR is built to surface (an outage) also silently kills that series' fill.
3. CSV chart export gets fabricated rows (Lite/Helpers/ContextMenuHelper.cs:291, Darling/.../ViewerServerTab.ChartContextMenu.cs:305): both iterate scatter.Data.GetScatterPoints() for export, so the injected NaN break-point (fake mid-gap timestamp, Y=NaN) is written out as a real CSV row.

All three stem from the same root cause: the new NaN break-points flow into every existing consumer of scatter.Data/GetScatterPoints(), not just the renderer this PR was written against, and none of the ten new unit tests exercise that interaction (they only test BreakAtGaps in isolation).

Core algorithm itself

TimeSeriesGaps.BreakAtGaps/GapThreshold look correct for genuine collection-timestamp series: robust median-of-positive-deltas, NaN-mid-gap injection, ascending-X assumption documented, short/duplicate-timestamp series pass through untouched. Test coverage for the algorithm in isolation is solid (outage break, two-outage, jitter tolerance, duplicate timestamps, slow-cadence proportional gap, too-few-points passthrough).

Not flagged

No missing-index DMV concerns (n/a to this change), no security-relevant input/secret/process handling in this diff, no obvious perf regression — the added per-render cost (sort of deltas, O(n) array rebuild) is negligible at chart-series sizes.

[claude]

Cadence-based gap-breaking applied to a sparse, incident-driven series — worth confirming this was deliberate.

GetBlockingDurationStatsAsync only emits a row for minutes where blocking actually happened (GROUP BY DATE_TRUNC('minute', event_time) over v_dmv_blocking_snapshots), and PadEnds exists specifically to bridge that sparsity across the full window with zero-value endpoints — the doc comment above literally calls it "the window-pinned analog of the Trends charts' zero-baseline expansion." That's the same shape as the expandedTimes/expandedCounts synthetic baseline in RenderBlockingTrendChart/RenderDeadlockTrendChart a few hundred lines up, which this PR deliberately kept on plain Add.Scatter with a comment explaining that cadence-based breaking "would lock onto the artificial deltas."

Here the median-spacing cadence gets derived from a mix of genuinely irregular incident-to-incident gaps plus the two synthetic pad-to-real deltas, then used to insert breaks. Depending on how bursty the incidents are relative to the window, that can either correctly sever the zero-baseline pad from a cluster of activity, or spuriously shatter the line inside a single blocking episode whose sub-gaps happen to exceed 3x the local median — blocking/deadlock incidents aren't collected on a steady cadence, so a wide gap here doesn't reliably mean "collector was offline" the way it does for the CPU/memory series this feature targets.

Same pattern repeats at BlockingTotalDurationChart (below), DeadlockWaitChart, and DeadlockTotalWaitChart in this file, and identically in Lite/Controls/ServerTab.BlockingStats.cs. Given the sibling zero-baseline charts were explicitly excluded, this looks like it may have been missed in the per-site audit rather than intentionally included — worth a second look before merge.

[claude]

Same concern as the Darling counterpart (ViewerServerTab.Blocking.cs, RenderBlockingDurationChart): this is a PadEnds-bridged sparse/incident-driven series (blocking only produces rows when it occurs), same shape as the expandedTimes/expandedCounts baseline that was deliberately excluded from Add.TimeSeries a bit further down in this file for exactly that reason. Applying cadence-derived gap-breaking here risks shattering the line inside a single blocking episode, since incident-to-incident gaps aren't a steady collection cadence. Repeats at BlockingTotalDurationChart, DeadlockWaitChart, DeadlockTotalWaitChart in this file — flagging once here for parity with the Darling comment.

[claude]

Review: #1947 — Break chart lines at collection gaps

Overview. Adds TimeSeriesGaps.BreakAtGaps + a TimeSeries() extension on Plot.Add (both in the shared PerformanceMonitor.Ui library, so Lite and Darling can't drift on the algorithm itself) that injects a NaN Y between two points whenever the gap exceeds 3x the series' own median positive spacing. Mechanically applied at ~104 call sites in both apps, plus follow-on fixes: CSV export now skips the injected NaN rows (both apps), and ChartStyle.StyleScatter's gradient-fill min/max now excludes NaN so one gap doesn't null out fill for the whole series.

Core algorithm. TimeSeriesGaps.cs is solid — guards on length < 3, excludes zero/duplicate-timestamp deltas from the median (so a shared collection cycle across series doesn't drag the threshold to zero), uses median rather than mean specifically so outage gaps can't inflate their own detection threshold, and passes through untouched (returning the original array references, not copies) when there's no measurable cadence. The 8 pinned unit tests in Lite.Tests/TimeSeriesGapsTests.cs cover both directions (outage breaks; jitter/duplicates/short-series/slow-cadence stay connected) and look correct by inspection. Couldn't execute them here — the projects target net10.0-windows/WPF and this sandbox is Linux — so this is a static read, not a verified run.

Lite/Darling parity. Checked this directly: grepped every remaining Add.Scatter( call in both apps post-patch. The ~20 that stayed on plain Scatter are the same shape in both apps — zeroLine empty-state placeholders and the expandedTimes/expandedCounts synthetic spike-baseline series (Blocking/Deadlock incident-count trend charts), both now carrying a comment explaining why cadence-breaking would misfire on synthetic deltas. No drift found between the two apps for this conversion.

Correctness concern (inline comments posted). The same "synthetic-baseline, exclude from cadence-breaking" reasoning that was applied to the incident-count trend charts doesn't appear to have been carried over to the sibling incident-duration/wait charts (BlockingDurationChart, BlockingTotalDurationChart, DeadlockWaitChart, DeadlockTotalWaitChart, in both Darling/.../ViewerServerTab.Blocking.cs and Lite/Controls/ServerTab.BlockingStats.cs). Those use the same PadEnds-bridged sparse/incident-driven data (blocking/deadlock duration rows only exist for minutes where something happened — confirmed via GetBlockingDurationStatsAsync's GROUP BY DATE_TRUNC('minute', event_time)), but got switched to Add.TimeSeries. Since incident-to-incident spacing isn't a steady collection cadence, this risks shattering the line mid-episode rather than only at genuine outages. Left detail on both files inline — this reads more like a gap in the per-site audit than an intentional choice, worth a second pass before merge.

Minor / non-blocking:

• The "anomaly marker" overlay scatters (e.g. CorrelatedTimelineLanesControl.xaml.cs) also got switched to Add.TimeSeries, but they're plotted with LineWidth = 0 (markers only, no connecting line), so the injected NaN breaks are inert — harmless, just unnecessary median/threshold computation over indices that aren't a time cadence either.
• No unit test coverage for the two follow-on fixes (CSV NaN-skip, ChartStyle NaN-aware min/max) — understandable given there's no WPF UI test harness in this repo, but worth knowing they're only covered by manual/visual verification.
• Didn't flag any security issues — this PR touches only local chart rendering/export logic, no new I/O, network, or input-handling surface.

Nice fix for a real, well-motivated UX problem (offline windows reading as continuous data), and the mechanism itself is careful and well-tested. The one thing I'd want resolved before merge is the PadEnds/incident-chart question above.