Figure out integration story for logging errors, especially using zap about multierr

[abhinav]

Support landed in zap. Next release will include this.

[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 #10.

@prashantv @akshayjshah Thoughts?