Add a Range Navigator Slicer/Filter to the querystore screen#113
Add a Range Navigator Slicer/Filter to the querystore screen#113rferraton wants to merge 3 commits intoerikdarlingdata:devfrom
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
📝 WalkthroughWalkthroughAdds an interactive TimeRangeSlicer control, integrates slicer-driven time-range fetching into the QueryStore UI and service layer, introduces QueryStoreTimeSlice model and time-slice query APIs, updates theme resources, and adds an app setting for slicer days. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
src/PlanViewer.App/Controls/TimeRangeSlicerControl.axaml.cs (1)
38-42: Consider unsubscribing from SizeChanged event.The
SizeChangedevent handler lambda is subscribed in the constructor but never unsubscribed. Consider using the control's lifecycle events to clean up, or use weak event patterns if the control could be disposed while the parent Border persists.🤖 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 38 - 42, The SizeChanged lambda subscribed in TimeRangeSlicerControl() via SlicerBorder.SizeChanged += (_, _) => Redraw() is never unsubscribed; change this to a named event handler (e.g., OnSlicerBorderSizeChanged) and unsubscribe it when the control is torn down (for Avalonia use Unloaded or override OnDetachedFromVisualTree/OnDetachedFromLogicalTree) or implement IDisposable to remove SlicerBorder.SizeChanged -= OnSlicerBorderSizeChanged so the handler is not kept alive after the control is disposed.src/PlanViewer.App/Controls/QueryStoreGridControl.axaml.cs (1)
55-56: Consider unsubscribing from event handler on control disposal.
TimeRangeSlicer.RangeChanged += OnTimeRangeChangedcreates a subscription but there's no corresponding unsubscription when the control is disposed. If the control can be recreated during the application lifecycle, this could cause a memory leak or duplicate event handling.♻️ Add cleanup on unload or implement IDisposable
public QueryStoreGridControl(ServerConnection serverConnection, ICredentialService credentialService, string initialDatabase, List<string> databases) { ... TimeRangeSlicer.RangeChanged += OnTimeRangeChanged; + this.Unloaded += (_, _) => TimeRangeSlicer.RangeChanged -= OnTimeRangeChanged; TimeRangeSlicer.IsExpanded = true;🤖 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 55 - 56, The control subscribes TimeRangeSlicer.RangeChanged += OnTimeRangeChanged but never unsubscribes, so add cleanup by either implementing IDisposable on the QueryStoreGridControl or handling the control's Unloaded event and calling TimeRangeSlicer.RangeChanged -= OnTimeRangeChanged; place the unsubscription in the Dispose/Unloaded handler to mirror the subscription and prevent memory leaks or duplicate calls; ensure the Unloaded/Dispose routine is wired up (e.g., register Unloaded in the constructor or implement Dispose and call it from parent) so OnTimeRangeChanged is reliably removed.src/PlanViewer.Core/Services/QueryStoreService.cs (2)
354-356: UnusedorderByMetricparameter.The
orderByMetricparameter is declared but never referenced in the SQL query or method body. The query always returns all metrics regardless of which one is passed. Either remove the parameter or use it to filter/sort the results.♻️ Option 1: Remove unused parameter
public static async Task<List<QueryStoreTimeSlice>> FetchTimeSliceDataAsync( - string connectionString, string orderByMetric = "cpu", + string connectionString, CancellationToken ct = default)🤖 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 354 - 356, The method FetchTimeSliceDataAsync declares orderByMetric but never uses it; either remove the parameter from the signature and update all callers, or apply it to the SQL query: modify the query built in FetchTimeSliceDataAsync (the command text / sql variable passed to SqlCommand) to accept a parameter (e.g., `@orderByMetric`) and use it to filter or order results (e.g., add "WHERE MetricName = `@orderByMetric`" or "ORDER BY CASE WHEN `@orderByMetric` = 'cpu' THEN cpu WHEN `@orderByMetric` = 'memory' THEN memory END") and add the parameter to the command (e.g., command.Parameters.AddWithValue("orderByMetric", orderByMetric)) to avoid SQL injection; keep the chosen behavior consistent with callers and update any unit tests.
374-401: Consider ordering ascending in SQL to avoid in-memory reversal.The query orders
DESCthen reverses the list in memory. You could simplify by orderingASCin the SQL query directly.♻️ Proposed change
-ORDER BY bucket_hour DESC;"; +ORDER BY bucket_hour ASC;"; ... - // Return in chronological order - rows.Reverse(); return rows;🤖 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 374 - 401, The SQL currently orders bucket_hour DESC and the code reads rows into List<QueryStoreTimeSlice> then calls rows.Reverse() to return chronological order; change the SQL ORDER BY clause to ASC (order by bucket_hour ASC) so rows are produced chronologically and remove the in-memory reversal (the rows.Reverse() call) in the method in QueryStoreService (the reader loop that builds QueryStoreTimeSlice and the subsequent rows.Reverse()). Ensure any comments about chronological order are updated if present.
🤖 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/TimeRangeSlicerControl.axaml.cs`:
- Around line 481-491: span may be zero causing a division-by-zero when
computing pivotInRange; before computing pivotInRange (using _rangeStart,
_rangeEnd, span, and MinNormInterval) guard against span == 0 (or <=
double.Epsilon) by replacing span with a safe non-zero value (e.g.
Math.Max(span, MinNormInterval) or set pivotInRange to 0.5 when span is zero) so
pivotInRange and subsequent newStart/newEnd calculations cannot divide by zero;
apply the guard immediately after computing span and use the guarded value for
pivotInRange and newSpan calculations.
In `@src/PlanViewer.App/Themes/DarkTheme.axaml`:
- Around line 55-66: SlicerHandleHoverBrush is defined but unused; either remove
the <SolidColorBrush x:Key="SlicerHandleHoverBrush" .../> resource or
document/attach it to the slicer handle hover state: delete the
SlicerHandleHoverBrush resource line if not needed, or add a comment above it
clarifying it’s reserved for future hover styling and update the Slicer handle
Style/VisualState (the control that uses
SlicerHandleBrush/SlicerHandleHoverBrush) to reference StaticResource
SlicerHandleHoverBrush for the PointerOver/MouseOver state.
---
Nitpick comments:
In `@src/PlanViewer.App/Controls/QueryStoreGridControl.axaml.cs`:
- Around line 55-56: The control subscribes TimeRangeSlicer.RangeChanged +=
OnTimeRangeChanged but never unsubscribes, so add cleanup by either implementing
IDisposable on the QueryStoreGridControl or handling the control's Unloaded
event and calling TimeRangeSlicer.RangeChanged -= OnTimeRangeChanged; place the
unsubscription in the Dispose/Unloaded handler to mirror the subscription and
prevent memory leaks or duplicate calls; ensure the Unloaded/Dispose routine is
wired up (e.g., register Unloaded in the constructor or implement Dispose and
call it from parent) so OnTimeRangeChanged is reliably removed.
In `@src/PlanViewer.App/Controls/TimeRangeSlicerControl.axaml.cs`:
- Around line 38-42: The SizeChanged lambda subscribed in
TimeRangeSlicerControl() via SlicerBorder.SizeChanged += (_, _) => Redraw() is
never unsubscribed; change this to a named event handler (e.g.,
OnSlicerBorderSizeChanged) and unsubscribe it when the control is torn down (for
Avalonia use Unloaded or override
OnDetachedFromVisualTree/OnDetachedFromLogicalTree) or implement IDisposable to
remove SlicerBorder.SizeChanged -= OnSlicerBorderSizeChanged so the handler is
not kept alive after the control is disposed.
In `@src/PlanViewer.Core/Services/QueryStoreService.cs`:
- Around line 354-356: The method FetchTimeSliceDataAsync declares orderByMetric
but never uses it; either remove the parameter from the signature and update all
callers, or apply it to the SQL query: modify the query built in
FetchTimeSliceDataAsync (the command text / sql variable passed to SqlCommand)
to accept a parameter (e.g., `@orderByMetric`) and use it to filter or order
results (e.g., add "WHERE MetricName = `@orderByMetric`" or "ORDER BY CASE WHEN
`@orderByMetric` = 'cpu' THEN cpu WHEN `@orderByMetric` = 'memory' THEN memory END")
and add the parameter to the command (e.g.,
command.Parameters.AddWithValue("orderByMetric", orderByMetric)) to avoid SQL
injection; keep the chosen behavior consistent with callers and update any unit
tests.
- Around line 374-401: The SQL currently orders bucket_hour DESC and the code
reads rows into List<QueryStoreTimeSlice> then calls rows.Reverse() to return
chronological order; change the SQL ORDER BY clause to ASC (order by bucket_hour
ASC) so rows are produced chronologically and remove the in-memory reversal (the
rows.Reverse() call) in the method in QueryStoreService (the reader loop that
builds QueryStoreTimeSlice and the subsequent rows.Reverse()). Ensure any
comments about chronological order are updated if present.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b6a2dc07-e3d3-4511-8a94-766513e95487
📒 Files selected for processing (7)
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/Themes/DarkTheme.axamlsrc/PlanViewer.Core/Models/QueryStoreTimeSlice.cssrc/PlanViewer.Core/Services/QueryStoreService.cs
| var span = _rangeEnd - _rangeStart; | ||
| var minInterval = MinNormInterval; | ||
|
|
||
| // Zoom in (wheel up) → shrink span; zoom out (wheel down) → expand span | ||
| var zoomFactor = e.Delta.Y > 0 ? 0.85 : 1.0 / 0.85; | ||
| var newSpan = Math.Clamp(span * zoomFactor, minInterval, 1.0); | ||
|
|
||
| // Keep the pivot point stable in the range | ||
| var pivotInRange = (pivot - _rangeStart) / span; | ||
| var newStart = pivot - pivotInRange * newSpan; | ||
| var newEnd = newStart + newSpan; |
There was a problem hiding this comment.
Potential division by zero if span is zero.
At line 489, span is used as a divisor. If _rangeStart equals _rangeEnd, this would cause a division by zero. While MinNormInterval should prevent this during normal operation, consider adding a guard.
🛡️ Proposed fix
var span = _rangeEnd - _rangeStart;
var minInterval = MinNormInterval;
+ if (span <= 0) return; // Guard against zero span
// Zoom in (wheel up) → shrink span; zoom out (wheel down) → expand span🤖 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 481
- 491, span may be zero causing a division-by-zero when computing pivotInRange;
before computing pivotInRange (using _rangeStart, _rangeEnd, span, and
MinNormInterval) guard against span == 0 (or <= double.Epsilon) by replacing
span with a safe non-zero value (e.g. Math.Max(span, MinNormInterval) or set
pivotInRange to 0.5 when span is zero) so pivotInRange and subsequent
newStart/newEnd calculations cannot divide by zero; apply the guard immediately
after computing span and use the guarded value for pivotInRange and newSpan
calculations.
| <!-- Time Range Slicer theme resources --> | ||
| <SolidColorBrush x:Key="SlicerBackgroundBrush" Color="#15171C"/> | ||
| <SolidColorBrush x:Key="SlicerChartLineBrush" Color="#2EAEF1"/> | ||
| <SolidColorBrush x:Key="SlicerChartFillBrush" Color="#332EAEF1"/> | ||
| <SolidColorBrush x:Key="SlicerOverlayBrush" Color="#99000000"/> | ||
| <SolidColorBrush x:Key="SlicerSelectedBrush" Color="#22FFFFFF"/> | ||
| <SolidColorBrush x:Key="SlicerHandleBrush" Color="#E4E6EB"/> | ||
| <SolidColorBrush x:Key="SlicerHandleHoverBrush" Color="#2EAEF1"/> | ||
| <SolidColorBrush x:Key="SlicerBorderBrush" Color="#3A3D45"/> | ||
| <SolidColorBrush x:Key="SlicerLabelBrush" Color="#99E4E6EB"/> | ||
| <SolidColorBrush x:Key="SlicerToggleBrush" Color="#E4E6EB"/> | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if SlicerHandleHoverBrush is used anywhere
rg -n "SlicerHandleHoverBrush" --type-add 'cs:*.cs' --type-add 'axaml:*.axaml' --type cs --type axamlRepository: erikdarlingdata/PerformanceStudio
Length of output: 191
Remove unused SlicerHandleHoverBrush resource or document its intended use.
The slicer theme resources are well-organized and consistently named. However, SlicerHandleHoverBrush (line 62) is defined but never used anywhere in the codebase. Either remove it or add a comment clarifying if it's reserved for a future hover state implementation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/PlanViewer.App/Themes/DarkTheme.axaml` around lines 55 - 66,
SlicerHandleHoverBrush is defined but unused; either remove the <SolidColorBrush
x:Key="SlicerHandleHoverBrush" .../> resource or document/attach it to the
slicer handle hover state: delete the SlicerHandleHoverBrush resource line if
not needed, or add a comment above it clarifying it’s reserved for future hover
styling and update the Slicer handle Style/VisualState (the control that uses
SlicerHandleBrush/SlicerHandleHoverBrush) to reference StaticResource
SlicerHandleHoverBrush for the PointerOver/MouseOver state.
erikdarlingdata
left a comment
erikdarlingdata
left a comment
There was a problem hiding this comment.
Thanks for the creative work on the slicer control — the interactive drag/zoom UX is well done. However, this PR has several issues that need to be addressed before it can be merged.
Must fix
-
Do not remove HoursBackBox or hardcode
hoursBack = 8760— The existing hours-back control limits the server-side query. Replacing it with a client-side slicer that fetches a full year of data will be extremely slow on production databases with large Query Store data sets. The slicer should be an additional client-side filter on top of the existing server-side time bound, not a replacement for it. Restore HoursBackBox. -
FetchTimeSliceDataAsynchas no time filter — The SQL scans ALL ofsys.query_store_runtime_statswith no WHERE clause on time. On databases with months of Query Store data, this is a heavy query. Add a time bound (e.g., match the hours-back value, or cap at 30 days). -
Do not auto-fetch on constructor —
Dispatcher.UIThread.Post(() => Fetch_Click(...))changes the existing behavior where users set up filters before clicking Fetch. This is a UX change that wasn't discussed. Remove the auto-fetch. -
Restore hoursBack parameter on ViewHistory_Click — The PR removes the hours-back pass-through to the history window, silently defaulting to 24. The user's chosen value should still be passed.
-
Slicer filters by
LastExecutedUtcbut chart shows interval start times — These are different times. A query's last execution time can be well after the interval start. This mismatch means the slicer's visual range won't accurately reflect what gets filtered in/out of the grid. Consider filtering by the same time dimension the chart displays.
Suggestions
-
orderByMetricparameter is accepted but unused inFetchTimeSliceDataAsync— either use it or remove it. -
Errors silently swallowed in
LoadTimeSlicerDataAsync— at minimum surface a status message if the slicer data fails to load. -
Slicer placement above the toolbar is unusual — most apps put the toolbar at the top. Consider placing the slicer between the toolbar and the grid, or making it collapsible (you already have the toggle, which is good).
The slicer control itself is solid work — just needs to be integrated without removing existing functionality.
- 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
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
src/PlanViewer.Core/Services/QueryStoreService.cs (4)
46-50: Consider placingCancellationTokenas the last parameter.By convention,
CancellationTokenis typically the final parameter in async method signatures. The new optionalstartUtc/endUtcparameters are placed after it, which works but may be unexpected for callers.Suggested parameter order
public static async Task<List<QueryStorePlan>> FetchTopPlansAsync( string connectionString, int topN = 25, string orderBy = "cpu", int hoursBack = 24, QueryStoreFilter? filter = null, - CancellationToken ct = default, - DateTime? startUtc = null, DateTime? endUtc = null) + DateTime? startUtc = null, DateTime? endUtc = null, + CancellationToken ct = default)🤖 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 46 - 50, The method signature for FetchTopPlansAsync places CancellationToken before the optional startUtc and endUtc parameters which breaks usual convention; update the FetchTopPlansAsync declaration so CancellationToken ct is the final parameter (move ct to after DateTime? endUtc) and adjust any call sites accordingly to preserve behavior and default values, ensuring the method overload/usage still compiles and tests pass.
132-143: ParameterizehoursBackfor consistency and consider validating partial range inputs.Two observations:
The
startUtc/endUtcbranch correctly uses parameters, but the fallback interpolateshoursBackdirectly into SQL. While safe forint, this inconsistency could lead to copy-paste issues. Parameterizing maintains a uniform pattern.If a caller provides only
startUtcor onlyendUtc, the code silently falls back tohoursBack. Consider validating or documenting this behavior.Parameterize hoursBack
if (startUtc.HasValue && endUtc.HasValue) { timeWhereClause = "WHERE rs.last_execution_time >= `@rangeStart` AND rs.last_execution_time < `@rangeEnd`"; parameters.Add(new SqlParameter("@rangeStart", startUtc.Value)); parameters.Add(new SqlParameter("@rangeEnd", endUtc.Value)); } else { - timeWhereClause = $"WHERE rs.last_execution_time >= DATEADD(HOUR, -{hoursBack}, GETUTCDATE())"; + timeWhereClause = "WHERE rs.last_execution_time >= DATEADD(HOUR, -@hoursBack, GETUTCDATE())"; + parameters.Add(new SqlParameter("@hoursBack", hoursBack)); }🤖 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 132 - 143, The time-range branch uses parameters for startUtc/endUtc but falls back to string-interpolating hoursBack in timeWhereClause; change the fallback to add a SqlParameter (e.g., parameters.Add(new SqlParameter("@hoursBack", hoursBack))) and use a parameterized clause "WHERE rs.last_execution_time >= DATEADD(HOUR, -@hoursBack, GETUTCDATE())" to make timeWhereClause consistent with the startUtc/endUtc branch; also add a simple validation in the method to detect partial ranges (when only startUtc or only endUtc is supplied) and either throw an ArgumentException or explicitly document/handle the behavior so callers know that both must be provided or neither (refer to symbols startUtc, endUtc, hoursBack, timeWhereClause, parameters, SqlParameter).
390-392: ParameterizedaysBackfor consistency with other methods.Similar to the earlier comment,
daysBackis interpolated directly into SQL. While safe forint, parameterizing maintains consistency.Parameterize daysBack
- var sql = $@" + const string sql = @" SET TRANSACTION ISOLATION LEVEL READ UNCOMMITTED; ... -WHERE rsi.start_time >= DATEADD(DAY, -{daysBack}, GETUTCDATE()) +WHERE rsi.start_time >= DATEADD(DAY, -@daysBack, GETUTCDATE()) ..."; await using var cmd = new SqlCommand(sql, conn) { CommandTimeout = 120 }; + cmd.Parameters.Add(new SqlParameter("@daysBack", daysBack)); await using var reader = await cmd.ExecuteReaderAsync(ct);🤖 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 390 - 392, The SQL in QueryStoreService that currently injects daysBack directly into the string (the fragment using DATEADD(DAY, -{daysBack}, GETUTCDATE())) should be changed to use a parameter (e.g., DATEADD(DAY, -@daysBack, GETUTCDATE())); update the method that builds/executesthat query in QueryStoreService to add a corresponding SqlParameter/DbParameter for "@daysBack" (or pass daysBack into the existing parameter collection) and remove the string interpolation; ensure the parameter is of the correct integer type and used wherever this SQL is executed (same pattern as other parameterized methods in this class).
392-419: Avoid in-memory reversal by usingORDER BY ASCin SQL.The query orders descending then reverses in memory. Ordering ascending in SQL eliminates the
Reverse()call and slightly reduces memory operations.Order chronologically in SQL
GROUP BY DATEADD(HOUR, DATEDIFF(HOUR, 0, rsi.start_time), 0) -ORDER BY bucket_hour DESC;"; +ORDER BY bucket_hour ASC;"; ... - // Return in chronological order - rows.Reverse(); return rows;🤖 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 392 - 419, The code in QueryStoreService (the reader loop that builds rows and currently calls rows.Reverse()) orders the SQL result DESC then reverses in memory; change the SQL ORDER BY from "bucket_hour DESC" to "bucket_hour ASC" (in the same SqlCommand/sql variable used to create the reader) and remove the rows.Reverse() call, and adjust the comment that currently says "Return in chronological order" since the DB will already return chronological order.
🤖 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.Core/Services/QueryStoreService.cs`:
- Around line 370-373: The parameter orderByMetric on FetchTimeSliceDataAsync is
unused; either remove it from the method signature and all callers (search for
FetchTimeSliceDataAsync) or implement its intended behavior by applying
metric-based sorting/filtering inside FetchTimeSliceDataAsync (e.g., map allowed
metric names like "cpu","io","memory" to the corresponding data field and apply
an ORDER BY in the query or a LINQ OrderBy on the loaded
List<QueryStoreTimeSlice> before returning); ensure you validate the metric
value and update callers and any interface/overloads accordingly.
---
Nitpick comments:
In `@src/PlanViewer.Core/Services/QueryStoreService.cs`:
- Around line 46-50: The method signature for FetchTopPlansAsync places
CancellationToken before the optional startUtc and endUtc parameters which
breaks usual convention; update the FetchTopPlansAsync declaration so
CancellationToken ct is the final parameter (move ct to after DateTime? endUtc)
and adjust any call sites accordingly to preserve behavior and default values,
ensuring the method overload/usage still compiles and tests pass.
- Around line 132-143: The time-range branch uses parameters for startUtc/endUtc
but falls back to string-interpolating hoursBack in timeWhereClause; change the
fallback to add a SqlParameter (e.g., parameters.Add(new
SqlParameter("@hoursBack", hoursBack))) and use a parameterized clause "WHERE
rs.last_execution_time >= DATEADD(HOUR, -@hoursBack, GETUTCDATE())" to make
timeWhereClause consistent with the startUtc/endUtc branch; also add a simple
validation in the method to detect partial ranges (when only startUtc or only
endUtc is supplied) and either throw an ArgumentException or explicitly
document/handle the behavior so callers know that both must be provided or
neither (refer to symbols startUtc, endUtc, hoursBack, timeWhereClause,
parameters, SqlParameter).
- Around line 390-392: The SQL in QueryStoreService that currently injects
daysBack directly into the string (the fragment using DATEADD(DAY, -{daysBack},
GETUTCDATE())) should be changed to use a parameter (e.g., DATEADD(DAY,
-@daysBack, GETUTCDATE())); update the method that builds/executesthat query in
QueryStoreService to add a corresponding SqlParameter/DbParameter for
"@daysBack" (or pass daysBack into the existing parameter collection) and remove
the string interpolation; ensure the parameter is of the correct integer type
and used wherever this SQL is executed (same pattern as other parameterized
methods in this class).
- Around line 392-419: The code in QueryStoreService (the reader loop that
builds rows and currently calls rows.Reverse()) orders the SQL result DESC then
reverses in memory; change the SQL ORDER BY from "bucket_hour DESC" to
"bucket_hour ASC" (in the same SqlCommand/sql variable used to create the
reader) and remove the rows.Reverse() call, and adjust the comment that
currently says "Return in chronological order" since the DB will already return
chronological order.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ba568af9-04d8-4446-9711-685b9d8405ff
📒 Files selected for processing (4)
src/PlanViewer.App/Controls/QueryStoreGridControl.axaml.cssrc/PlanViewer.App/Controls/TimeRangeSlicerControl.axaml.cssrc/PlanViewer.App/Services/AppSettingsService.cssrc/PlanViewer.Core/Services/QueryStoreService.cs
✅ Files skipped from review due to trivial changes (1)
- src/PlanViewer.App/Services/AppSettingsService.cs
🚧 Files skipped from review as they are similar to previous changes (2)
- src/PlanViewer.App/Controls/TimeRangeSlicerControl.axaml.cs
- src/PlanViewer.App/Controls/QueryStoreGridControl.axaml.cs
| public static async Task<List<QueryStoreTimeSlice>> FetchTimeSliceDataAsync( | ||
| string connectionString, string orderByMetric = "cpu", | ||
| int daysBack = 30, | ||
| CancellationToken ct = default) |
There was a problem hiding this comment.
Unused parameter orderByMetric.
The orderByMetric parameter is declared but never referenced in the method body. This appears to be dead code or an incomplete implementation. Either remove it or implement the intended metric-based filtering/sorting.
If unused, remove the parameter
public static async Task<List<QueryStoreTimeSlice>> FetchTimeSliceDataAsync(
- string connectionString, string orderByMetric = "cpu",
+ string connectionString,
int daysBack = 30,
CancellationToken ct = default)📝 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.
| public static async Task<List<QueryStoreTimeSlice>> FetchTimeSliceDataAsync( | |
| string connectionString, string orderByMetric = "cpu", | |
| int daysBack = 30, | |
| CancellationToken ct = default) | |
| public static async Task<List<QueryStoreTimeSlice>> FetchTimeSliceDataAsync( | |
| string connectionString, | |
| int daysBack = 30, | |
| CancellationToken ct = default) |
🤖 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 370 - 373,
The parameter orderByMetric on FetchTimeSliceDataAsync is unused; either remove
it from the method signature and all callers (search for
FetchTimeSliceDataAsync) or implement its intended behavior by applying
metric-based sorting/filtering inside FetchTimeSliceDataAsync (e.g., map allowed
metric names like "cpu","io","memory" to the corresponding data field and apply
an ORDER BY in the query or a LINQ OrderBy on the loaded
List<QueryStoreTimeSlice> before returning); ensure you validate the metric
value and update callers and any interface/overloads accordingly.
After the first openning, the user would be able to enlarge or move the range. I removed the dropdown filter with the time filter is removed because it interfere with the RangeFilter and i think the RangeFilter do the same job better.
I still have some "strange" behavior when moving the Range filter You can close this PR and i will propose a new one but before i would like to be OK with you for:
Preview :
|
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
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
src/PlanViewer.App/Controls/TimeRangeSlicerControl.axaml.cs (1)
503-512:⚠️ Potential issue | 🟡 MinorGuard the wheel-zoom math against a collapsed selection.
If a restored selection clamps both ends to the same normalized value,
pivotInRangedivides by zero here. That can happen when an older selection is reapplied to a different slicer timespan.🛡️ Proposed fix
var span = _rangeEnd - _rangeStart; var minInterval = MinNormInterval; + if (span <= 0) + return; // Zoom in (wheel up) → shrink span; zoom out (wheel down) → expand span🤖 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 503 - 512, The zoom math divides by span which can be zero when a restored selection collapsed both ends; update the wheel-zoom handler so before computing pivotInRange you guard against a zero or near-zero span (use _rangeEnd, _rangeStart, MinNormInterval and span) — e.g., if span <= 0 or below a small epsilon, set span = Math.Max(span, MinNormInterval) or treat pivotInRange as 0 (no pivoting) so the division is avoided and newStart/newSpan remain valid. Ensure the guard is applied immediately prior to calculating pivotInRange and affects subsequent newSpan/newStart calculations.
🤖 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: BuildSearchFilter() is computed into the local variable
filter but never used, so range fetches call
QueryStoreService.FetchTopPlansAsync without the search filter; update the
FetchTopPlansAsync call in the method (the invocation that currently passes
_connectionString, topN, orderBy, ct: ct, startUtc: _slicerStartUtc, endUtc:
_slicerEndUtc) to include the built filter variable (filter) so the
QueryStoreService.FetchTopPlansAsync receives and applies the search filter
(preserving topN, orderBy, ct, startUtc, endUtc arguments).
In `@src/PlanViewer.App/Controls/TimeRangeSlicerControl.axaml.cs`:
- Around line 102-108: GetDateTimeAtNorm() maps normalized positions by actual
timestamps, but Redraw() currently spaces plotted points by index (i * stepX),
causing visual drift when hours are missing; update Redraw() (and the similar
code referenced around lines 189-199) to compute each point's x using its
IntervalStartUtc (converted to the same normalized/ticks scale used by
GetDateTimeAtNorm) or alternatively zero-fill missing hourly buckets before
plotting so indices align with wall-clock hours; adjust any stepX/normalization
math to use IntervalStartUtc.Ticks (or a common epoch) so plotted series matches
the slicer’s UTC window.
- Around line 138-149: GetMetricValues() is aliasing "avg-*" modes to the same
Total* series (using _data.Select(d => d.TotalCpu) etc.), so the slicer still
draws totals and misleads the visual cue; update GetMetricValues() to compute
per-bucket averages for avg-* modes (e.g., map "avg-cpu" to _data.Select(d =>
d.TotalExecutions > 0 ? d.TotalCpu / d.TotalExecutions : 0.0).ToArray(),
similarly for avg-duration, avg-reads, avg-writes, avg-physical-reads,
avg-memory) and keep non-avg modes returning the Total* series as before; ensure
you reference _metric and _data and guard against division by zero when
accessing TotalExecutions.
In `@src/PlanViewer.Core/Services/QueryStoreService.cs`:
- Around line 365-392: Clamp and sanitize the daysBack input in
FetchTimeSliceDataAsync: ensure daysBack is bounded to a safe range (e.g., at
least 1 and at most 30) and compute a local int clampedDays =
Math.Clamp(daysBack, 1, 30) (or Math.Min/Max if Math.Clamp unavailable); use
clampedDays in the SQL instead of daysBack, add an upper row cap by changing the
SELECT to SELECT TOP (1000) to enforce the 1000 hourly-bucket limit, and ensure
the WHERE uses rsi.start_time BETWEEN DATEADD(DAY, -clampedDays, GETUTCDATE())
AND GETUTCDATE() to avoid future windows. Update references in the method
(FetchTimeSliceDataAsync, daysBack, sql) accordingly.
- Around line 132-143: FetchTopPlansAsync uses rs.last_execution_time while
slicer buckets on rsi.start_time, causing off-by-window-edge mismatches; change
the time filter in FetchTopPlansAsync to use rsi.start_time (keep the same
parameter names `@rangeStart/`@rangeEnd and add SqlParameter values as before) so
both queries align on interval start, and ensure the alternate hoursBack branch
still computes the same reference (DATEADD against GETUTCDATE()) but applied to
rsi.start_time. Also add validation in FetchTimeSliceDataAsync to enforce the
documented 30-day cap on daysBack (clamp or reject values > 30) so callers
cannot request unbounded history; update any related variable/hardcode names
accordingly (e.g., daysBack, hoursBack, FetchTimeSliceDataAsync,
FetchTopPlansAsync, timeWhereClause).
---
Duplicate comments:
In `@src/PlanViewer.App/Controls/TimeRangeSlicerControl.axaml.cs`:
- Around line 503-512: The zoom math divides by span which can be zero when a
restored selection collapsed both ends; update the wheel-zoom handler so before
computing pivotInRange you guard against a zero or near-zero span (use
_rangeEnd, _rangeStart, MinNormInterval and span) — e.g., if span <= 0 or below
a small epsilon, set span = Math.Max(span, MinNormInterval) or treat
pivotInRange as 0 (no pivoting) so the division is avoided and newStart/newSpan
remain valid. Ensure the guard is applied immediately prior to calculating
pivotInRange and affects subsequent newSpan/newStart calculations.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8ddc4f99-3f36-41af-9222-cda1e9c79e45
📒 Files selected for processing (4)
src/PlanViewer.App/Controls/QueryStoreGridControl.axamlsrc/PlanViewer.App/Controls/QueryStoreGridControl.axaml.cssrc/PlanViewer.App/Controls/TimeRangeSlicerControl.axaml.cssrc/PlanViewer.Core/Services/QueryStoreService.cs
🚧 Files skipped from review as they are similar to previous changes (1)
- src/PlanViewer.App/Controls/QueryStoreGridControl.axaml
| 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 new range fetch.
BuildSearchFilter() is computed and then ignored, so every slicer-driven fetch now returns the full result set. Query ID / plan ID / hash / module searches stop working on the new path, and invalid IDs currently fall through to an unfiltered fetch.
🐛 Proposed fix
var topN = (int)(TopNBox.Value ?? 25);
var orderBy = _lastFetchedOrderBy;
var filter = BuildSearchFilter();
+ if (!string.IsNullOrWhiteSpace(SearchValueBox.Text) && filter == null)
+ return;
FetchButton.IsEnabled = false;
LoadButton.IsEnabled = false;
StatusText.Text = "Fetching plans...";
_rows.Clear();
_filteredRows.Clear();
try
{
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, BuildSearchFilter() is computed into the local variable filter but never
used, so range fetches call QueryStoreService.FetchTopPlansAsync without the
search filter; update the FetchTopPlansAsync call in the method (the invocation
that currently passes _connectionString, topN, orderBy, ct: ct, startUtc:
_slicerStartUtc, endUtc: _slicerEndUtc) to include the built filter variable
(filter) so the QueryStoreService.FetchTopPlansAsync receives and applies the
search filter (preserving topN, orderBy, ct, startUtc, endUtc arguments).
| private DateTime GetDateTimeAtNorm(double norm) | ||
| { | ||
| if (_data.Count == 0) return DateTime.UtcNow; | ||
| var first = _data[0].IntervalStartUtc; | ||
| var last = _data[^1].IntervalStartUtc.AddHours(1); | ||
| var ticks = first.Ticks + (long)((last.Ticks - first.Ticks) * norm); | ||
| return new DateTime(Math.Clamp(ticks, first.Ticks, last.Ticks), DateTimeKind.Utc); |
There was a problem hiding this comment.
Don't space buckets by index when the selection maps by wall-clock time.
GetDateTimeAtNorm() converts handle position using actual timestamps, but Redraw() places every point at i * stepX. As soon as an hour is missing from the Query Store data, the chart compresses that gap and the plotted series drifts away from the UTC window the slicer reports and fetches. Either zero-fill missing hourly buckets or compute x from IntervalStartUtc.
Also applies to: 189-199
🤖 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 102
- 108, GetDateTimeAtNorm() maps normalized positions by actual timestamps, but
Redraw() currently spaces plotted points by index (i * stepX), causing visual
drift when hours are missing; update Redraw() (and the similar code referenced
around lines 189-199) to compute each point's x using its IntervalStartUtc
(converted to the same normalized/ticks scale used by GetDateTimeAtNorm) or
alternatively zero-fill missing hourly buckets before plotting so indices align
with wall-clock hours; adjust any stepX/normalization math to use
IntervalStartUtc.Ticks (or a common epoch) so plotted series matches the
slicer’s UTC window.
| 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(), |
There was a problem hiding this comment.
avg-* modes still draw total series.
avg-cpu, avg-duration, avg-reads, etc. are all mapped to QueryStoreTimeSlice.Total*, and the label is still "Total ...". When the grid is sorted by an average metric, the slicer background no longer reflects the selected metric, so the visual cue is misleading. Either add per-bucket averages to the slicer data or stop aliasing the average modes here.
Also applies to: 153-165
🤖 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
- 149, GetMetricValues() is aliasing "avg-*" modes to the same Total* series
(using _data.Select(d => d.TotalCpu) etc.), so the slicer still draws totals and
misleads the visual cue; update GetMetricValues() to compute per-bucket averages
for avg-* modes (e.g., map "avg-cpu" to _data.Select(d => d.TotalExecutions > 0
? d.TotalCpu / d.TotalExecutions : 0.0).ToArray(), similarly for avg-duration,
avg-reads, avg-writes, avg-physical-reads, avg-memory) and keep non-avg modes
returning the Total* series as before; ensure you reference _metric and _data
and guard against division by zero when accessing TotalExecutions.
| // Time-range filter: prefer explicit start/end over hoursBack | ||
| string timeWhereClause; | ||
| if (startUtc.HasValue && endUtc.HasValue) | ||
| { | ||
| timeWhereClause = "WHERE rs.last_execution_time >= @rangeStart AND rs.last_execution_time < @rangeEnd"; | ||
| parameters.Add(new SqlParameter("@rangeStart", startUtc.Value)); | ||
| parameters.Add(new SqlParameter("@rangeEnd", endUtc.Value)); | ||
| } | ||
| else | ||
| { | ||
| timeWhereClause = $"WHERE rs.last_execution_time >= DATEADD(HOUR, -{hoursBack}, GETUTCDATE())"; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
wc -l src/PlanViewer.Core/Services/QueryStoreService.csRepository: erikdarlingdata/PerformanceStudio
Length of output: 130
🏁 Script executed:
# Get the API signature at lines 44-50
sed -n '44,50p' src/PlanViewer.Core/Services/QueryStoreService.csRepository: erikdarlingdata/PerformanceStudio
Length of output: 529
🏁 Script executed:
# Get the time-range filter code at lines 132-143
sed -n '132,143p' src/PlanViewer.Core/Services/QueryStoreService.csRepository: erikdarlingdata/PerformanceStudio
Length of output: 667
🏁 Script executed:
# Get the XML comment and FetchTimeSliceDataAsync at lines 365-392
sed -n '365,392p' src/PlanViewer.Core/Services/QueryStoreService.csRepository: erikdarlingdata/PerformanceStudio
Length of output: 1555
🏁 Script executed:
# Get the context around lines 387-391
sed -n '380,400p' src/PlanViewer.Core/Services/QueryStoreService.csRepository: erikdarlingdata/PerformanceStudio
Length of output: 1230
Align time column semantics between slicer and plan fetch.
The slicer buckets data by rsi.start_time (interval start) but FetchTopPlansAsync filters on rs.last_execution_time (actual execution time). When the UI passes a time range from the slicer to FetchTopPlansAsync, the two queries may exclude different rows at the window edges because they reference different time columns.
Additionally, FetchTimeSliceDataAsync does not validate daysBack against its documented 30-day limit, allowing callers to bypass the stated constraint and retrieve unbounded historical data.
🤖 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 132 - 143,
FetchTopPlansAsync uses rs.last_execution_time while slicer buckets on
rsi.start_time, causing off-by-window-edge mismatches; change the time filter in
FetchTopPlansAsync to use rsi.start_time (keep the same parameter names
`@rangeStart/`@rangeEnd and add SqlParameter values as before) so both queries
align on interval start, and ensure the alternate hoursBack branch still
computes the same reference (DATEADD against GETUTCDATE()) but applied to
rsi.start_time. Also add validation in FetchTimeSliceDataAsync to enforce the
documented 30-day cap on daysBack (clamp or reject values > 30) so callers
cannot request unbounded history; update any related variable/hardcode names
accordingly (e.g., daysBack, hoursBack, FetchTimeSliceDataAsync,
FetchTopPlansAsync, timeWhereClause).
| /// <summary> | ||
| /// Fetches hourly-aggregated metric data for the time-range slicer. | ||
| /// Limits data to the last <paramref name="daysBack"/> days (default 30). | ||
| /// Returns up to 1000 hourly buckets in chronological order. | ||
| /// </summary> | ||
| public static async Task<List<QueryStoreTimeSlice>> FetchTimeSliceDataAsync( | ||
| string connectionString, string orderByMetric = "cpu", | ||
| int daysBack = 30, | ||
| CancellationToken ct = default) | ||
| { | ||
| var sql = $@" | ||
| SET TRANSACTION ISOLATION LEVEL READ UNCOMMITTED; | ||
|
|
||
| SELECT | ||
| DATEADD(HOUR, DATEDIFF(HOUR, 0, rsi.start_time), 0) AS bucket_hour, | ||
| SUM(rs.avg_cpu_time * rs.count_executions) / 1000.0 AS total_cpu_ms, | ||
| SUM(rs.avg_duration * rs.count_executions) / 1000.0 AS total_duration_ms, | ||
| SUM(rs.avg_logical_io_reads * rs.count_executions) AS total_reads, | ||
| SUM(rs.avg_logical_io_writes * rs.count_executions) AS total_writes, | ||
| SUM(rs.avg_physical_io_reads * rs.count_executions) AS total_physical_reads, | ||
| SUM(rs.avg_query_max_used_memory * rs.count_executions) * 8.0 / 1024.0 AS total_memory_mb, | ||
| SUM(rs.count_executions) AS total_executions | ||
| FROM sys.query_store_runtime_stats rs | ||
| JOIN sys.query_store_runtime_stats_interval rsi | ||
| ON rs.runtime_stats_interval_id = rsi.runtime_stats_interval_id | ||
| WHERE rsi.start_time >= DATEADD(DAY, -{daysBack}, GETUTCDATE()) | ||
| GROUP BY DATEADD(HOUR, DATEDIFF(HOUR, 0, rsi.start_time), 0) | ||
| ORDER BY bucket_hour DESC;"; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Enforce the documented slicer cap instead of trusting daysBack.
This method says it is limited to the last 30 days / 1000 hourly buckets, but the query currently accepts any value that comes from settings. A large or negative daysBack can either force a much heavier Query Store scan than intended or produce an empty future-looking window.
🛡️ Proposed fix
public static async Task<List<QueryStoreTimeSlice>> FetchTimeSliceDataAsync(
string connectionString, string orderByMetric = "cpu",
int daysBack = 30,
CancellationToken ct = default)
{
+ daysBack = Math.Clamp(daysBack, 1, 30);
+
var sql = $@"
SET TRANSACTION ISOLATION LEVEL READ UNCOMMITTED;📝 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.
| /// <summary> | |
| /// Fetches hourly-aggregated metric data for the time-range slicer. | |
| /// Limits data to the last <paramref name="daysBack"/> days (default 30). | |
| /// Returns up to 1000 hourly buckets in chronological order. | |
| /// </summary> | |
| public static async Task<List<QueryStoreTimeSlice>> FetchTimeSliceDataAsync( | |
| string connectionString, string orderByMetric = "cpu", | |
| int daysBack = 30, | |
| CancellationToken ct = default) | |
| { | |
| var sql = $@" | |
| SET TRANSACTION ISOLATION LEVEL READ UNCOMMITTED; | |
| SELECT | |
| DATEADD(HOUR, DATEDIFF(HOUR, 0, rsi.start_time), 0) AS bucket_hour, | |
| SUM(rs.avg_cpu_time * rs.count_executions) / 1000.0 AS total_cpu_ms, | |
| SUM(rs.avg_duration * rs.count_executions) / 1000.0 AS total_duration_ms, | |
| SUM(rs.avg_logical_io_reads * rs.count_executions) AS total_reads, | |
| SUM(rs.avg_logical_io_writes * rs.count_executions) AS total_writes, | |
| SUM(rs.avg_physical_io_reads * rs.count_executions) AS total_physical_reads, | |
| SUM(rs.avg_query_max_used_memory * rs.count_executions) * 8.0 / 1024.0 AS total_memory_mb, | |
| SUM(rs.count_executions) AS total_executions | |
| FROM sys.query_store_runtime_stats rs | |
| JOIN sys.query_store_runtime_stats_interval rsi | |
| ON rs.runtime_stats_interval_id = rsi.runtime_stats_interval_id | |
| WHERE rsi.start_time >= DATEADD(DAY, -{daysBack}, GETUTCDATE()) | |
| GROUP BY DATEADD(HOUR, DATEDIFF(HOUR, 0, rsi.start_time), 0) | |
| ORDER BY bucket_hour DESC;"; | |
| /// <summary> | |
| /// Fetches hourly-aggregated metric data for the time-range slicer. | |
| /// Limits data to the last <paramref name="daysBack"/> days (default 30). | |
| /// Returns up to 1000 hourly buckets in chronological order. | |
| /// </summary> | |
| public static async Task<List<QueryStoreTimeSlice>> FetchTimeSliceDataAsync( | |
| string connectionString, string orderByMetric = "cpu", | |
| int daysBack = 30, | |
| CancellationToken ct = default) | |
| { | |
| daysBack = Math.Clamp(daysBack, 1, 30); | |
| var sql = $@" | |
| SET TRANSACTION ISOLATION LEVEL READ UNCOMMITTED; | |
| SELECT | |
| DATEADD(HOUR, DATEDIFF(HOUR, 0, rsi.start_time), 0) AS bucket_hour, | |
| SUM(rs.avg_cpu_time * rs.count_executions) / 1000.0 AS total_cpu_ms, | |
| SUM(rs.avg_duration * rs.count_executions) / 1000.0 AS total_duration_ms, | |
| SUM(rs.avg_logical_io_reads * rs.count_executions) AS total_reads, | |
| SUM(rs.avg_logical_io_writes * rs.count_executions) AS total_writes, | |
| SUM(rs.avg_physical_io_reads * rs.count_executions) AS total_physical_reads, | |
| SUM(rs.avg_query_max_used_memory * rs.count_executions) * 8.0 / 1024.0 AS total_memory_mb, | |
| SUM(rs.count_executions) AS total_executions | |
| FROM sys.query_store_runtime_stats rs | |
| JOIN sys.query_store_runtime_stats_interval rsi | |
| ON rs.runtime_stats_interval_id = rsi.runtime_stats_interval_id | |
| WHERE rsi.start_time >= DATEADD(DAY, -{daysBack}, GETUTCDATE()) | |
| GROUP BY DATEADD(HOUR, DATEDIFF(HOUR, 0, rsi.start_time), 0) | |
| ORDER BY bucket_hour DESC;"; |
🤖 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 365 - 392,
Clamp and sanitize the daysBack input in FetchTimeSliceDataAsync: ensure
daysBack is bounded to a safe range (e.g., at least 1 and at most 30) and
compute a local int clampedDays = Math.Clamp(daysBack, 1, 30) (or Math.Min/Max
if Math.Clamp unavailable); use clampedDays in the SQL instead of daysBack, add
an upper row cap by changing the SELECT to SELECT TOP (1000) to enforce the 1000
hourly-bucket limit, and ensure the WHERE uses rsi.start_time BETWEEN
DATEADD(DAY, -clampedDays, GETUTCDATE()) AND GETUTCDATE() to avoid future
windows. Update references in the method (FetchTimeSliceDataAsync, daysBack,
sql) accordingly.
|
@rferraton My experience with Query Store seems quite different from yours. I often see ones that are 5-10GB and quite difficult to get data out of quickly. A lot of that informed the way that I wrote
I'll close this out as requested now. As always, thanks for your time and contributions. Thanks, |
2 participants
What does this PR do?
Add a time range filter in the query store with the selected sorting metric as background of the time range graphic selector
Which component(s) does this affect?
How was this tested?
Checklist
dotnet build -c Debug)dotnet test)Summary by CodeRabbit
New Features
Chores