Migrated to slog method signatures in service files - #40468
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 migrates service layer files from go-kit/log to slog logging as part of the ongoing effort tracked in issue #40054. The migration converts logging method calls from the go-kit/log pattern (level.Error(logger).Log(...)) to slog's context-aware methods (logger.ErrorContext(ctx, ...)).
Changes:
- Migrated logging calls in service files from go-kit/log to slog, converting
level.{Error,Info,Debug,Warn}(logger).Log(...)patterns tologger.{Error,Info,Debug,Warn}Context(ctx, ...) - Updated logger adapters (NanoDEPLogger, slogAdapter) to use slog patterns with context support
- Removed go-kit/log/level imports from files where logging has been fully migrated
Reviewed changes
Copilot reviewed 23 out of 23 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| server/service/users.go | Migrated error logging calls to slog pattern |
| server/service/schedule/schedule.go | Migrated all logging calls (debug, info, error) in schedule management to slog |
| server/service/queries.go | Migrated query-related error logging to slog |
| server/service/orbit.go | Migrated extensive logging across Orbit enrollment, disk encryption, and script/software installation flows |
| server/service/live_queries.go | Migrated live query campaign logging to slog |
| server/service/hosts.go | Migrated host detail and encryption key logging to slog |
| server/service/handler.go | Updated NanoMDMLogger adapter and MDM enrollment middleware logging |
| server/service/devices.go | Migrated fleetd error logging to slog |
| server/service/conditional_access_microsoft.go | Migrated conditional access error logging to slog |
| server/service/appconfig.go | Migrated app config validation logging to slog |
| server/mdm/apple/apple_mdm.go | Updated DEP service to use new NanoDEPLogger signature with context |
| server/logging/nanodep.go | Rewrote NanoDEPLogger adapter to use slog with context stored as field |
| ee/server/service/vpp.go | Migrated VPP app error and warning logging to slog |
| ee/server/service/setup_experience.go | Migrated setup experience warning logging to slog |
| ee/server/service/request_certificate.go | Migrated certificate request error and info logging to slog |
| ee/server/service/in_house_apps.go | Migrated in-house app error logging to slog |
| ee/server/service/hosts.go | Migrated host wipe debug logging to slog |
| ee/server/service/devices.go | Migrated device migration webhook debug logging to slog |
| ee/server/service/condaccess/log_adapter.go | Renamed and rewrote adapter from kitlogAdapter to slogAdapter using slog with context |
| ee/server/service/condaccess/idp.go | Updated IDP service to use slogAdapter instead of kitlogAdapter |
| ee/server/service/certificate_authorities.go | Migrated certificate authority validation and batch processing logging to slog |
| cmd/fleet/vuln_process.go | Updated vulnerability processing command logging and configureVulnPath signature |
| cmd/fleet/cron.go | Updated cron job to pass context to configureVulnPath |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
WalkthroughThis PR continues the migration from go-kit/log level-based logging to context-aware slog logging. It removes the go-kit/log/level import dependency across multiple service files and replaces logging calls with context-aware logger methods (InfoContext, ErrorContext, DebugContext, WarnContext). Additionally, the PR updates logging adapters and adds context parameters to functions where needed, such as configureVulnPath, to enable context propagation through the logging chain. 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)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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.
Actionable comments posted: 3
🧹 Nitpick comments (1)
ee/server/service/condaccess/log_adapter.go (1)
23-25:*lnmethods should preserve line-style formatting semantics.Line 24, Line 36, and Line 52 use
fmt.Sprint(v...), which can collapse expectedPrintln-style formatting. Considerfmt.Sprintln(trim trailing newline before logging if desired) for parity.♻️ Proposed adjustment
import ( "context" "fmt" "log/slog" + "strings" ) @@ func (k *slogAdapter) Println(v ...any) { - k.logger.InfoContext(k.ctx, fmt.Sprint(v...)) + k.logger.InfoContext(k.ctx, strings.TrimSuffix(fmt.Sprintln(v...), "\n")) } @@ func (k *slogAdapter) Fatalln(v ...any) { - k.logger.ErrorContext(k.ctx, fmt.Sprint(v...)) + k.logger.ErrorContext(k.ctx, strings.TrimSuffix(fmt.Sprintln(v...), "\n")) } @@ func (k *slogAdapter) Panicln(v ...any) { - msg := fmt.Sprint(v...) + msg := strings.TrimSuffix(fmt.Sprintln(v...), "\n") k.logger.ErrorContext(k.ctx, msg) panic(msg) }Also applies to: 35-37, 51-53
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ee/server/service/condaccess/log_adapter.go` around lines 23 - 25, The Println-style methods on slogAdapter currently use fmt.Sprint(v...) which loses the newline/spacing semantics; update each "*ln" method (e.g., slogAdapter.Println and the other ln-suffixed methods in the same file) to use fmt.Sprintln(v...) and then trim the trailing newline (e.g., via strings.TrimSuffix(..., "\n")) before passing the result into k.logger.InfoContext/k.logger.ErrorContext so the logged message preserves Println formatting while not including a literal trailing newline.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ee/server/service/certificate_authorities.go`:
- Around line 506-507: The log message in the branch using
svc.logger.DebugContext incorrectly states "no certificate authority changes to
apply" while that branch is actually hit when dryRun is true and changes are
intentionally skipped; update the log call in the function handling batch apply
(where dryRun is checked) to a clear dry-run message such as "dry run enabled;
skipping application of certificate authority changes" (optionally include
contextual info like number of pending changes or identifiers) and keep the same
logger/context usage (svc.logger.DebugContext(ctx, ...), referencing the dryRun
variable and the surrounding batch apply logic).
In `@ee/server/service/request_certificate.go`:
- Around line 97-102: The ErrorContext calls in request_certificate.go are
logging sensitive identifiers (csr_email, csr_upn, idp_username) in
svc.logger.ErrorContext when CSR and IDP values mismatch; update the calls in
the svc.logger.ErrorContext usages (the ones referencing csrEmail, csrUsername
and introspectionResponse.Username, including the similar log at the later
occurrence around line 137) to stop emitting raw PII—either remove those
key/value pairs entirely or replace them with non-identifying placeholders
(e.g., "redacted" or a boolean/hash indicator) and keep the existing messages
and error handling (e.g., InvalidCSRError) intact so behavior is unchanged but
PII is not logged.
In `@server/service/users.go`:
- Line 136: The log call svc.logger.ErrorContext(ctx, "password not set during
admin user creation", "err", err) is logging a stale or nil err value; update
the branch in the admin user creation logic (the code path that checks for a
missing password) to either remove the "err" field from the ErrorContext call or
pass a real error value (e.g., create and use a new error like
errors.New("password not set") or wrap context with fmt.Errorf) so the logged
fields accurately reflect the failure; locate the check around the admin
creation function where svc.logger.ErrorContext is invoked and fix that single
call to avoid emitting a misleading nil/stale err.
---
Nitpick comments:
In `@ee/server/service/condaccess/log_adapter.go`:
- Around line 23-25: The Println-style methods on slogAdapter currently use
fmt.Sprint(v...) which loses the newline/spacing semantics; update each "*ln"
method (e.g., slogAdapter.Println and the other ln-suffixed methods in the same
file) to use fmt.Sprintln(v...) and then trim the trailing newline (e.g., via
strings.TrimSuffix(..., "\n")) before passing the result into
k.logger.InfoContext/k.logger.ErrorContext so the logged message preserves
Println formatting while not including a literal trailing newline.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (23)
cmd/fleet/cron.gocmd/fleet/vuln_process.goee/server/service/certificate_authorities.goee/server/service/condaccess/idp.goee/server/service/condaccess/log_adapter.goee/server/service/devices.goee/server/service/hosts.goee/server/service/in_house_apps.goee/server/service/request_certificate.goee/server/service/setup_experience.goee/server/service/vpp.goserver/logging/nanodep.goserver/mdm/apple/apple_mdm.goserver/service/appconfig.goserver/service/conditional_access_microsoft.goserver/service/devices.goserver/service/handler.goserver/service/hosts.goserver/service/live_queries.goserver/service/orbit.goserver/service/queries.goserver/service/schedule/schedule.goserver/service/users.go
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #40468 +/- ##
==========================================
- Coverage 66.27% 66.26% -0.01%
==========================================
Files 2461 2461
Lines 197390 197385 -5
Branches 8716 8716
==========================================
- Hits 130811 130799 -12
- Misses 54739 54744 +5
- Partials 11840 11842 +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:
|
|
|
||
| func (l *NanoMDMLogger) Info(keyvals ...interface{}) { | ||
| level.Info(l.logger).Log(keyvals...) | ||
| l.logger.InfoContext(context.TODO(), "", keyvals...) |
juan-fdz-hawa
approved these changes
Feb 26, 2026
4 tasks
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 #40054
Checklist for submitter
changes/,orbit/changes/oree/fleetd-chrome/changes.Testing
Summary by CodeRabbit
Release Notes