Migrating MDM files to slog method signatures. - #40263
Conversation
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
WalkthroughThis pull request continues the migration of logging from go-kit/log to slog across MDM service files. It replaces all occurrences of the go-kit/log level API (level.Debug, level.Error, etc.) with context-aware slog logging methods (svc.logger.DebugContext, ErrorContext, WarnContext, InfoContext) in server/service/apple_mdm.go, server/service/mdm.go, and server/service/microsoft_mdm.go. The changes remove the dependency on github.com/go-kit/log/level and update logging statements while preserving existing error handling and control flow. Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
server/service/microsoft_mdm.go (1)
1081-1084: Consider passingerrdirectly instead oferr.Error().
slogacceptserrorvalues natively;err.Error()narrows the attribute to a plain string, discarding the error type for structured backends. Other call sites in this file (e.g., line 2635:"err", err) already use the idiomatic form.♻️ Suggested change
- svc.logger.DebugContext(ctx, "invalid discover message", - "err", err.Error(), - "request_xml", string(req.Raw), - ) + svc.logger.DebugContext(ctx, "invalid discover message", + "err", err, + "request_xml", string(req.Raw), + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/service/microsoft_mdm.go` around lines 1081 - 1084, In svc.logger.DebugContext(ctx, "invalid discover message", ...) replace the "err", err.Error() attribute with "err", err so the error is passed as an error value (preserving type and structured metadata) alongside the existing "request_xml", string(req.Raw) arguments; update the call site using the svc.logger.DebugContext method, keeping ctx and req.Raw unchanged.server/service/mdm.go (2)
813-813: ConsiderWarnContextfor a failed datastore call.The error from
GetVPPAppInstallStatusByCommandUUIDis intentionally swallowed (VPP status is best-effort), which is fine, but logging it atDebugmakes the failure invisible in production where debug logging is usually disabled.WarnContextwould keep the same non-fatal semantics while ensuring the failure surfaces in normal log levels.♻️ Suggested change
- svc.logger.DebugContext(ctx, "failed to check if VPP app is installed", "err", err, "command_uuid", commandUUID) + svc.logger.WarnContext(ctx, "failed to check if VPP app is installed", "err", err, "command_uuid", commandUUID)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/service/mdm.go` at line 813, The log for the best-effort datastore call currently uses svc.logger.DebugContext for the error returned by GetVPPAppInstallStatusByCommandUUID; change that call to use svc.logger.WarnContext so the swallowed, non-fatal error is visible at normal log levels (keep the same message keys like "err" and "command_uuid" and the same context variable commandUUID and function GetVPPAppInstallStatusByCommandUUID).
1075-1075: Prefer"err"over"details"for error key.The conventional slog key name for an error value is
"err", which tools and log aggregators often recognise specially."details"works but departs from the pattern used elsewhere in the migrated codebase.♻️ Suggested change
- svc.logger.ErrorContext(ctx, "unauthorized to view some team commands", "details", authzErr) + svc.logger.ErrorContext(ctx, "unauthorized to view some team commands", "err", authzErr)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/service/mdm.go` at line 1075, Change the log key from "details" to the conventional "err" in the ErrorContext call so the error is recognized consistently; update the call to svc.logger.ErrorContext(...) that currently passes "details", authzErr to instead pass "err", authzErr (look for the ErrorContext invocation around svc.logger.ErrorContext and the authzErr variable in mdm.go).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@server/service/mdm.go`:
- Line 813: The log for the best-effort datastore call currently uses
svc.logger.DebugContext for the error returned by
GetVPPAppInstallStatusByCommandUUID; change that call to use
svc.logger.WarnContext so the swallowed, non-fatal error is visible at normal
log levels (keep the same message keys like "err" and "command_uuid" and the
same context variable commandUUID and function
GetVPPAppInstallStatusByCommandUUID).
- Line 1075: Change the log key from "details" to the conventional "err" in the
ErrorContext call so the error is recognized consistently; update the call to
svc.logger.ErrorContext(...) that currently passes "details", authzErr to
instead pass "err", authzErr (look for the ErrorContext invocation around
svc.logger.ErrorContext and the authzErr variable in mdm.go).
In `@server/service/microsoft_mdm.go`:
- Around line 1081-1084: In svc.logger.DebugContext(ctx, "invalid discover
message", ...) replace the "err", err.Error() attribute with "err", err so the
error is passed as an error value (preserving type and structured metadata)
alongside the existing "request_xml", string(req.Raw) arguments; update the call
site using the svc.logger.DebugContext method, keeping ctx and req.Raw
unchanged.
There was a problem hiding this comment.
Pull request overview
This PR continues the ongoing slog migration effort (issue #40054, following #38889) by converting MDM-related service files from go-kit/log to slog. The migration updates all logging calls in the MDM service layer to use the slog-style context-aware logging methods.
Changes:
- Migrated logging calls from
level.Debug/Info/Warn/Error(logger).Log(...)tologger.DebugContext/InfoContext/WarnContext/ErrorContext(ctx, ...) - Removed unused
go-kit/log/levelimport from MDM service files - Added changelog entry documenting completion of slog migration
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| server/service/microsoft_mdm.go | Migrated 7 logging calls to slog pattern; removed level import |
| server/service/mdm.go | Migrated 6 logging calls to slog pattern; removed level import |
| server/service/apple_mdm.go | Migrated 80+ logging calls to slog pattern; removed level import |
| changes/40054-slog-2 | Added user-facing changelog entry for completed migration |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #40263 +/- ##
=======================================
Coverage 66.27% 66.27%
=======================================
Files 2459 2459
Lines 197024 196999 -25
Branches 8609 8609
=======================================
- Hits 130568 130554 -14
+ Misses 54637 54626 -11
Partials 11819 11819
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:
|
# Conflicts: # server/service/apple_mdm.go
Related issue: Resolves #40054
Checklist for submitter
changes/,orbit/changes/oree/fleetd-chrome/changes.Testing
Summary by CodeRabbit