Next set of slog migration changes for MDM - #39981
Merged
Merged
Conversation
Member
Author
|
@coderabbitai full review |
Contributor
✅ Actions performedFull review triggered. |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR continues the ongoing slog migration effort (ADR-0008) by migrating MDM-related code from go-kit/log to slog. The changes maintain backward compatibility using the platform logging adapter while gradually converting internal functions to use slog directly.
Changes:
- Migrated Windows MDM profile management, Microsoft MDM profile verification, and maintained apps sync functions from kitlog.Logger to *slog.Logger
- Updated all MDM-related log calls to use slog's context-aware methods (DebugContext, InfoContext, ErrorContext)
- Updated test code to use slog.New(slog.DiscardHandler) instead of log.NewNopLogger()
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| server/service/osquery_utils/queries.go | Removed platformlogging wrapper when calling Microsoft MDM functions; now passes logger directly |
| server/service/microsoft_mdm_test.go | Updated test logger instantiation to use slog.DiscardHandler |
| server/service/microsoft_mdm.go | Changed ReconcileWindowsProfiles signature to accept *slog.Logger and migrated all log calls to context-aware slog methods |
| server/service/integration_mdm_test.go | Added .SlogLogger() call when passing logger to ReconcileWindowsProfiles |
| server/service/integration_mdm_profiles_test.go | Added .SlogLogger() call when passing logger to VerifyHostMDMProfiles |
| server/service/apple_mdm.go | Added .SlogLogger() calls when passing logger to profile variable replacement functions |
| server/mdm/profiles/profile_variables.go | Changed function signatures to accept *slog.Logger and migrated log calls to ErrorContext |
| server/mdm/microsoft/profile_verifier_test.go | Updated test logger instantiation to use slog.DiscardHandler |
| server/mdm/microsoft/profile_verifier.go | Changed function signatures to accept *slog.Logger and migrated all log calls to context-aware slog methods |
| server/mdm/maintainedapps/testing_utils.go | Updated test logger instantiation to use slog.DiscardHandler |
| server/mdm/maintainedapps/sync.go | Changed Refresh function signature to accept *slog.Logger |
| cmd/fleet/cron.go | Added .SlogLogger() calls when passing logger to ReconcileWindowsProfiles and maintained_apps.Refresh |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
WalkthroughThis PR migrates logging across MDM, Apple/Microsoft MDM, DEP, and related service and test code from go-kit/log and platform-specific logging to Go's standard library slog. It updates many function signatures and struct fields to accept Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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 (1)
server/service/microsoft_mdm.go (1)
639-639: Consider usingErrorContextorWarnContextfor profile build failures.Lines 639 and 677 log
"error building command from profile"at Info level. Since these represent actual errors that cause a profile to be silently skipped (viacontinue),WarnContextorErrorContextwould better reflect severity and make debugging easier in production.This is a minor suggestion — the current level may be preserving the original behavior.
Also applies to: 677-677
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/service/microsoft_mdm.go` at line 639, In isEligibleForWindowsMDMMigration, replace the Info-level logs that print "error building command from profile" (and the corresponding InfoContext call at the other occurrence) with a higher-severity log such as WarnContext or ErrorContext so profile build failures are surfaced (e.g., change logger.InfoContext(..., "error building command from profile", ...) to logger.WarnContext or logger.ErrorContext for that message); keep the same context fields so callers can identify the profile/host but escalate the log level to reflect that the profile was skipped due to an error.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@server/mdm/maintainedapps/testing_utils.go`:
- Around line 104-132: The test is creating a logger with
slog.New(slog.DiscardHandler) which is incompatible; update every call that
passes slog.New(slog.DiscardHandler) (e.g., in calls to Refresh in
testing_utils.go and related tests) to construct a compatible discard logger by
creating a handler that accepts an io.Writer (for example, use a text or JSON
handler targeting io.Discard) and pass slog.New(...) that handler instead, and
add the necessary import for io.
In `@server/service/microsoft_mdm_test.go`:
- Around line 768-775: The test
TestReconcileWindowsProfileWithCertificateFailureDoesNotAddManagedCertificate
constructs a logger using slog.DiscardHandler which can be incompatible; replace
that instantiation with a compatible handler (e.g., create the logger via
slog.New with slog.NewTextHandler or slog.NewJSONHandler targeting io.Discard
and an appropriate slog.HandlerOptions) and update imports accordingly so the
test uses a compatible discard handler instead of slog.DiscardHandler.
- Around line 712-715: Replace the incompatible use of slog.DiscardHandler when
creating logger in the test: instead of slog.New(slog.DiscardHandler) construct
a compatible discard handler (e.g., slog.NewTextHandler(io.Discard,
&slog.HandlerOptions{}) or slog.NewJSONHandler(io.Discard,
&slog.HandlerOptions{})) and pass that to slog.New; update the logger
initialization (logger := slog.New(...)) in the test where ctx and ds are set
(the lines with ctx := context.Background(), ds := new(mock.Store), logger :=
slog.New(...)) so the test uses a supported handler rather than
slog.DiscardHandler.
- Around line 833-840:
TestReconcileWindowsProfilesWithOneHostFailingStillAddsManagedCertificate uses
slog.New(slog.DiscardHandler) which may not be available across Go versions;
replace the DiscardHandler usage in that test by creating a logger with
slog.New(slog.NewTextHandler(io.Discard, &slog.HandlerOptions{})) (ensure io is
imported) so the test uses a compatible discard sink instead of
slog.DiscardHandler.
---
Nitpick comments:
In `@server/service/microsoft_mdm.go`:
- Line 639: In isEligibleForWindowsMDMMigration, replace the Info-level logs
that print "error building command from profile" (and the corresponding
InfoContext call at the other occurrence) with a higher-severity log such as
WarnContext or ErrorContext so profile build failures are surfaced (e.g., change
logger.InfoContext(..., "error building command from profile", ...) to
logger.WarnContext or logger.ErrorContext for that message); keep the same
context fields so callers can identify the profile/host but escalate the log
level to reflect that the profile was skipped due to an error.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #39981 +/- ##
==========================================
+ Coverage 66.29% 66.30% +0.01%
==========================================
Files 2446 2444 -2
Lines 196006 195794 -212
Branches 8574 8444 -130
==========================================
- Hits 129942 129830 -112
+ Misses 54297 54197 -100
Partials 11767 11767
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:
|
Member
Author
|
@coderabbitai full review |
Contributor
✅ Actions performedFull review triggered. |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 22 out of 22 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
server/mdm/maintainedapps/sync.go (1)
36-43: Unusedloggerparameter inRefresh.The
loggerparameter is accepted but never referenced in the function body. If this is a preparatory signature change for Phase 2, consider adding a brief comment or using_ = loggerto signal intent and silence potential linter warnings.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/mdm/maintainedapps/sync.go` around lines 36 - 43, The Refresh function signature accepts a logger (*slog.Logger) but never uses it, causing a linter warning; either reference it or explicitly ignore it to signal intent. Update Refresh (function Refresh) to use the logger for relevant log events (e.g., log start, FetchAppsList error, or upsert result) or, if no logging is needed yet, add a deliberate ignore like `_ = logger` or a brief comment noting it's reserved for Phase 2 to silence linters; ensure the change is limited to the Refresh function signature/body so callers remain unchanged.server/service/apple_mdm.go (1)
5637-5656: HoistSlogLogger()once per function for clarity.Since this path can run inside variable-processing loops, consider caching the slog logger once and reusing it.
Suggested diff
func preprocessProfileContents( ctx context.Context, appConfig *fleet.AppConfig, ds fleet.Datastore, scepConfig fleet.SCEPConfigService, digiCertService fleet.DigiCertService, logger *platformlogging.Logger, @@ ) error { + slogLogger := logger.SlogLogger() // This method replaces Fleet variables ($FLEET_VAR_<NAME>) in the profile // contents, generating a unique profile for each host. For a 2KB profile and // 30K hosts, this method may generate ~60MB of profile data in memory. @@ - replacedContents, replacedVariable, err := profiles.ReplaceCustomSCEPChallengeVariable(ctx, logger.SlogLogger(), fleetVar, customSCEPCAs, hostContents) + replacedContents, replacedVariable, err := profiles.ReplaceCustomSCEPChallengeVariable(ctx, slogLogger, fleetVar, customSCEPCAs, hostContents) @@ - replacedContents, managedCertificate, replacedVariable, err := profiles.ReplaceCustomSCEPProxyURLVariable(ctx, logger.SlogLogger(), ds, appConfig, fleetVar, customSCEPCAs, hostContents, hostUUID, profUUID) + replacedContents, managedCertificate, replacedVariable, err := profiles.ReplaceCustomSCEPProxyURLVariable(ctx, slogLogger, ds, appConfig, fleetVar, customSCEPCAs, hostContents, hostUUID, profUUID)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/service/apple_mdm.go` around lines 5637 - 5656, Hoist logger.SlogLogger() to a single local variable at the top of the function and use that variable in the calls to profiles.ReplaceCustomSCEPChallengeVariable and profiles.ReplaceCustomSCEPProxyURLVariable (and any other places inside the variable-processing loop) instead of calling logger.SlogLogger() repeatedly; update the callsites to pass the cached slogLogger variable and leave the rest of the logic (hostContents updates and appending to managedCertificatePayloads) unchanged.server/mdm/apple/apple_mdm.go (1)
901-905: Type[]anyfor slog attrs is fine butlogCountsForResultsstill returns[]interface{}.The helper
logCountsForResults(line 947) returns[]interface{}while the localattrsis declared as[]any. These are identical in Go, so no issue, but consider updatinglogCountsForResultsto return[]anyfor consistency with the modern style used here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/mdm/apple/apple_mdm.go` around lines 901 - 905, Change the return type of the helper logCountsForResults from []interface{} to []any so it matches the local attrs declaration (attrs := []any{...}) and modern Go styles; update the function signature for logCountsForResults and any callers to use []any and adjust any internal variables to the any alias where needed to keep types consistent.
🤖 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/mdm/apple/apple_mdm.go`:
- Around line 894-899: The log message is contradictory because it reports "no
error was returned" while the condition currently checks err != nil; update the
conditional in apple_mdm.go to check err == nil (i.e., replace the err != nil
check with err == nil) so the block that calls logger.ErrorContext(ctx, "assign
profile: no error was returned but some devices were not assigned a status in
the response", "devices", implicitlyFailedAssignments) only runs when there
truly is no error and implicitlyFailedAssignments > 0; alternatively, if the
original intent was to log when an error exists, change the log text to reflect
the presence of err and include err in the ErrorContext call—adjust the
condition and message around the implicitlyFailedAssignments/err check
accordingly.
---
Nitpick comments:
In `@server/mdm/apple/apple_mdm.go`:
- Around line 901-905: Change the return type of the helper logCountsForResults
from []interface{} to []any so it matches the local attrs declaration (attrs :=
[]any{...}) and modern Go styles; update the function signature for
logCountsForResults and any callers to use []any and adjust any internal
variables to the any alias where needed to keep types consistent.
In `@server/mdm/maintainedapps/sync.go`:
- Around line 36-43: The Refresh function signature accepts a logger
(*slog.Logger) but never uses it, causing a linter warning; either reference it
or explicitly ignore it to signal intent. Update Refresh (function Refresh) to
use the logger for relevant log events (e.g., log start, FetchAppsList error, or
upsert result) or, if no logging is needed yet, add a deliberate ignore like `_
= logger` or a brief comment noting it's reserved for Phase 2 to silence
linters; ensure the change is limited to the Refresh function signature/body so
callers remain unchanged.
In `@server/service/apple_mdm.go`:
- Around line 5637-5656: Hoist logger.SlogLogger() to a single local variable at
the top of the function and use that variable in the calls to
profiles.ReplaceCustomSCEPChallengeVariable and
profiles.ReplaceCustomSCEPProxyURLVariable (and any other places inside the
variable-processing loop) instead of calling logger.SlogLogger() repeatedly;
update the callsites to pass the cached slogLogger variable and leave the rest
of the logic (hostContents updates and appending to managedCertificatePayloads)
unchanged.
JordanMontgomery
previously approved these changes
Feb 19, 2026
# Conflicts: # server/mdm/apple/apple_mdm.go
JordanMontgomery
approved these changes
Feb 19, 2026
This was referenced Feb 20, 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 #38889
Incremental set of slog migration changes for MDM packages.
Checklist for submitter
changes/,orbit/changes/oree/fleetd-chrome/changes.Testing
Summary by CodeRabbit