Feature/growth rates vlf count#608
Closed
ClaudioESSilva wants to merge 11 commits intoerikdarlingdata:devfrom
Closed
Feature/growth rates vlf count#608ClaudioESSilva wants to merge 11 commits intoerikdarlingdata:devfrom
ClaudioESSilva wants to merge 11 commits intoerikdarlingdata:devfrom
Conversation
Contributor
📝 WalkthroughWalkthroughAdds auto-growth and VLF-count metrics end-to-end (collection SQL, DB schema, upgrade), surfaces them in data models and UI, and introduces per-column DataGrid filtering infrastructure and filter UI for the Database Sizes / FinOps views. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Dashboard/Controls/FinOpsContent.xaml (1)
786-937:⚠️ Potential issue | 🟠 MajorAdd accessible names for per-column filter buttons.
The textless filter buttons at lines 786-937 are icon-only controls that lack accessible names. The
ColumnFilterButtonStyle(defined in Dashboard/Themes/LightTheme.xaml and DarkTheme.xaml around lines 379-410) does not injectAutomationProperties.NameorToolTipproperties, and the individual buttons do not define these properties either. Screen readers will announce these as generic buttons without context, making them ambiguous to users with assistive technologies.Each button should have a column-specific accessible name (e.g., via
AutomationProperties.Name="{Binding ColumnName, StringFormat='Filter {0} column'}"or by updating the style to derive accessible names from the adjacentTextBlockcontent).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Dashboard/Controls/FinOpsContent.xaml` around lines 786 - 937, The filter buttons are icon-only and lack accessible names; update them so screen readers get a meaningful label: add an AutomationProperties.Name (or ToolTip) to each Button that derives the column-specific text (e.g., "Filter Database column") by binding to the Button.Tag or the adjacent TextBlock content, or modify the ColumnFilterButtonStyle to set AutomationProperties.Name using TemplateBinding to Tag (or a passed-in property) so all buttons (e.g., those with Tag="DatabaseName", "FileTypeDesc", "TotalSizeMb", etc.) expose a column-specific accessible name while keeping the existing Click handler DatabaseSizesFilter_Click unchanged.
🧹 Nitpick comments (2)
Lite/Services/RemoteCollectorService.DatabaseSize.cs (1)
119-125: Computevlf_countonce per database instead of per row.
(SELECT COUNT(*) FROM sys.dm_db_log_info(...))is currently in the row projection, so it can be repeated for each matching file row. Precomputing bydatabase_id(CTE/APPLY) will reduce redundant DMV calls.Also applies to: 178-184
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Lite/Services/RemoteCollectorService.DatabaseSize.cs` around lines 119 - 125, The vlf_count is being computed per file row by calling (SELECT COUNT(*) FROM sys.dm_db_log_info(mf.database_id)) inside the projection; change this to compute vlf_count once per database_id (e.g., a CTE or CROSS APPLY that selects database_id, COUNT(*) AS vlf_count FROM sys.dm_db_log_info(...) GROUP BY database_id) and then join/apply that result into the main query so the projection can reference the precomputed vlf_count; update the CASE for vlf_count (and the duplicate occurrence around the other projection) to use the joined vlf_count when mf.type = 1 and NULL otherwise.Dashboard/Services/DatabaseService.FinOps.cs (1)
2364-2382: Keep the growth/VLF presentation rules in one place.This block is now mirrored in
Lite/Services/LocalDataService.FinOps.cs:2054-2072. Pulling the formatting/sort rules into a shared helper or value object would make it much harder for the Dashboard and Lite views to drift the next time these display semantics change.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Dashboard/Services/DatabaseService.FinOps.cs` around lines 2364 - 2382, The presentation and sorting logic for growth and VLF (GrowthDisplay, AutoGrowthSort, VlfCountDisplay, VlfCountSort) is duplicated in Dashboard/Services/DatabaseService.FinOps.cs and Lite/Services/LocalDataService.FinOps.cs; extract these rules into a single shared helper/value object (e.g., FileGrowthDisplayHelper or FileGrowthViewModel) that exposes the same computed properties (GrowthDisplay, AutoGrowthSort, VlfCountDisplay, VlfCountSort) and reuse it from both DatabaseService and LocalDataService to centralize formatting/sort semantics and eliminate duplication.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@install/52_collect_database_size_stats.sql`:
- Around line 213-218: The vlf_count scalar subquery calling
sys.dm_db_log_info(DB_ID()) can fail if the DMV is unavailable or the collector
lacks VIEW SERVER STATE / VIEW DATABASE PERFORMANCE STATE; replace the direct
scalar subquery with a guarded lookup that returns NULL on failure—for example,
move the DMV call into a TRY/CATCH or an outer APPLY that tests capability first
(e.g. check server version/permissions or wrap the COUNT(*) FROM
sys.dm_db_log_info(DB_ID()) in BEGIN TRY/END TRY ... BEGIN CATCH RETURN NULL),
and use that result for vlf_count when df.type = 1; apply the same guarded
pattern to the second occurrence mentioned for lines 324–329 so VLF collection
failures do not abort the whole database snapshot.
In `@Lite/Database/DuckDbInitializer.cs`:
- Around line 587-600: The migration block for v22 currently swallows ALTER
TABLE failures (inside the fromVersion < 22 branch) by catching Exception and
only logging via _logger.LogWarning, which can leave the schema and collectors
out of sync; update the DuckDbInitializer migration code so that failures from
ExecuteNonQueryAsync calls (the ALTER TABLE statements on database_size_stats)
are not suppressed — either remove the try/catch or rethrow the caught exception
after logging (replace _logger.LogWarning with a fatal action), ensuring the
exception bubbles out of the method that handles migrations so the process fails
fast and the schema version is not advanced incorrectly.
---
Outside diff comments:
In `@Dashboard/Controls/FinOpsContent.xaml`:
- Around line 786-937: The filter buttons are icon-only and lack accessible
names; update them so screen readers get a meaningful label: add an
AutomationProperties.Name (or ToolTip) to each Button that derives the
column-specific text (e.g., "Filter Database column") by binding to the
Button.Tag or the adjacent TextBlock content, or modify the
ColumnFilterButtonStyle to set AutomationProperties.Name using TemplateBinding
to Tag (or a passed-in property) so all buttons (e.g., those with
Tag="DatabaseName", "FileTypeDesc", "TotalSizeMb", etc.) expose a
column-specific accessible name while keeping the existing Click handler
DatabaseSizesFilter_Click unchanged.
---
Nitpick comments:
In `@Dashboard/Services/DatabaseService.FinOps.cs`:
- Around line 2364-2382: The presentation and sorting logic for growth and VLF
(GrowthDisplay, AutoGrowthSort, VlfCountDisplay, VlfCountSort) is duplicated in
Dashboard/Services/DatabaseService.FinOps.cs and
Lite/Services/LocalDataService.FinOps.cs; extract these rules into a single
shared helper/value object (e.g., FileGrowthDisplayHelper or
FileGrowthViewModel) that exposes the same computed properties (GrowthDisplay,
AutoGrowthSort, VlfCountDisplay, VlfCountSort) and reuse it from both
DatabaseService and LocalDataService to centralize formatting/sort semantics and
eliminate duplication.
In `@Lite/Services/RemoteCollectorService.DatabaseSize.cs`:
- Around line 119-125: The vlf_count is being computed per file row by calling
(SELECT COUNT(*) FROM sys.dm_db_log_info(mf.database_id)) inside the projection;
change this to compute vlf_count once per database_id (e.g., a CTE or CROSS
APPLY that selects database_id, COUNT(*) AS vlf_count FROM
sys.dm_db_log_info(...) GROUP BY database_id) and then join/apply that result
into the main query so the projection can reference the precomputed vlf_count;
update the CASE for vlf_count (and the duplicate occurrence around the other
projection) to use the joined vlf_count when mf.type = 1 and NULL otherwise.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e99e9d8f-f72f-412e-949a-b2642e88c7f2
📒 Files selected for processing (12)
Dashboard/Controls/FinOpsContent.xamlDashboard/Controls/FinOpsContent.xaml.csDashboard/Services/DatabaseService.FinOps.csLite/Controls/FinOpsTab.xamlLite/Database/DuckDbInitializer.csLite/Database/Schema.csLite/Services/LocalDataService.FinOps.csLite/Services/RemoteCollectorService.DatabaseSize.csinstall/02_create_tables.sqlinstall/06_ensure_collection_table.sqlinstall/52_collect_database_size_stats.sqlupgrades/2.2.0-to-2.3.0/03_add_growth_vlf_columns.sql
Update Lite/Database/DuckDbInitializer.cs Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
erikdarlingdata
requested changes
Mar 17, 2026
Owner
erikdarlingdata
left a comment
erikdarlingdata
left a comment
There was a problem hiding this comment.
Good work overall — the feature itself is solid. Two blockers before merge:
1. upgrade.txt missing the new script
upgrades/2.2.0-to-2.3.0/upgrade.txt doesn't list 03_add_growth_vlf_columns.sql:
01_widen_query_stats_columns.sql
02_widen_audit_columns.sql
Without it, the upgrade script won't run for existing installations. They'll never get the new columns. Add 03_add_growth_vlf_columns.sql to the file.
2. ALTER TABLE blocks in install script 52_collect_database_size_stats.sql
The IF NOT EXISTS + ALTER TABLE block (lines 641-678) should be removed from the install script. Schema changes for existing installations go exclusively in the upgrades/ folder — the install scripts only need the columns in the 02_create_tables.sql CREATE TABLE block (which you've already done correctly).
The install scripts run on every install/upgrade after the upgrade scripts, so by the time 52_ executes, the upgrade script will have already added the columns. Having ALTER TABLE in both places creates a maintenance burden and goes against the project convention.
Not a blocker but worth noting
The Dashboard filter implementation hand-rolls the popup/state/button-style logic (~100 lines) instead of using DataGridFilterManager<T> which already handles all of that. The existing pattern is in ServerTab.xaml.cs lines 3841-3942. Works fine as-is but adds duplicate code to maintain.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
install/52_collect_database_size_stats.sql (1)
167-172:⚠️ Potential issue | 🟠 MajorGuard
sys.dm_db_log_info()so VLF lookup failures don’t drop per-database rows.At Line 170 and Line 281, the direct DMV call can fail (permissions/version), which causes the insert to fail for that database iteration. Please wrap this lookup in a local TRY/CATCH and fall back to
NULLforvlf_count.🛠️ Suggested pattern
+DECLARE `@vlf_count` int = NULL; +BEGIN TRY + SELECT `@vlf_count` = COUNT(*) FROM sys.dm_db_log_info(DB_ID()); +END TRY +BEGIN CATCH + SET `@vlf_count` = NULL; +END CATCH; ... - vlf_count = CASE WHEN df.type = 1 THEN (SELECT COUNT(*) FROM sys.dm_db_log_info(DB_ID())) ELSE NULL END + vlf_count = CASE WHEN df.type = 1 THEN `@vlf_count` ELSE NULL ENDWhat SQL Server versions and permissions are required for sys.dm_db_log_info(), and what fallback pattern is recommended when permission is missing?As per coding guidelines,
**/*.sql: “Watch for: ... missing error handling (TRY/CATCH) ...”.Also applies to: 278-283
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@install/52_collect_database_size_stats.sql` around lines 167 - 172, The vlf_count calculation using sys.dm_db_log_info(DB_ID()) can error and abort the INSERT; change the inline CASE to call a small guarded block that sets vlf_count via a local TRY/CATCH: when df.type = 1, BEGIN TRY SELECT `@vlf_count` = (SELECT COUNT(*) FROM sys.dm_db_log_info(DB_ID())); END TRY BEGIN CATCH SET `@vlf_count` = NULL; END CATCH, otherwise set `@vlf_count` = NULL; then use that `@vlf_count` in place of the direct DMV call in the CASE expression (reference vlf_count, df.type, and sys.dm_db_log_info).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Dashboard/Controls/FinOpsContent.xaml.cs`:
- Around line 683-687: The Escape key handler in ColumnFilterPopup.xaml.cs
currently only raises FilterCleared leaving the active filter unchanged; update
that handler so it mirrors ClearButton_Click by raising both FilterCleared and
FilterApplied with an empty filter state. Specifically, in the Escape/KeyDown
handler for ColumnFilterPopup, construct the same empty filter object used by
ClearButton_Click and invoke both the FilterCleared and FilterApplied events
(the same event signatures used by ColumnFilterPopup.ClearButton_Click) so that
FilterPopup_DbSizeFilterApplied receives the empty filter and
_dbSizesFilterMgr.SetFilter(...) is called.
- Around line 78-79: The _dbSizesFilterMgr field can be null when events fire
before OnLoaded, so either initialize it in the constructor or add defensive
null-checks where it is used: create the
DataGridFilterManager<FinOpsDatabaseSizeStats> instance (same call as in
OnLoaded: new
DataGridFilterManager<FinOpsDatabaseSizeStats>(DatabaseSizesDataGrid)) in the
class constructor to eliminate the event-ordering race, or in
LoadDatabaseSizesAsync and the filter popup handlers (the methods
ServerSelector_SelectionChanged → RefreshDataAsync → LoadDatabaseSizesAsync and
the popup handler methods) check _dbSizesFilterMgr for null and return/skip
filter logic if null instead of using the null-forgiving operator.
---
Duplicate comments:
In `@install/52_collect_database_size_stats.sql`:
- Around line 167-172: The vlf_count calculation using
sys.dm_db_log_info(DB_ID()) can error and abort the INSERT; change the inline
CASE to call a small guarded block that sets vlf_count via a local TRY/CATCH:
when df.type = 1, BEGIN TRY SELECT `@vlf_count` = (SELECT COUNT(*) FROM
sys.dm_db_log_info(DB_ID())); END TRY BEGIN CATCH SET `@vlf_count` = NULL; END
CATCH, otherwise set `@vlf_count` = NULL; then use that `@vlf_count` in place of the
direct DMV call in the CASE expression (reference vlf_count, df.type, and
sys.dm_db_log_info).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 36f3ff71-d698-4074-b7d6-5aaf369e990d
📒 Files selected for processing (4)
Dashboard/Controls/FinOpsContent.xaml.csDashboard/Services/DataGridFilterManager.csinstall/52_collect_database_size_stats.sqlupgrades/2.2.0-to-2.3.0/upgrade.txt
✅ Files skipped from review due to trivial changes (1)
- upgrades/2.2.0-to-2.3.0/upgrade.txt
Contributor
Author
|
All addressed. Thanks |
erikdarlingdata
added a commit
that referenced
this pull request
Mar 18, 2026
Resolves XAML conflict by taking dev's FinOpsContent.xaml and adding the two new Database Sizes columns (Auto Growth, VLF Count) from ClaudioESSilva's feature/GrowthRates-VLFCount branch. All other files auto-merged cleanly. Changes from PR #608: - Dashboard + Lite: new GrowthDisplay and VlfCountDisplay columns on Database Sizes grid - SQL collector: collect file growth settings and VLF counts - Upgrade script: 2.2.0-to-2.3.0/03_add_growth_vlf_columns.sql - Dashboard: DataGridFilterManager for Database Sizes column filtering - DuckDB schema: new growth/VLF columns in database_size_stats Co-Authored-By: Cláudio Silva <ClaudioESSilva@users.noreply.github.com> Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
4 tasks
Owner
|
@ClaudioESSilva The merge conflict on this one is pretty rugged, and in testing I'm hitting some other issues. I've carried your changes over to #625 to deal with the conflict and address. Thanks for this, though. |
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?
Implements #567
Which component(s) does this affect?
How was this tested?
Lite:
i. Open the Lite version
ii. Check if the columns get populated
Dashboard
i. Open InstallerGUI
ii. Run Install (to upgrade)
iii. Wait 1 minute for the collector
iv. Open the Dashboard, go to FinOps -> Database Sizes grid and check the columns return data
Some screenshots
Checklist
dotnet build -c Debug)Summary by CodeRabbit