Fix and port Memory Pressure Events feature (#865)#866
Conversation
Chart previously filtered to HIGH severity only (indicator>=3), which on most servers never fires, producing an empty chart even when sp_pressuredetector- level medium pressure (indicator=2) was occurring constantly. Switch to stacked bars per hour, split by SQL Server (process) vs Operating System (system), with severe events capped on top of medium in a darker shade. Extend ChartHoverHelper to support BarPlot tooltips. Add MCP guidance for interpreting indicator values and routing to the right follow-up tool. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
erikdarlingdata
left a comment
There was a problem hiding this comment.
Review summary
What this PR does
- Fixes #865: Memory Pressure Events chart was filtering on
Severity == "HIGH"(effectivelyindicator >= 3), which almost never fires. New filter isindicator >= 2, matchingsp_pressuredetector's medium/severe semantics. - Switches the chart from a scatter line to four stacked bar series (SQL Server medium/severe + OS medium/severe), drawn at symmetric X offsets per hour bucket, so SQL-process vs OS-level pressure are visually separable.
- Teaches
ChartHoverHelperto handleBarPlotin addition toScatter, and formats integer-valued tooltips without.0. - Expands the
get_memory_pressure_eventsMCP tool[Description]and adds an "Interpreting Memory Pressure Events" section toMcpInstructions.cs— indicator scale, process vs system semantics, follow-up tool table, common patterns.
Repo-policy checks
- Base branch:
dev✅ - PlanAnalyzer: not touched, no sync concern ✅
- Schema / upgrade scripts: no SQL changes ✅
- Lite-first: PR description explicitly confirms Lite has neither this chart nor a memory-pressure MCP tool. Verified — no
MemoryContent.*underLite/and no matching tool inLite/Mcp/. ✅ - SignPath / build.yml: no workflow changes ✅
What's good
- Stacked-bar geometry is right:
sqlSevereBarsusesValueBase = sqlMedium, Value = sqlMedium + sqlSevereso severe sits on top of medium.FindNearestBarcorrectly reports the segment height (Value - ValueBase), so the tooltip shows the individual segment's count rather than the running total. That's the non-obvious bit and it's handled. - Y-axis floor of
5.0avoids a single-event day rendering as one huge bar dominating the plot. - The new MCP section is genuinely useful — indicator scale + the "system-only fires but process does not" guidance is exactly what an LLM needs to avoid blaming SQL Server for host-level pressure.
Needs attention (see inline comments)
ChartHoverHelper.cs:204-206— the integer-formatting change affects every scatter chart using the helper, not just the new bar chart. Likely cosmetic regression on whole-number samples elsewhere (memory, PLE, wait-time charts). Scope it to bar-originated hovers or make it construction-time configurable.ChartHoverHelper.cs:134-137—FindNearestBardoes twoPlot.GetPixelcalls × every bar, every mouse move. HoisthalfWidthPxout of the inner loop. Not a blocker, just a lurking perf issue at 30-day windows.MemoryPressureEventItem.Severity(Models/MemoryPressureEventItem.cs:13) is now dead — last reader was the filter this PR removed. Drop it or mark it.MemoryContent.xaml.cs:1227—maxBarCounttakesmax(sqlTotal, osTotal)rather than sum; correct only because SQL and OS bars are X-offset rather than stacked. Worth a one-line comment to protect against future "fixes".
Test coverage
No new tests. The indicator-filter and per-hour bucketing logic is pure data transformation and would be trivially testable if extracted from LoadMemoryPressureEventsChart. Not a blocker — Dashboard UI code isn't currently tested — but a natural place to start if you ever want regression coverage on chart inputs.
Task list
Two unchecked items in the PR description (verifying severe-event stacking + the empty-state message on a live server) are the right things to confirm before merge.
Generated by Claude Code
| string valueFormatted = (bestPoint.Y == Math.Floor(bestPoint.Y)) | ||
| ? bestPoint.Y.ToString("N0") | ||
| : bestPoint.Y.ToString("N1"); |
There was a problem hiding this comment.
This format switch is cross-cutting: it fires for every ChartHoverHelper consumer, not just the new bar chart. Scatter values that happen to be whole numbers (e.g. a memory trend reading of exactly 100 GB or a PLE of 1000) will now render as 100 / 1000 while neighbouring samples render as 99.7 / 1000.4 — inconsistent unit precision in the same tooltip.
If the goal is integer-looking counts for the bar chart only, guard it on _barPlots having produced the winner (e.g. track bool bestIsBar through FindNearestBar) or parameterise the helper's format at construction time. Otherwise this will likely show up as a cosmetic regression on the other memory/wait/cpu charts.
Generated by Claude Code
| var topPixel = _chart.Plot.GetPixel(new ScottPlot.Coordinates(bar.Position, bar.Value)); | ||
| double halfWidthPx = Math.Abs( | ||
| _chart.Plot.GetPixel(new ScottPlot.Coordinates(bar.Position + bar.Size / 2, bar.Value)).X | ||
| - topPixel.X); |
There was a problem hiding this comment.
Two Plot.GetPixel calls per bar, per mouse move. A 30-day window at 24h buckets × up to 4 stacked series = up to ~2,880 bars, re-computed every 30 ms while the mouse is inside the chart. halfWidthPx depends only on bar.Size (constant per series here), so hoist it to a per-series calculation outside the inner loop. Also consider pixel-space binary-searching on Position once the bars are sorted — the inner foreach is effectively an O(N) scan per mouse event.
Not a blocker for this PR, but worth a follow-up if the 30-day view feels laggy on hover.
Generated by Claude Code
| int sqlMedium = g.Count(d => d.MemoryIndicatorsProcess == 2); | ||
| int sqlSevere = g.Count(d => d.MemoryIndicatorsProcess >= 3); | ||
| int osMedium = g.Count(d => d.MemoryIndicatorsSystem == 2); | ||
| int osSevere = g.Count(d => d.MemoryIndicatorsSystem >= 3); |
There was a problem hiding this comment.
After this PR MemoryPressureEventItem.Severity (Models/MemoryPressureEventItem.cs:13) is no longer consumed anywhere in the Dashboard — the last reader was the old d.Severity.Equals("HIGH"…) filter. DatabaseService.Memory.cs:151 still reads column ordinal 6 and populates it, so it's dead-but-still-materialised. Either drop the field and stop selecting the column, or leave a // kept for MCP/grid binding comment so the next person doesn't delete it accidentally.
Generated by Claude Code
| var pressureLimits = MemoryPressureEventsChart.Plot.Axes.GetLimits(); | ||
| MemoryPressureEventsChart.Plot.Axes.SetLimitsY(0, pressureLimits.Top * 1.05); | ||
| MemoryPressureEventsChart.Plot.YLabel("Pressure Events per Hour"); | ||
| MemoryPressureEventsChart.Plot.Axes.SetLimitsY(0, Math.Max(maxBarCount * 1.1, 5.0)); |
There was a problem hiding this comment.
maxBarCount only tracks sqlTotal / osTotal separately (whichever one side is taller). Because SQL and OS bars are drawn at different X offsets (x - barOffset vs x + barOffset) they never stack on top of each other, so this is correct — but it's subtle. Worth a one-line comment next to this so a future reader doesn't "fix" it to sqlTotal + osTotal, which would over-scale the Y axis.
Generated by Claude Code
Lite was missing the RING_BUFFER_RESOURCE_MONITOR collector entirely — no collector, no table, no chart, no MCP tool. This adds the full feature: - Schema: new memory_pressure_events table + index, schema v25, added to ArchivableTables, server-id-fix list, and ArchiveService. - Collector: CollectMemoryPressureEventsAsync queries the ring buffer and client-side-dedupes against DuckDB's MAX(sample_time). Azure SQL DB returns zero rows (ring buffer not exposed there). Scheduled every 5 min (Aggressive and Balanced presets) or 15 min (Low-Impact). - UI: new 'Memory Pressure Events' sub-tab on the Memory tab with the same stacked-bar chart as Dashboard (SQL Server medium/severe, Operating System medium/severe). Wired into full-load and sub-tab-switch refresh paths. - Hover: ported the BarPlot support from Dashboard's ChartHoverHelper so bar tooltips work and report the correct segment height for stacked bars. - MCP: new get_memory_pressure_events tool + the 'Interpreting Memory Pressure Events' guidance section in McpInstructions. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Companion update to the new memory_pressure_events table added in this PR. SchemaStatements_MatchTableCount asserts the total table count; needs to move from 29 to 30 to reflect the new table. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Summary
Fixes the Memory Pressure Events chart in Dashboard and ports the full feature to Lite, which was missing it entirely. Fixes #865.
Dashboard fix
Chart previously filtered to HIGH severity only (
indicator >= 3). On most servers that threshold is essentially never crossed, so the chart was empty even whensp_pressuredetector-level medium pressure (indicator = 2) was firing constantly — which is exactly what the reporter on #865 was seeing.indicator >= 3) bars stacked on top of medium (indicator = 2) bars in a darker shade.ChartHoverHelperto supportBarPlotso tooltips work; integer counts format without trailing.0.get_memory_pressure_eventstool description now explains the indicator scale, andMcpInstructions.cshas anInterpreting Memory Pressure Eventssection with process-vs-system interpretation, follow-up-tool tables, and named patterns (bursty, flat-line, only-one-side-firing, etc.).Lite port
Lite had no memory pressure events feature at all. This adds the full stack:
memory_pressure_eventstable + index, DuckDB schema v25, added toArchivableTables, server-id-fix list, andArchiveService.CollectMemoryPressureEventsAsyncqueriesRING_BUFFER_RESOURCE_MONITORand client-side-dedupes against DuckDB'sMAX(sample_time). Azure SQL DB returns zero rows (ring buffer not exposed there). Scheduled every 5 min in Aggressive/Balanced presets, 15 min in Low-Impact.ChartHoverHelper.get_memory_pressure_eventstool and the sameInterpreting Memory Pressure Eventsguidance section.Test plan
Not changed
collect.memory_pressure_events_collector) and its schema unchanged.