Less verbose output for multierr #470
Conversation
Following the discussions on #460, uber-go/multierr#6, and (most recently) uber-go/multierr#23, reduce log verbosity for `multierr`. This is fully backward-compatible with the last released version of zap. The small changes introduced here do two things: 1. First, we either report `errorCauses` or `errorVerbose`, but not both. 2. Second, we prefer `errorCauses` to `errorVerbose`. I think that this addresses our top-level wants without breaking any interfaces or removing behavior we've already shipped. If we ever decide to cut a new major release of zap, we should treat errors like durations and times - they're special types for which users choose a formatter. In a future release, we can add an `ErrorEncoder` interface that the JSON encoder and console encoder implement, and make the error field attempt an upcast into that type. That would let the user supply their own error encoder (much like they supply their own time and duration encoders now). Even if we do that, though, I suspect that we'll want to preserve the behavior here as the default.
Codecov Report
@@ Coverage Diff @@
## master #470 +/- ##
==========================================
- Coverage 97.32% 97.21% -0.12%
==========================================
Files 36 36
Lines 1907 1902 -5
==========================================
- Hits 1856 1849 -7
- Misses 42 44 +2
Partials 9 9
Continue to review full report at Codecov.
|
zapcore/error.go
Outdated
| if group, ok := err.(errorGroup); ok { | ||
| return enc.AddArray(key+"Causes", errArray(group.Errors())) | ||
| } | ||
|
|
||
| if fancy, ok := err.(fmt.Formatter); ok { | ||
| verbose := fmt.Sprintf("%+v", fancy) |
There was a problem hiding this comment.
nit: use type switch now that it's one or the other?
switch e := err.(type) {
case errorGroup:
...
case fmt.Formatter:
...
}
There was a problem hiding this comment.
Oh...yeah, that totally makes sense.
There was a problem hiding this comment.
I think this is a step in the right direction, so we should definitely do this.
I'm still thinking about customizability:
uber-go/multierr#23 (comment)
I'd imagine the default error encoder has the same behaviour as this PR.
|
Yep, outlined a plan for making this configurable in the PR description. I'm not planning to put cycles into that until we need that customizability or someone else files an issue, though. |
Following the discussions on #460, uber-go/multierr#6, and
(most recently) uber-go/multierr#23, reduce log verbosity for
multierr. This is fully backward-compatible with the last releasedversion of zap.
The small changes introduced here do two things:
errorCausesorerrorVerbose, but notboth.
errorCausestoerrorVerbose.I think that this addresses our top-level wants without breaking any
interfaces or removing behavior we've already shipped.
In a future minor, we can add an
ErrorEncoderinterface that theJSON encoder and console encoder implement, and make the error field
attempt an upcast into that type. That would let the user supply their
own error encoder (much like they supply their own time and duration
encoders now). Even if we do that, though, I suspect that we'll want to
preserve the behavior here as the default.