Add patch policy and software automation usage statistics - #50766
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #50766 +/- ##
=======================================
Coverage 68.40% 68.41%
=======================================
Files 3963 3963
Lines 255174 255194 +20
Branches 13420 13420
=======================================
+ Hits 174560 174591 +31
+ Misses 65016 65008 -8
+ Partials 15598 15595 -3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Warning
- Copilot's review of this pull request may be incomplete because some of the changed files are excluded by your Copilot content exclusion settings. See Excluding content from Copilot for details.
Pull request overview
This PR updates the Fleet server’s usage statistics payload to include new patch-policy/software-automation metrics and to enrich the Fleet-maintained-apps stats from simple slug arrays into structured per-app objects (matching the shape documented in the related issue/docs work).
Changes:
- Change
fleetMaintainedAppsMacOS/fleetMaintainedAppsWindowsfrom[]stringto[]FleetMaintainedAppUsage{name, patchPolicy, softwareAutomation}in the statistics payload. - Add
numPoliciesAutomationEnabledSoftware, derived from the same predicate as the publicautomation_type=softwarefilter by extracting it intopoliciesSoftwareAutomationClause. - Update the MySQL statistics query to properly aggregate per-slug results (via
GROUP BY+MAX(...)) and expand tests to cover the new behavior and payload shape.
Reviewed changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| server/fleet/statistics.go | Adds the new usage-stat struct/type changes and introduces the new numPoliciesAutomationEnabledSoftware field. |
| server/datastore/mysql/statistics.go | Populates the new statistics fields; updates the FMA “in use” query to return per-app objects with aggregated booleans. |
| server/datastore/mysql/statistics_test.go | Adds/extends tests for FMA usage object shape and the new “policies automation enabled (software)” count. |
| server/datastore/mysql/policies.go | Extracts and reuses the automation_type=software predicate via policiesSoftwareAutomationClause; adds the DB count helper. |
| cmd/fleet/serve_test.go | Updates the pinned serialized usage-stats JSON payload to the new field shapes and adds the new numeric stat. |
Files excluded by content exclusion policy (1)
- changes/50762-usage-stats-patch-policies.md
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
WalkthroughThe statistics payload now includes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 1
🧹 Nitpick comments (2)
server/datastore/mysql/statistics.go (1)
332-334: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCorrect the comment about empty-slice serialization.
The comment states that the slices serialize as
[]instead ofnull. The payload fieldsFleetMaintainedAppsMacOSandFleetMaintainedAppsWindowsinserver/fleet/statistics.gouseomitempty, so an empty slice is omitted from the JSON entirely. Themakecalls still matter for callers and tests that assert non-nil, but the stated reason is wrong.📝 Proposed comment fix
- // Initialize as empty slices (not nil) so they serialize as [] instead of null + // Initialize as empty slices (not nil) so callers get a non-nil result. Note the payload + // fields use omitempty, so empty slices are omitted from the statistics JSON. macOSApps = make([]fleet.FleetMaintainedAppUsage, 0) windowsApps = make([]fleet.FleetMaintainedAppUsage, 0)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/datastore/mysql/statistics.go` around lines 332 - 334, Update the comment above the macOSApps and windowsApps initializations to state that the slices are deliberately non-nil for callers and tests, rather than claiming they serialize as [] instead of null. Keep both make calls unchanged.server/datastore/mysql/statistics_test.go (1)
928-937: 🔒 Security & Privacy | 🔵 TrivialSuppress the G202 SQL string-concatenation finding.
This
fmt.Sprintfis used only in tests with hard-coded column/value fragments and?data binding; add a targeted//nolint:gosec // G202comment with a matching reason to keep lint noise down.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/datastore/mysql/statistics_test.go` around lines 928 - 937, Suppress the targeted gosec G202 finding on the fmt.Sprintf SQL construction inside insertPolicy by adding a //nolint:gosec comment with a reason noting that the query fragments are hard-coded test values and data remains parameter-bound.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@server/datastore/mysql/statistics_test.go`:
- Line 886: Correct the setup comment near the test fixture to state that it
creates only a software installer, then extend the test’s policy setup and
assertions around policiesSoftwareAutomationClause to include a policy with
vpp_apps_teams_id populated. Ensure the added fixture exercises the
vpp_apps_teams_id branch alongside the existing software_installer_id and patch
branches.
---
Nitpick comments:
In `@server/datastore/mysql/statistics_test.go`:
- Around line 928-937: Suppress the targeted gosec G202 finding on the
fmt.Sprintf SQL construction inside insertPolicy by adding a //nolint:gosec
comment with a reason noting that the query fragments are hard-coded test values
and data remains parameter-bound.
In `@server/datastore/mysql/statistics.go`:
- Around line 332-334: Update the comment above the macOSApps and windowsApps
initializations to state that the slices are deliberately non-nil for callers
and tests, rather than claiming they serialize as [] instead of null. Keep both
make calls unchanged.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 23c5622f-9eee-4863-b116-64f3e181fe58
⛔ Files ignored due to path filters (1)
changes/50762-usage-stats-patch-policies.mdis excluded by!**/*.md
📒 Files selected for processing (5)
cmd/fleet/serve_test.goserver/datastore/mysql/policies.goserver/datastore/mysql/statistics.goserver/datastore/mysql/statistics_test.goserver/fleet/statistics.go
| require.NoError(t, err) | ||
| assert.Equal(t, 0, count) | ||
|
|
||
| // Set up an installer and a VPP app so the automation columns can be populated. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix the inaccurate comment and add coverage for vpp_apps_teams_id.
Line 886 states that the fixture sets up "an installer and a VPP app". The test creates only a software installer. No VPP app is created, and no policy sets vpp_apps_teams_id.
policiesSoftwareAutomationClause has three branches: software_installer_id, vpp_apps_teams_id, and type = 'patch'. This test covers two of them. A regression that drops the vpp_apps_teams_id term would not fail any test here.
Correct the comment, and add a policy that sets vpp_apps_teams_id so the third branch is exercised.
📝 Proposed comment correction
- // Set up an installer and a VPP app so the automation columns can be populated.
+ // Set up a software installer so the automation columns can be populated.I can generate the VPP app fixture and the extra assertion if you want.
Also applies to: 939-956
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@server/datastore/mysql/statistics_test.go` at line 886, Correct the setup
comment near the test fixture to state that it creates only a software
installer, then extend the test’s policy setup and assertions around
policiesSoftwareAutomationClause to include a policy with vpp_apps_teams_id
populated. Ensure the added fixture exercises the vpp_apps_teams_id branch
alongside the existing software_installer_id and patch branches.
Related issue: Resolves #50762
Adds the usage statistics documented in #49980 (merged to
docs-v4.91.0):numPoliciesAutomationEnabledSoftware— the number of policies with a software automation.fleetMaintainedAppsMacOS/fleetMaintainedAppsWindows— each entry changes from a slug string to{ name, patchPolicy, softwareAutomation }.Notes for review:
automation_type=softwareis extracted fromcreateAutomationClauseintopoliciesSoftwareAutomationClauseand shared, so the stat can't drift from what the API returns. The reconstructed SQL is byte-identical to the previous literal.softwareAutomationis deliberately narrower — it means "this patch policy also carries an install-software automation." Using the shared clause here would make it identical topatchPolicy(every matched policy istype='patch'), so the field would carry no information.GROUP BY+MAX(...): one slug backs up to2 × N_teamsinstaller rows (cached versions plus per-team rows). The oldSELECT DISTINCTcollapsed those only because every row was identical; with per-team booleans it would emit duplicate slugs.Important
Depends on #50760, which must ship first. fleetdm.com currently declares these two fields as
type: ['string']. Sails rejects an array of objects against that declaration withE_INVALID, which becomes anE_INVALID_ARGINS→ 400 for the whole request — so until #50760 lands, an upgraded server would have its entire usage statistics payload dropped, not just these fields.Checklist for submitter
Changes file added for user-visible changes in
changes/,orbit/changes/oree/fleetd-chrome/changes.See Changes files for more information.
Input data is properly validated,
SELECT *is avoided, SQL injection is prevented (using placeholders for values in statements), JS inline code is prevented especially for url redirects, and untrusted data interpolated into shell scripts/commands is validated against shell metacharacters.Testing
Added/updated automated tests
QA'd all new/changed functionality manually
Automated coverage:
TestStatistics/FleetMaintainedAppsInUse— patch policy with and without a software automation, a dynamic policy on the same title, the same app on a second team (asserting one entry per slug), and a team-scoped patch policy that must not apply to another team's installer.TestStatistics/PoliciesAutomationEnabledSoftware— installer and VPP automations count; script and calendar automations don't; a patch policy counts even with no install automation, pinning parity withautomation_type=software.TestMaybeSendStatistics— pins the serialized payload against the shape published in [Docs] Usage stats changes #49980.TestListTeamPoliciesAutomationTypeSoftware— unchanged behaviour after extracting the shared clause.Both new predicates were mutation-tested: removing
OR p.type = 'patch'or the team-scoping predicate each fails a test rather than passing silently.End-to-end QA is left unchecked deliberately — it can't be completed until #50760 is deployed, since the receiving webhook would reject the new payload today.
Summary by CodeRabbit
New Features
Bug Fixes