Batch extension label-membership checks in GetOrbitConfig#49154
Batch extension label-membership checks in GetOrbitConfig#49154sharon-fdm wants to merge 2 commits into
Conversation
Replace N+1 per-extension HostMemberOfAllLabels calls in filterExtensionsForHost with a single HostMembershipForLabels query that fetches all label memberships at once, then filters in memory.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #49154 +/- ##
==========================================
- Coverage 68.10% 67.95% -0.16%
==========================================
Files 3732 3745 +13
Lines 235790 238286 +2496
Branches 12517 12517
==========================================
+ Hits 160596 161935 +1339
- Misses 60772 61699 +927
- Partials 14422 14652 +230
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:
|
|
|
||
| // HostMembershipForLabels returns the set of label names (from the provided list) that the host is a member of. | ||
| // Labels that do not exist are not included in the result. | ||
| HostMembershipForLabels(ctx context.Context, hostID uint, labelNames []string) (map[string]bool, error) |
There was a problem hiding this comment.
Why a new method instead of modifying HostMemberOfAllLabels? Adding a new method means zero risk to other callers. The old method stays untouched, and filterExtensionsForHost switches to the new one.
- setboolcheck: HostMembershipForLabels return type changed from
map[string]bool to map[string]struct{} (Fleet convention for sets)
- staticcheck: replaced ptr.String/ptr.Bool with new() in test
|
|
||
| // HostMembershipForLabels returns the set of label names (from the provided list) that the host is a member of. | ||
| // Labels that do not exist are not included in the result. | ||
| HostMembershipForLabels(ctx context.Context, hostID uint, labelNames []string) (map[string]struct{}, error) |
There was a problem hiding this comment.
Why a new method instead of modifying HostMemberOfAllLabels? Adding a new method means zero risk to other callers. The old method stays untouched, and filterExtensionsForHost switches to the new one.
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 optimizes Orbit config generation by removing an N+1 label-membership query pattern in filterExtensionsForHost (called during GetOrbitConfig) and replacing it with a single batched datastore query for a host’s membership across all labels referenced by configured extensions.
Changes:
- Added a new datastore API
HostMembershipForLabelsto fetch a host’s membership across a provided label list in one query. - Updated
filterExtensionsForHostto batch label lookup and filter extensions in-memory. - Added a MySQL integration test for the new datastore method.
Reviewed changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| server/service/orbit.go | Replaces per-extension membership queries with a single batched membership lookup and in-memory filtering. |
| server/fleet/datastore.go | Adds HostMembershipForLabels to the datastore interface. |
| server/mock/datastore_mock.go | Extends the datastore mock to support HostMembershipForLabels. |
| server/datastore/mysql/labels.go | Implements HostMembershipForLabels with a single SELECT ... IN (?) query. |
| server/datastore/mysql/labels_test.go | Adds integration test coverage for HostMembershipForLabels. |
| changes/batch-extension-label-check | Changes entry for the user-visible optimization (content excluded from review). |
Files excluded by content exclusion policy (1)
- changes/batch-extension-label-check
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| result := make(map[string]struct{}, len(names)) | ||
| for _, n := range names { | ||
| result[n] = struct{}{} | ||
| } | ||
| return result, nil |
| { | ||
| name: "nonexistent labels are not included", | ||
| hostID: h1.ID, | ||
| labelNames: []string{allHostsLabel.Name, "nonexistent-label"}, | ||
| expectedResult: map[string]struct{}{allHostsLabel.Name: {}}, | ||
| }, |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
WalkthroughAdds a 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
Related issue: Resolves #45320
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.Summary
filterExtensionsForHost(called on every Orbit config fetch, ~30s per host) had an N+1 query pattern: it calledHostMemberOfAllLabelsonce per extension in a loop, issuing a separate DB query for each.This PR replaces the N queries with a single batch query via a new
HostMembershipForLabelsdatastore method that returns which labels (from a given list) the host belongs to. Extension filtering then happens in-memory.Changes
HostMembershipForLabels(ctx, hostID, labelNames) -> map[string]bool-- singleSELECT l.name FROM labels l JOIN label_membershipqueryfilterExtensionsForHostinserver/service/orbit.go-- collects all unique label names across extensions, calls the new method once, filters in-memoryBenchmark
Ran a local end-to-end benchmark against the live
POST /api/fleet/orbit/configendpoint to measure the real-world impact.Setup:
main(before) and this PR branch (after), same database and test dataResults:
Extrapolation to 100,000 hosts
At 100k hosts with a 30-second check-in interval (3,333 req/s steady state):
The improvement scales linearly with extension count:
Testing
Automated
testHostMembershipForLabelsMySQL integration test covering: empty input, full membership, partial membership, nonexistent labels, nonexistent host, host with no membershipstestHostMemberOfAllLabelsunchanged and unaffectedManual QA
Summary by CodeRabbit