Replaced all kitlog.Logger instances with the intermediate *logging.Logger - #40425
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 is part of the ongoing slog migration effort (continuing from #38889, tracked in #40054). It replaces all kitlog.Logger (go-kit/log) instances with the intermediate *logging.Logger type throughout the codebase.
Changes:
- Updated all function signatures accepting logger parameters from
kitlog.Loggerto*logging.Logger - Added
AddSourcefield tologging.Optionsto enable source file and line number in log entries - Removed compile-time assertion that
*Loggerimplementskitlog.Logger(intentional move away from kitlog) - Created
zerologSlogHandlerin orbit SCEP client to adapt zerolog.Logger to slog.Handler for use with*logging.Logger - Enhanced
Logger.With()to return*Loggerdirectly instead ofkitlog.Logger, eliminating need for type assertions
Reviewed changes
Copilot reviewed 71 out of 71 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| server/platform/logging/logging.go | Added AddSource field to Options struct for source file logging |
| server/platform/logging/kitlog_adapter.go | Removed kitlog.Logger compile-time assertion, removed import of kitlog package |
| server/platform/logging/kitlog_adapter_test.go | Removed TestKitlogAdapterLevels test (level wrappers being phased out), simplified test setup |
| server/service/*.go | Updated logger parameter types from kitlog.Logger to *logging.Logger across service layer |
| server/datastore/mysql/*.go | Updated logger parameter types in datastore functions |
| server/logging/*.go | Updated all log writer constructors to accept *logging.Logger |
| server/mdm/scep/**/*.go | Updated SCEP server/client logger types and removed unnecessary level wrappers |
| ee/server/service/*.go | Updated EE service layer logger types |
| ee/orbit/pkg/scep/scep.go | Implemented zerologSlogHandler to bridge zerolog and slog/logging.Logger |
| cmd/fleet/serve.go | Updated devSQLInterceptor and helper function logger types |
| cmd/fleetctl/**/*.go | Updated fleetctl command logger types |
| pkg/mdm/mdmtest/apple.go | Updated test MDM client logger type |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
WalkthroughThis PR systematically migrates logging infrastructure across the codebase from go-kit/log types to a platform-specific 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.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/platform/logging/kitlog_adapter_test.go (1)
14-37:⚠️ Potential issue | 🟠 MajorAvoid parallel subtests sharing a single handler.
TestKitlogAdapterruns subtests in parallel while sharing one handler/adapter, soLastRecord()can observe logs from the other subtest and make this flaky. Create a handler/adapter per subtest or dropt.Parallel()inside the subtests.✅ Suggested fix: give each subtest its own handler/adapter
func TestKitlogAdapter(t *testing.T) { t.Parallel() - handler := testutils.NewTestHandler() - adapter := NewLogger(slog.New(handler)) - t.Run("basic logging via Log method", func(t *testing.T) { t.Parallel() + handler := testutils.NewTestHandler() + adapter := NewLogger(slog.New(handler)) err := adapter.Log("msg", "hello world", "key", "value") require.NoError(t, err) @@ t.Run("with context via With", func(t *testing.T) { t.Parallel() + handler := testutils.NewTestHandler() + adapter := NewLogger(slog.New(handler)) contextLogger := adapter.With("component", "test-component") err := contextLogger.Log("msg", "message with context")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/platform/logging/kitlog_adapter_test.go` around lines 14 - 37, The test shares a single handler/adapter across parallel subtests which makes LastRecord() flaky; for each subtest create its own handler and adapter instead of using the outer handler/adapter (e.g., call testutils.NewTestHandler() and NewLogger(slog.New(handler)) inside each t.Run) so contextLogger := adapter.With(...) and calls to adapter.Log or contextLogger.Log operate on an isolated handler per subtest; update both subtests in TestKitlogAdapter to initialize their own handler/adapter and leave the t.Parallel() calls if you want parallelism.
🧹 Nitpick comments (4)
ee/orbit/pkg/scep/scep.go (1)
311-313:WithGroupsilently drops the group name.Returning
hunchanged means any grouped attributes from callers will lose their namespace. This is acceptable if slog groups are never used on this path, but worth a brief comment so future maintainers don't expect group support.Optional: add a comment
func (h *zerologSlogHandler) WithGroup(_ string) slog.Handler { - return h + // zerolog does not support slog groups; attributes within a group + // are flattened into the parent namespace. + return h }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ee/orbit/pkg/scep/scep.go` around lines 311 - 313, The WithGroup method on zerologSlogHandler currently ignores the provided group name and returns h unchanged, which silently drops grouped attribute namespaces; update the WithGroup implementation to either preserve group semantics or, if groups are intentionally unsupported, add a concise comment above zerologSlogHandler.WithGroup stating that slog groups are intentionally ignored on this handler (mentioning any rationale such as compatibility with zerolog or no-op behavior) so future maintainers won't expect group support; keep the function body unchanged if choosing the comment route.server/mdm/scep/cmd/scepclient/scepclient.go (1)
64-64:lginfois now a redundant alias forlogger.Since the previous level-filtered wrapper (likely
level.Info(logger)) is gone,lginfoandloggerpoint to the same instance. The variable can be removed and its two use-sites (lines 187, 191) can calllogger.Log(...)directly.♻️ Proposed cleanup
- lginfo := logger - client, err := scepclient.New(cfg.serverURL, logger)- lginfo.Log("pkiStatus", "PENDING", "msg", "sleeping for 30 seconds, then trying again.") + logger.Log("pkiStatus", "PENDING", "msg", "sleeping for 30 seconds, then trying again.")- lginfo.Log("pkiStatus", "SUCCESS", "msg", "server returned a certificate.") + logger.Log("pkiStatus", "SUCCESS", "msg", "server returned a certificate.")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/mdm/scep/cmd/scepclient/scepclient.go` at line 64, Remove the redundant lginfo alias and call logger directly: delete the lginfo := logger declaration and replace the two lginfo.Log(...) call sites (currently using lginfo at the scepclient command execution points) with logger.Log(...) (preserving the same log level and fields/arguments). Ensure no other references to lginfo remain and run go vet/static checks to confirm compilation.server/service/mdm_scep.go (1)
26-26: Wire the provided logger or remove the parameter.
NewSCEPServiceaccepts a logger but always setsdebugLoggerto a no-op, so caller-supplied logging is ignored. Consider using the passed logger (with a nil fallback) or dropping the parameter to avoid a misleading API.Suggested fix
func NewSCEPService(ds fleet.MDMAssetRetriever, signer scepserver.CSRSignerContext, logger *logging.Logger) scepserver.Service { + if logger == nil { + logger = logging.NewNopLogger() + } return &service{ signer: signer, - debugLogger: logging.NewNopLogger(), + debugLogger: logger, ds: ds, } }Also applies to: 86-90
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/service/mdm_scep.go` at line 26, NewSCEPService currently ignores the provided logger and always assigns a no-op to the service field debugLogger; update NewSCEPService (and the other constructor/initialization at the block around lines 86-90) to wire the caller-supplied logger into the service by assigning s.debugLogger = logger when logger != nil and only fall back to a no-op when logger is nil (alternatively remove the logger parameter if you prefer to not accept it); reference the NewSCEPService function and the service struct's debugLogger field when making the change.server/service/endpoint_setup.go (1)
329-344: Pre-existing:deferin loop leaks file descriptors until function return.Not introduced by this PR, but worth noting:
defer resp.Body.Close()(line 333) anddefer file.Close()(line 344) are inside theforloop but deferred to function exit. For many scripts, this accumulates open file handles. Consider closing them explicitly at the end of each iteration or extracting the loop body into a helper function.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/service/endpoint_setup.go` around lines 329 - 344, The loop that downloads scripts calls httpClient.Do(req) and then uses defer resp.Body.Close() and defer file.Close() inside the loop body, which defers closes until the surrounding function returns and can leak file descriptors; modify the code so resp.Body.Close() and file.Close() are called explicitly at the end of each iteration (or move the loop body into a helper like downloadScript(scriptName, localPath) so the defers run at helper return) — ensure you remove the in-loop defers and replace them with immediate Close() calls (or keep defers inside the new helper) for the http response and the created file obtained via os.Create(localPath).
🤖 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/orbit/pkg/scep/scep.go`:
- Around line 307-309: WithAttrs currently returns &zerologSlogHandler{logger:
h.logger, attrs: append(h.attrs, attrs...)}, which can reuse h.attrs' backing
array and cause sibling handlers to share/overwrite entries; change WithAttrs
(method on zerologSlogHandler) to allocate a fresh slice (or use slices.Concat)
and copy both h.attrs and the incoming attrs into it before returning the new
zerologSlogHandler so the parent’s backing array is never aliased.
- Around line 290-305: The Handle method currently ignores r.Level and always
starts with h.logger.Info(), losing severity; update zerologSlogHandler.Handle
to inspect r.Level and choose the appropriate zerolog event (e.g.,
h.logger.Debug(), Info(), Warn(), Error(), Trace() or Fatal() as appropriate)
before attaching attrs and message, using r.Level to map
slog.LevelDebug/Info/Warn/Error etc. to the corresponding zerolog level and then
proceed to call event.Interface(...) for h.attrs and r.Attrs(...) and finally
event.Msg(r.Message).
---
Outside diff comments:
In `@server/platform/logging/kitlog_adapter_test.go`:
- Around line 14-37: The test shares a single handler/adapter across parallel
subtests which makes LastRecord() flaky; for each subtest create its own handler
and adapter instead of using the outer handler/adapter (e.g., call
testutils.NewTestHandler() and NewLogger(slog.New(handler)) inside each t.Run)
so contextLogger := adapter.With(...) and calls to adapter.Log or
contextLogger.Log operate on an isolated handler per subtest; update both
subtests in TestKitlogAdapter to initialize their own handler/adapter and leave
the t.Parallel() calls if you want parallelism.
---
Nitpick comments:
In `@ee/orbit/pkg/scep/scep.go`:
- Around line 311-313: The WithGroup method on zerologSlogHandler currently
ignores the provided group name and returns h unchanged, which silently drops
grouped attribute namespaces; update the WithGroup implementation to either
preserve group semantics or, if groups are intentionally unsupported, add a
concise comment above zerologSlogHandler.WithGroup stating that slog groups are
intentionally ignored on this handler (mentioning any rationale such as
compatibility with zerolog or no-op behavior) so future maintainers won't expect
group support; keep the function body unchanged if choosing the comment route.
In `@server/mdm/scep/cmd/scepclient/scepclient.go`:
- Line 64: Remove the redundant lginfo alias and call logger directly: delete
the lginfo := logger declaration and replace the two lginfo.Log(...) call sites
(currently using lginfo at the scepclient command execution points) with
logger.Log(...) (preserving the same log level and fields/arguments). Ensure no
other references to lginfo remain and run go vet/static checks to confirm
compilation.
In `@server/service/endpoint_setup.go`:
- Around line 329-344: The loop that downloads scripts calls httpClient.Do(req)
and then uses defer resp.Body.Close() and defer file.Close() inside the loop
body, which defers closes until the surrounding function returns and can leak
file descriptors; modify the code so resp.Body.Close() and file.Close() are
called explicitly at the end of each iteration (or move the loop body into a
helper like downloadScript(scriptName, localPath) so the defers run at helper
return) — ensure you remove the in-loop defers and replace them with immediate
Close() calls (or keep defers inside the new helper) for the http response and
the created file obtained via os.Create(localPath).
In `@server/service/mdm_scep.go`:
- Line 26: NewSCEPService currently ignores the provided logger and always
assigns a no-op to the service field debugLogger; update NewSCEPService (and the
other constructor/initialization at the block around lines 86-90) to wire the
caller-supplied logger into the service by assigning s.debugLogger = logger when
logger != nil and only fall back to a no-op when logger is nil (alternatively
remove the logger parameter if you prefer to not accept it); reference the
NewSCEPService function and the service struct's debugLogger field when making
the change.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (71)
cmd/fleet/serve.gocmd/fleetctl/fleetctl/preview.gocmd/fleetctl/integrationtest/gitops/gitops_enterprise_integration_test.gocmd/osquery-perf/hostidentity/hostidentity.goee/orbit/pkg/scep/scep.goee/orbit/pkg/scep/scep_test.goee/server/service/condaccess/depot/depot.goee/server/service/condaccess/idp.goee/server/service/condaccess/scep.goee/server/service/est/est.goee/server/service/hostidentity/httpsig/httpsig.goee/server/service/hostidentity/httpsig/middleware.goee/server/service/hostidentity/scep.goee/server/service/scep_proxy.goee/server/service/scep_proxy_test.goee/server/service/software_installers.goee/server/service/testing_utils.gopkg/mdm/mdmtest/apple.goserver/datastore/mysql/apple_mdm.goserver/datastore/mysql/hosts.goserver/datastore/mysql/mdm.goserver/datastore/mysql/mysql.goserver/datastore/mysql/operating_system_vulnerabilities.goserver/datastore/mysql/scim.goserver/datastore/mysql/software.goserver/logging/filesystem.goserver/logging/filesystem_test.goserver/logging/firehose.goserver/logging/firehose_test.goserver/logging/kinesis.goserver/logging/kinesis_test.goserver/logging/lambda.goserver/logging/lambda_test.goserver/logging/logging.goserver/logging/nats.goserver/logging/nats_test.goserver/logging/pubsub.goserver/logging/webhook.goserver/logging/webhook_test.goserver/mdm/lifecycle/lifecycle.goserver/mdm/lifecycle/lifecycle_test.goserver/mdm/scep/client/client.goserver/mdm/scep/cmd/scepclient/scepclient.goserver/mdm/scep/cmd/scepserver/scepserver.goserver/mdm/scep/csrverifier/executable/csrverifier.goserver/mdm/scep/server/endpoint.goserver/mdm/scep/server/service.goserver/mdm/scep/server/service_logging.goserver/mdm/scep/server/transport.goserver/mdm/scep/server/transport_test.goserver/platform/logging/kitlog_adapter.goserver/platform/logging/kitlog_adapter_test.goserver/platform/logging/logging.goserver/service/activities.goserver/service/async/async.goserver/service/devices_url_auth_test.goserver/service/endpoint_campaigns.goserver/service/endpoint_middleware.goserver/service/endpoint_middleware_test.goserver/service/endpoint_setup.goserver/service/endpoint_setup_test.goserver/service/endpoint_utils.goserver/service/frontend.goserver/service/frontend_test.goserver/service/integrationtest/scep_server/scep.goserver/service/mdm_scep.goserver/service/modules/activities/activities.goserver/service/osquery.goserver/service/service_campaign_test.goserver/service/service_campaigns.goserver/service/software_installers_test.go
💤 Files with no reviewable changes (1)
- server/platform/logging/kitlog_adapter.go
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #40425 +/- ##
==========================================
+ Coverage 66.26% 66.29% +0.02%
==========================================
Files 2460 2460
Lines 196773 197278 +505
Branches 8754 8596 -158
==========================================
+ Hits 130394 130782 +388
- Misses 54563 54656 +93
- Partials 11816 11840 +24
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:
|
…itlog # Conflicts: # ee/server/service/condaccess/depot/depot.go # server/datastore/mysql/apple_mdm.go # server/datastore/mysql/hosts.go # server/datastore/mysql/mdm.go # server/datastore/mysql/mysql.go # server/datastore/mysql/operating_system_vulnerabilities.go # server/datastore/mysql/scim.go # server/datastore/mysql/software.go
ksykulev
approved these changes
Feb 25, 2026
ksykulev
left a comment
ksykulev
left a comment
Contributor
There was a problem hiding this comment.
I see we switched to using *logging.Logger everywhere. Is the goal to eventually use .SlogLogger() everywhere?
| delete(fields, "msg") | ||
| } | ||
| } | ||
| func (h *zerologSlogHandler) Handle(_ context.Context, r slog.Record) error { |
Contributor
There was a problem hiding this comment.
The old Log method didn't use the context, this one accepts it. Do we want to use it anywhere? Maybe for the request or trace id? idk just a thought. Non-block comment.
Member
Author
There was a problem hiding this comment.
Yes, we want to use context if we can, since then we can link log lines to OTEL traces.
I'm just focusing on transitioning to slog and not fixing context everywhere. (I fix it some places if not too hard.)
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