Fire all hooks, even in case of an error

[flimzy]

The current behavior aborts logging with an error, the first time a hook returns an error.

For me this meant that a misconfiguration of loggly resulted in sentry not getting logs. This PR changes that behavior, such that all hooks will now be called, even in the case of an error with one of them.

[freeformz]

The semantics of Fire can't be changed in the v1.X.X release series because that would change the logrus.Hook interface and break everyone who uses Hooks.

It's debatable whether or not it's a good idea to have errors cancel processing the rest of the hooks. As you point out a misconfigured hook or one that starts erroring for a random reason greatly impacts your ability to continue logging properly.

My suggestion would be to extend Hook a little and implement something like*:

type Continuer interface {
  Continue() bool
}

// If the error returned from Fire is a Continuer and Continue() is true then the 
// rest of the error will be logged and the remaining Hooks will be called.

IMO it's important not to remove Fire's error though. Some hooks will filter message/data contents to remove PII info and if those hooks were to fail for some reason, it could be a security incident to log the unfiltered data.

*Modulo naming, which is hard.

[flimzy]

Sorry, I didn't ralize that LevelHooks was part of the public API. I'm also a bit confused as to why it is, since it's documented as an 'internal type'.

I'll see about modifying my PR.

[flimzy]

I've pushed a new implementation of this PR, which retains the original LevelHooks.Fire() API.

It adds a new, unexported type, to report all errors. If we think others will call LevelHooks.Fire() directly, and may want access to all errors, that type should perhaps be exported. I'm not sure if that's a meaningful use case, though, so maybe wait until someone requests it?

[freeformz]

Continuing to run the remaining hooks after a hook returned an error would also be a change that is too large, at least IMO, for anything more than a full version release (v1.4.2 -> v2). It's a significant change to the behavior of hooks. And it's a possible security problem (see my comment above).

I think it's necessary for hook authors and/or users of hooks to decide if hook processing should stop after the first error or not. I'm also not convinced one way or the other the best way to go about that.

Related to this there is also #986, which is related, but different.

Maybe something combined that addresses everything detailed in both issues would be workable?

[flimzy]

Thanks for the additional comments.

How would you feel about a configuration option to enable this behavior? I think it would be possible to add a config option to the Logger type to control this behavior. Perhaps FireAllOnError bool, or similar? If you think this is a reasonable change, I can work on adjusting the PR.

If not, I think at minimum it's appropriate to document that the first hook failure will abort logging. I hope that such a doc change PR won't be controversial :)

[freeformz]

Please do the doc PR. FWIW: I thought it was already documented, but re-reading the godoc it appears that I was wrong.

I feel that a PR to add something like "FireAllHooks", defaulting to false (maintains backwards compatibility), with appropriate documentation detailing default and true behavior is a decent option. It should also play well with #986.

[flimzy]

I've updated this PR now with the new configuration flag (off by default). Please let me know your thoughts.

[freeformz]

Maybe?

When false (default), the first hook that returns an error will stop any unfired hooks from firing and that error will be returned. When true, all hooks are fired and any errors are collected and returned.

Does this mean that mutliErr should be mentioned and exported?

i.e. I'm asking myself how would I process / handle the errors in the case of true and the documentation doesn't really give me any guidance.

[flimzy]

So I'm digging into the error proposals for Go, and the closest relevant one I think is this. It's only a proposal, and probably won't be accepted as is, at least not for a long time, but it's simple, and I think it might be the right approach. Basically, it suggests implementing a specific interface:

interface {
       Errors() []error
}

I could make multiErr satisfy this interface, without exporting multiErr directly. Then it's just a matter of documenting how to access the multiple errors, probably with a brief example.

[flimzy]

I've updated the PR once again with some changes to multi-error handling. I don't know if any or all of it is a good idea. I welcome your thoughts :)

[freeformz]

Nice. I like the use of the interface to define the behavior of Errors().
There are a few minor nits I'd like to clean up and I'd like to take a look at supporting go1.13 error wrapping if possible, but they don't need to be done in this PR IMO.

[flimzy]

I'd like to take a look at supporting go1.13 error wrapping if possible

What, specifically, would you like to support here? I added Unwrap() to the multiErr type, but it only returns the first error, since the Go 1.13 errors don't have any explicit support for multiple errors. There's probably more that could be done, but it wasn't obvious to me which direction to take.

[freeformz]

I think it's worth experimenting with the %w formatting verb as well, but like I said, that isn't something that needs to happen in this PR, IMO.