Add error.Recovered().#9
Conversation
Signed-off-by: Jeff Ortel <jortel@redhat.com>
|
Warning Rate limit exceeded@jortel has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 13 minutes and 26 seconds before requesting another review. ⌛ 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. 📒 Files selected for processing (1)
WalkthroughAdds error.Recovered(p any) to convert recovered panic values into errors; replaces old logr.WithName usage with a new logr.New(name, level, kvpair...) API and level constants, updates logger internals and tests to use explicit levels, and bumps the Go version to 1.21. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Caller
participant Func as FunctionWithDefer
participant RecoveredFn as error.Recovered
Caller->>Func: call()
rect rgba(224,240,255,0.6)
note right of Func: runtime may panic(value)
Func-->>Func: panic(value)
Func->>Func: deferred recover() captures p
Func->>RecoveredFn: Recovered(p)
alt p is nil
RecoveredFn-->>Func: nil
else p is error
RecoveredFn-->>Func: same error
else p non-error
RecoveredFn-->>Func: Errorf("%+v", p)
end
end
Func-->>Caller: returns error or nil
sequenceDiagram
autonumber
actor App
participant New as logr.New
participant Env as ENV(LOG_LEVEL)
participant Sink as logr.Sink
App->>New: New(name, providedLevel, kvpair...)
New->>Env: read LOG_LEVEL (if set)
alt LOG_LEVEL valid
New-->>New: override providedLevel with env value
end
New->>Sink: set sink.level = max(NORMAL, computedLevel)
New-->>App: Logger wrapping Sink
App->>Sink: optional WithLevel/Level adjustments
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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.
Actionable comments posted: 1
🧹 Nitpick comments (2)
error/error.go (1)
35-35: Consider clarifying the comment.The phrasing "returns an error recovered panic" is slightly awkward.
Apply this diff to improve clarity:
-// Recovered returns an error recovered panic. +// Recovered converts a recovered panic value into an error.error/error_test.go (1)
111-111: Remove unnecessary return statement.The
returnstatement at the end of the test function is unnecessary.Apply this diff:
err = fn(nil) g.Expect(err).To(gomega.BeNil()) - return }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
error/error.go(1 hunks)error/error_test.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
error/error_test.go (1)
error/error.go (2)
Recovered(36-45)New(48-53)
🔇 Additional comments (1)
error/error.go (1)
40-43: Verify: Should error-type panics be wrapped to capture the recovery stack trace?When
pis already anerror, it's returned as-is without wrapping. This means no stack trace is captured at the point of recovery—only the original error is preserved.If the panic was triggered with a plain
errors.New()orfmt.Errorf(), the resulting error will lack any stack trace. Consider whether wrapping withWrap(err)would be more consistent with the package's behavior, ensuring stack traces are captured at recovery points.If wrapping is desired, apply this diff:
err, cast := p.(error) - if !cast { + if cast { + err = Wrap(err) + } else { err = Errorf("%+v", p) }
Signed-off-by: Jeff Ortel <jortel@redhat.com>
Signed-off-by: Jeff Ortel <jortel@redhat.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
logr/log_test.go (1)
10-16: Consider testing the LOG_LEVEL environment override.The test verifies basic logger creation and level assignment, but doesn't test the
LOG_LEVELenvironment variable override behavior introduced inNew(). Consider adding a test case that setsLOG_LEVELand verifies it overrides the passed level parameter.Example test addition:
func TestNewWithEnvOverride(t *testing.T) { g := gomega.NewGomegaWithT(t) os.Setenv("LOG_LEVEL", "5") defer os.Unsetenv("LOG_LEVEL") log := New("test", 2) g.Expect(log.GetSink().(*Sink).level).To(gomega.Equal(5)) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (4)
cmd/main.go(1 hunks)go.mod(1 hunks)logr/log_test.go(1 hunks)logr/logger.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
cmd/main.go (1)
logr/logger.go (1)
New(30-43)
logr/log_test.go (1)
logr/logger.go (2)
New(30-43)Sink(20-27)
🪛 GitHub Actions: RunTests
logr/log_test.go
[error] 29-29: TestList execution logged an unexpected error path; test output indicates failures in New()/list path.
🔇 Additional comments (6)
go.mod (1)
3-3: Go 1.21 required for built-inmax()function.The version bump to Go 1.21 is necessary because
logr/logger.goline 41 uses themax(0, level)built-in function, which was introduced in Go 1.21.logr/log_test.go (2)
19-27: LGTM! API migration and typo fix look good.The migration from
WithNametoNew("Test", 0)is correct, and the struct name correction from "Persion" to "Person" fixes the typo.
29-29: Verify the original review's assumptions are incorrect.The diff confirms that line 29 (
err := liberr.New("Test")) was not modified by this PR—it existed in the previous version unchanged. The PR only modified earlier lines inTestList(): replacingWithName()withNew()and fixing a typo.Since line 29 wasn't changed, any pipeline failure at that line is not caused by this PR's changes. The review conflates "line referenced in pipeline failure" with "line modified in PR," which are different concerns. Additionally, the PR objectives mentioned
error.Recovered()were addressed in commitb38ad84, not in this PR.Likely an incorrect or invalid review comment.
cmd/main.go (1)
11-11: LGTM! Correct migration to the new API.The change from
WithName("Test", "and", "fields")toNew("Test", 0, "and", "fields")correctly adopts the new constructor. Passinglevel=0maintains the default verbosity behavior.logr/logger.go (2)
29-43: Verify the LOG_LEVEL override behavior is intentional.The
New()function unconditionally overrides thelevelparameter whenLOG_LEVELis set and valid. This means:
- If
LOG_LEVEL="5"is set, callingNew("test", 2)will result in level 5, not 2- The passed
levelparameter is silently ignoredWhile this enables runtime configuration, it could be surprising to callers who explicitly pass a level. Consider documenting this behavior clearly, or using the env var as a default only when level is 0 or negative.
If this override-all behavior is intentional, please confirm and ensure it's documented in the function's godoc comment.
45-48: LGTM! Backward compatibility maintained.The
WithName()function now delegates toNew(name, 0, kvpair...), which maintains backward compatibility by using the lowest verbosity level (0) as the default. The compatibility wrapper pattern is appropriate here.
Signed-off-by: Jeff Ortel <jortel@redhat.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
logr/logger.go (1)
137-148: Fix incomplete state copying in Sink methods.Both
Sink.WithNameandSink.WithValuescreate newSinkinstances but fail to copy critical fields from the parent sink:
WithName(lines 138-140) only copies thename, losingdelegate,fields,development,structured, and crucially thelevel.WithValues(lines 143-148) only copiesnameandfields, still losingdelegate,development,structured, andlevel.This means derived loggers created via these methods lose their configuration, including the log level introduced in this PR.
Apply this diff to preserve all sink state:
func (s *Sink) WithName(name string) logr.LogSink { - return &Sink{name: name} + return &Sink{ + delegate: s.delegate, + fields: s.fields, + name: name, + development: s.development, + structured: s.structured, + level: s.level, + } } func (s *Sink) WithValues(kvpair ...any) logr.LogSink { + merged := log.Fields{} + for k, v := range s.fields { + merged[k] = v + } + for k, v := range fields(kvpair) { + merged[k] = v + } return &Sink{ - name: s.name, - fields: fields(kvpair), + delegate: s.delegate, + fields: merged, + name: s.name, + development: s.development, + structured: s.structured, + level: s.level, } }
🧹 Nitpick comments (1)
logr/logger.go (1)
35-49: Consider validation and document the environment override behavior.The implementation has a few concerns:
- The
LOG_LEVELenvironment variable always overrides the explicitly providedlevelparameter, which may be surprising to callers who intentionally pass a specific level value.- There's no upper bound validation for the environment value—negative values are caught by
max(NORMAL, level), but extremely large values (e.g., 999) are not constrained.- This environment parsing happens on every call to
New, which could be inefficient if called frequently.Consider these improvements:
func New(name string, level int, kvpair ...any) logr.Logger { v := os.Getenv(EnvLevel) if v != "" { if n, err := strconv.Atoi(v); err == nil { + // Validate environment level is within reasonable bounds + if n < 0 || n > DEBUG { + // Either clamp or log warning + n = max(NORMAL, min(n, DEBUG)) + } level = n } } return logr.New( &Sink{ name: name, fields: fields(kvpair), level: max(NORMAL, level), }) }Additionally, document in the function's godoc that
LOG_LEVELenvironment variable takes precedence over thelevelparameter.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
logr/log_test.go(1 hunks)logr/logger.go(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
logr/log_test.go (1)
logr/logger.go (2)
New(36-49)Sink(26-33)
🔇 Additional comments (5)
logr/log_test.go (2)
3-9: LGTM!The import additions are appropriate for the new test functionality.
24-41: LGTM!The migration from
WithNametoNewwith a level of 0 (NORMAL) is correct, and the typo fix fromPersiontoPersonimproves code clarity.logr/logger.go (3)
19-23: LGTM!The log level constants provide clear semantic meaning and follow Go naming conventions.
51-54: LGTM!The delegation to
Newwith a defaultNORMALlevel is a clean design. This maintains backward compatibility for callers who don't need to specify a custom level.
56-81: LGTM!The removal of level initialization from
Initis correct, as level configuration has been moved to theNewconstructor. The remaining environment-driven formatter configuration is appropriate.
Signed-off-by: Jeff Ortel <jortel@redhat.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
logr/log_test.go (1)
11-32: Restore the previousEnvLevelvalue instead of always unsetting it.The anonymous block in
TestNewnow unsetsEnvLevelafter the test, but it still doesn’t restore any pre-existing value. IfEnvLevelwas set before this test runs, it will be cleared permanently, which can surprise other tests or callers.Consider capturing and restoring the prior value (as suggested in the earlier review comment), for example:
func() { old, ok := os.LookupEnv(EnvLevel) _ = os.Setenv(EnvLevel, "2") defer func() { if ok { _ = os.Setenv(EnvLevel, old) } else { _ = os.Unsetenv(EnvLevel) } }() log := New("test", 0) g.Expect(log).NotTo(gomega.BeNil()) g.Expect(log.GetSink().(*Sink).level).To(gomega.Equal(2)) }()This keeps the environment fully consistent before and after the test while still validating the
EnvLeveloverride behavior.
🧹 Nitpick comments (1)
logr/logger.go (1)
117-122: Preserve full Sink state inWithLevelinstead of rebuilding a partial Sink.
WithLevelcurrently constructs a newSinkwith onlyname,fields, andlevel, droppingdelegate,development,structured, and any future fields. That makes it tightly coupled to callers re-wrapping the new sink vialogr.New, and will break if the level-adjust helper is ever switched to the idiomaticlogger.WithSink(s.WithLevel(n))pattern recommended in the go-logr docs. (pkg.go.dev)To make
WithLevelrobust and future-proof, shallow-copy the receiver and only overridelevel:func (s *Sink) WithLevel(level int) logr.LogSink { - return &Sink{ - name: s.name, - fields: s.fields, - level: max(0, level), - } + ns := *s // preserve delegate, flags, and existing fields + ns.level = max(0, level) + return &ns }This keeps all existing configuration (formatter, structured/dev flags, delegate) while adjusting the level only.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
logr/log_test.go(1 hunks)logr/logger.go(1 hunks)logr/pkg.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
logr/pkg.go (1)
logr/logger.go (1)
Sink(14-21)
logr/log_test.go (2)
logr/pkg.go (3)
New(23-36)Level(44-52)EnvLevel(13-13)logr/logger.go (1)
Sink(14-21)
🔇 Additional comments (2)
logr/log_test.go (1)
34-51: Tests correctly exercise the newNew/LevelAPI.
TestNewandTestListboth validate the intended behavior ofNewandLevel, and the migration from the oldWithNameconstructor toNew("Test", 0)looks consistent and clear.logr/pkg.go (1)
10-41: Env constants andNewconstructor behavior look solid.The
Env*constants, level constants, andNew/WithNamehelpers form a clear API:
NewrespectsLOG_LEVELwhen set, clamps to at leastNORMAL, and seeds theSinkwith name and fields.WithNameis a simple convenience wrapper that keeps the default level semantics.This aligns with the PR objective of centralizing logger construction and making level selection explicit.
Signed-off-by: Jeff Ortel <jortel@redhat.com>
Signed-off-by: Jeff Ortel <jortel@redhat.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
logr/logger.go (3)
104-113: WithName overwrites existing name rather than composing hierarchically
WithNamesetsname: nameand discards any existings.name. In many logr implementations,WithNameappends to the existing name (e.g.,parent.child) so nested components retain hierarchy. If you intend hierarchical names, consider composing with the parent rather than overwriting it.Example:
func (s *Sink) WithName(name string) logr.LogSink { - return &Sink{ - delegate: s.delegate, - fields: s.fields, - name: name, - development: s.development, - structured: s.structured, - level: s.level, - } + return &Sink{ + delegate: s.delegate, + fields: s.fields, + name: joinName(s.name, name), + development: s.development, + structured: s.structured, + level: s.level, + } } + +func joinName(parent, child string) string { + if parent == "" { + return child + } + return parent + "." + child +}
117-125: WithValues drops existing fields instead of merging; verify desired semantics
WithValuesbuildsfields: fields(kvpair)and doesn’t incorporates.fields, so chaininglogger.WithValues("a", 1).WithValues("b", 2)loses"a"from the second logger. If you want cumulative context (which is typical with logr), you likely want to merge the existing fields with the new ones.One possible approach:
func (s *Sink) WithValues(kvpair ...any) logr.LogSink { - return &Sink{ - delegate: s.delegate, - fields: fields(kvpair), - name: s.name, - development: s.development, - structured: s.structured, - level: s.level, - } + return &Sink{ + delegate: s.delegate, + fields: mergeFields(s.fields, fields(kvpair)), + name: s.name, + development: s.development, + structured: s.structured, + level: s.level, + } } + +func mergeFields(base, extra log.Fields) log.Fields { + if len(base) == 0 { + return extra + } + out := log.Fields{} + for k, v := range base { + out[k] = v + } + for k, v := range extra { + out[k] = v + } + return out +}If overwriting is intentional, consider documenting that
WithValuesis non-cumulative to avoid surprises.
128-136: WithLevel clamps negative levels to 0; confirm this matches your logging model
WithLeveluseslevel: max(0, n), which:
- Prevents callers from ever setting a negative level.
- Relies on Go 1.21’s predeclared
max(consistent with the go.mod bump).- Means
Level(logger, -1)(or similar) can’t be used to “turn off” logging via a very low threshold.If your intended contract is “levels are non‑negative verbosity values (0 = normal, higher = more verbose)”, this is fine and worth documenting. If you instead want to allow negative levels for stricter filtering, you may want to drop the clamp:
func (s *Sink) WithLevel(n int) logr.LogSink { return &Sink{ delegate: s.delegate, fields: s.fields, name: s.name, development: s.development, structured: s.structured, - level: max(0, n), + level: n, } }
1 participant
Add new logr constructor that supports setting the level:
Adjust the level.
Add convenience for:
Ensures recovered panic is an error.
Summary by CodeRabbit
New Features
Tests
Chores