Feature/query store timeslicer#114
Merged
erikdarlingdata merged 5 commits intoerikdarlingdata:devfrom Mar 19, 2026
Merged
Conversation
…tric display in background of the range is base on the sorting metric in the drop down selector Auto fetch when opening the query store
- Fix selection bugs : fetch is relaunch when moving - Selection default selection on top 25 query limited to 24h using Rangetime which display the time selection - number of hours selected in the Range selector is displayed in the top left of the Rangetime selector
Add SelectionChanged handler for "Order by" ComboBox to allow users to switch metrics and refresh data without losing time range selection. Preserve slicer selection across metric changes and suppress redundant fetches. Remove row limit from time-slice SQL query to ensure all data is available for the slicer : using where filtering only
… instead of query_store_runtime_stats.last_execution_time only
…ecution_time to filter on fact table first
📝 WalkthroughWalkthroughThis PR introduces an interactive time range slicer component to the Query Store view, allowing users to visualize and select time ranges from aggregated metrics. It refactors the plan-fetching logic to use date-range parameters, adds hourly metric aggregation queries to the service layer, and integrates the new slicer into the existing QueryStoreGridControl. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Comment |
Contributor
Author
|
Nota : No effort on UTC conversion for now. all is in UTC (Range linechart, data, filter...). I would propose to create tools settings to adjust that later with user preferences but i would like to avoid headache this evening |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/PlanViewer.App/Controls/QueryStoreGridControl.axaml.cs`:
- Around line 157-171: The code builds a search filter via BuildSearchFilter()
but never passes it into QueryStoreService.FetchTopPlansAsync, so search
controls don't affect results; update the FetchTopPlansAsync call in
QueryStoreGridControl.axaml.cs to include the filter parameter (e.g., pass
filter: filter) alongside the existing args (connectionString/_connectionString,
topN, orderBy/_lastFetchedOrderBy, ct, startUtc/_slicerStartUtc,
endUtc/_slicerEndUtc) so the built filter is applied when fetching plans.
- Around line 308-316: LoadTimeSlicerDataAsync currently calls
TimeRangeSlicer.LoadData(sliceData, metric) and thus resets the slicer to
defaults; before calling LoadData capture the current
TimeRangeSlicer.SelectionStart and SelectionEnd (or a flag whether a custom
selection exists), then call LoadData(sliceData, metric) and restore the
captured SelectionStart/SelectionEnd back onto TimeRangeSlicer (or call an
overload of LoadData that accepts start/end if available). Ensure you still
honor CancellationToken ct (check ct.IsCancellationRequested before restoring)
and use the existing method names LoadTimeSlicerDataAsync,
TimeRangeSlicer.LoadData, TimeRangeSlicer.SelectionStart, and
TimeRangeSlicer.SelectionEnd to locate where to add the capture/restore logic.
In `@src/PlanViewer.App/Controls/TimeRangeSlicerControl.axaml.cs`:
- Around line 138-165: The avg-* branches are incorrectly returning Total*
values and labels; add per-bucket average properties to the QueryStoreTimeSlice
model (e.g., AvgCpu, AvgDuration, AvgReads, AvgWrites, AvgPhysicalReads,
AvgMemory) and populate them in QueryStoreService.FetchTimeSliceDataAsync so
each time-slice contains both Total* and Avg* fields, then update
TimeRangeSlicerControl.GetMetricValues to return the corresponding Avg*
properties for "avg-*" cases (e.g., "avg-cpu" => _data.Select(d =>
d.AvgCpu).ToArray()) and update GetMetricLabel to use "Avg ..." labels for those
branches (e.g., "avg-cpu" => "Avg CPU (ms)"); ensure numeric types match (cast
ints to double where needed) and leave non-avg branches using Total* as before.
- Around line 189-198: The chart currently spaces points by array index which
collapses gaps because FetchTimeSliceDataAsync(...) returns only hours with
data; instead compute each point's X from its actual timestamp (use the bucket's
IntervalStartUtc) mapped into the control's time span (same mapping
GetDateTimeAtNorm uses) so plotted points align with wall-clock time; in
TimeRangeSlicerControl.axaml.cs modify the loop that builds linePoints to read
the timestamps corresponding to values (or the returned bucket objects) and
calculate x = (bucket.IntervalStartUtc - rangeStart).TotalSeconds /
rangeDuration.TotalSeconds * w (or equivalent normalized mapping) before adding
new Point(x,y), or alternatively pre-fill zero-valued hourly buckets in the data
returned by FetchTimeSliceDataAsync to ensure dense hourly spacing—choose the
timestamp-based X computation for correctness.
In `@src/PlanViewer.Core/Services/QueryStoreService.cs`:
- Line 325: The filter using rs.first_execution_time with DATEADD(HOUR,
-@hoursBack, GETUTCDATE()) wrongly excludes query-store rows that had an earlier
first_execution_time but later executions inside the window; modify the
predicate in the QueryStoreService SQL (references to rs.first_execution_time,
`@hoursBack`, DATEADD/GETUTCDATE()) to use an overlap-safe column such as
rs.last_execution_time or remove the runtime_stats-side filter after measuring;
update the same pattern at the other occurrences noted (the similar predicates
around lines referenced as 392-393) so the history view and slicer are not
undercounted.
- Around line 134-138: The UTC window currently excludes buckets that end
exactly on the boundary because QueryStoreService builds timeWhereClause with
"rsi.end_time < `@rangeEnd`"; change the predicate so the upper boundary is
inclusive (e.g., use rsi.end_time <= `@rangeEnd`) or alternatively compare the
bucket start to the end boundary (e.g., use rsi.start_time < `@rangeEnd`) to match
TimeRangeSlicerControl semantics; update the timeWhereClause string accordingly
where it is set and leave the `@rangeStart/`@rangeEnd SqlParameter additions
intact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1afa25e6-53b9-4b20-a76a-4e26a01f7e65
📒 Files selected for processing (8)
src/PlanViewer.App/Controls/QueryStoreGridControl.axamlsrc/PlanViewer.App/Controls/QueryStoreGridControl.axaml.cssrc/PlanViewer.App/Controls/TimeRangeSlicerControl.axamlsrc/PlanViewer.App/Controls/TimeRangeSlicerControl.axaml.cssrc/PlanViewer.App/Services/AppSettingsService.cssrc/PlanViewer.App/Themes/DarkTheme.axamlsrc/PlanViewer.Core/Models/QueryStoreTimeSlice.cssrc/PlanViewer.Core/Services/QueryStoreService.cs
Comment on lines
+157
to
+171
| var topN = (int)(TopNBox.Value ?? 25); | ||
| var orderBy = _lastFetchedOrderBy; | ||
| var filter = BuildSearchFilter(); | ||
|
|
||
| FetchButton.IsEnabled = false; | ||
| LoadButton.IsEnabled = false; | ||
| StatusText.Text = "Fetching..."; | ||
| StatusText.Text = "Fetching plans..."; | ||
| _rows.Clear(); | ||
| _filteredRows.Clear(); | ||
|
|
||
| try | ||
| { | ||
| var plans = await QueryStoreService.FetchTopPlansAsync( | ||
| _connectionString, topN, orderBy, hoursBack, filter, ct); | ||
| _connectionString, topN, orderBy, ct: ct, | ||
| startUtc: _slicerStartUtc, endUtc: _slicerEndUtc); |
There was a problem hiding this comment.
Pass the built search filter into the range fetch.
BuildSearchFilter() still runs on Line 159, but FetchTopPlansAsync is called without filter: on Lines 169-171. After this refactor, the Search by controls no longer affect the fetched results.
Suggested fix
var plans = await QueryStoreService.FetchTopPlansAsync(
- _connectionString, topN, orderBy, ct: ct,
+ _connectionString, topN, orderBy, filter: filter, ct: ct,
startUtc: _slicerStartUtc, endUtc: _slicerEndUtc);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/PlanViewer.App/Controls/QueryStoreGridControl.axaml.cs` around lines 157
- 171, The code builds a search filter via BuildSearchFilter() but never passes
it into QueryStoreService.FetchTopPlansAsync, so search controls don't affect
results; update the FetchTopPlansAsync call in QueryStoreGridControl.axaml.cs to
include the filter parameter (e.g., pass filter: filter) alongside the existing
args (connectionString/_connectionString, topN, orderBy/_lastFetchedOrderBy, ct,
startUtc/_slicerStartUtc, endUtc/_slicerEndUtc) so the built filter is applied
when fetching plans.
Comment on lines
+308
to
+316
| private async System.Threading.Tasks.Task LoadTimeSlicerDataAsync(string metric, CancellationToken ct) | ||
| { | ||
| try | ||
| { | ||
| var sliceData = await QueryStoreService.FetchTimeSliceDataAsync( | ||
| _connectionString, metric, _slicerDaysBack, ct); | ||
| if (ct.IsCancellationRequested) return; | ||
| if (sliceData.Count > 0) | ||
| TimeRangeSlicer.LoadData(sliceData, metric); |
There was a problem hiding this comment.
Keep the current range when refreshing slicer data.
This path always calls TimeRangeSlicer.LoadData(sliceData, metric) with no selection bounds, so clicking Fetch after narrowing the range jumps back to the default last 24 hours. Refreshing the metric data should preserve TimeRangeSlicer.SelectionStart / TimeRangeSlicer.SelectionEnd.
Suggested fix
private async System.Threading.Tasks.Task LoadTimeSlicerDataAsync(string metric, CancellationToken ct)
{
try
{
+ var selectionStart = TimeRangeSlicer.SelectionStart;
+ var selectionEnd = TimeRangeSlicer.SelectionEnd;
var sliceData = await QueryStoreService.FetchTimeSliceDataAsync(
_connectionString, metric, _slicerDaysBack, ct);
if (ct.IsCancellationRequested) return;
if (sliceData.Count > 0)
- TimeRangeSlicer.LoadData(sliceData, metric);
+ TimeRangeSlicer.LoadData(sliceData, metric, selectionStart, selectionEnd);
else
StatusText.Text = "No time-slicer data available.";
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/PlanViewer.App/Controls/QueryStoreGridControl.axaml.cs` around lines 308
- 316, LoadTimeSlicerDataAsync currently calls
TimeRangeSlicer.LoadData(sliceData, metric) and thus resets the slicer to
defaults; before calling LoadData capture the current
TimeRangeSlicer.SelectionStart and SelectionEnd (or a flag whether a custom
selection exists), then call LoadData(sliceData, metric) and restore the
captured SelectionStart/SelectionEnd back onto TimeRangeSlicer (or call an
overload of LoadData that accepts start/end if available). Ensure you still
honor CancellationToken ct (check ct.IsCancellationRequested before restoring)
and use the existing method names LoadTimeSlicerDataAsync,
TimeRangeSlicer.LoadData, TimeRangeSlicer.SelectionStart, and
TimeRangeSlicer.SelectionEnd to locate where to add the capture/restore logic.
Comment on lines
+138
to
+165
| private double[] GetMetricValues() | ||
| { | ||
| return _metric switch | ||
| { | ||
| "cpu" or "avg-cpu" => _data.Select(d => d.TotalCpu).ToArray(), | ||
| "duration" or "avg-duration" => _data.Select(d => d.TotalDuration).ToArray(), | ||
| "reads" or "avg-reads" => _data.Select(d => d.TotalReads).ToArray(), | ||
| "writes" or "avg-writes" => _data.Select(d => d.TotalWrites).ToArray(), | ||
| "physical-reads" or "avg-physical-reads" => _data.Select(d => d.TotalPhysicalReads).ToArray(), | ||
| "memory" or "avg-memory" => _data.Select(d => d.TotalMemory).ToArray(), | ||
| "executions" => _data.Select(d => (double)d.TotalExecutions).ToArray(), | ||
| _ => _data.Select(d => d.TotalCpu).ToArray(), | ||
| }; | ||
| } | ||
|
|
||
| private string GetMetricLabel() | ||
| { | ||
| return _metric switch | ||
| { | ||
| "cpu" or "avg-cpu" => "Total CPU (ms)", | ||
| "duration" or "avg-duration" => "Total Duration (ms)", | ||
| "reads" or "avg-reads" => "Total Reads", | ||
| "writes" or "avg-writes" => "Total Writes", | ||
| "physical-reads" or "avg-physical-reads" => "Total Physical Reads", | ||
| "memory" or "avg-memory" => "Total Memory (MB)", | ||
| "executions" => "Executions", | ||
| _ => "Total CPU (ms)", | ||
| }; |
There was a problem hiding this comment.
avg-* modes are still rendering total-series data.
All avg-* branches read the same Total* fields, and the labels still say Total .... When the grid switches to Avg CPU, Avg Duration, and the other average modes, the slicer background stops matching the selected metric. Add per-bucket averages to src/PlanViewer.Core/Models/QueryStoreTimeSlice.cs / src/PlanViewer.Core/Services/QueryStoreService.cs::FetchTimeSliceDataAsync(...) and switch these cases to those values.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/PlanViewer.App/Controls/TimeRangeSlicerControl.axaml.cs` around lines 138
- 165, The avg-* branches are incorrectly returning Total* values and labels;
add per-bucket average properties to the QueryStoreTimeSlice model (e.g.,
AvgCpu, AvgDuration, AvgReads, AvgWrites, AvgPhysicalReads, AvgMemory) and
populate them in QueryStoreService.FetchTimeSliceDataAsync so each time-slice
contains both Total* and Avg* fields, then update
TimeRangeSlicerControl.GetMetricValues to return the corresponding Avg*
properties for "avg-*" cases (e.g., "avg-cpu" => _data.Select(d =>
d.AvgCpu).ToArray()) and update GetMetricLabel to use "Avg ..." labels for those
branches (e.g., "avg-cpu" => "Avg CPU (ms)"); ensure numeric types match (cast
ints to double where needed) and leave non-avg branches using Total* as before.
Comment on lines
+189
to
+198
| var n = values.Length; | ||
| var stepX = w / n; | ||
|
|
||
| // Draw filled area + line for the chart | ||
| var linePoints = new List<Point>(n); | ||
| for (int i = 0; i < n; i++) | ||
| { | ||
| var x = i * stepX + stepX / 2; | ||
| var y = chartBottom - (values[i] / max) * chartHeight; | ||
| linePoints.Add(new Point(x, y)); |
There was a problem hiding this comment.
Plot buckets by timestamp, not by array index.
src/PlanViewer.Core/Services/QueryStoreService.cs::FetchTimeSliceDataAsync(...) only returns hours that have runtime stats, so quiet hours are absent. Spacing every point evenly here collapses those gaps, while GetDateTimeAtNorm() still maps the handles across real wall-clock time. On sparse data the shaded range will no longer line up with the plotted hours. Either insert zero-value hourly buckets before drawing or compute x from IntervalStartUtc.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/PlanViewer.App/Controls/TimeRangeSlicerControl.axaml.cs` around lines 189
- 198, The chart currently spaces points by array index which collapses gaps
because FetchTimeSliceDataAsync(...) returns only hours with data; instead
compute each point's X from its actual timestamp (use the bucket's
IntervalStartUtc) mapped into the control's time span (same mapping
GetDateTimeAtNorm uses) so plotted points align with wall-clock time; in
TimeRangeSlicerControl.axaml.cs modify the loop that builds linePoints to read
the timestamps corresponding to values (or the returned bucket objects) and
calculate x = (bucket.IntervalStartUtc - rangeStart).TotalSeconds /
rangeDuration.TotalSeconds * w (or equivalent normalized mapping) before adding
new Point(x,y), or alternatively pre-fill zero-valued hourly buckets in the data
returned by FetchTimeSliceDataAsync to ensure dense hourly spacing—choose the
timestamp-based X computation for correctness.
Comment on lines
+134
to
+138
| if (startUtc.HasValue && endUtc.HasValue) | ||
| { | ||
| timeWhereClause = "WHERE rsi.start_time >= @rangeStart AND rsi.end_time < @rangeEnd"; | ||
| parameters.Add(new SqlParameter("@rangeStart", startUtc.Value)); | ||
| parameters.Add(new SqlParameter("@rangeEnd", endUtc.Value)); |
There was a problem hiding this comment.
Include the boundary bucket in the explicit UTC window.
TimeRangeSlicerControl computes the selected end at the end of the last bucket, not its start. With rsi.end_time < @rangeEnd``, a bucket that ends exactly on that boundary is excluded, so the default last-24-hours selection can miss its newest hour. Make the upper bound inclusive or compare rsi.start_time to `@rangeEnd` instead.
Suggested fix
- timeWhereClause = "WHERE rsi.start_time >= `@rangeStart` AND rsi.end_time < `@rangeEnd`";
+ timeWhereClause = "WHERE rsi.start_time >= `@rangeStart` AND rsi.start_time < `@rangeEnd`";📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (startUtc.HasValue && endUtc.HasValue) | |
| { | |
| timeWhereClause = "WHERE rsi.start_time >= @rangeStart AND rsi.end_time < @rangeEnd"; | |
| parameters.Add(new SqlParameter("@rangeStart", startUtc.Value)); | |
| parameters.Add(new SqlParameter("@rangeEnd", endUtc.Value)); | |
| if (startUtc.HasValue && endUtc.HasValue) | |
| { | |
| timeWhereClause = "WHERE rsi.start_time >= `@rangeStart` AND rsi.start_time < `@rangeEnd`"; | |
| parameters.Add(new SqlParameter("@rangeStart", startUtc.Value)); | |
| parameters.Add(new SqlParameter("@rangeEnd", endUtc.Value)); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/PlanViewer.Core/Services/QueryStoreService.cs` around lines 134 - 138,
The UTC window currently excludes buckets that end exactly on the boundary
because QueryStoreService builds timeWhereClause with "rsi.end_time <
`@rangeEnd`"; change the predicate so the upper boundary is inclusive (e.g., use
rsi.end_time <= `@rangeEnd`) or alternatively compare the bucket start to the end
boundary (e.g., use rsi.start_time < `@rangeEnd`) to match TimeRangeSlicerControl
semantics; update the timeWhereClause string accordingly where it is set and
leave the `@rangeStart/`@rangeEnd SqlParameter additions intact.
| ON rs.plan_id = p.plan_id | ||
| WHERE p.query_id = @queryId | ||
| AND rsi.start_time >= DATEADD(HOUR, -@hoursBack, GETUTCDATE()) | ||
| AND rs.first_execution_time >= DATEADD(HOUR, -@hoursBack, GETUTCDATE()) --performance: filter runtime_stats by time directly |
There was a problem hiding this comment.
first_execution_time is trimming valid stats rows.
A Query Store stats row can have first_execution_time before the cutoff and later executions still inside the selected period. These predicates undercount both the history view and the slicer around the window boundary. If you need a runtime-stats-side filter for scan reduction, make it overlap-safe, such as last_execution_time, or drop it after measuring.
Also applies to: 392-393
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/PlanViewer.Core/Services/QueryStoreService.cs` at line 325, The filter
using rs.first_execution_time with DATEADD(HOUR, -@hoursBack, GETUTCDATE())
wrongly excludes query-store rows that had an earlier first_execution_time but
later executions inside the window; modify the predicate in the
QueryStoreService SQL (references to rs.first_execution_time, `@hoursBack`,
DATEADD/GETUTCDATE()) to use an overlap-safe column such as
rs.last_execution_time or remove the runtime_stats-side filter after measuring;
update the same pattern at the other occurrences noted (the similar predicates
around lines referenced as 392-393) so the history view and slicer are not
undercounted.
erikdarlingdata
approved these changes
Mar 19, 2026
Owner
erikdarlingdata
left a comment
erikdarlingdata
left a comment
There was a problem hiding this comment.
Looks good — nice iteration from #113. The interval-based time filtering, metric-refresh on sort change, and selection preservation are solid improvements.
A couple of minor suggestions for future consideration (not blocking):
- A short debounce (e.g., 300ms) on
OnTimeRangeChangedbefore triggeringFetchPlansForRangeAsyncwould reduce rapid cancel-fetch cycles when the user is dragging the slicer handle - The
daysBackinterpolation in the SQL could be parameterized for consistency, though it's not a security risk since it comes from settings
We'll handle both of those + a time display picker (UTC/server/local) in a follow-up.
Owner
|
@rferraton this is so cool, I can't stop messing with it. I'm gonna port this idea over to the monitoring tools too. Seriously killer stuff! I added in the local/server/utc time picker stuff just to round it out. |
5 tasks
erikdarlingdata
added a commit
that referenced
this pull request
Mar 26, 2026
Two regressions introduced in the time slicer PR (#114): 1. Search filter (module, query_id, etc.) was built via BuildSearchFilter() but never passed to FetchTopPlansAsync — the filter parameter was dropped when the call site switched to named parameters for startUtc/ endUtc. Add filter: back to the call. 2. Clicking Fetch reloads the slicer via LoadTimeSlicerDataAsync, which calls LoadData without preserving the current selection. LoadData defaults to last 24h when no selection is provided, discarding the user's custom range. Pass the current _slicerStartUtc/_slicerEndUtc through to LoadData's selectionStart/selectionEnd parameters. Fixes #147 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
5 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
Which component(s) does this affect?
How was this tested?
I have give a look on generated query and add filter that (i hope) would limit impact on QS with long history
Checklist
dotnet build -c Debug)dotnet test)Summary by CodeRabbit