Figure out integration story for logging errors, especially using zap

[prashantv]

Right now, if we do zap.Error(err) and the error is a multierr, then we end up with a single concatenated string that contains all the errors.

Ideally we would have output more similar to zap.Errors when writing out a multiple errors.

The multierr just returns an error, which may be:

• nil if there were no errors
• single error if there was only one error
• type wrapping []error if there's multiple errors

I think the experience we want is a single function that takes the error, and returns either:

• zap.Skip() if there was no error
• zap.Error(..) if there's a single error
• zap.Errors(...) if there are multiple errors

cc @abhinav and @akshayjshah

I've documented some options for this below, I'm leaning towards the latter options. Would be great to document any other options.

Expose Causes(error) []error from multierr

The user would be required to do:

var errs error
errs = multierr.Append(errs, ...)
logger.Error("Here are the errors.", zap.Errors(multierr.Causes(errs)))

The Causes function would always allocate a slice, and return a slice with either 0, 1, or N elements depending on the input. The final output would always contain "errors" even if there's 0 errors or just 1 error.

Implement zap.MarshalLogArray in the internal multierr type

Since the underlying type is not exported, the user would probably do:

var errs error
errs = multierr.Append(errs, ...)
logger.Error("Here are the errors.", zap.Any("errors", errs))

This would require a dependency on zap from multierr -- if there is a dependency, I think the other way makes more sense.

The final output would always contain "errors" (similar to above).

Have zap implicitly recognize multierr

zap could recognize a multierr error, and automatically get all the errors using Causes() []error and use zap.Errors instead.

zap would need to depend on multierr (which seems like a more reasonable dependency) but implicit handling may be a little unexpected.

Add explicit zap.MultiError(error) field type

This would still require a dependency, but would make the checks more explicit. zap.MultiError would return fields depending on the number of errors. This would be the ideal user experience, but extends the zap interface to add another Field constructor which takes error. This may be confusing to users.

Have a subpackage of multierr that provides the above field constructor

We could add a multierr/zerr package for users logging multierr with zap. This avoids cross dependencies between the core packages, but the zerr package could depend on both multierr and zap, and provide a field constructor Error(err error) zap.Field that does the same logic as the previous option.

The user experience is a little worse since users now need to import a second package. However, the cost may be worth avoiding cross dependencies or extending zap's interface.

[abhinav]

What about zap recognizing the Causes() []error without depending on
multierr? We can add to the contract that if an error contains multiple
errors, it implements the interface,

type errorGroup interface { Causes() []error }

zap will simply define its own copy of the interface and attempt to type cast
errors into it.

[prashantv]

Yep that avoids the dependency, it is still implicit, and I think @akshayjshah had concerns about this implicit behaviour.

I think the zap -> multierr dependency isn't a big deal. I think the real question is about whether the implicit behaviour is OK -- especially if it affects other types. If we want to make sure that other types aren't affected, then we can make an interface with an unexported method:

type MultiError interface {
  error 
  Causes() []error
  // private method to ensure other types don't implement this
  multierror()
}

That way, the special handling will only affect multierr errors. I really don't like the idea of exposing that interface though, I'd prefer to not expose it, or expose it without any private methods.

[akshayjshah]

In general, I'd prefer to respect the error interface. If we want our errors to have special serialization logic, let's implement fmt.Formatter with support for %+v - then we'll get something like this:

{
  "error": "oh no!; bad things happened; write failed",
  "errorVerbose": " 3 errors:\\n* oh no!\\n* bad things happened\\n* write failed",
}

If we really want to represent the multiple errors as a JSON array, though, I'd prefer to just export

type MultiError interface {
  Causes() []error
}

and handle it explicitly in zap.Error.

[prashantv]

I don't think the fmt.Formatter implementation works well for multierr -- you get no extra information, and the verbose error is harder to read.

I'm OK with exporting MultiError but making it clear that the interface is not guaranteed to be implemented by an error returned from multierr (since we only implement it if there's actually multiple errors, so Append(nil, errors.New("")) will not implement that interface.

I'd like zap.Error to use errors as a key if it detects a MultiError instead of the standard error key.

[akshayjshah]

I don't think the fmt.Formatter implementation works well for multierr -- you get no extra information, and the verbose error is harder to read.

Does any of this add extra information? This seems largely presentation-related.

I'm OK with exporting MultiError but making it clear that the interface is not guaranteed to be implemented by an error returned from multierr (since we only implement it if there's actually multiple errors, so Append(nil, errors.New("")) will not implement that interface.

Yep, totally makes sense. If we don't call the interface MultiError, it'll be clearer that we don't always return it...but this'll definitely take some documentation.

I'd like zap.Error to use errors as a key if it detects a MultiError instead of the standard error key.

This makes using a search index challenging, since you can't tell what field zap.Error(err) will put the message in. Especially for application code that's many layers away from the origin of the error(s), I suspect that having some errors under error and others under errors will be quite confusing.

I'd prefer this:

"error": {
  "error": "oh no!; bad things happened; write failed",
  "causes": [
    "oh no!",
    "bad things happened",
    "write failed"
  ]
}

This also gives us a clear path to support pkg/error's Causer interface - we'll just have a causes array with a single element.

[abhinav]

So to summarize,

• multierr will export a Causes() []error method on its error type. The
  contract is that the returned error may have that method.

• zap adds special handling on zap.Error where if the error has a
  Causes() []error method, it uses the object,

  {
    "error": err.Error(),
    "causes": [e.Error() for e in err.Causes()],
  }
  

  Otherwise it defaults to err.Error().

Open questions:

• Does multierr export the interface? I like the idea of exporting the
  method but not the interface because it ensures we keep using error rather
  than multierr.Error.
• If we don't export the interface, do we expose a multierr.Causes(error) []error
  function? This will resolve Caller to 'demux' list of multiple errors returned #10.

@prashantv @akshayjshah Thoughts?

[abhinav]

Support landed in zap. Next release will include this.