Incremental migration to slog - #40120
Conversation
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Pull request overview
This PR continues the incremental migration from go-kit/log to slog across multiple packages in the Fleet server codebase. The migration updates logger types from kitlog.Logger/*logging.Logger to *slog.Logger and replaces go-kit logging calls with slog's context-aware logging methods.
Changes:
- Migrates logging in pubsub, live_query, errorstore, geoip, and SCIM packages from go-kit/log to slog
- Updates test files to use
slog.New(slog.DiscardHandler)for no-op logging - Adds context awareness to logging calls throughout migrated code
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| server/service/testing_utils.go | Updates calls to errorstore and scim to pass slog logger via SlogLogger() adapter |
| server/pubsub/testing_utils.go | Replaces logging.NewNopLogger() with slog.New(slog.DiscardHandler) in test setup |
| server/pubsub/redis_query_results.go | Migrates redisQueryResults logger field to *slog.Logger and updates all logging calls to use InfoContext/ErrorContext/DebugContext |
| server/live_query/redis_live_query_test.go | Updates test to use slog.New(slog.DiscardHandler) for no-op logging |
| server/live_query/redis_live_query.go | Migrates redisLiveQuery logger to *slog.Logger and updates WarnContext calls (uses context.TODO() where context unavailable) |
| server/fleet/geoip.go | Migrates MaxMindGeoIP logger to *slog.Logger and updates DebugContext calls with proper context |
| server/errorstore/errors_test.go | Updates all test cases to use slog.New(slog.DiscardHandler) instead of kitlog.NewNopLogger() |
| server/errorstore/errors.go | Migrates Handler logger to *slog.Logger and updates ErrorContext calls throughout |
| ee/server/scim/users_test.go | Updates test handler to use slog.New(slog.DiscardHandler) for logger |
| ee/server/scim/users.go | Migrates UserHandler logger to *slog.Logger, extracts context from requests, and updates all logging calls to use slog methods with context |
| ee/server/scim/scim.go | Migrates SCIM middleware and error handlers to use *slog.Logger, updates scimErrorLogger adapter |
| ee/server/scim/groups.go | Migrates GroupHandler logger to *slog.Logger, extracts context from requests, and updates all logging calls to use slog methods with context |
| cmd/fleet/serve.go | Updates initialization code to call SlogLogger() adapter when passing logger to migrated components |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
WalkthroughThis pull request migrates the logging backend from go-kit/log to Go's standard library slog across multiple Fleet components. Function signatures are updated to accept Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (2 warnings, 1 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}
There was a problem hiding this comment.
🧹 Nitpick comments (4)
server/live_query/redis_live_query.go (1)
253-253:context.TODO()prevents context-aware log correlation.
collectBatchQueriesForHostdoesn't receive acontext.Context, socontext.TODO()is used here. Whilecontext.TODO()is the correct placeholder idiom, downstream logging infrastructure (e.g., trace-ID propagation) won't see the request context. Same applies to line 428 inside the goroutine inloadCache.Consider threading
ctxthroughcollectBatchQueriesForHostandloadCache(the call chain originates fromQueriesForHostwhich could carry a context) in a follow-up refactor.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/live_query/redis_live_query.go` at line 253, collectBatchQueriesForHost currently uses context.TODO() when calling r.logger.WarnContext (and loadCache spawns a goroutine that does the same), which prevents request-scoped trace/log correlation; update the function signatures to accept a context.Context (add ctx parameter to collectBatchQueriesForHost and loadCache and propagate it from QueriesForHost), replace context.TODO() with the passed ctx in r.logger.WarnContext calls, and ensure the goroutine in loadCache captures and uses that ctx (or a derived context) so logging and tracing downstream receive the real request context.cmd/fleet/serve.go (1)
87-89: Duplicate import ofgithub.com/go-kit/log.Lines 87-88 import the same package under two aliases (
logandkitlog). Both resolve to the same type. This is pre-existing and not introduced by this PR, but worth consolidating to a single alias in a future cleanup.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/fleet/serve.go` around lines 87 - 89, The file imports the same package twice as two aliases (`log` and `kitlog`), causing a duplicate import; remove one of the imports (choose either `log` or `kitlog`) and update any references to the removed alias (e.g., occurrences of kitlog or log used with functions like kitlog.NewJSONLogger or level.Error) to use the remaining alias so all usages reference the single imported symbol.ee/server/scim/scim.go (1)
377-394:context.TODO()inerrorHandler— consider accepting context.
errorHandleris always called from HTTP handler contexts wherer.Context()is available (lines 212 and 370), yet it usescontext.TODO()forErrorContextcalls. Passing acontext.Contextparameter would allow proper context propagation for tracing/observability.Suggested improvement
-func errorHandler(w http.ResponseWriter, logger *slog.Logger, detail string, status int) { +func errorHandler(w http.ResponseWriter, logger *slog.Logger, ctx context.Context, detail string, status int) { scimErr := scimerrors.ScimError{ Status: status, Detail: detail, } raw, err := json.Marshal(scimErr) if err != nil { - logger.ErrorContext(context.TODO(), "failed marshaling scim error", "scimError", scimErr, "err", err) + logger.ErrorContext(ctx, "failed marshaling scim error", "scimError", scimErr, "err", err) return } w.Header().Set("Content-Type", "application/scim+json") w.WriteHeader(scimErr.Status) _, err = w.Write(raw) if err != nil { - logger.ErrorContext(context.TODO(), "failed writing response", "err", err) + logger.ErrorContext(ctx, "failed writing response", "err", err) } }The callers would need updating too (the closure at line 211 and the call at line 370).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ee/server/scim/scim.go` around lines 377 - 394, Change errorHandler to accept a context.Context parameter and use that context when calling logger.ErrorContext instead of context.TODO(); update its signature (errorHandler) and all call sites (the HTTP handler closure that currently calls errorHandler and the direct call near where scimerrors.ScimError is created) to pass r.Context() from the incoming *http.Request, ensuring you still set headers/status and write the marshaled scimErr as before; keep using scimerrors.ScimError, json.Marshal, and logger.ErrorContext but replace context.TODO() with the propagated ctx parameter.ee/server/scim/users.go (1)
593-614: Pre-existing: User email logged at Info/Warn level.These lines log
fleetUser.Email(PII) at Info and Warn levels. While this isn't new to this PR (the slog migration preserves existing logging behavior), it's worth flagging for a future pass to ensure compliance with PII logging policies.Lines 594, 608, and 614 all include
"email", fleetUser.Emailin structured log output.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ee/server/scim/users.go` around lines 593 - 614, The logs in the SCIM user deletion flow currently include PII (fleetUser.Email) in calls to u.logger.InfoContext and u.logger.WarnContext (the "skipping deletion..." log, the "cannot delete last global admin..." warn, and the "deleting fleet user via SCIM deletion" info); remove or redact the raw email before logging to avoid emitting PII. Update the three logging sites that reference "email", fleetUser.Email to either omit the email key entirely or replace it with a non-PII token (e.g., a hashed/partial redaction string derived from fleetUser.Email) so that u.logger.InfoContext / u.logger.WarnContext no longer receive the plain email value. Ensure the change is applied in the SCIM deletion flow around the checks for fleetUser.APIOnly / fleetUser.SSOEnabled and the GlobalRole/CountGlobalAdmins branch.
🤖 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/serve.go`:
- Around line 87-89: The file imports the same package twice as two aliases
(`log` and `kitlog`), causing a duplicate import; remove one of the imports
(choose either `log` or `kitlog`) and update any references to the removed alias
(e.g., occurrences of kitlog or log used with functions like
kitlog.NewJSONLogger or level.Error) to use the remaining alias so all usages
reference the single imported symbol.
In `@ee/server/scim/scim.go`:
- Around line 377-394: Change errorHandler to accept a context.Context parameter
and use that context when calling logger.ErrorContext instead of context.TODO();
update its signature (errorHandler) and all call sites (the HTTP handler closure
that currently calls errorHandler and the direct call near where
scimerrors.ScimError is created) to pass r.Context() from the incoming
*http.Request, ensuring you still set headers/status and write the marshaled
scimErr as before; keep using scimerrors.ScimError, json.Marshal, and
logger.ErrorContext but replace context.TODO() with the propagated ctx
parameter.
In `@ee/server/scim/users.go`:
- Around line 593-614: The logs in the SCIM user deletion flow currently include
PII (fleetUser.Email) in calls to u.logger.InfoContext and u.logger.WarnContext
(the "skipping deletion..." log, the "cannot delete last global admin..." warn,
and the "deleting fleet user via SCIM deletion" info); remove or redact the raw
email before logging to avoid emitting PII. Update the three logging sites that
reference "email", fleetUser.Email to either omit the email key entirely or
replace it with a non-PII token (e.g., a hashed/partial redaction string derived
from fleetUser.Email) so that u.logger.InfoContext / u.logger.WarnContext no
longer receive the plain email value. Ensure the change is applied in the SCIM
deletion flow around the checks for fleetUser.APIOnly / fleetUser.SSOEnabled and
the GlobalRole/CountGlobalAdmins branch.
In `@server/live_query/redis_live_query.go`:
- Line 253: collectBatchQueriesForHost currently uses context.TODO() when
calling r.logger.WarnContext (and loadCache spawns a goroutine that does the
same), which prevents request-scoped trace/log correlation; update the function
signatures to accept a context.Context (add ctx parameter to
collectBatchQueriesForHost and loadCache and propagate it from QueriesForHost),
replace context.TODO() with the passed ctx in r.logger.WarnContext calls, and
ensure the goroutine in loadCache captures and uses that ctx (or a derived
context) so logging and tracing downstream receive the real request context.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #40120 +/- ##
=======================================
Coverage 66.29% 66.29%
=======================================
Files 2446 2446
Lines 196002 196018 +16
Branches 8640 8640
=======================================
+ Hits 129931 129946 +15
- Misses 54304 54306 +2
+ Partials 11767 11766 -1
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:
|
sgress454
left a comment
sgress454
left a comment
There was a problem hiding this comment.
Looked at all files and I couldn't detect any issues with my human eyes.
| level.Error(g.logger).Log("msg", "failed to get displayName", "err", err) | ||
| g.logger.ErrorContext(r.Context(), "failed to get displayName", "err", err) |
There was a problem hiding this comment.
FWIW I like the ctx := r.Context() pattern better than calling r.Context() everywhere. I don't think it matters here because these all just return immediately after logging, and we don't update the context anywhere, but if we did (for example, to add a log topic) then we'd want to use the var.
| // We do not save unauthenticated error details; we simply log them. | ||
| level.Info(logger).Log( | ||
| "msg", "unauthenticated request", | ||
| logger.InfoContext(r.Context(), "unauthenticated request", |
There was a problem hiding this comment.
same comment re: r.Context() vs. a shared ctx var
| level.Error(logger).Log("msg", "failed marshaling scim error", "scimError", scimErr, "err", err) | ||
| logger.ErrorContext(context.TODO(), "failed marshaling scim error", "scimError", scimErr, "err", err) |
There was a problem hiding this comment.
Is the goal to add context to every log to enable log traces? Otherwise we could just use logger.Error() here.
There was a problem hiding this comment.
Yes. We have a lint rule that explicitly forbids logger.Error()
Related issue: Resolves #40054
Checklist for submitter
If some of the following don't apply, delete the relevant line.
changes/,orbit/changes/oree/fleetd-chrome/changes.Testing
Summary by CodeRabbit