slog migration: initLogger + serve.go + cron + schedule - #40699
Merged
Conversation
Member
Author
|
@coderabbitai full review |
Contributor
✅ Actions performedFull review triggered. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #40699 +/- ##
==========================================
- Coverage 66.32% 66.28% -0.05%
==========================================
Files 2466 2467 +1
Lines 196887 197496 +609
Branches 8618 8618
==========================================
+ Hits 130594 130905 +311
- Misses 54454 54736 +282
- Partials 11839 11855 +16
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:
|
Contributor
There was a problem hiding this comment.
Pull request overview
This pull request continues the slog migration effort (part 2 of #38889) by migrating logging in core initialization, server startup, cron jobs, and scheduling infrastructure from the kitlog-based *logging.Logger wrapper to *slog.Logger directly.
Changes:
- Updated
initLoggerto return*slog.Loggerdirectly instead of wrapping it in*logging.Logger - Migrated serve.go logging calls from kitlog patterns to slog patterns (InfoContext, ErrorContext, etc.)
- Updated all cron job and schedule functions to accept and use
*slog.Logger - Added context parameter to
TriggerFatalErrorandSetFatalErrorHandlerfor better error context propagation - Updated integration and unit tests to use slog logger initialization patterns
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| cmd/fleet/main.go | Changed initLogger to return *slog.Logger directly instead of *logging.Logger |
| cmd/fleet/serve.go | Migrated all logging calls from kitlog to slog patterns; removed .SlogLogger() calls |
| cmd/fleet/serve_test.go | Updated test logger initialization to use slog.New(slog.DiscardHandler) |
| cmd/fleet/cron.go | Updated all cron functions to accept *slog.Logger; removed .SlogLogger() adapter calls |
| cmd/fleet/cron_test.go | Updated test logger initialization to use slog patterns |
| cmd/fleet/vuln_process.go | Updated vulnerability processing functions to accept *slog.Logger |
| server/service/schedule/schedule.go | Migrated Schedule type to use *slog.Logger; updated WithLogger option |
| server/service/integration_mdm_test.go | Updated test schedule creation to use *slog.Logger directly |
| server/service/integration_enterprise_test.go | Updated calendar schedule creation to use slog logger initialization |
| server/cron/calendar_cron.go | Updated all calendar cron functions to accept *slog.Logger |
| server/cron/calendar_cron_test.go | Updated test logger initialization to use slog patterns |
| server/platform/mysql/retry.go | Added context.Context parameter to TriggerFatalError and SetFatalErrorHandler |
| server/platform/mysql/retry_test.go | Updated tests to pass context to TriggerFatalError |
| server/platform/mysql/common.go | Updated TriggerFatalError calls to pass context |
| server/datastore/mysql/sessions.go | Updated TriggerFatalError call to pass context |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
WalkthroughThis pull request migrates logging infrastructure from the custom Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (1 warning, 2 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 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}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (7)
server/service/integration_mdm_test.go (1)
332-369: Consider extracting repeated job lifecycle logging into a small helper.The start/complete logging pattern is duplicated across several scheduled jobs, which makes future edits noisy.
Refactor sketch
+logJob := func(ctx context.Context, jobName string, onDone func(), run func(context.Context) error) error { + logger.InfoContext(ctx, "Starting job", "job", jobName, "test", s.T().Name(), "time", time.Now()) + if onDone != nil { + defer func() { + logger.InfoContext(ctx, "Completing job", "job", jobName, "test", s.T().Name(), "time", time.Now()) + onDone() + }() + } + return run(ctx) +}Then call it from each
schedule.WithJob(...)closure.Also applies to: 429-437
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/service/integration_mdm_test.go` around lines 332 - 369, The repeated start/complete logging and onProfileJobDone defer logic in each schedule.WithJob closure (seen around the closures for "manage_apple_profiles", "manage_apple_declarations", and "manage_windows_profiles" which call logger.InfoContext and s.onProfileJobDone, then invoke ReconcileAppleProfiles/ReconcileAppleDeclarations/ReconcileWindowsProfiles) should be extracted into a small helper wrapper (e.g., wrapJob(jobName string, job func(context.Context) error) func(context.Context) error) that logs start/complete with time, handles the s.onProfileJobDone defer, calls the passed job function, and returns its error; replace each inline closure passed to schedule.WithJob with a call to this wrapper passing the specific reconcile function.cmd/fleet/cron.go (6)
187-187: Empty message string in log call.Same issue as above—consider using a descriptive message.
Suggested fix
- logger.DebugContext(ctx, "", "vulnAutomationEnabled", vulnAutomationEnabled) + logger.DebugContext(ctx, "vulnerability automation status", "automation_enabled", vulnAutomationEnabled)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/fleet/cron.go` at line 187, The log call uses an empty message string in logger.DebugContext(ctx, "", "vulnAutomationEnabled", vulnAutomationEnabled); update this call to supply a descriptive message (e.g., "vulnerability automation enabled" or similar) while keeping the existing structured key "vulnAutomationEnabled" and value vulnAutomationEnabled so the log entry is meaningful; locate the logger.DebugContext invocation in cron.go and replace the empty string argument with the descriptive message.
459-460: Empty message string in log call.Suggested fix
- logger.DebugContext(refreshCtx, "", "goval_dictionary-sync-downloaded", d) + logger.DebugContext(refreshCtx, "goval_dictionary sync downloaded", "file", d)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/fleet/cron.go` around lines 459 - 460, The log call inside the loop over downloaded uses logger.DebugContext(refreshCtx, "", "goval_dictionary-sync-downloaded", d) with an empty message string; update the call to provide a meaningful message (e.g., "goval dictionary downloaded" or similar) so the log entry isn't missing a human-readable message, keeping the same context (refreshCtx), tag ("goval_dictionary-sync-downloaded") and payload (d) to preserve structure and semantics.
306-306: Inconsistent slog key-value pattern.The key
"found new"contains a space which is atypical. Consider using snake_case or camelCase for keys.Suggested fix
- logger.DebugContext(ctx, "custom-vulnerabilities-analysis-done", "found new", len(vulns)) + logger.DebugContext(ctx, "custom-vulnerabilities-analysis-done", "found_new", len(vulns))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/fleet/cron.go` at line 306, The log call using logger.DebugContext with the kv pair key "found new" uses a space-including key which breaks the project's slog key naming convention; update the call in the DebugContext invocation (logger.DebugContext) to use a single-token key like "found_new" or "foundNew" (and adjust any other callers if present) so the kv pair conforms to snake_case/camelCase conventions and remains consistent with other structured logs.
400-401: Empty message string in log call.Suggested fix
- logger.DebugContext(refreshCtx, "", "oval-sync-downloaded", d) + logger.DebugContext(refreshCtx, "oval sync downloaded", "file", d)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/fleet/cron.go` around lines 400 - 401, The log call uses an empty message string in logger.DebugContext inside the loop over downloaded items; update that call (in cmd/fleet/cron.go where logger.DebugContext is invoked for "oval-sync-downloaded") to provide a meaningful message (e.g., "downloaded OVAL file" or "downloaded OVAL feed entry") or a formatted message including d details so the log is readable and informative.
356-361: Inconsistent slog key-value pattern with spaces.Several keys contain spaces (
"os name","os version","display version","found new"). Consider using consistent key naming conventions (snake_case or camelCase) for better log parsing.Suggested fix
logger.DebugContext(analyzeCtx, "msrc-analysis-done", - "os name", o.Name, - "os version", o.Version, - "display version", o.DisplayVersion, + "os_name", o.Name, + "os_version", o.Version, + "display_version", o.DisplayVersion, "elapsed", elapsed, - "found new", len(r)) + "found_new", len(r))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/fleet/cron.go` around lines 356 - 361, The log call using logger.DebugContext(analyzeCtx, "msrc-analysis-done", ...) uses keys with spaces ("os name", "os version", "display version", "found new"); change these to a consistent machine-friendly format (e.g., os_name, os_version, display_version, found_new or camelCase osName, osVersion, displayVersion, foundNew) and update the logger.DebugContext invocation accordingly (keeping analyzeCtx, "msrc-analysis-done", o.Name, o.Version, o.DisplayVersion, elapsed, len(r) as values tied to the new keys); also scan nearby/related logging calls to make key naming consistent across the file.
91-91: Empty message string in log call.The first argument after context should be a meaningful message. Here the message is empty and
"periodicity"is used as a key-value attribute.Suggested fix
- logger.InfoContext(ctx, "", "periodicity", config.Periodicity) + logger.InfoContext(ctx, "vulnerability scan configuration", "periodicity", config.Periodicity)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/fleet/cron.go` at line 91, The log call uses an empty message string: replace logger.InfoContext(ctx, "", "periodicity", config.Periodicity) with a call that supplies a meaningful message (e.g., "cron periodicity configured" or "starting cron") while keeping the key-value attribute "periodicity" and config.Periodicity; update the invocation at the logger.InfoContext call site so the first string argument is non-empty and descriptive.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@cmd/fleet/cron.go`:
- Line 187: The log call uses an empty message string in
logger.DebugContext(ctx, "", "vulnAutomationEnabled", vulnAutomationEnabled);
update this call to supply a descriptive message (e.g., "vulnerability
automation enabled" or similar) while keeping the existing structured key
"vulnAutomationEnabled" and value vulnAutomationEnabled so the log entry is
meaningful; locate the logger.DebugContext invocation in cron.go and replace the
empty string argument with the descriptive message.
- Around line 459-460: The log call inside the loop over downloaded uses
logger.DebugContext(refreshCtx, "", "goval_dictionary-sync-downloaded", d) with
an empty message string; update the call to provide a meaningful message (e.g.,
"goval dictionary downloaded" or similar) so the log entry isn't missing a
human-readable message, keeping the same context (refreshCtx), tag
("goval_dictionary-sync-downloaded") and payload (d) to preserve structure and
semantics.
- Line 306: The log call using logger.DebugContext with the kv pair key "found
new" uses a space-including key which breaks the project's slog key naming
convention; update the call in the DebugContext invocation (logger.DebugContext)
to use a single-token key like "found_new" or "foundNew" (and adjust any other
callers if present) so the kv pair conforms to snake_case/camelCase conventions
and remains consistent with other structured logs.
- Around line 400-401: The log call uses an empty message string in
logger.DebugContext inside the loop over downloaded items; update that call (in
cmd/fleet/cron.go where logger.DebugContext is invoked for
"oval-sync-downloaded") to provide a meaningful message (e.g., "downloaded OVAL
file" or "downloaded OVAL feed entry") or a formatted message including d
details so the log is readable and informative.
- Around line 356-361: The log call using logger.DebugContext(analyzeCtx,
"msrc-analysis-done", ...) uses keys with spaces ("os name", "os version",
"display version", "found new"); change these to a consistent machine-friendly
format (e.g., os_name, os_version, display_version, found_new or camelCase
osName, osVersion, displayVersion, foundNew) and update the logger.DebugContext
invocation accordingly (keeping analyzeCtx, "msrc-analysis-done", o.Name,
o.Version, o.DisplayVersion, elapsed, len(r) as values tied to the new keys);
also scan nearby/related logging calls to make key naming consistent across the
file.
- Line 91: The log call uses an empty message string: replace
logger.InfoContext(ctx, "", "periodicity", config.Periodicity) with a call that
supplies a meaningful message (e.g., "cron periodicity configured" or "starting
cron") while keeping the key-value attribute "periodicity" and
config.Periodicity; update the invocation at the logger.InfoContext call site so
the first string argument is non-empty and descriptive.
In `@server/service/integration_mdm_test.go`:
- Around line 332-369: The repeated start/complete logging and onProfileJobDone
defer logic in each schedule.WithJob closure (seen around the closures for
"manage_apple_profiles", "manage_apple_declarations", and
"manage_windows_profiles" which call logger.InfoContext and s.onProfileJobDone,
then invoke
ReconcileAppleProfiles/ReconcileAppleDeclarations/ReconcileWindowsProfiles)
should be extracted into a small helper wrapper (e.g., wrapJob(jobName string,
job func(context.Context) error) func(context.Context) error) that logs
start/complete with time, handles the s.onProfileJobDone defer, calls the passed
job function, and returns its error; replace each inline closure passed to
schedule.WithJob with a call to this wrapper passing the specific reconcile
function.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
cmd/fleet/cron.gocmd/fleet/cron_test.gocmd/fleet/main.gocmd/fleet/serve.gocmd/fleet/serve_test.gocmd/fleet/vuln_process.goserver/cron/calendar_cron.goserver/cron/calendar_cron_test.goserver/datastore/mysql/sessions.goserver/platform/mysql/common.goserver/platform/mysql/retry.goserver/platform/mysql/retry_test.goserver/service/integration_enterprise_test.goserver/service/integration_mdm_test.goserver/service/schedule/schedule.go
🔥 Files not summarized due to errors (2)
- server/service/integration_mdm_test.go: Error: Server error: no LLM provider could handle the message
- server/service/integration_enterprise_test.go: Error: Server error: no LLM provider could handle the message
cdcme
approved these changes
Feb 27, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Related issue: Resolves #40540
Almost done with slog migration.
Checklist for submitter
changes/,orbit/changes/oree/fleetd-chrome/changes.Testing
Summary by CodeRabbit