Zap integration (again)

[akshayjshah]

trafficstars

Currently, errors produced by this package implement fmt.Formatter with special support for the "%+v" format. We should consider removing this support, since it doesn't actually add any information (which is typically the purpose of the + flag).

err := multierr.Append(errors.New("foo"), errors.New("bar"))
fmt.Println(err.Error())
// foo; bar
fmt.Printf("%v\n", err)
// foo; bar
fmt.Printf("%+v\n", err)
// the following errors occurred:
//  - foo
//  - bar

This came up because it makes logging these errors with zap particularly verbose - see post-merge discussion on uber-go/zap#460.

[prashantv]

When displaying errors to users, it's nice not to have a single line of errors separated by semicolons. %+v comes in handy at that point.

I don't think + has a single purpose, it's actually pretty varied: https://play.golang.org/p/gBjEMtOiiS

It usually means more verbose output, but more verbose could mean anything, and I think the handling of errors for this package fits in with that definition.

The more I think about it, the more it seems like to really solve https://github.com/uber-go/zap/pull/460, we really want to identify the specific error rather than using a generic interface which doesn't convey much about the underlying data?

[akshayjshah]

Collating some ideas from uber-go/zap#460, I think there are two solutions on the table right now.

1. multierr exports an IsMultierr function, which zap uses to special-case this package. For multierr, zap outputs "error": err.Error(), "errorCauses": [...]. Net result: transparent support for pkg/errors, multierr, and other errors, with less repetition for multierr. Zap continues to output error, errorVerbose, and errorCauses fields, depending on the error type.
2. zap drops support for fancy errors completely. This still lets zap.Error handle nil errors and call err.Error() under the hood, but it means that we won't have automatic support for logging stack traces from pkg/errors or causes from multierr. (We could also ship a VerboseError field constructor that uses fmt.Sprintf("%+v", err) instead.)

Given the number of error packages out in the world, I'm inclined toward (2). This keeps the user experience of both packages nice, reduces coupling, keeps zap's default output terse, and generally behaves as most users expect.

[abhinav]

Approach 2 sounds reasonable to me.

We'll need to be able to argue that the output is not part of the API contract because otherwise this would qualify as a breaking change.

[prashantv]

The more I think about this, the more it seems like we need to make zap more flexible and allow users to choose what they want. I think the absolute minimum that Error should do is:

• Skip if err is nil
• Log err.Error() as a field

On the other hand, I would also like to:

• Include the stacktrace as a field if pkg/errors is used. Ideally just the stacktrace as a errorStack field, which might require post-processing of the %+v output. However having errorVerbose would be a fine compromise
• For multierr, log errors as an array with each error. (I'm not sure whether we would still include the err.Error() as a field).

Given no options, I think the safest option is to do the minimum. However, we should think about allowing custom error encoding (and possibly allow these to be stacked) so users can have Error have custom behaviours.

[akshayjshah]

I think we can accomplish all four goals (nil handling, err.Error() as a field, verbose output for pkg/errors, and array output for multierr) without any backward-incompatible changes, and without the current level of verbosity for multierr.

Unfortunately, we'll have to include err.Error() for multierr - ElasticSearch, Solr, and similar systems get very upset if a field has multiple types, so we should try to keep the type of the error field consistent.

[prashantv]

Sure, I think we can achieve pretty much what we want by skipping verbose for multierr. Longer term though, I'd like to think about making this more flexible -- it's possible users with custom formatting don't actually have a more verbose message where errorVerbose is not required.

Regarding the multiple types, I intended to avoid the issue by having it add the field errors (note the plural) if it's a multierr, and error otherwise. I'm not sure that's a good default behaviour though, but if we allowed formatting to be customized, the user could opt-in.