Finishing mysql package migration to slog#40350
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 the MySQL platform/datastore layers (and related tools/commands/tests) to use log/slog loggers and context-aware logging instead of go-kit/log and Fleet’s legacy logger types.
Changes:
- Switch MySQL DB option/config plumbing and datastore logger fields to
*slog.Logger, updating call sites to passlogger.SlogLogger()where needed. - Replace go-kit
level.*(...).Log(...)usage withDebugContext/InfoContext/WarnContext/ErrorContext. - Update tests/utilities and a few helper signatures to accept/propagate
context.Contextfor slog logging.
Reviewed changes
Copilot reviewed 42 out of 42 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| tools/mysql-tests/rds/iam_auth.go | Switch test tool to initialize/use slog logger. |
| tools/mdm/apple/setupexperience/main.go | Pass slog logger into MySQL options via adapter. |
| tools/mdm/apple/applebmapi/main.go | Pass slog logger into MySQL options via adapter. |
| tools/mdm/apple/apnspush/main.go | Pass slog logger into MySQL options via adapter. |
| tools/dbutils/schema_generator.go | Use discard *slog.Logger for schema generator MySQL init. |
| server/platform/mysql/common.go | Change DBOptions logger to *slog.Logger and update connection logging. |
| server/datastore/mysql/vpp.go | Migrate vpp datastore logs to slog. |
| server/datastore/mysql/testing_utils.go | Update integration test datastore logger setup to slog. |
| server/datastore/mysql/statistics.go | Migrate statistics datastore logs to slog. |
| server/datastore/mysql/software_titles.go | Migrate warning log to slog (policy association case). |
| server/datastore/mysql/software_test.go | Update tests for slog logger + ctx propagation changes. |
| server/datastore/mysql/software_installers.go | Migrate installer label-conflict warning log to slog. |
| server/datastore/mysql/software.go | Migrate software datastore logs to slog and add ctx to helper(s). |
| server/datastore/mysql/setup_experience.go | Migrate setup experience logs to slog. |
| server/datastore/mysql/scripts.go | Migrate scripts datastore logs to slog. |
| server/datastore/mysql/scim.go | Migrate SCIM datastore logs + logger type to *slog.Logger. |
| server/datastore/mysql/queries.go | Migrate query stats deletion error logs to slog. |
| server/datastore/mysql/policies.go | Migrate policies datastore logs and logger plumbing to slog. |
| server/datastore/mysql/operating_system_vulnerabilities.go | Add ctx + slog logger to Linux vuln result processing. |
| server/datastore/mysql/nanomdm_storage.go | Pass slog logger to nano MDM MySQL storage. |
| server/datastore/mysql/mysql_test.go | Update mysql datastore tests to use discard slog logger. |
| server/datastore/mysql/mysql.go | Change datastore logger field to *slog.Logger and update txn helpers/log calls. |
| server/datastore/mysql/microsoft_mdm.go | Migrate Windows MDM logs to slog; thread ctx into BitLocker helper. |
| server/datastore/mysql/mdm.go | Migrate MDM datastore logs and logger types to slog. |
| server/datastore/mysql/labels.go | Thread ctx into OS settings filter helpers. |
| server/datastore/mysql/in_house_apps.go | Migrate label-conflict warning log to slog. |
| server/datastore/mysql/hosts.go | Migrate host datastore logs to slog; thread ctx into OS settings filters. |
| server/datastore/mysql/host_identity_scep.go | Pass slog logger into retry tx helper. |
| server/datastore/mysql/host_certificates.go | Migrate host certificates logs to slog. |
| server/datastore/mysql/disk_encryption.go | Migrate disk encryption logs to slog. |
| server/datastore/mysql/config.go | Change mysql.Logger(...) DBOption signature to accept *slog.Logger. |
| server/datastore/mysql/apple_mdm.go | Migrate Apple MDM datastore logs + helper logger types to slog. |
| server/datastore/mysql/android_mysql.go | Change Android datastore logger type to *slog.Logger and update tx helpers. |
| server/datastore/mysql/android_hosts.go | Migrate Android hosts logging to slog. |
| server/datastore/mysql/android_enterprise_test.go | Update Android enterprise tests to pass slog logger to truncate helper. |
| server/datastore/mysql/android_device_test.go | Update Android device tests to pass slog logger to truncate helper. |
| server/datastore/mysql/android.go | Migrate Android datastore logs to slog. |
| server/datastore/mysql/activities.go | Migrate activities datastore push error logging to slog. |
| ee/server/service/condaccess/depot/depot.go | Migrate conditional access SCEP depot logger to *slog.Logger. |
| ee/server/integrationtest/condaccess/suite.go | Pass slog logger into conditional access SCEP depot creation. |
| cmd/fleet/vuln_process.go | Pass slog logger into mysql datastore creation. |
| cmd/fleet/serve.go | Pass slog logger into mysql datastore + conditional access depot creation. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
WalkthroughThis PR migrates logging from go-kit/log and custom logging types to Go's standard log/slog across the codebase. It changes logger types to slog.Logger, replaces level. calls with context-aware methods (InfoContext, ErrorContext, DebugContext, WarnContext), threads context into many datastore and helper functions, and updates DB/logger wiring and test helpers to use slog equivalents. A few public/internal function signatures and struct fields were adjusted to accept *slog.Logger and some cached-stmt/delete helpers now accept context. Possibly related PRs
🚥 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 |
Member
Author
|
@coderabbitai full review |
Contributor
✅ Actions performedFull review triggered. |
![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/datastore/mysql/mdm.go (1)
2614-2659:⚠️ Potential issue | 🟠 MajorPII leakage: email addresses logged at Info level.
Multiple Info-level log calls in
reconcileHostEmailsFromMdmIdpAccountsDBemit email addresses — both embedded in a dynamically constructed message (Lines 2616–2621) and as a structured"email"attribute (Lines 2631, 2650, 2659). Email addresses are PII under GDPR/CCPA, and logging them at Info level risks exposing them in log aggregators, stdout captures, or wherever the slog handler writes.Additionally, the message construction at Line 2616 concatenates email values directly into the message string, which is both a PII concern and non-idiomatic slog (the message should remain static).
Consider logging only opaque identifiers (e.g., host IDs, row counts, or account UUIDs) instead of email addresses:
🛡️ Proposed fix — strip emails from log statements
if len(misses) > 0 { - // log the emails we'll be deleting - msg := "reconcile host emails: deleting emails" - for _, m := range misses { - idsToDelete = append(idsToDelete, m.ID) - msg += fmt.Sprintf(" %s", m.Email) - } - logger.InfoContext(ctx, msg, "host_id", hostID) + for _, m := range misses { + idsToDelete = append(idsToDelete, m.ID) + } + logger.DebugContext(ctx, "reconcile host emails: deleting stale mdm idp account emails", + "host_id", hostID, "count", len(misses)) }- logger.InfoContext(ctx, "reconcile host emails: found duplicate mdm idp account host email", "host_id", hostID, "email", idp.Email, "account_uuid", idp.UUID) + logger.InfoContext(ctx, "reconcile host emails: found duplicate mdm idp account host email", "host_id", hostID, "account_uuid", idp.UUID)- logger.InfoContext(ctx, "reconcile host emails: update host mdm idp account email", "host_id", hostID, "email", idpEmail) + logger.DebugContext(ctx, "reconcile host emails: update host mdm idp account email", "host_id", hostID)- logger.InfoContext(ctx, "reconcile host emails: insert host mdm idp account email", "host_id", hostID, "email", idpEmail, "account_uuid", idp.UUID) + logger.DebugContext(ctx, "reconcile host emails: insert host mdm idp account email", "host_id", hostID, "account_uuid", idp.UUID)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/datastore/mysql/mdm.go` around lines 2614 - 2659, In reconcileHostEmailsFromMdmIdpAccountsDB, remove PII by stopping inclusion of idp.Email and idpEmail in Info-level logs and stop concatenating email values into the log message; instead log static messages and non-PII identifiers (e.g., "host_id", hostID, "account_uuid", idp.UUID) and counts (e.g., len(idsToDelete), len(hits)). Replace the dynamic-built msg that appends m.Email with a static message like "reconcile host emails: deleting N emails" and pass "delete_count", len(idsToDelete) (and optionally the slice length or individual DB row IDs if non-PII) to logger.InfoContext; similarly change the duplicate and update/insert logs (calls to logger.InfoContext that reference idp.Email or idpEmail) to omit email fields and instead include "account_uuid" or counts/ids. Ensure all logger.InfoContext calls in this function no longer include email values and that message strings remain static (no direct concatenation of email values).
♻️ Duplicate comments (1)
server/datastore/mysql/disk_encryption.go (1)
170-172: Same log field mismatch as above (host_idvshost).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/datastore/mysql/disk_encryption.go` around lines 170 - 172, The log field key and value are mismatched in the ds.logger.ErrorContext call in disk_encryption.go: replace the field name "host_id" with the correct key "host" (or alternatively pass host.ID if you intend the key to be "host_id") so the key/value pair is consistent; update the ds.logger.ErrorContext invocation accordingly (reference: ds.logger.ErrorContext(ctx, "Primary key already exists in LUKS host_disk_encryption_keys. Falling back to update", "host_id", host)).
🧹 Nitpick comments (3)
server/datastore/mysql/mdm.go (1)
2189-2189: Non-idiomatic slog: use structured attributes instead offmt.Sprintfin the message string.Both lines embed dynamic data inside the message via
fmt.Sprintf, which prevents log aggregators from grouping identical events and is inconsistent with the slog migration style used everywhere else in this file.♻️ Proposed refactor
-ds.logger.DebugContext(ctx, fmt.Sprintf("counted %d android hosts for profile %s with mdm turned on but no profiles", row.Count, profileUUID)) +ds.logger.DebugContext(ctx, "counted android hosts for profile with mdm turned on but no profiles", + "count", row.Count, "profile_uuid", profileUUID)-ds.logger.DebugContext(ctx, fmt.Sprintf("counted %d windows hosts for profile %s with mdm turned on but no profiles", row.Count, profileUUID)) +ds.logger.DebugContext(ctx, "counted windows hosts for profile with mdm turned on but no profiles", + "count", row.Count, "profile_uuid", profileUUID)Also applies to: 2259-2259
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/datastore/mysql/mdm.go` at line 2189, Replace the fmt.Sprintf usage in the DebugContext call so the message is static and the dynamic fields are passed as structured attributes; specifically update the DebugContext invocation(s) that currently use fmt.Sprintf("counted %d android hosts for profile %s with mdm turned on but no profiles", row.Count, profileUUID) to something like a constant message string and include row.Count and profileUUID as attributes (e.g., "counted android hosts for profile with mdm on but no profiles", slog.Int("count", row.Count), slog.String("profile_uuid", profileUUID)); apply the same change to the similar call referencing these symbols (the DebugContext calls using row.Count and profileUUID around the referenced lines).server/datastore/mysql/microsoft_mdm.go (2)
831-839: Duplicate log message obscures two distinct conditions.Both branches log
"no bitlocker status found for host", but they represent different states: one issql.ErrNoRows(no MDM row at all) and the other is an empty status string after the CASE expression. Using distinct messages would make triage easier.♻️ Proposed fix
- ds.logger.DebugContext(ctx, "no bitlocker status found for host", "host_id", host.ID) + ds.logger.DebugContext(ctx, "no mdm row found for host, defaulting bitlocker status to enforcing", "host_id", host.ID) dest.Status = fleet.DiskEncryptionEnforcing } if dest.Status == "" { // This is unexpected. We know that disk encryption is enabled so we treat it failed to draw // attention to the issue and log potential debugging - ds.logger.DebugContext(ctx, "no bitlocker status found for host", "host_id", host.ID) + ds.logger.DebugContext(ctx, "empty bitlocker status for host, defaulting to failed", "host_id", host.ID) dest.Status = fleet.DiskEncryptionFailed🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/datastore/mysql/microsoft_mdm.go` around lines 831 - 839, The two ds.logger.DebugContext calls currently use the same message "no bitlocker status found for host" for different conditions; update them to distinct, descriptive messages so triage is easier: when handling the sql.ErrNoRows branch (where you set dest.Status = fleet.DiskEncryptionEnforcing) log something like "no MDM bitlocker row for host" including "host_id" and host.ID, and when handling the empty CASE result branch (where you set dest.Status = fleet.DiskEncryptionFailed) log something like "empty bitlocker status from MDM CASE for host" including "host_id" and host.ID; keep the same logger call ds.logger.DebugContext and the surrounding logic unchanged.
1088-1088: Avoidfmt.Sprintfin slog message; use structured key-value attrs instead.
fmt.Sprintfis evaluated eagerly regardless of the logger's level. Passing the data as slog attributes is the idiomatic approach and allows the logger to skip formatting when the level is disabled.♻️ Proposed refactor
- ds.logger.DebugContext(ctx, fmt.Sprintf("counted %d windows hosts on team %v with mdm turned on but no profiles or bitlocker status", c.Count, teamID)) + ds.logger.DebugContext(ctx, "counted windows hosts with mdm turned on but no profiles or bitlocker status", "count", c.Count, "team_id", teamID)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/datastore/mysql/microsoft_mdm.go` at line 1088, Replace the eager fmt.Sprintf call passed into ds.logger.DebugContext with structured slog key/value attributes so logging is deferred; specifically, in the DebugContext invocation that currently does fmt.Sprintf("counted %d windows hosts on team %v with mdm turned on but no profiles or bitlocker status", c.Count, teamID), call ds.logger.DebugContext with a short message string and supply "count" (c.Count) and "team_id" (teamID) as separate attributes/fields so formatting is only done when the debug level is enabled.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@server/datastore/mysql/disk_encryption.go`:
- Around line 56-57: The log call in disk_encryption.go is passing the entire
host struct as the "host_id" field; update the ds.logger.ErrorContext invocation
(the call using ds.logger.ErrorContext(ctx, "Primary key already exists in
host_disk_encryption_keys. Falling back to update", "host_id", host)) to pass
the host's numeric ID (host.ID) instead of the full host object so the "host_id"
field contains only the identifier and reduces leaked data.
In `@server/datastore/mysql/software_test.go`:
- Line 3573: The test calls hostSoftwareInstalledPathsDelta with a nil logger
which causes a panic because hostSoftwareInstalledPathsDelta calls
logger.DebugContext(); update the test so it passes a discard logger instance
(e.g., slog.New(slog.DiscardHandler())) instead of nil when invoking
hostSoftwareInstalledPathsDelta (the call at toI, toD, err :=
hostSoftwareInstalledPathsDelta(...)). Ensure other subtests already using
slog.New(slog.DiscardHandler()) remain consistent.
---
Outside diff comments:
In `@server/datastore/mysql/mdm.go`:
- Around line 2614-2659: In reconcileHostEmailsFromMdmIdpAccountsDB, remove PII
by stopping inclusion of idp.Email and idpEmail in Info-level logs and stop
concatenating email values into the log message; instead log static messages and
non-PII identifiers (e.g., "host_id", hostID, "account_uuid", idp.UUID) and
counts (e.g., len(idsToDelete), len(hits)). Replace the dynamic-built msg that
appends m.Email with a static message like "reconcile host emails: deleting N
emails" and pass "delete_count", len(idsToDelete) (and optionally the slice
length or individual DB row IDs if non-PII) to logger.InfoContext; similarly
change the duplicate and update/insert logs (calls to logger.InfoContext that
reference idp.Email or idpEmail) to omit email fields and instead include
"account_uuid" or counts/ids. Ensure all logger.InfoContext calls in this
function no longer include email values and that message strings remain static
(no direct concatenation of email values).
---
Duplicate comments:
In `@server/datastore/mysql/disk_encryption.go`:
- Around line 170-172: The log field key and value are mismatched in the
ds.logger.ErrorContext call in disk_encryption.go: replace the field name
"host_id" with the correct key "host" (or alternatively pass host.ID if you
intend the key to be "host_id") so the key/value pair is consistent; update the
ds.logger.ErrorContext invocation accordingly (reference:
ds.logger.ErrorContext(ctx, "Primary key already exists in LUKS
host_disk_encryption_keys. Falling back to update", "host_id", host)).
---
Nitpick comments:
In `@server/datastore/mysql/mdm.go`:
- Line 2189: Replace the fmt.Sprintf usage in the DebugContext call so the
message is static and the dynamic fields are passed as structured attributes;
specifically update the DebugContext invocation(s) that currently use
fmt.Sprintf("counted %d android hosts for profile %s with mdm turned on but no
profiles", row.Count, profileUUID) to something like a constant message string
and include row.Count and profileUUID as attributes (e.g., "counted android
hosts for profile with mdm on but no profiles", slog.Int("count", row.Count),
slog.String("profile_uuid", profileUUID)); apply the same change to the similar
call referencing these symbols (the DebugContext calls using row.Count and
profileUUID around the referenced lines).
In `@server/datastore/mysql/microsoft_mdm.go`:
- Around line 831-839: The two ds.logger.DebugContext calls currently use the
same message "no bitlocker status found for host" for different conditions;
update them to distinct, descriptive messages so triage is easier: when handling
the sql.ErrNoRows branch (where you set dest.Status =
fleet.DiskEncryptionEnforcing) log something like "no MDM bitlocker row for
host" including "host_id" and host.ID, and when handling the empty CASE result
branch (where you set dest.Status = fleet.DiskEncryptionFailed) log something
like "empty bitlocker status from MDM CASE for host" including "host_id" and
host.ID; keep the same logger call ds.logger.DebugContext and the surrounding
logic unchanged.
- Line 1088: Replace the eager fmt.Sprintf call passed into
ds.logger.DebugContext with structured slog key/value attributes so logging is
deferred; specifically, in the DebugContext invocation that currently does
fmt.Sprintf("counted %d windows hosts on team %v with mdm turned on but no
profiles or bitlocker status", c.Count, teamID), call ds.logger.DebugContext
with a short message string and supply "count" (c.Count) and "team_id" (teamID)
as separate attributes/fields so formatting is only done when the debug level is
enabled.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (42)
cmd/fleet/serve.gocmd/fleet/vuln_process.goee/server/integrationtest/condaccess/suite.goee/server/service/condaccess/depot/depot.goserver/datastore/mysql/activities.goserver/datastore/mysql/android.goserver/datastore/mysql/android_device_test.goserver/datastore/mysql/android_enterprise_test.goserver/datastore/mysql/android_hosts.goserver/datastore/mysql/android_mysql.goserver/datastore/mysql/apple_mdm.goserver/datastore/mysql/config.goserver/datastore/mysql/disk_encryption.goserver/datastore/mysql/host_certificates.goserver/datastore/mysql/host_identity_scep.goserver/datastore/mysql/hosts.goserver/datastore/mysql/in_house_apps.goserver/datastore/mysql/labels.goserver/datastore/mysql/mdm.goserver/datastore/mysql/microsoft_mdm.goserver/datastore/mysql/mysql.goserver/datastore/mysql/mysql_test.goserver/datastore/mysql/nanomdm_storage.goserver/datastore/mysql/operating_system_vulnerabilities.goserver/datastore/mysql/policies.goserver/datastore/mysql/queries.goserver/datastore/mysql/scim.goserver/datastore/mysql/scripts.goserver/datastore/mysql/setup_experience.goserver/datastore/mysql/software.goserver/datastore/mysql/software_installers.goserver/datastore/mysql/software_test.goserver/datastore/mysql/software_titles.goserver/datastore/mysql/statistics.goserver/datastore/mysql/testing_utils.goserver/datastore/mysql/vpp.goserver/platform/mysql/common.gotools/dbutils/schema_generator.gotools/mdm/apple/apnspush/main.gotools/mdm/apple/applebmapi/main.gotools/mdm/apple/setupexperience/main.gotools/mysql-tests/rds/iam_auth.go
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #40350 +/- ##
========================================
Coverage 66.27% 66.28%
========================================
Files 2459 2459
Lines 196694 197047 +353
Branches 8715 8715
========================================
+ Hits 130364 130603 +239
- Misses 54516 54623 +107
- Partials 11814 11821 +7
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:
|
jahzielv
approved these changes
Feb 24, 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