feat: refactor LogFields to use shared RequestContext#24
Conversation
LogFields now wraps *options.Options and stores log fields in the shared RequestContext, eliminating a separate context.WithValue per request. All methods delegate to the underlying Options; Range uses RangeSlice for slice-snapshot iteration. Depends on go-coldbrew/options#17. The go.mod replace directive must be removed and the options version bumped once that PR is merged and tagged.
|
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 0 minutes and 55 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. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR refactors Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ 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 |
There was a problem hiding this comment.
Pull request overview
Refactors loggers.LogFields to share storage with the options request context, reducing per-request context allocations and centralizing context-field handling.
Changes:
- Refactor
LogFieldsto wrap*options.Optionsand delegate field operations (incl.Range()viaRangeSlice()). - Update context helpers (
AddToLogContext,FromContext) to delegate tooptionsequivalents. - Regenerate gomarkdoc READMEs and adjust module metadata (
go.mod/go.sum).
Reviewed changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
loggers/fields.go |
Re-implements LogFields on top of *options.Options and updates context helpers to use options APIs. |
loggers/README.md |
Regenerated gomarkdoc output reflecting new LogFields API docs and updated line links. |
loggers/slog/README.md |
Regenerated gomarkdoc output with updated source line links. |
wrap/README.md |
Regenerated gomarkdoc output with updated source line links. |
go.mod |
Adds a local replace for github.com/go-coldbrew/options (noted as temporary). |
go.sum |
Removes checksums for github.com/go-coldbrew/options v0.2.6 (consistent with local replace usage). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Add adds or modifies a log field. | ||
| func (o *LogFields) Add(key string, value any) { | ||
| if len(key) > 0 { | ||
| o.mu.Lock() | ||
| if o.m == nil { | ||
| o.m = make(map[string]any, 2) | ||
| } | ||
| o.m[key] = value | ||
| o.mu.Unlock() | ||
| o.inner.Add(key, value) | ||
| } |
There was a problem hiding this comment.
LogFields is an exported type, but its methods now dereference o.inner without guarding against nil. This makes the zero value of LogFields (e.g., var lf LogFields or &LogFields{}) panic on Add, which is a behavioral/API change from the previous implementation where the zero value was usable. Consider making the methods nil-safe (no-op/empty behavior when inner is nil) or providing an exported constructor and documenting that LogFields must be obtained via FromContext/AddToLogContext only.
There was a problem hiding this comment.
Fixed — all methods now guard against nil inner. The zero value of LogFields is safe (no-op). Added doc noting LogFields should be obtained via FromContext/AddToLogContext.
| replace github.com/go-coldbrew/options => ../options | ||
|
|
There was a problem hiding this comment.
The committed replace github.com/go-coldbrew/options => ../options will break CI/builds for anyone who doesn't have a sibling ../options directory, and it also changes the module graph for downstream consumers. Please avoid committing local-path replaces; use a go.work file for local development, or bump the github.com/go-coldbrew/options version to the tagged release once options#17 is available and remove the replace before merging.
| replace github.com/go-coldbrew/options => ../options |
There was a problem hiding this comment.
Acknowledged — this replace directive is intentional for local development while options#17 is in review. It will be removed and replaced with a proper version bump once options is tagged. This PR should not be merged until then.
![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 `@go.mod`:
- Around line 5-8: Remove the local sibling-module replacement from go.mod (the
line `replace github.com/go-coldbrew/options => ../options`) and keep only the
proper `require github.com/go-coldbrew/options v0.2.6` entry; do not merge this
replace directive into the branch — instead leave the PR as draft until
`github.com/go-coldbrew/options` is tagged upstream and then update the required
version in go.mod to the released tag so that loggers/fields.go uses the
released API.
🪄 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: dac5979a-fca8-440e-94e8-bb2e87523390
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (5)
go.modloggers/README.mdloggers/fields.gologgers/slog/README.mdwrap/README.md
The zero value of LogFields (inner == nil) is now safe — methods are no-ops instead of panicking. Documents that LogFields should be obtained via FromContext/AddToLogContext.
Summary
LogFieldsnow wraps*options.Optionsinstead of owning its ownsync.RWMutex + mapAddToLogContext/FromContextdelegate tooptions.AddToLogFields/options.LogFieldsFromContextRequestContextas options — onecontext.WithValueinstead of twoRange()callsRangeSlice()to preserve slice-snapshot iteration behaviornewLogFields(), legacy context key, andsyncimportDepends on: go-coldbrew/options#17 (must be merged and tagged first)
Note:
go.modcontains a temporaryreplacedirective for local development. This will be removed and the options version bumped once options#17 is tagged.Test plan
logtests pass (log,loggers/slog,wrap, all backend examples)make lintpasses (0 issues)errors,interceptors,coreSummary by CodeRabbit
Release Notes
New Features
LogFieldsfor sync.Map-compatible operations:Delete,Load,Range, andStore.Documentation
Chores