${key}Verbose and ${key}Causes for wrapped errors

[jkawamoto]

encodeError cannot provide ${key}Verbose and ${key}Causes fields
if the error implements fmt.Formatter and/or errorGroup interfaces
but is wrapped.

This change uses errors.As instead of a type switch to check if
the given error and the underlying errors are compatible with the
above interfaces, and computes ${key}Verbose and ${key}Causes fields
with unwrapped error.

This commit also upgrades github.com/pkg/errors to v0.9.1 because
v0.8.1 doesn't support Unwrap method.

[codecov]

Codecov Report

Merging #941 (7233263) into master (3748251) will increase coverage by 0.10%.
Report is 190 commits behind head on master.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #941      +/-   ##
==========================================
+ Coverage   98.02%   98.12%   +0.10%     
==========================================
  Files          44       44              
  Lines        1974     1975       +1     
==========================================
+ Hits         1935     1938       +3     
+ Misses         30       29       -1     
+ Partials        9        8       -1     

Files	Coverage Δ
zapcore/error.go	96.87% <100.00%> (+0.10%)	⬆️

... and 1 file with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[abhinav]

Thanks! Would you mind rebasing on master?

[jkawamoto]

Thanks for the quick review. I've just rebased and pushed.

[jkawamoto]

@abhinav Shall I rebase it one more time so that it has a linear history?

[abhinav]

@jkawamoto No, that shouldn't be necessary. We'll squash this into one commit when we merge it.
I would give a couple more days for the second review.

[prashantv]

Sorry for the delay in reviewing -- while the change itself looks fine, though there is a small behaviour change that I was worried about.

We should figure out what are the behaviour changes that this causes, and see if we should be backwards compatible (e.g., log both errorCauses and errorVerbose, though that leads to a lot of duplication).

[prashantv]

This is a slight behaviour change, what previously used errorVerbose now uses errorCauses.

[jkawamoto]

Yes, in some cases, log entries will have errorCauses instead of errorVerbose. However, this change affects mostly only uber-go/multierr users. From the discussion in #470, they prefer errorCauses than errorVerbose if both are available. So, I think this change would be fine.

[prashantv]

Agree that for the multierr use-cases, having errorCauses instead of errorVerbose is fine. That problem made me think of more complex case, where it's not quite clear what the right behaviour should be.

Currently, we only check the first error to see if it matches errorGroup or fmt.Formatter, but with this change, if some sub-error matches errorGroup or fmt.Formatter, only that sub-error be used to populate the errorCauses / errorVerbose field. Let's take a real example:

  underlying := multierr.Combine(errors.New("a failed"), errors.New("b failed"))

  wrapped1 := fmt.Errorf("wrap1: %w", underlying)
  wrapped2 := fmt.Errorf("wrap2: %w", wrapped1)
  wrapped3 := fmt.Errorf("wrap3 :%w", wrapped2)

  logger.Error("no wrap", zap.Error(wrapped3))

This will look like:

"error":"wrap3 :wrap2: wrap1: a failed; b failed",
"errorCauses":[{"error":"a failed"},{"error":"b failed"}]

This is worse with errorVerbose, since the verbose output will actually be just the underlying error, it's not actually a longer version of error.

Given that, I'm not sure if this is something we should change by default. Instead, we could allow error encoding to be more flexible (also discussed in uber-go/multierr#23), so then users can choose how they want their errors encoded.

[jkawamoto]

I think it's a little subjective but in your example, this output

"error":"wrap3 :wrap2: wrap1: a failed; b failed",
"errorCauses":[{"error":"a failed"},{"error":"b failed"}]

is the desired one for me. I can retrieve the log with a JSON query and process logs with jq, and the additional messages ("wrap3 :wrap2: wrap1") can be found in the error field.

The problem with errorVerbose is that it's not easy to query logs with it. Even counting how many errors are underlying needs a string parser. So, I'd like to have errorCauses if it's available.