Reduced redundant database calls in the osquery distributed query results hot path#42157
Reduced redundant database calls in the osquery distributed query results hot path#42157
Conversation
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Pull request overview
This PR refactors the osquery distributed results ingestion path to reduce redundant configuration/datastore lookups during detail query processing, improving scalability on a very hot server code path.
Changes:
- Introduces a
hostDetailQueryConfigstruct andloadHostDetailQueryConfighelper to pre-load configuration for detail query building/ingestion. - Plumbs the pre-loaded config through
SubmitDistributedQueryResults→ingestQueryResults→ detail ingestion helpers to avoid per-result reloading. - Adds a changelog entry describing the reduced redundant DB calls.
Reviewed changes
Copilot reviewed 1 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| server/service/osquery.go | Preloads and threads detail-ingestion configuration through the distributed results ingestion path to reduce redundant datastore calls. |
| changes/35467-detail-query-config-preload | Documents the performance improvement in the changelog. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
WalkthroughThe pull request introduces configuration pre-loading to the osquery distributed query detail query ingestion hot path. A new 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 Tip CodeRabbit can generate a title for your PR based on the changes with custom instructions.Set the |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@server/service/osquery.go`:
- Around line 723-728: GetDetailQueries is still being reconstructed on the hot
path because hostDetailQueryConfig currently only caches inputs (appConfig,
features, mdmTeamConfig, conditionalAccessMicrosoftEnabled) but not the resolved
detail-query map; functions detailQueriesForHost, directIngestDetailQuery, and
ingestDetailQuery rebuild the full map repeatedly. Fix by adding a cached
resolved map field (e.g., detailQueries map[string]DetailQuery or similar) and a
sync.Once or lazy getter method on hostDetailQueryConfig (e.g.,
getDetailQueries()) that builds and stores the map once from GetDetailQueries
and returns the cached map thereafter; update detailQueriesForHost,
directIngestDetailQuery, and ingestDetailQuery to call that getter instead of
calling GetDetailQueries directly so the expensive build happens once per config
instance.
- Around line 1113-1120: The current call to loadHostDetailQueryConfig (stored
in detailConfig / ac) runs on every distributed/write request; move that call so
it is lazy-loaded only when handling a detail result whose name begins with
"fleet_detail_query_" (i.e., inside the branch that processes detail query
results), rather than before processing all results. Keep AppConfig loading
separate for the label/policy/live-query save paths so those flows do not
trigger HostFeatures/TeamMDMConfig work; when you relocate the call, ensure any
error returned by loadHostDetailQueryConfig only aborts the detail ingestion
path and not the overall submit flow. Locate references to detailConfig, ac, and
loadHostDetailQueryConfig in the distributed/write handling code and adjust
control flow and error handling accordingly.
🪄 Autofix (Beta)
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
Run ID: 80993c46-4012-4b85-b914-571ff3cc4045
📒 Files selected for processing (2)
changes/35467-detail-query-config-preloadserver/service/osquery.go
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #42157 +/- ##
==========================================
- Coverage 66.49% 66.48% -0.01%
==========================================
Files 2516 2516
Lines 201987 202026 +39
Branches 9071 9071
==========================================
+ Hits 134309 134316 +7
- Misses 55545 55576 +31
- Partials 12133 12134 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…x gosec G118 - Cache the resolved GetDetailQueries map in hostDetailQueryConfig so it is built once per request instead of once per detail query result - Lazy-load detailConfig only when a detail query result is present, avoiding unnecessary HostFeatures/TeamMDMConfig DB calls for payloads with only label/policy/live-query results - Load AppConfig separately for post-loop label/policy processing - Fix gosec G118: pass request context to serialUpdateHost goroutine using context.WithoutCancel to preserve OTEL trace context without inheriting request cancellation
|
Hi @lucasmrod, I'm assigning this PR review to you since this is in orchestration domain. Thank you. |
Related issue: Resolves #42156
The core change: instead of loading AppConfig, HostFeatures, TeamMDMConfig, and rebuilding the detail query map independently inside each call to
directIngestDetailQueryandingestDetailQuery(so ~2N times per check-in with N detail results), we load everything once into ahostDetailQueryConfigstruct and pass it through.Before
After
The detail config is lazy-loaded — if a check-in only has label/policy results and no detail queries, the HostFeatures/TeamMDMConfig calls are skipped entirely.
Other changes bundled in
serialUpdateHostnow receives the request context and usescontext.WithoutCancel(ctx)instead ofcontext.Background(), so the background goroutine preserves OTEL traces and logging context without being subject to request cancellation.Deferred save host at the end of
SubmitDistributedQueryResultsreuses the already-loaded AppConfig instead of loading it a third time. The old code silently skipped the host save if that third load failed.Checklist for submitter
If some of the following don't apply, delete the relevant line.
changes/,orbit/changes/oree/fleetd-chrome/changes.Testing
Summary by CodeRabbit