-
Notifications
You must be signed in to change notification settings - Fork 557
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
Error wrapping and returned error types... #9
Comments
When `zerolog` logs an error message with an `Error()` finalizer, it returns a `error`. If the `StackTrace()` event has been called, the `error` will be a wrapped error using `github.com/pkg/errors` and will include the stack trace. There are a few subtle differences introduced with this change: 1. It is possible to defeat this behavior using the Fields() method. 2. Only one error can be handled by an Event at a time. Time was spent considering introducing an accumulator for errors that would be evaluated at finalizer-time, but this seemed unnecessary given errors should be wrapped and multiple calls to `Err()` seemed unlikely in practice. The behavior of the `Msg()` finalizer was left in tact in order to avoid any performance penalty in non-error handling conditions. The deferral of error handling can best be observed by looking at the performance hit of an `Err()` Event with and without `StackTrace()`: ``` BenchmarkErrorContextFields-8 20000000 84.8 ns/op BenchmarkErrorContextFields_WithStackTrace-8 500000 2552 ns/op BenchmarkLogContextFields-8 30000000 47.6 ns/op PASS ok github.com/rs/zerolog 4.642s ``` Fixes: rs#9
I would suggest support for error wrappers like Probably it could be done with a test for kind of |
If anything, it would have to be in a separate package as I don't want to force external deps on people using |
See discussion in #11 |
As per #9 and to offer a different approach from #11 and #35 this PR introduces custom error serialization with sane defaults without breaking the existing APIs. This is just a first draft and is missing tests. Also, a bit of code duplication which I feel could be reduced but it serves to get the idea across. It provides global error marshalling by exposing a `var ErrorMarshalFunc func(error) interface{}` in zerolog package that by default is a function that returns the passed argument. It should be overriden if you require custom error marshalling. Then in every function that accept error or array of errors `ErrorMarshalFunc` is called on the error and then the result of it is processed like this: - if it implements `LogObjectMarshaler`, serialize it as an object - if it is a string serialize as a string - if it is an error, serialize as a string with the result of `Error()` - else serialize it as an interface The side effect of this change is that the encoders don't need the `AppendError/s` methods anymore, as the errors are serialized directly to other types.
Fixed by #35 |
First things first: thank you for the library. It's very interesting and promising. I've used it a few times in the last week with good success but noticed something in practice that I wanted to bring up.
Would you be interested in an
Error()
orErrorf()
event processor that would return anerror
object? I'm already in the error path at this point in time and am less concerned about performance.I'm hoping to arrive at something like the following in order to reduce redundant code:
The thinking being that t
Stacktrace()
would capture the current stack trace and include it in theerror
object returned to the caller, and theError()
event handler would return a wrapped error similar togithub.com/pkg/errors
,github.com/hashicorp/errwrap
,github.com/contiv/errored
.Thoughts? Thanks in advance!
The text was updated successfully, but these errors were encountered: