Conversation
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Pull request overview
This PR continues the repository’s slog migration by updating osquery detail-query ingestion (and a maintained-apps validation CLI) to use log/slog instead of go-kit/log, and adjusts server/test call sites accordingly.
Changes:
- Migrate
osquery_utilsDetailQueryhooks (QueryFunc/IngestFunc/Direct*IngestFunc) fromlog.Loggerto*slog.Logger(with context-aware logging). - Update Fleet server/service code paths to pass
svc.logger.SlogLogger()into osquery ingestion and preprocessing. - Update unit/integration tests and the
cmd/maintained-apps/validatetool to use slog-based loggers.
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| server/service/osquery_utils/queries.go | Converts detail query hooks + many ingest functions to *slog.Logger and updates logging calls. |
| server/service/osquery_utils/queries_test.go | Updates tests to use t.Context() and slog discard/text handlers. |
| server/service/osquery_utils/disk_encryption_helpers.go | Migrates helper to accept *slog.Logger and use DebugContext. |
| server/service/osquery_utils/disk_encryption_helpers_test.go | Updates tests to pass slog discard logger. |
| server/service/osquery.go | Passes svc.logger.SlogLogger() into osquery detail query ingestion and adds ctx to preprocessing helpers. |
| server/service/osquery_test.go | Updates tests/bench to call preprocessing with ctx + slog discard logger. |
| server/service/orbit.go | Updates disk encryption helper call sites to pass svc.logger.SlogLogger(). |
| server/service/integration_core_test.go | Updates ingest tests to use slog JSON handler log capture and adjusts assertions. |
| server/service/integration_mdm_test.go | Replaces nop logger with slog discard logger in detail query ingest calls. |
| server/service/integration_enterprise_test.go | Replaces nop logger with slog discard logger in detail query ingest calls. |
| cmd/maintained-apps/validate/main.go | Migrates CLI logger to slog and updates logging calls throughout validation flow. |
| cmd/maintained-apps/validate/darwin.go | Migrates validate helpers to slog (InfoContext/etc.) and updates MutateSoftwareOnIngestion call. |
| cmd/maintained-apps/validate/windows.go | Migrates validate helpers to slog (InfoContext/etc.) and updates MutateSoftwareOnIngestion call. |
| cmd/maintained-apps/validate/app_commander.go | Migrates commander logging to slog and threads ctx into filesystem-change detection logs. |
| cmd/maintained-apps/validate/app_commander_test.go | Updates tests to pass ctx and use slog discard loggers. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
WalkthroughThis PR migrates the codebase from go-kit/log to Go's standard slog package. Changes include replacing logger type declarations from Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts (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: 3
🤖 Fix all issues with AI agents
In `@cmd/maintained-apps/validate/main.go`:
- Line 166: The call to appExists is passing cfg.logger instead of the per-app
logger appLogger, which strips the `"app"` context from logs; update the
invocation of appExists to pass appLogger (the logger with the `"app"` key
attached) so all logs inside appExists retain per-app context—i.e., replace the
cfg.logger argument with appLogger in the appExists(...) call that currently
uses ac.Name, ac.UniqueIdentifier, ac.Version, ac.AppPath.
In `@server/service/osquery_utils/queries_test.go`:
- Around line 56-57: The test calls MutateSoftwareOnIngestion with dcvViewer
instead of the intended noOp, so the noOp object is never mutated and the
assertion on noOp.Version is meaningless; change the call to
MutateSoftwareOnIngestion(t.Context(), noOp, slog.New(slog.DiscardHandler)) so
the no-op case is exercised and the subsequent assert.Equal(t, "2024",
noOp.Version) actually validates the no-op behavior for the noOp variable.
In `@server/service/osquery_utils/queries.go`:
- Line 2906: Replace the empty message string in the logger.DebugContext calls
inside the luksVerifyQueryIngester code path with a concise, human-readable
message (e.g., "luksVerifyQueryIngester error" or similar) so the first slog
argument is meaningful; update both occurrences (the one shown with
"method","luksVerifyQueryIngester" and the other at the later call around line
2917) to include contextual text while keeping the existing structured fields
(component, method, host, err) unchanged.
🧹 Nitpick comments (8)
server/service/integration_mdm_test.go (1)
6148-6148: Repeatedslog.New(slog.DiscardHandler)allocations across test call sites — acceptable but could be simplified.Five call sites each allocate a new discard logger. For test code this is fine, but if you want to tidy it up, a single
discardLoggerlocal at the top of the test function would reduce noise.Also applies to: 6179-6179, 6245-6245, 6492-6492, 6513-6513
cmd/maintained-apps/validate/darwin.go (1)
31-43: Prefer slog's native key-value args overfmt.Sprintf.Using
fmt.Sprintfinsideslog.InfoContextcollapses everything into an unstructured message string, which defeats slog's structured logging model. The same pattern appears throughout this file (lines 31, 37, 40–43, 193, 237, 243, 251, 260, 269).With structured args, log aggregators can filter/index on individual fields, and the allocations from
fmt.Sprintfare avoided when the log level is disabled.Example refactor for this block
- appLogger.InfoContext(ctx, fmt.Sprintf("Forcing LaunchServices refresh for: '%s'", appPath)) + appLogger.InfoContext(ctx, "Forcing LaunchServices refresh", "appPath", appPath) err := forceLaunchServicesRefresh(appPath) if err != nil { return fmt.Errorf("Error forcing LaunchServices refresh: %v. Attempting to continue", err) } - appLogger.InfoContext(ctx, fmt.Sprintf("Attempting to remove quarantine for: '%s'", appPath)) + appLogger.InfoContext(ctx, "Attempting to remove quarantine", "appPath", appPath) quarantineResult, err := removeAppQuarantine(appPath) - appLogger.InfoContext(ctx, fmt.Sprintf("Quarantine output error: %v", quarantineResult.QuarantineOutputError)) - appLogger.InfoContext(ctx, fmt.Sprintf("Quarantine status: %s", quarantineResult.QuarantineStatus)) - appLogger.InfoContext(ctx, fmt.Sprintf("Spctl output error: %v", quarantineResult.SpctlOutputError)) - appLogger.InfoContext(ctx, fmt.Sprintf("spctl status: %s", quarantineResult.SpctlStatus)) + appLogger.InfoContext(ctx, "Quarantine removal result", + "quarantineOutputError", quarantineResult.QuarantineOutputError, + "quarantineStatus", quarantineResult.QuarantineStatus, + "spctlOutputError", quarantineResult.SpctlOutputError, + "spctlStatus", quarantineResult.SpctlStatus, + )The same applies to all other
fmt.Sprintf-wrapped calls inappExists(e.g., lines 193, 237, 243, 251, 260, 269).server/service/osquery.go (1)
1570-1597:ErrorContextfor a non-fatal extra-query failure may be too noisy — considerWarnContext.Line 1591 uses
logger.ErrorContextfor a case where an extra software query returned errors, but execution continues normally. The previous go-kit code likely used a lower severity.slog.ErrorContextcould trigger alerts in production log monitoring for what is essentially a degraded-but-not-broken path.WarnContextorDebugContextmay be more appropriate here.cmd/maintained-apps/validate/app_commander.go (1)
70-83: Consider using slog structured key-value pairs instead offmt.Sprintf.Throughout this file, log messages use
fmt.Sprintfto format the message string. With slog, the idiomatic approach is to use structured key-value attributes, which enables better log parsing and filtering.For example:
// Instead of: ac.appLogger.InfoContext(ctx, fmt.Sprintf("New application detected at: %s", appPath)) // Prefer: ac.appLogger.InfoContext(ctx, "new application detected", "path", appPath)This applies to many call sites in this file (lines 70, 83, 88, 91–92, 95, 99, 103, 126, 129, 131). Not blocking, but worth adopting for new slog code.
Also applies to: 88-95, 99-103, 126-131
cmd/maintained-apps/validate/windows.go (1)
53-66: Samefmt.Sprintfnote applies here.Same optional refactor as in
app_commander.go— prefer slog structured attributes overfmt.Sprintfin log messages for better queryability. For example:// Instead of: logger.InfoContext(ctx, fmt.Sprintf("Looking for app: %s, version: %s", appName, appVersion)) // Prefer: logger.InfoContext(ctx, "looking for app", "app", appName, "version", appVersion)Also applies to: 77-77, 92-92, 97-97, 151-151
cmd/maintained-apps/validate/main.go (1)
44-44: Consider using slog's structured key-value args instead offmt.Sprintf.Throughout this file, the pattern
cfg.logger.ErrorContext(ctx, fmt.Sprintf("Error ...: %v", err))bakes the error into the message string. slog is designed for structured logging — usingcfg.logger.ErrorContext(ctx, "error getting list of apps", "error", err)makes errors filterable/indexable. This is a migration PR so deferring is fine, but it applies to nearly every log call here.server/service/osquery_utils/queries.go (2)
204-208: Consider using structured key-value pairs instead offmt.Sprintffor the slog message.Throughout this file (~20 instances), dynamic values are embedded in the message via
fmt.Sprintf. With slog, the idiomatic approach is a static message string plus structured key-value attributes, which improves log indexing and filtering. This is a pattern-level suggestion — not blocking.Example for this location:
Illustrative refactor
- logger.ErrorContext(ctx, fmt.Sprintf("detail_query_os_version expected single result got %d", len(rows)), - "component", "service", "method", "IngestFunc") + logger.ErrorContext(ctx, "detail_query_os_version unexpected result count", + "count", len(rows), "component", "service", "method", "IngestFunc")
2242-2257: Multipledeferregistrations possible across the two sanitizer loops.If a software entry matches both a
basicAppSanitizersentry and acustomAppSanitizersentry, two separatedefer/recoverclosures are registered. This is pre-existing behavior (not introduced by this PR), but worth noting: the seconddefer's panic handler would captures.Name/s.Versionvalues already mutated by the first sanitizer. Not blocking.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #39883 +/- ##
=======================================
Coverage 66.26% 66.27%
=======================================
Files 2439 2439
Lines 195437 195375 -62
Branches 8550 8550
=======================================
- Hits 129512 129478 -34
+ Misses 54197 54167 -30
- Partials 11728 11730 +2
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:
|
2 participants
Related issue: Resolves #38889
Plan was to convert
osquery_utilspackage to slog. Picked up some additional code that was related.Checklist for submitter
changes/,orbit/changes/oree/fleetd-chrome/changes.Testing
Summary by CodeRabbit
Release Notes
Refactor
Tests
Note: This release contains internal infrastructure improvements with no user-facing changes or new features.