Change Datastore.logger type to *logging.Logger#39938
Conversation
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
WalkthroughThis PR migrates the logger dependency across the MySQL datastore layer and related tooling from go-kit/log to an internal logging package located at server/platform/logging. Changes include updating struct fields from log.Logger to *logging.Logger, modifying function signatures in constructors and option builders, replacing imports, and updating logger initialization calls from log.NewNopLogger() and log.NewLogfmtLogger() to their logging package equivalents throughout the MySQL datastore, tests, utilities, and command-line tools. Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts (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.
🤖 Fix all issues with AI agents
Before applying any fix, first verify the finding against the current code and
decide whether a code change is actually needed. If the finding is not valid or
no change is required, do not modify code for that item and briefly explain why
it was skipped.
In `@server/datastore/mysql/testing_utils.go`:
- Line 1024: The test currently passes go-kit's log.NewNopLogger into
activity_bootstrap.New (svc, _ := activity_bootstrap.New(...,
log.NewNopLogger())), which prevents removing the go-kit/log import; in a
follow-up migrate activity_bootstrap.New to accept the project's new logger
type/interface and update this call site to pass the new nop logger (or adapter)
from the project's logging package instead of log.NewNopLogger so you can remove
go-kit/log from testing_utils.go; update the activity_bootstrap.New signature
and any callers (or add a small adapter function) to bridge the old go-kit
logger to the new logger interface.
In `@server/platform/mysql/common.go`:
- Around line 164-177: WithTxx and WithReadOnlyTxx still accept log.Logger while
DBOptions.Logger is now *logging.Logger, creating a mixed-type surface; update
these functions' signatures to accept *logging.Logger (and update any internal
logging calls to use the logging.Logger API) and propagate the type change to
all callers (or add a small adapter that converts *logging.Logger -> log.Logger
if you need incremental rollout); ensure DBOptions.Logger is used consistently
and run tests to catch any mismatched call sites.
🧹 Nitpick comments (2)
🤖 Fix all nitpicks with AI agents
Before applying any fix, first verify the finding against the current code and decide whether a code change is actually needed. If the finding is not valid or no change is required, do not modify code for that item and briefly explain why it was skipped. In `@server/datastore/mysql/testing_utils.go`: - Line 1024: The test currently passes go-kit's log.NewNopLogger into activity_bootstrap.New (svc, _ := activity_bootstrap.New(..., log.NewNopLogger())), which prevents removing the go-kit/log import; in a follow-up migrate activity_bootstrap.New to accept the project's new logger type/interface and update this call site to pass the new nop logger (or adapter) from the project's logging package instead of log.NewNopLogger so you can remove go-kit/log from testing_utils.go; update the activity_bootstrap.New signature and any callers (or add a small adapter function) to bridge the old go-kit logger to the new logger interface. In `@server/platform/mysql/common.go`: - Around line 164-177: WithTxx and WithReadOnlyTxx still accept log.Logger while DBOptions.Logger is now *logging.Logger, creating a mixed-type surface; update these functions' signatures to accept *logging.Logger (and update any internal logging calls to use the logging.Logger API) and propagate the type change to all callers (or add a small adapter that converts *logging.Logger -> log.Logger if you need incremental rollout); ensure DBOptions.Logger is used consistently and run tests to catch any mismatched call sites.server/platform/mysql/common.go (1)
164-177:WithTxxandWithReadOnlyTxxstill uselog.Logger— intentional for incremental migration.These two functions accept
log.LoggerwhileDBOptions.Loggerwas changed to*logging.Logger. This is consistent with the stated incremental migration approach but creates a mixed-type surface within the same file. Worth noting for the next migration phase.Also applies to: 195-226
🤖 Prompt for AI Agents
Before applying any fix, first verify the finding against the current code and decide whether a code change is actually needed. If the finding is not valid or no change is required, do not modify code for that item and briefly explain why it was skipped. In `@server/platform/mysql/common.go` around lines 164 - 177, WithTxx and WithReadOnlyTxx still accept log.Logger while DBOptions.Logger is now *logging.Logger, creating a mixed-type surface; update these functions' signatures to accept *logging.Logger (and update any internal logging calls to use the logging.Logger API) and propagate the type change to all callers (or add a small adapter that converts *logging.Logger -> log.Logger if you need incremental rollout); ensure DBOptions.Logger is used consistently and run tests to catch any mismatched call sites.server/datastore/mysql/testing_utils.go (1)
1024-1024: Consider migratingactivity_bootstrap.Newlogger parameter in a follow-up.
log.NewNopLogger()(go-kit) is still used here becauseactivity_bootstrap.Newhasn't been migrated yet. This is fine for now, but worth tracking so thego-kit/logimport can eventually be removed from this file.🤖 Prompt for AI Agents
Before applying any fix, first verify the finding against the current code and decide whether a code change is actually needed. If the finding is not valid or no change is required, do not modify code for that item and briefly explain why it was skipped. In `@server/datastore/mysql/testing_utils.go` at line 1024, The test currently passes go-kit's log.NewNopLogger into activity_bootstrap.New (svc, _ := activity_bootstrap.New(..., log.NewNopLogger())), which prevents removing the go-kit/log import; in a follow-up migrate activity_bootstrap.New to accept the project's new logger type/interface and update this call site to pass the new nop logger (or adapter) from the project's logging package instead of log.NewNopLogger so you can remove go-kit/log from testing_utils.go; update the activity_bootstrap.New signature and any callers (or add a small adapter function) to bridge the old go-kit logger to the new logger interface.
There was a problem hiding this comment.
Pull request overview
This PR changes the type of the logger field in the MySQL datastore and related components from log.Logger (go-kit) to *logging.Logger (new slog-based logger). This is preparatory work for the incremental migration to slog as described in ADR-0008. The *logging.Logger type implements the log.Logger interface, allowing existing code that uses go-kit logging patterns to continue working unchanged.
Changes:
- Updated
Datastore.loggerfield type fromlog.Loggerto*logging.Logger - Updated
DBOptions.Loggerfield type inserver/platform/mysql/common.go - Updated
AndroidDatastore.loggerfield type and constructor - Replaced all instances of
log.NewNopLogger()andlog.NewLogfmtLogger()withlogging.NewNopLogger()andlogging.NewLogfmtLogger()in test files and tools
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| server/datastore/mysql/mysql.go | Changed Datastore.logger field type to *logging.Logger and updated default logger creation |
| server/datastore/mysql/config.go | Updated Logger DBOption to accept *logging.Logger instead of log.Logger |
| server/datastore/mysql/android_mysql.go | Changed AndroidDatastore.logger and constructor parameter to use *logging.Logger |
| server/platform/mysql/common.go | Updated DBOptions.Logger field to *logging.Logger type |
| server/datastore/mysql/testing_utils.go | Updated test logger creation to use logging.NewNopLogger() and logging.NewLogfmtLogger() |
| server/datastore/mysql/mysql_test.go | Replaced log.NewNopLogger() with logging.NewNopLogger() in test mocks |
| server/datastore/mysql/software_test.go | Updated test loggers to use new logging package functions |
| tools/mysql-tests/rds/iam_auth.go | Updated logger creation to use logging.NewLogfmtLogger() |
| tools/mdm/apple/setupexperience/main.go | Updated logger creation to use logging.NewLogfmtLogger() |
| tools/mdm/apple/applebmapi/main.go | Updated logger creation to use logging.NewLogfmtLogger() |
| tools/dbutils/schema_generator.go | Updated to use logging.NewNopLogger() |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Well hello there, neighbor! I took a nice careful look through all the changes in this PR, and I'm happy to report that everything looks just wonderful. This PR cleanly migrates the MySQL datastore layer from
The architecture test in Files Reviewed (11 files)
|
# Conflicts: # server/datastore/mysql/testing_utils.go
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #39938 +/- ##
=======================================
Coverage 66.27% 66.27%
=======================================
Files 2439 2439
Lines 195476 195478 +2
Branches 8654 8654
=======================================
+ Hits 129542 129559 +17
+ Misses 54198 54188 -10
+ Partials 11736 11731 -5
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:
|
|
Hi @iansltx can you take a look at this small cross-functional slog change? I'm trying to spread the slog reviews among other engineers since I'm making a lot of incremental PRs. |
|
@getvictor There's First Impressions work ahead of this (#39572, #39847, #39873) that I need to review, so you'll want to assign this to someone else. |
|
@dantecatalfamo assigning to you for review since Ian is not available right now |
Related issue: Resolves #38889
This is preparatory work before incrementally converting datastore/mysql files to directly use *slog.Logger.
This will be done by using
logger.SlogLogger()to get the underlying*slog.LoggerChecklist for submitter
changes/,orbit/changes/oree/fleetd-chrome/changes.Testing
Summary by CodeRabbit