slog migration: platform/mysql and related logic - #40072
Merged
Merged
Conversation
Member
Author
|
@coderabbitai full review |
Contributor
✅ Actions performedFull review triggered. |
Contributor
There was a problem hiding this comment.
Pull request overview
Continues the slog migration by updating platform/mysql (and related datastore/MDM call sites) to use log/slog loggers instead of go-kit loggers, including transaction helpers and SCEP/MDM storage integrations.
Changes:
- Switch
platform/mysqltransaction helpers and connector factory signatures to*slog.Loggerand update internal logging calls. - Update datastore and NanoMDM MySQL storage integrations to pass
*slog.Logger(mostly viads.logger.SlogLogger()/ discard slog loggers in tests). - Add a small adapter in the standalone NanoMDM CLI to bridge a
nanolib/log.Loggerinto a*slog.Logger.
Reviewed changes
Copilot reviewed 21 out of 21 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| server/platform/mysql/testing_utils/testing_utils.go | Update truncate helper signature to accept *slog.Logger. |
| server/platform/mysql/retry_test.go | Migrate tests to use slog.New(slog.DiscardHandler) instead of go-kit nop logger. |
| server/platform/mysql/retry.go | Change WithRetryTxx to accept *slog.Logger and migrate panic-rollback logging. |
| server/platform/mysql/common.go | Change connector factory + WithTxx/WithReadOnlyTxx to *slog.Logger; update connection retry logging to slog. |
| server/mdm/nanomdm/storage/mysql/queue.go | Remove nanolib/go-kit logging adapter and pass slog logger directly into retry helper. |
| server/mdm/nanomdm/storage/mysql/mysql.go | Convert NanoMDM MySQL storage logger usage to slog (InfoContext/ErrorContext), update options API. |
| server/mdm/nanomdm/cli/cli.go | Bridge nanolib logger to slog via a custom slog.Handler for mysql storage config. |
| server/datastore/mysql/testing_utils.go | Pass ds.logger.SlogLogger() into platform mysql test truncate helper. |
| server/datastore/mysql/rdsauth/connector.go | Update IAM connector factory signature to accept *slog.Logger. |
| server/datastore/mysql/policies.go | Update internal helpers to pass *slog.Logger into retry helper where needed. |
| server/datastore/mysql/nanomdm_storage_test.go | Use discard slog logger in NanoMDM storage tests. |
| server/datastore/mysql/nanomdm_storage.go | Update NanoMDM storage wiring to use slog logger and pass into nanomdm mysql storage. |
| server/datastore/mysql/mysql.go | Update NewHostIdentitySCEPDepot signature to accept *slog.Logger; pass slog loggers to tx helpers. |
| server/datastore/mysql/host_identity_scep.go | Pass ds.logger.SlogLogger() into retry helper. |
| server/datastore/mysql/android_mysql.go | Pass ds.logger.SlogLogger() into retry/tx helpers. |
| server/datastore/mysql/android_enterprise_test.go | Update truncate helper call to pass ds.logger.SlogLogger(). |
| server/datastore/mysql/android_device_test.go | Update truncate helper call to pass ds.logger.SlogLogger(). |
| server/activity/internal/testutils/testutils.go | Update truncate helper call to use discard slog logger. |
| ee/server/service/hostidentity/depot/depot.go | Change depot logger type to *slog.Logger and migrate log call to slog. |
| ee/server/integrationtest/hostidentity/suite.go | Update to pass logger.SlogLogger().With(...) to host identity depot. |
| cmd/fleet/serve.go | Update to pass logger.SlogLogger().With(...) to host identity depot. |
Comments suppressed due to low confidence (1)
ee/server/service/hostidentity/depot/depot.go:48
- NewHostIdentitySCEPDepot accepts a *slog.Logger but doesn’t validate it. If a nil logger is passed, methods like Put will panic when calling d.logger.InfoContext. Consider either rejecting nil early (panic/return error) or defaulting to a no-op slog logger (e.g., slog.New(slog.DiscardHandler)) to make this API safer.
func NewHostIdentitySCEPDepot(db *sqlx.DB, ds fleet.Datastore, logger *slog.Logger, cfg *config.FleetConfig) (*HostIdentitySCEPDepot, error) {
if err := db.Ping(); err != nil {
return nil, err
}
return &HostIdentitySCEPDepot{
db: db,
ds: ds,
logger: logger,
config: cfg,
}, nil
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
WalkthroughThis pull request implements a widespread migration from the go-kit/log framework to the standard Go slog logging package across multiple components. The changes systematically replace 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.
🧹 Nitpick comments (7)
server/mdm/nanomdm/cli/cli.go (2)
180-181:WithGroupsilently drops the group name — add a comment.The current implementation ignores the
nameargument with no explanation. Per theslog.Handlercontract, callers expect attributes to be nested under the group name. Since nanolib has no grouping concept, this is an acceptable limitation, but it should be documented to prevent future confusion.📝 Suggested doc comment
func (h *nanoLibSlogHandler) WithGroup(_ string) slog.Handler { + // nanolib/log has no concept of attribute groups; groups are silently dropped. return h }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/mdm/nanomdm/cli/cli.go` around lines 180 - 181, The WithGroup method on nanoLibSlogHandler currently ignores the provided group name; update the function nanoLibSlogHandler.WithGroup to include a clear doc comment stating that nanolib does not support attribute grouping and therefore the name argument is intentionally dropped (i.e., the method returns the same handler), so callers understand this limitation rather than assuming a bug; keep the implementation unchanged but add the explanatory comment directly above the WithGroup method.
155-171:slog.LevelWarnandslog.LevelErrorsilently downgrade to nanolibInfo.The condition
r.Level >= slog.LevelInfois always true forWarn(4),Error(8), andInfo(0), so all three levels are forwarded toh.logger.Info(). Operational error messages from the MySQL storage (e.g. rollback failures, batch-update errors) will therefore appear in the CLI output asInfo, making them harder to triage.Consider emitting the slog level as a key-value attribute so severity is preserved even within nanolib's two-level model:
♻️ Suggested refinement
+ kvs = append(kvs, "level", r.Level.String()) if r.Level >= slog.LevelInfo { h.logger.Info(kvs...) } else { h.logger.Debug(kvs...) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/mdm/nanomdm/cli/cli.go` around lines 155 - 171, In nanoLibSlogHandler.Handle, the current condition (r.Level >= slog.LevelInfo) always evaluates true for Warn and Error and thus downgrades severity; change the routing threshold to compare against slog.LevelWarn (so Warn and Error map to h.logger.Info while lower levels go to h.logger.Debug) and also append the original slog level as a key-value pair (e.g., key "slog.level" with value r.Level.String() or r.Level) into the kvs slice before logging so the original severity is preserved even though nanolib only has Info/Debug; update references to h.logger.Info and h.logger.Debug accordingly in the function nanoLibSlogHandler.Handle.server/activity/internal/testutils/testutils.go (1)
51-51:tdb.Loggeris already initialized as a discard logger — reuse it instead of allocating a second one.♻️ Proposed fix
- mysql_testing_utils.TruncateTables(t, tdb.DB, slog.New(slog.DiscardHandler), nil, "host_activities", "activities", "hosts", "users") + mysql_testing_utils.TruncateTables(t, tdb.DB, tdb.Logger, nil, "host_activities", "activities", "hosts", "users")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/activity/internal/testutils/testutils.go` at line 51, The call to mysql_testing_utils.TruncateTables creates a new discard logger instead of reusing the already-initialized tdb.Logger; update the TruncateTables invocation in testutils.go to pass tdb.Logger (instead of slog.New(slog.DiscardHandler)) so the existing discard logger is reused when calling mysql_testing_utils.TruncateTables(t, tdb.DB, ..., nil, "host_activities", "activities", "hosts", "users").cmd/fleet/serve.go (1)
1505-1514: Asymmetric logger APIs on adjacent depot creation calls — expected but worth tracking.Line 1505 now passes
logger.SlogLogger().With(...)(*slog.Logger) toNewHostIdentitySCEPDepot, while Line 1514 still passeslogger.With(...)(go-kitlog.Logger) toNewConditionalAccessSCEPDepot. This is expected sinceNewConditionalAccessSCEPDepotisn't migrated in this PR, but the two calls should be aligned oncecondaccessis migrated.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/fleet/serve.go` around lines 1505 - 1514, Adjacent depot constructors use different logger types: NewHostIdentitySCEPDepot is passed logger.SlogLogger().With(...) (a *slog.Logger) while NewConditionalAccessSCEPDepot is still passed logger.With(...) (go-kit log.Logger). When migrating Conditional Access to the slog API, update the call to NewConditionalAccessSCEPDepot to use logger.SlogLogger().With("component", "conditional-access-scep-depot") so both depots accept the same slog.Logger type (or, if keeping go-kit for both, change the host identity call to use logger.With). Ensure the chosen logger type matches the constructor signature of NewConditionalAccessSCEPDepot (or update that constructor) and keep the "component" field identical.server/datastore/mysql/rdsauth/connector.go (1)
73-99:Connector.loggeris stored but never referenced inConnect()orDriver().The field is carried through the migration but has no active usage. Consider either removing it (and dropping the
loggerparameter fromNewConnectorFactory's returned closure) until it's actually needed, or adding a comment indicating its intended future use.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/datastore/mysql/rdsauth/connector.go` around lines 73 - 99, Connector.logger is declared on type Connector but never used in Connect() or Driver(); either remove the logger field and drop the logger parameter from the NewConnectorFactory closure/constructor that creates *Connector, or leave the field and add a short TODO comment on Connector.logger explaining it's reserved for future instrumentation/metrics so linters/reviewers know it's intentional; update the Connector struct and the factory/constructor (and any calls that pass logger) to keep signatures consistent, and ensure Connector.Connect and Connector.Driver reflect the chosen change.server/platform/mysql/common.go (1)
104-113: Connection retry logging usescontext.Background()— acceptable but consider threading context throughNewDB.
NewDBdoesn't receive acontext.Context, socontext.Background()is the only option. This is fine for now, but ifNewDBis ever refactored to accept a context (e.g., for cancellation during startup), this log call should be updated.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/platform/mysql/common.go` around lines 104 - 113, The retry loop in NewDB currently calls opts.Logger.SlogLogger().WarnContext with context.Background(), which prevents propagation of caller cancellation or deadlines; update NewDB to accept a context.Context parameter (e.g., func NewDB(ctx context.Context, opts ...)) and thread that ctx into the retry loop, replacing context.Background() with the passed ctx when calling opts.Logger.SlogLogger().WarnContext and when sleeping/returning so that connection attempts respect cancellation and deadlines.server/datastore/mysql/mysql.go (1)
186-194: Inconsistent migration:NewConditionalAccessSCEPDepotstill useslog.Logger.
NewHostIdentitySCEPDepot(line 186) has been migrated to*slog.Logger, but the adjacentNewConditionalAccessSCEPDepot(line 192) still takeslog.Logger. This creates an inconsistency in the public API surface ofDatastore. Consider migrating both in this PR for consistency, or leave a TODO to track it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/datastore/mysql/mysql.go` around lines 186 - 194, The two Datastore constructors are inconsistent: NewHostIdentitySCEPDepot uses *slog.Logger while NewConditionalAccessSCEPDepot still accepts log.Logger; change NewConditionalAccessSCEPDepot's logger parameter to *slog.Logger and update its call to condaccessdepot.NewConditionalAccessSCEPDepot to pass the *slog.Logger, and then update the condaccessdepot.NewConditionalAccessSCEPDepot signature (and any call sites) to accept *slog.Logger instead of log.Logger; ensure imports are updated and run tests or build to catch remaining references, or alternatively add a TODO noting the mismatch if you prefer deferring the full migration.
🤖 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 1505-1514: Adjacent depot constructors use different logger types:
NewHostIdentitySCEPDepot is passed logger.SlogLogger().With(...) (a
*slog.Logger) while NewConditionalAccessSCEPDepot is still passed
logger.With(...) (go-kit log.Logger). When migrating Conditional Access to the
slog API, update the call to NewConditionalAccessSCEPDepot to use
logger.SlogLogger().With("component", "conditional-access-scep-depot") so both
depots accept the same slog.Logger type (or, if keeping go-kit for both, change
the host identity call to use logger.With). Ensure the chosen logger type
matches the constructor signature of NewConditionalAccessSCEPDepot (or update
that constructor) and keep the "component" field identical.
In `@server/activity/internal/testutils/testutils.go`:
- Line 51: The call to mysql_testing_utils.TruncateTables creates a new discard
logger instead of reusing the already-initialized tdb.Logger; update the
TruncateTables invocation in testutils.go to pass tdb.Logger (instead of
slog.New(slog.DiscardHandler)) so the existing discard logger is reused when
calling mysql_testing_utils.TruncateTables(t, tdb.DB, ..., nil,
"host_activities", "activities", "hosts", "users").
In `@server/datastore/mysql/mysql.go`:
- Around line 186-194: The two Datastore constructors are inconsistent:
NewHostIdentitySCEPDepot uses *slog.Logger while NewConditionalAccessSCEPDepot
still accepts log.Logger; change NewConditionalAccessSCEPDepot's logger
parameter to *slog.Logger and update its call to
condaccessdepot.NewConditionalAccessSCEPDepot to pass the *slog.Logger, and then
update the condaccessdepot.NewConditionalAccessSCEPDepot signature (and any call
sites) to accept *slog.Logger instead of log.Logger; ensure imports are updated
and run tests or build to catch remaining references, or alternatively add a
TODO noting the mismatch if you prefer deferring the full migration.
In `@server/datastore/mysql/rdsauth/connector.go`:
- Around line 73-99: Connector.logger is declared on type Connector but never
used in Connect() or Driver(); either remove the logger field and drop the
logger parameter from the NewConnectorFactory closure/constructor that creates
*Connector, or leave the field and add a short TODO comment on Connector.logger
explaining it's reserved for future instrumentation/metrics so linters/reviewers
know it's intentional; update the Connector struct and the factory/constructor
(and any calls that pass logger) to keep signatures consistent, and ensure
Connector.Connect and Connector.Driver reflect the chosen change.
In `@server/mdm/nanomdm/cli/cli.go`:
- Around line 180-181: The WithGroup method on nanoLibSlogHandler currently
ignores the provided group name; update the function
nanoLibSlogHandler.WithGroup to include a clear doc comment stating that nanolib
does not support attribute grouping and therefore the name argument is
intentionally dropped (i.e., the method returns the same handler), so callers
understand this limitation rather than assuming a bug; keep the implementation
unchanged but add the explanatory comment directly above the WithGroup method.
- Around line 155-171: In nanoLibSlogHandler.Handle, the current condition
(r.Level >= slog.LevelInfo) always evaluates true for Warn and Error and thus
downgrades severity; change the routing threshold to compare against
slog.LevelWarn (so Warn and Error map to h.logger.Info while lower levels go to
h.logger.Debug) and also append the original slog level as a key-value pair
(e.g., key "slog.level" with value r.Level.String() or r.Level) into the kvs
slice before logging so the original severity is preserved even though nanolib
only has Info/Debug; update references to h.logger.Info and h.logger.Debug
accordingly in the function nanoLibSlogHandler.Handle.
In `@server/platform/mysql/common.go`:
- Around line 104-113: The retry loop in NewDB currently calls
opts.Logger.SlogLogger().WarnContext with context.Background(), which prevents
propagation of caller cancellation or deadlines; update NewDB to accept a
context.Context parameter (e.g., func NewDB(ctx context.Context, opts ...)) and
thread that ctx into the retry loop, replacing context.Background() with the
passed ctx when calling opts.Logger.SlogLogger().WarnContext and when
sleeping/returning so that connection attempts respect cancellation and
deadlines.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #40072 +/- ##
==========================================
+ Coverage 65.55% 66.29% +0.73%
==========================================
Files 2255 2443 +188
Lines 178307 195803 +17496
Branches 8608 8608
==========================================
+ Hits 116894 129801 +12907
- Misses 50340 54250 +3910
- Partials 11073 11752 +679
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:
|
| rowsAffected, _ := result.RowsAffected() | ||
| if rowsAffected > 0 { | ||
| d.logger.Log("msg", "revoked existing host identity certificate", "name", name) | ||
| d.logger.InfoContext(context.Background(), "revoked existing host identity certificate", "name", name) |
Contributor
There was a problem hiding this comment.
As a side note, we should eventually update these calls to use a consistent context provided by the caller, though that's out of scope for this.
Member
Author
There was a problem hiding this comment.
I agree and good catch. This is code we brought over from a 3rd party library, so it'll need some changes.
juan-fdz-hawa
approved these changes
Feb 19, 2026
This was referenced Feb 23, 2026
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