feat: switch SetupLogger to slog-native log.SetDefault#79
Conversation
Replace BaseLogger-based setup with log.SetDefault(log.NewHandler(...)) which also calls slog.SetDefault, enabling native slog.LogAttrs calls to automatically get ColdBrew context fields. - Remove cbslog (loggers/slog) import — backend logic now in log.Handler - Bump log to v0.4.0 - SetupLogger parses level first, passes it to NewHandler in one call Ref: go-coldbrew/log#27
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 1 minutes and 23 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughDependency Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
There was a problem hiding this comment.
Pull request overview
Updates core logging initialization to use ColdBrew’s slog-native handler via log.SetDefault, ensuring native slog.LogAttrs calls receive ColdBrew context fields.
Changes:
- Switch
SetupLoggerfrom the deprecatedSetLogger(NewLogger(...))path toSetDefault(NewHandler(...)). - Remove the
log/loggers/slog(cbslog) import and rely on handler-based configuration. - Bump
github.com/go-coldbrew/logdependency tov0.4.0.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| initializers.go | Updates SetupLogger to use log.SetDefault(log.NewHandler(...)) and adjusts comments/imports accordingly. |
| go.mod | Bumps github.com/go-coldbrew/log to v0.4.0. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
… test - Fix misordered log fields: use "msg" for message, "err" for the error - Add assertion that SetupLogger wires slog.Default to *log.Handler
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 4 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@core_coverage_test.go`:
- Around line 1313-1316: The test cleanup currently calls
slog.SetDefault(prevSlog) before log.SetDefault(prevHandler), but
log.SetDefault(...) internally calls slog.SetDefault(...) and thus will
overwrite the explicit slog restoration; swap the calls so
log.SetDefault(prevHandler) runs first and then slog.SetDefault(prevSlog) to
ensure slog's state is correctly restored. Update the t.Cleanup anonymous func
that references slog.SetDefault, log.SetDefault, prevSlog and prevHandler
accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 4 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
SetupLogger now checks log.DefaultIsSet() before overwriting — if the user called log.SetDefault in init(), core only updates the log level. - Bump log to v0.4.1 (adds DefaultIsSet) - Fix error log field ordering in SetupLogger - Add TestSetupLogger_RespectsExistingHandler
There was a problem hiding this comment.
♻️ Duplicate comments (1)
core_coverage_test.go (1)
1313-1316:⚠️ Potential issue | 🟡 MinorSwap cleanup order to correctly restore global slog state.
Line 1314 and Line 1335 restore
slogbeforelog, butlog.SetDefault(...)can resetslogagain. Restorelogfirst, thenslog, in both cleanup blocks.🔧 Suggested fix
t.Cleanup(func() { - slog.SetDefault(prevSlog) - log.SetDefault(prevHandler) + log.SetDefault(prevHandler) + slog.SetDefault(prevSlog) }) ... t.Cleanup(func() { - slog.SetDefault(prevSlog) - log.SetDefault(prevHandler) + log.SetDefault(prevHandler) + slog.SetDefault(prevSlog) })#!/bin/bash # Verify cleanup ordering in this test file and check SetupLogger/log.SetDefault usage context. rg -n -C3 't\.Cleanup\(func\(\)\s*\{|slog\.SetDefault\(prevSlog\)|log\.SetDefault\(prevHandler\)' core_coverage_test.go rg -n -C4 'func SetupLogger|log\.SetDefault\(' initializers.goAlso applies to: 1334-1337
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core_coverage_test.go` around lines 1313 - 1316, The cleanup blocks currently restore slog before log which can cause log.SetDefault to reconfigure slog; in each t.Cleanup where you call slog.SetDefault(prevSlog) and log.SetDefault(prevHandler) (e.g., the blocks referencing prevSlog and prevHandler), swap the order so you call log.SetDefault(prevHandler) first and then slog.SetDefault(prevSlog); update both occurrences (around the current t.Cleanup blocks) to restore log first, then slog to reliably restore global state after SetupLogger/log.SetDefault usage.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@core_coverage_test.go`:
- Around line 1313-1316: The cleanup blocks currently restore slog before log
which can cause log.SetDefault to reconfigure slog; in each t.Cleanup where you
call slog.SetDefault(prevSlog) and log.SetDefault(prevHandler) (e.g., the blocks
referencing prevSlog and prevHandler), swap the order so you call
log.SetDefault(prevHandler) first and then slog.SetDefault(prevSlog); update
both occurrences (around the current t.Cleanup blocks) to restore log first,
then slog to reliably restore global state after SetupLogger/log.SetDefault
usage.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d4bc0aa9-91a6-4874-aa1f-73b04efd85d9
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (3)
core_coverage_test.gogo.modinitializers.go
✅ Files skipped from review due to trivial changes (1)
- go.mod
🚧 Files skipped from review as they are similar to previous changes (1)
- initializers.go
Summary
log.SetLogger(log.NewLogger(cbslog.NewLogger(...)))withlog.SetDefault(log.NewHandler(...))log.SetDefaultalso callsslog.SetDefault, enabling nativeslog.LogAttrscalls to automatically get ColdBrew context fields (trace ID, gRPC method, etc.)cbslog(loggers/slog) import — backend logic is now inlog.Handlergo-coldbrew/logto v0.4.0Why
Without this change,
core.New()sets up logging via the deprecatedBaseLoggerpath, which never callsslog.SetDefault. This means nativeslog.LogAttrscalls in service code don't get ColdBrew context fields — breaking the cookiecutter template's switch toslog.LogAttrs(go-coldbrew/cookiecutter-coldbrew#34).Test plan
make test— CI will verifymake lint— CI will verifyRef: go-coldbrew/log#27, go-coldbrew/cookiecutter-coldbrew#34
Summary by CodeRabbit