fix: Err() should record error when ErrorStackMarshaler returns nil#764
fix: Err() should record error when ErrorStackMarshaler returns nil#764Yanhu007 wants to merge 2 commits into
Conversation
When ErrorStackMarshaler returns nil (e.g. for errors without stack traces), Err() returned the event without recording the error field. This caused error messages to silently disappear from log output. The nil case should skip the stack field but still fall through to AnErr() to record the error itself. Regression introduced in f6fbd33. Fixes rs#762
There was a problem hiding this comment.
Pull request overview
Fixes a regression where Event.Err() could drop the error field when ErrorStackMarshaler returns nil (e.g., errors without stack traces), while still allowing stack output when available.
Changes:
- Adjust
Event.Err()stack-marshaling switch so anilstack-marshaler result skips only the stack field (and does not short-circuit the error field recording).
Comments suppressed due to low confidence (1)
event.go:483
- This change fixes
Event.Err()whenErrorStackMarshalerreturnsnil, but the same early-return behavior still exists inContext.Err()(context.go:210-226) and in stack handling insideappendFieldList(fields.go:82-99). If the intent of this PR/issue is thatErr()should always still record theerrorfield when the stack marshaler returnsnil, those call sites should be updated too; otherwise the behavior will remain inconsistent depending on whether the error is added via anEvent, viaLogger.With().Err(...), or viaFields(...). Also note there are existing tests that currently assert the old behavior (e.g.TestEvent_ErrWithStackMarshalerNil,TestContext_ErrWithNilStackMarshaler) and will need to be updated to match the new contract.
if e.stack && ErrorStackMarshaler != nil {
switch m := ErrorStackMarshaler(err).(type) {
case nil:
// Stack marshaler returned nil; skip the stack field
// but still record the error below.
case LogObjectMarshaler:
e = e.Object(ErrorStackFieldName, m)
case error:
e = e.Str(ErrorStackFieldName, m.Error())
case string:
e = e.Str(ErrorStackFieldName, m)
default:
e = e.Interface(ErrorStackFieldName, m)
}
}
return e.AnErr(ErrorFieldName, err)
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Apply the same fix to Context.Err() which had the identical bug: case nil returned early without recording the error field. Spotted by code review feedback.
|
Good catch on Both |
|
This one doesn't update the unit tests, probably should prefer #758 |
|
#763 was merged so this should be closed. |
Fixes #762
Problem
Since commit f6fbd33, when
ErrorStackMarshalerreturnsnil(e.g. for errors that don't have stack traces),Err()returns immediately without callingAnErr(ErrorFieldName, err). This silently drops the error message from log output.Before the regression:
{"level":"error","error":"something failed","message":"..."}After the regression:
{"level":"error","message":"..."}(error field is missing!)
Fix
Change
case nil: return eto an emptycase nil:so control falls through to theAnErr()call that records the error field. The stack field is correctly skipped when the marshaler returns nil.