Changes needed before gokit/log to slog transition. - #39527
Conversation
# Conflicts: # cmd/fleet/main.go # cmd/fleet/vuln_process.go # server/platform/logging/logging.go
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #39527 +/- ##
==========================================
+ Coverage 66.22% 66.23% +0.01%
==========================================
Files 2436 2436
Lines 195013 195017 +4
Branches 8454 8587 +133
==========================================
+ Hits 129144 129173 +29
+ Misses 54159 54138 -21
+ Partials 11710 11706 -4
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:
|
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
WalkthroughReplaces go-kit/kitlog logging with the new slog-based Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 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 full review |
✅ Actions performedFull review triggered. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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.go (1)
28-63:⚠️ Potential issue | 🟡 MinorOdd-length
keyvalssilently drops the last element.If
keyvalshas an odd number of elements, the loop boundlen(keyvals)-1skips the trailing key. This mirrors go-kit/log's own behavior (it would pair the trailing key withErrMissingValue), but the silent drop here is a slight behavior change. Unlikely to matter in practice since callers always pass pairs.
🧹 Nitpick comments (7)
ee/server/service/hostidentity/scep.go (1)
144-144:renewalMiddlewarestill acceptskitlog.Logger— consider migrating for consistency.This unexported function still takes
kitlog.Logger, which keeps thekitlogimport alive. It works fine today since*logging.Loggersatisfies the interface, but migrating it to*logging.Loggerwould let you drop thekitlogimport from this file entirely and be consistent with the rest of the changes.ee/server/service/condaccess/log_adapter.go (1)
10-12: Stale comment: referenceskitlog.Loggerbut the field is now*logging.Logger.Line 10's comment still says "adapts kitlog.Logger to saml logger.Interface" but the logger field is now
*logging.Logger. Consider updating the comment to reflect the new type.Proposed fix
-// kitlogAdapter adapts kitlog.Logger to saml logger.Interface +// kitlogAdapter adapts *logging.Logger to saml logger.Interfaceserver/service/integration_logger_test.go (1)
236-250:assertIPAddrLoggedhelper could produce confusing failures if multiple records have the attribute.The helper counts occurrences of
ip_addrandx_for_ip_addracross all records and asserts exactly 1 of each. If a future test case produces additional log records containing these attributes, the assertion will fail with a non-obvious message (expected 1, got 2). Consider adding a descriptive message to the assertions for easier debugging.Suggested improvement
- assert.Equal(t, 1, ipAddrCount) - assert.Equal(t, 1, xForIPAddrCount) + assert.Equal(t, 1, ipAddrCount, "expected exactly 1 record with ip_addr attribute") + assert.Equal(t, 1, xForIPAddrCount, "expected exactly 1 record with x_for_ip_addr attribute")server/mdm/apple/apple_mdm.go (1)
19-31: Two similarly-named logging imports may cause confusion.The file imports both
"github.com/fleetdm/fleet/v4/server/logging"(aslogging) and"github.com/fleetdm/fleet/v4/server/platform/logging"(asplatformlogging). This is functional but the naming proximity could trip up future contributors.Not blocking — just a note for awareness during the eventual Phase 3 cleanup when
server/loggingis retired.cmd/fleet/serve.go (1)
85-86: Duplicate import ofgithub.com/go-kit/log.Both
log(Line 85) andkitlog(Line 86) alias the same package. While both aliases appear to be in use (log.Loggeron Line 2001,kitlog.Loggeron Lines 1760/1921), consider unifying to a single alias to reduce confusion during the ongoing migration.cmd/fleet/cron.go (2)
1173-1225:verifyDiskEncryptionKeysuses barelogger.Log(...)consistently.Note that this function uses
logger.Log("err", ...)andlogger.Log("inf", ...)directly rather than going throughlevel.Error(logger)orlevel.Info(logger). This is pre-existing behavior and not introduced by this PR, but it means these log lines won't have alevelkey. Consider aligning withlevel.Error(logger).Log(...)in a follow-up for consistency.
1486-1490:cleanupCronStatsOnShutdownalso uses barelogger.Log.Same observation as
verifyDiskEncryptionKeys— nolevelkey in log output. Pre-existing, not introduced here.
There was a problem hiding this comment.
Pull request overview
This PR prepares the codebase for a gradual go-kit/log → slog migration by standardizing on Fleet’s server/platform/logging adapter and removing most remaining kitlog.With usage so log enrichment and future slog-native calls become simpler and type-safe.
Changes:
- Replace many
kitlog.Loggerusages with*platform/logging.Loggerand convertkitlog.With(...)tologger.With(...). - Update the core adapter (
server/platform/logging) and add convenience constructors (NewNopLogger,NewLogfmtLogger,NewJSONLogger). - Refactor numerous unit/integration tests to validate logs via
slogrecords/test handler rather than parsing serialized output.
Reviewed changes
Copilot reviewed 84 out of 84 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tools/mdm/apple/apnspush/main.go | Switch tool logger to platform logging and With helper. |
| server/worker/worker_test.go | Use platform NewNopLogger in worker tests. |
| server/worker/worker.go | Worker now stores *logging.Logger; replace kitlog.With with logger.With. |
| server/worker/macos_setup_assistant_test.go | Use platform NewNopLogger in test. |
| server/worker/db_migrations_test.go | Use platform nop/JSON loggers in test scaffolding. |
| server/worker/apple_mdm_test.go | Replace kitlog constructors with platform logging constructors. |
| server/service/testing_utils.go | Update test server/logger plumbing to platform logging types. |
| server/service/testing_client.go | Use platform nop logger for integration tests when disabled. |
| server/service/service_campaigns.go | Replace log.With usage with svc.logger.With. |
| server/service/service.go | Service stores *logging.Logger; constructor updated accordingly. |
| server/service/schedule/schedule.go | Schedule now stores *logging.Logger; update logger option and defaults. |
| server/service/osquery_test.go | Update test loggers to platform logging constructors. |
| server/service/osquery.go | Replace log.With usage with svc.logger.With. |
| server/service/integrationtest/suite.go | Integration suite logger type updated to *logging.Logger. |
| server/service/integrationtest/android/suite.go | Use platform logfmt logger in suite setup. |
| server/service/integration_sso_test.go | Use platform nop logger when integration-test logging disabled. |
| server/service/integration_smtp_test.go | Use platform nop logger when integration-test logging disabled. |
| server/service/integration_mdm_test.go | Replace kitlog constructors/types with platform logging equivalents. |
| server/service/integration_mdm_lifecycle_test.go | Replace kitlog JSON logger with platform JSON logger. |
| server/service/integration_logger_test.go | Rewrite log assertions to use slog.Record capture via test handler. |
| server/service/integration_live_queries_test.go | Use platform nop logger when integration-test logging disabled. |
| server/service/integration_install_test.go | Switch install test logger plumbing to platform logging. |
| server/service/integration_enterprise_test.go | Switch enterprise integration tests to platform logging and With. |
| server/service/hosts_test.go | Use platform nop logger in service struct construction. |
| server/service/handler_test.go | Update handler tests to pass platform nop logger. |
| server/service/handler.go | Handler APIs now accept *logging.Logger; replace kitlog.With with logger.With. |
| server/service/calendar/calendar.go | Calendar helpers now accept *logging.Logger and use .With. |
| server/service/apple_mdm_test.go | Replace kitlog usage with platform logging in MDM tests. |
| server/service/apple_mdm_ddm_test.go | Use platform logfmt logger in DDM test. |
| server/service/apple_mdm.go | Update MDM service structs/functions to use *platformlogging.Logger and .With. |
| server/pubsub/testing_utils.go | Use platform nop logger for redis query results tests. |
| server/pubsub/redis_query_results.go | Replace log.Logger with *logging.Logger; update With usage. |
| server/platform/logging/logging.go | Add discard handler + convenience constructors for nop/logfmt/json loggers. |
| server/platform/logging/kitlog_adapter_test.go | Update adapter test to assert on *Logger rather than old adapter type. |
| server/platform/logging/kitlog_adapter.go | Rename/reshape adapter as Logger, add .With and .SlogLogger(). |
| server/mdm/apple/apple_mdm_test.go | Replace go-kit nop logger with platform nop logger. |
| server/mdm/apple/apple_mdm_external_test.go | Replace go-kit nop logger with platform nop logger. |
| server/mdm/apple/apple_mdm.go | Update DEP service to use *platformlogging.Logger and .With. |
| server/mdm/apple/apple_bm.go | Update ABM metadata helpers to accept *logging.Logger. |
| server/logging/nanodep.go | Update NanoDEP logger adapter to use *platform/logging.Logger. |
| server/launcher/server.go | Launcher server now accepts *logging.Logger. |
| server/launcher/launcher_test.go | Use platform nop logger in launcher test wrapper. |
| server/launcher/launcher.go | Launcher wrapper now stores *logging.Logger. |
| server/health/health_test.go | Use platform nop logger in health tests. |
| server/health/health.go | Health handlers now take *logging.Logger and use .With. |
| server/fleet/mdm_test.go | Use platform nop logger in DEP client test. |
| server/cron/calendar_cron_test.go | Use platform logging constructors/types in calendar cron tests. |
| server/cron/calendar_cron.go | Calendar cron functions now accept *logging.Logger and use .With. |
| ee/server/service/software_installers.go | Replace log.With with svc.logger.With in EE service. |
| ee/server/service/service.go | EE service now stores/accepts *logging.Logger. |
| ee/server/service/request_certificate_test.go | Use platform logfmt logger in EE test. |
| ee/server/service/mdm_external_test.go | Use platform nop logger in EE MDM external test. |
| ee/server/service/hostidentity/scep.go | Host identity SCEP registration now takes *logging.Logger and .With. |
| ee/server/service/digicert/digicert.go | DigiCert service now stores *logging.Logger; default logger updated. |
| ee/server/service/condaccess/scep.go | Conditional access SCEP registration now takes *logging.Logger and .With. |
| ee/server/service/condaccess/log_adapter.go | SAML log adapter updated to use *logging.Logger. |
| ee/server/service/condaccess/idp_test.go | Use platform nop logger in conditional access IdP tests. |
| ee/server/service/condaccess/idp.go | Conditional access IdP now stores/accepts *logging.Logger and .With. |
| ee/server/service/certificate_authorities_test.go | Use platform logfmt logger in EE CA tests. |
| ee/server/service/calendar_test.go | Use platform nop logger in EE calendar test. |
| ee/server/scim/scim.go | SCIM registration now takes *logging.Logger and uses .With. |
| ee/server/integrationtest/scim/suite.go | Use platform logfmt logger in SCIM integration suite. |
| ee/server/integrationtest/hostidentity/suite.go | Use platform logfmt logger + .With for depot setup. |
| ee/server/integrationtest/condaccess/suite.go | Use platform logfmt logger + .With for depot setup. |
| ee/server/calendar/google_calendar_test.go | Use platform logfmt logger in calendar tests. |
| ee/server/calendar/google_calendar_mock.go | Use platform logging and .With in mock API. |
| ee/server/calendar/google_calendar_load.go | Use platform logging and .With in load API. |
| ee/server/calendar/google_calendar_integration_test.go | Use platform logfmt logger in integration test config. |
| ee/server/calendar/google_calendar.go | GoogleCalendarConfig now uses *logging.Logger. |
| cmd/maintained-apps/validate/windows.go | Update callbacks to accept *logging.Logger. |
| cmd/maintained-apps/validate/main.go | Replace kitlog setup/filtering with platform slog logger construction. |
| cmd/maintained-apps/validate/darwin.go | Update callbacks to accept *logging.Logger. |
| cmd/maintained-apps/validate/app_commander_test.go | Use platform nop logger in commander tests. |
| cmd/maintained-apps/validate/app_commander.go | Commander now stores *logging.Logger. |
| cmd/maintained-apps/main.go | Replace kitlog logger setup with platform slog logger construction. |
| cmd/fleetctl/fleetctl/trigger_test.go | Use platform nop logger in fleetctl trigger tests. |
| cmd/fleetctl/fleetctl/query_test.go | Use platform JSON logger without go-kit level filter setup. |
| cmd/fleet/vuln_process.go | Use .With("cron", ...) on platform logger. |
| cmd/fleet/serve_test.go | Update test schedule/logger wiring to platform logging types. |
| cmd/fleet/serve.go | Replace kitlog.With calls with .With(...) on platform logger. |
| cmd/fleet/main.go | initLogger now returns *logging.Logger and uses logging.NewLogger. |
| cmd/fleet/cron_test.go | Use platform nop logger in cron tests. |
| cmd/fleet/cron.go | Cron/scheduler functions now accept *logging.Logger and use .With. |
| .golangci.yml | Exclude (*platform/logging.Logger).Log from errcheck exclusions list. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // This is a drop-in replacement for kitlog.NewJSONLogger(). | ||
| func NewJSONLogger(output io.Writer) *Logger { | ||
| return NewLogger(NewSlogLogger(Options{Output: output, JSON: true, Debug: true})) | ||
| } |
There was a problem hiding this comment.
Do we want to hard code these to Debug: true always?
There was a problem hiding this comment.
Currently, this is only used in tests. So, I don't think it is a big deal.
I think a better pattern is to tie logging to t.Log in tests so that we can see which test produced which log output. This is useful when tests run in parallel.
# Conflicts: # cmd/fleet/cron.go # cmd/fleet/serve_test.go # server/worker/worker_test.go
Related issue: Resolves #38889
PLEASE READ BELOW before looking at file changes
Before converting individual files/packages to slog, we generally need to make these 2 changes to make the conversion easier:
kitlog.Withsince they are not fully compatible with our kitlog adapter*logging.LoggerNote: that I did not replace absolutely all uses of
kitlog.Logger, but I did remove all uses ofkitlog.Withexcept for these due to complexity:Most of the changes in this PR follow these patterns:
kitlog.Loggertype →*logging.Loggerkitlog.With(logger, ...)→logger.With(...)kitlog.NewNopLogger() → logging.NewNopLogger(), including similar variations such aslogging.NewLogfmtLogger(w)andlogging.NewJSONLogger(w)Unique changes that the PR review should focus on:
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