feat: switch template logging to native slog.LogAttrs#34
Conversation
Replace all go-coldbrew/log calls with native slog.LogAttrs using typed attribute constructors. Demonstrates the zero-boxing high-performance logging path enabled by the slog-native Handler (go-coldbrew/log#27). - service/service.go: slog.LogAttrs + AddAttrsToContext example in Echo handler - service/auth/auth.go: slog.LogAttrs with slog.String/slog.Any - main.go: slog.LogAttrs for service exit error - config/config.go: slog.LogAttrs for config load error No go-coldbrew/log import needed except in service.go for AddAttrsToContext. Ref: go-coldbrew/log#27
📝 WalkthroughWalkthroughReplaced ColdBrew logging wrapper usage with Go's log/slog across the app, updated ColdBrew dependency versions, adjusted config/main error logging, added context-field logging guidance to docs, and changed Echo to accept and use a context.Context parameter. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested reviewers
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 docstrings
🧪 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 the template’s logging approach by replacing go-coldbrew/log call sites with native log/slog LogAttrs usage, while keeping go-coldbrew/log only for context attribute propagation.
Changes:
- Replace
log.Info/Warn/Errorcalls withslog.LogAttrsand typed attrs (slog.String,slog.Any). - Add an
Echoexample demonstratingAddAttrsToContext+slog.LogAttrs. - Update service exit/config error logging to use
slogwitherrattribute.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| {{cookiecutter.app_name}}/service/service.go | Switch handler logging to slog.LogAttrs and demonstrate request-scoped attrs via AddAttrsToContext. |
| {{cookiecutter.app_name}}/service/auth/auth.go | Convert auth failure warnings to slog.LogAttrs with typed attributes. |
| {{cookiecutter.app_name}}/main.go | Log cb.Run() failures using slog.LogAttrs and err attr. |
| {{cookiecutter.app_name}}/config/config.go | Log config-load failures using slog.LogAttrs with err attr. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Log message length instead of raw user input in Echo handler (Copilot) - Update AGENTS.md to mention slog.LogAttrs and AddAttrsToContext (Copilot)
There was a problem hiding this comment.
Pull request overview
Updates the generated service template to use Go’s native structured logging (log/slog) via the high-performance slog.LogAttrs API while keeping ColdBrew context attribute injection available for request-scoped fields.
Changes:
- Replaced
go-coldbrew/logcall sites withslog.LogAttrsand typed attrs (slog.String,slog.Any, etc.). - Added an
Echoexample showingAddAttrsToContext+slog.LogAttrsusage. - Updated template guidance in
AGENTS.mdto reflect the new logging approach.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| {{cookiecutter.app_name}}/service/service.go | Uses slog.LogAttrs and demonstrates adding typed attrs to context. |
| {{cookiecutter.app_name}}/service/auth/auth.go | Switches auth warning logs to slog.LogAttrs with typed attrs. |
| {{cookiecutter.app_name}}/main.go | Logs service exit errors via slog.LogAttrs instead of go-coldbrew/log. |
| {{cookiecutter.app_name}}/config/config.go | Logs config-load errors via slog.LogAttrs. |
| {{cookiecutter.app_name}}/AGENTS.md | Updates developer guidance to mention slog.LogAttrs and context attrs. |
💡 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: 2
🧹 Nitpick comments (1)
{{cookiecutter.app_name}}/AGENTS.md (1)
61-61: Consider clarifying the “context is first parameter everywhere” wording.This line can be read as absolute; repo convention exempts purely structural unexported helpers that do not propagate/use context. A brief qualifier would prevent over-application.
Based on learnings: In
{{cookiecutter.app_name}}/**/*.go, the context-first rule applies to exported functions and unexported functions that actually propagate/use context; purely structural helper functions are exempt.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @{{cookiecutter.app_name}}/AGENTS.md at line 61, Clarify the absolute wording about context by changing the sentence that currently reads "context.Context is the first parameter everywhere" to a qualified rule: state that in {{cookiecutter.app_name}}/**/*.go the convention is to place context.Context first for exported functions and for unexported functions that propagate or use context, while purely structural/unrelated private helper functions are exempt; keep references to interceptors, slog.LogAttrs, ColdBrew Handler, and github.com/go-coldbrew/log.AddAttrsToContext (cblog) intact so readers still know how to add typed fields and propagate trace/log options.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @{{cookiecutter.app_name}}/go.mod:
- Line 18: Replace the unavailable module version by updating the dependency
declaration for github.com/go-coldbrew/log to a proxy-indexed release (e.g.,
change v0.4.0 to v0.3.2) in go.mod so the Go module proxy can resolve it; after
editing run `go mod tidy`/`go get` to ensure the lockfile and module cache are
updated and verify the build succeeds.
In @{{cookiecutter.app_name}}/service/service.go:
- Around line 71-74: The Error handler (svc.Error) currently creates a plain
error with errors.New and returns errors.Wrap, which doesn't produce a gRPC
status; change it to return a gRPC status error using
status.Error(codes.Internal, "...") (or status.Errorf) with the same message,
keep the slog.LogAttrs(ctx, slog.LevelInfo, "error requested") call, and remove
the errors.Wrap usage so the function returns (nil, status.Error(codes.Internal,
"<endpoint error message>")) to match other endpoints like healthcheck.go and
auth.go.
---
Nitpick comments:
In @{{cookiecutter.app_name}}/AGENTS.md:
- Line 61: Clarify the absolute wording about context by changing the sentence
that currently reads "context.Context is the first parameter everywhere" to a
qualified rule: state that in {{cookiecutter.app_name}}/**/*.go the convention
is to place context.Context first for exported functions and for unexported
functions that propagate or use context, while purely structural/unrelated
private helper functions are exempt; keep references to interceptors,
slog.LogAttrs, ColdBrew Handler, and
github.com/go-coldbrew/log.AddAttrsToContext (cblog) intact so readers still
know how to add typed fields and propagate trace/log options.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8d73db4d-2311-46ef-8d38-0b5dca4737be
📒 Files selected for processing (6)
{{cookiecutter.app_name}}/AGENTS.md{{cookiecutter.app_name}}/config/config.go{{cookiecutter.app_name}}/go.mod{{cookiecutter.app_name}}/main.go{{cookiecutter.app_name}}/service/auth/auth.go{{cookiecutter.app_name}}/service/service.go
There was a problem hiding this comment.
Pull request overview
Updates the generated ColdBrew Go service template to use Go’s native log/slog structured logging APIs (slog.LogAttrs) while preserving ColdBrew’s context-based attribute propagation via AddAttrsToContext.
Changes:
- Replaced prior
go-coldbrew/logcall sites withslog.LogAttrsusing typedslog.Attrs. - Added an
Echoexample demonstratingcblog.AddAttrsToContext+slog.LogAttrs. - Updated template docs and bumped
github.com/go-coldbrew/logtov0.4.0.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| {{cookiecutter.app_name}}/service/service.go | Switches service logs to slog.LogAttrs and demonstrates AddAttrsToContext usage. |
| {{cookiecutter.app_name}}/service/auth/auth.go | Converts auth warning logs to slog.LogAttrs with typed attrs. |
| {{cookiecutter.app_name}}/main.go | Logs cb.Run() failure via slog.LogAttrs instead of the ColdBrew logging helper. |
| {{cookiecutter.app_name}}/config/config.go | Uses slog.LogAttrs for config-load errors. |
| {{cookiecutter.app_name}}/go.mod | Updates github.com/go-coldbrew/log dependency to v0.4.0. |
| {{cookiecutter.app_name}}/AGENTS.md | Documents the slog.LogAttrs + AddAttrsToContext pattern for service code. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- core v0.1.45 → v0.1.51 (slog-native SetupLogger, DefaultIsSet) - log v0.3.1 → v0.4.1 (slog-native Handler, AddAttrsToContext, DefaultIsSet) - errors v0.2.13 → v0.2.15 - interceptors v0.1.20 → v0.1.25 - tracing v0.2.1 → v0.2.2
There was a problem hiding this comment.
Pull request overview
Updates the template to use Go’s native log/slog structured logging API (via slog.LogAttrs) in runtime code paths, while keeping go-coldbrew/log only for context attribute propagation.
Changes:
- Replaced ColdBrew
log.*calls withslog.LogAttrsacross service, auth, config, and main entrypoint. - Added an
Echoexample demonstratingAddAttrsToContext+slog.LogAttrs. - Bumped several
go-coldbrew/*module versions ingo.mod.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| {{cookiecutter.app_name}}/service/service.go | Switches to slog.LogAttrs and demonstrates adding typed request attrs via context. |
| {{cookiecutter.app_name}}/service/auth/auth.go | Converts auth failure logging to slog.LogAttrs with typed attrs. |
| {{cookiecutter.app_name}}/main.go | Logs cb.Run() error using slog.LogAttrs (with err attr). |
| {{cookiecutter.app_name}}/config/config.go | Converts config-load error logging to slog.LogAttrs. |
| {{cookiecutter.app_name}}/go.mod | Updates ColdBrew module versions. |
| {{cookiecutter.app_name}}/AGENTS.md | Documents the new slog.LogAttrs + AddAttrsToContext logging guidance. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
hooks/post_gen_project.py (1)
54-56: Use absolute paths or path resolution for subprocess commands to harden bootstrap execution.Bare command names in
Popen([...])rely on PATH lookup, which is avoidable in bootstrap scripts. This affects all subprocess calls in the file (lines 43, 49, 56, 59, 65). Use absolute paths (e.g.,/usr/bin/go) orshutil.which()to resolve them explicitly before execution.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hooks/post_gen_project.py` around lines 54 - 56, Replace bare command names in all Popen([...]) calls with resolved absolute paths: use shutil.which to locate the executable (e.g., resolve "go" via shutil.which("go") and error if not found) and then pass the resolved path as the first element of the argv list to Popen; update every subprocess invocation in this file (the Popen calls around PROJECT_DIRECTORY) to use the resolved executable path and keep the cwd and flags unchanged so bootstrap execution doesn't rely on PATH lookups.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@hooks/post_gen_project.py`:
- Around line 54-56: The pre-mock "go mod tidy -e" step currently ignores the
process exit status; update the Popen call in hooks/post_gen_project.py (the
Popen invocation that runs ["go","mod","tidy","-e"] with cwd=PROJECT_DIRECTORY)
to capture the completed process, check its returncode, and handle non-zero
results explicitly (log/print the stderr/stdout for context and abort/raise
SystemExit with a non-zero exit code) so real module errors stop the post-gen
script instead of allowing later, harder-to-debug failures.
- Line 48: Update the progress step totals so they are consistent across the
prints: find the print statements showing "Step 1/4" and "Step 2/5" (and any
other "Step X/Y" strings) and change them to reflect the new total of 5 steps
(e.g., "Step 1/5", "Step 2/5", etc.) so all CLI progress messages use the same
denominator; ensure you update every occurrence of the literal "Step X/Y" in the
script (search for the exact strings "Step 1/4" and "Step 2/5") to avoid mixed
totals.
---
Nitpick comments:
In `@hooks/post_gen_project.py`:
- Around line 54-56: Replace bare command names in all Popen([...]) calls with
resolved absolute paths: use shutil.which to locate the executable (e.g.,
resolve "go" via shutil.which("go") and error if not found) and then pass the
resolved path as the first element of the argv list to Popen; update every
subprocess invocation in this file (the Popen calls around PROJECT_DIRECTORY) to
use the resolved executable path and keep the cwd and flags unchanged so
bootstrap execution doesn't rely on PATH lookups.
🪄 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
This PR updates the generated service template to use Go’s native structured logging (log/slog) via slog.LogAttrs, while keeping github.com/go-coldbrew/log only for adding typed context attributes.
Changes:
- Replaced ColdBrew
log.*call sites withslog.LogAttrs(...)(and typed attrs) across service, auth, main, and config. - Added an
AddAttrsToContext+slog.LogAttrsexample inEcho. - Updated proto init hook sequencing and bumped several ColdBrew dependency versions.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| hooks/post_gen_project.py | Adds a pre-mock go mod tidy -e step and updates progress step counts. |
| {{cookiecutter.app_name}}/service/service.go | Switches to slog.LogAttrs and demonstrates AddAttrsToContext typed attrs. |
| {{cookiecutter.app_name}}/service/auth/auth.go | Replaces warn logging with slog.LogAttrs + typed attrs. |
| {{cookiecutter.app_name}}/main.go | Logs cb.Run() error using slog.LogAttrs when non-nil. |
| {{cookiecutter.app_name}}/config/config.go | Logs config load errors using slog.LogAttrs. |
| {{cookiecutter.app_name}}/go.mod | Updates ColdBrew module versions (core/errors/interceptors/log/tracing). |
| {{cookiecutter.app_name}}/AGENTS.md | Documents slog.LogAttrs + go-coldbrew/log.AddAttrsToContext pattern. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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 `@hooks/post_gen_project.py`:
- Line 44: Replace PATH-dependent executable names with absolute paths resolved
via shutil.which for all Popen invocations in both init_proto() and init_git():
locate every Popen([...], cwd=PROJECT_DIRECTORY) that uses "go", "make" or "git"
(in init_proto() — the calls at lines corresponding to the five go/make
invocations; and in init_git() — the four entries in GIT_COMMANDS) and resolve
each command before calling Popen; if shutil.which returns None for any of the
executables ("go", "make", "git"), log an error and exit/raise instead of
invoking Popen to avoid PATH injection. Ensure you update the command lists
passed to Popen to use the resolved executable paths while keeping the original
arguments unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
Summary
go-coldbrew/logto nativeslog.LogAttrswith typed attrsservice/service.goEcho handler showsAddAttrsToContext+slog.LogAttrspatterngo-coldbrew/logimport remaining is forAddAttrsToContextin service.goChanges
service/service.golog.Info(ctx, ...)slog.LogAttrs(ctx, slog.LevelInfo, ...)+AddAttrsToContextdemoservice/auth/auth.golog.Warn(ctx, ...)slog.LogAttrs(ctx, slog.LevelWarn, ..., slog.String(...))main.golog.Error(ctx, cb.Run())slog.LogAttrs(ctx, slog.LevelError, ..., slog.Any("err", err))config/config.golog.Error(ctx, ...)slog.LogAttrs(ctx, slog.LevelError, ..., slog.Any("err", err))Test plan
make test && make lintRef: go-coldbrew/log#27
Summary by CodeRabbit
Documentation
Chores