Reduce MySQL reader load on GET /hosts with device_mapping + search query (#47722)#48488
Conversation
…47722) When listing hosts with device_mapping=true, the device mapping was computed via a LEFT JOIN on a derived table with GROUP BY over host_emails. Because of the GROUP BY, MySQL materialized the full aggregation across all hosts before applying the outer LIMIT, paying the full cost on every page load. The count query reused the same options and paid the same cost a second time per page. - Compute device_mapping as a correlated subquery in the SELECT list so it is evaluated only for the rows actually returned, each as an indexed lookup on idx_host_emails_host_id_email. Matches the existing host_additional pattern. - Set opt.DeviceMapping = false in CountHosts since device_mapping is never selected when counting, mirroring opt.DisableIssues.
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.
Tip: disable this comment in your organization's Code Review settings.
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 reduces MySQL reader load for GET /api/v1/fleet/hosts when device_mapping=true (especially with pagination + search) by avoiding an expensive host_emails aggregation that previously had to be fully materialized regardless of the outer LIMIT.
Changes:
- Replaces the
host_emailsderived-tableLEFT JOIN ... GROUP BYfor device mapping with a correlated subquery inListHosts, so aggregation is evaluated only for returned rows. - Removes the now-unneeded device-mapping join from
applyHostFilters. - Ensures
CountHostsdoesn’t carryDeviceMappingoptions (though device_mapping is no longer part of the count query either way).
Reviewed changes
Copilot reviewed 1 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| server/datastore/mysql/hosts.go | Switches device_mapping to a correlated subquery, removes the derived join, and adjusts CountHosts options to avoid unnecessary work. |
| changes/47722-host-device-mapping-reader-load | User-visible change note (diff content excluded by policy; not reviewed). |
Files excluded by content exclusion policy (1)
- changes/47722-host-device-mapping-reader-load
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // device_mapping is never selected when counting, so skip its (expensive) subquery. | ||
| opt.DeviceMapping = false |
Walkthrough
🚥 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #48488 +/- ##
=======================================
Coverage 67.90% 67.91%
=======================================
Files 3678 3677 -1
Lines 233675 233633 -42
Branches 12412 12426 +14
=======================================
- Hits 158687 158661 -26
+ Misses 60725 60712 -13
+ Partials 14263 14260 -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:
|
nulmete
left a comment
There was a problem hiding this comment.
LGTM 👍
Could you include a before/after comparison if possible? Would feel more confident approving seeing that
Definitely. My bad for not including it in the PR description. Please take a look. |
Related issue: Resolves #47722
The issue was from a customer running
GET /api/v1/fleet/hosts?device_mapping=true&page=1&per_page=100&query=<ADDRESS>%40example.comon a script in a for loop. This change reduces the impact of the API on such workflows.Results from my local load test:
EXPLAIN ANALYZE:
Tests with 10k hosts:
Checklist for submitter
If some of the following don't apply, delete the relevant line.
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
Where appropriate, automated tests simulate multiple hosts and test for host isolation (updates to one hosts's records do not affect another)
QA'd all new/changed functionality manually
What & why
GET /api/v1/fleet/hosts?device_mapping=true&page=1&per_page=100&query=<email>caused high MySQL reader load on instances with ~10k hosts. Each page load ran an expensive aggregation over the entirehost_emailstable even though only ~100 rows are returned.Root cause: with
device_mapping=true,applyHostFiltersadded aLEFT JOINon a derived table withGROUP BY host_idoverhost_emails. Because of theGROUP BY, MySQL must fully materialize that derived table (aggregating every row for all hosts) before the outerWHERE/LIMIT 100can be applied, so the full cost is paid on every page request regardless of result size.CountHostsreused the same options, materializing the aggregation a second time per page load.Fixes (both in
server/datastore/mysql/hosts.go):SELECTlist (only whenopt.DeviceMapping), so it is evaluated only for the rows actually returned, each as an indexed lookup onidx_host_emails_host_id_email. This matches the existinghost_additionalpattern in the same query.opt.DeviceMapping = falseinCountHosts— the column is never selected for counting — mirroring the existingopt.DisableIssueshandling.Notes
idx_host_emails_host_id_email (host_id, email)already exists, so the correlated subquery resolves via an indexed lookup per returned row.TestHosts(full suite) passes, includingHostDeviceMapping,CustomHostDeviceMapping, andIDPHostDeviceMapping(the last two verify thecustom_*→customandidp→mdm_idp_accountssource translation still works through the new subquery).EXPLAIN ANALYZEon a ~10k-host dataset before/after, per the issue. I did not have access to such a dataset.Summary by CodeRabbit