errors: Support multierr and pkg/errors' causer #460
Conversation
Codecov Report
@@ Coverage Diff @@
## master #460 +/- ##
==========================================
+ Coverage 97.17% 97.22% +0.04%
==========================================
Files 35 36 +1
Lines 1876 1907 +31
==========================================
+ Hits 1823 1854 +31
Misses 43 43
Partials 10 10
Continue to review full report at Codecov.
|
abhinav
force-pushed
the
multierr-integration
branch
from
5ec9f19 to
4a35849
Compare
zapcore/error.go
Outdated
| return encodeError("error", e.err, enc) | ||
| } | ||
|
|
||
| func (e *errArrayElem) Release() { |
There was a problem hiding this comment.
Elsewhere in zap, I believe that we're calling these methods Free. Would be nice to be consistent here.
This changes how we log errors to break down multierr and pkg/errors
errors if possible.
If an error is a multierr error, its individual items will be listed
under `${key}Causes` as objects. Roughly,
"error": "foo; bar; baz",
"errorVerbose":
`the following errors occurred:
- foo
- bar
- baz`,
"errorCauses": [
{"error": "foo"},
{"error": "bar"},
{"error": "baz"},
]
Similarly, if an error is a pkg/errors causer, the cause will be listed
under `${key}Causes` by itself.
"error": "foo: bar",
"errorVerbose": "foo: bar\n[stack trace]"
"errorCauses": [
{
"error": "bar",
"errorVerbose": "bar\n[stack trace]",
},
]
See: uber-go/multierr#6
abhinav
force-pushed
the
multierr-integration
branch
from
d7e56a7 to
506766d
Compare
There was a problem hiding this comment.
I don't think we should release this integration -- this is a pretty poor user experience. If they use a multierr, they now get 3 copies of the same error, one separated by semicolons, one by newlines, and one as separate array elements.
For Causer, it makes sense to have errorCauses and the %+v version, since we get the list of errors separately from one, and the stack trace from the other.
|
@prashantv That's a good point, and one that I didn't stop to consider fully before we landed this. Appreciate the vigilance. I mulled this over a while last night, and I'm increasingly dissatisfied with this whole approach to fancy errors - it feels odd and un-idiomatic to me. The
If we don't like |
This reverts commit acea907.
|
I'm not convinced that we should do something special for how The simplest option is to have
Perhaps we should have a separate |
|
or have a configurable error encoder, as is done with a very other types? I
gotta say, we've been using zap.Error() with our own error class which
implements Formatter, and we built our own multiline-friendly encoder, and
it's great for development to see readable stack traces for errors.
…On Tue, Jul 4, 2017 at 11:13 AM Prashant Varanasi ***@***.***> wrote:
I'm not convinced that we should do something special for how pkg/errors
works, but not for multierr. The main issue is that the interfaces we're
dealing with don't really specify how they're used. E.g., even if a type
implements Formatter, there's no guarantee it'll have unique output.
The simplest option is to have zap.Error output error: err.Error(). It's
unsurprising and doesn't automatically add any extra fields, nor does it
bloat the output if a pkg/errors error is used. The issue with forcing
users to call err.Error() is:
- The callsite changes from a simple zap.Error(err) to zap.String("error",
err.Error())
- If the err was somehow nil, it'll panic
Perhaps we should have a separate ErrorVerbose that does extra checks,
but in that I'd want to detect multierr explicitly, and only log error
and errorCauses for it.
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#460 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAGlMVltstwjWm7y72jJOnz3-MkuE3K7ks5sKgIxgaJpZM4OK5ul>
.
|
|
Let's discuss on uber-go/multierr#23 - I made the mistake of kicking this discussion off in two places, and it's a bit confusing. |
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.
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.
This changes how we log errors to break down multierr and pkg/errors
errors if possible.
If an error is a multierr error, its individual items will be listed
under
${key}Causesas objects. Roughly,Similarly, if an error is a pkg/errors causer, the cause will be listed
under
${key}Causesby itself.See: uber-go/multierr#6