Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Zap integration (again) #23

Open
akshayjshah opened this issue Jul 3, 2017 · 6 comments
Open

Zap integration (again) #23

akshayjshah opened this issue Jul 3, 2017 · 6 comments

Comments

@akshayjshah
Copy link

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
Copy link
Collaborator

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 uber-go/zap#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
Copy link
Author

akshayjshah commented Jul 4, 2017

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.

@akshayjshah akshayjshah changed the title Remove fmt.Formatter implementation Zap integration (again) Jul 4, 2017
@abhinav
Copy link
Collaborator

abhinav commented Jul 5, 2017

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
Copy link
Collaborator

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
Copy link
Author

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.

akshayjshah pushed a commit to uber-go/zap that referenced this issue Jul 6, 2017
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.
@prashantv
Copy link
Collaborator

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.

akshayjshah added a commit to uber-go/zap that referenced this issue Jul 7, 2017
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.
@abhinav abhinav removed their assignment Apr 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants