fix: record error field when ErrorStackMarshaler returns nil

[walsm232]

Note

I am not certain that this is a bug but it does look like one - at least the behaviour has changed in a way that I am not sure is expected / intended but I'm hoping you folks might be able to help confirm / take a look

In v1.35, Event.Err / Context.Err (and the stack path in appendFieldList for error values) return early when ErrorStackMarshaler(err) returns nil. That skips the subsequent AnErr / field encoding, so the main error field is omitted from the log line.

A nil return from ErrorStackMarshaler means “nothing to put in the stack field” (e.g. plain errors.New / fmt.Errorf without a pkg/errors-style stack). I don't think it should mean “omit the error entirely”

This regressed from v1.34, where an empty case nil branch still fell through to AnErr.

The behavior change was introduced in #748 (commit f6fbd33).

How to Reproduce

Minimal setup: enable stack on the logger (With().Stack() or log.Log().Stack()), set a global ErrorStackMarshaler that returns nil (same idea as go.elastic.co/ecszerolog/internal.MarshallStack for plain errors.New / fmt.Errorf errors), then log with .Err(err).Msg("...").

With zerolog v1.34.x, the JSON line includes the error field (default key "error", or whatever ErrorFieldName is set to, e.g. ECS error.message).

With zerolog v1.35.x, that field is missing: the line typically only has message (and level/timestamp), even though err is non-nil.

Extra Notes / Question

When ErrorStackMarshaler(err) returns nil, is it intentional that Err() no longer records the main error field (i.e. AnErr is skipped), or should nil mean only “no value for the stack field”, with the error text still emitted as in v1.34?

If the new behaviour is intended, it would help to document that nil from ErrorStackMarshaler suppresses the entire Err() payload, since that affects common setups (e.g. ECS logging) where nil only indicates “no stack trace available” for typical Go errors I think.

#748’s description says Context/Fields were restructured when ErrorStackMarshaler returns nil for coverage but it doesn’t define intended behaviour for whether Err() should still record the error. Was dropping the error field intentional? Or am I maybe doing something wrong?

[Copilot]

Pull request overview

This PR restores the pre-v1.35 behavior where a nil return from ErrorStackMarshaler(err) suppresses only the stack field (e.g. "stack") but does not suppress the primary error field (e.g. "error"), fixing a regression introduced by the early-return logic added in #748.

Changes:

• Stop early-returning from Event.Err() and Context.Err() when ErrorStackMarshaler returns nil, so AnErr(ErrorFieldName, err) still runs.
• Stop early-returning from appendFieldList’s error+stack path when ErrorStackMarshaler returns nil, so field processing continues normally.
• Update tests to assert that the error field is still emitted when the stack marshaler returns nil.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
fields.go	Removes early return on ErrorStackMarshaler(...) == nil so subsequent fields aren’t skipped and stack omission doesn’t alter other output.
event.go	Ensures Err(err) still records the main error field even when no stack trace is available.
event_test.go	Updates expectation to include the error field for the nil-stack-marshaler case.
context.go	Ensures Context.Err(err) still records the main error field even when no stack trace is available.
context_test.go	Updates expectation to include the error field for the nil-stack-marshaler case.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[walsm232]

cc: @IDisposable in case you get an opportunity to look as I think you made the related changes

[IDisposable]

Dropping the error field was definitely an oversight. Sorry for introducing this bug. Your fix look perfect.

If it affected the coverage, it's legitimately because of a limitation of the testing tool.