Implement transport level error handlers#863
Implement transport level error handlers#863peterbourgon merged 14 commits intogo-kit:masterfrom sagikazarmark:error-handler
Conversation
peterbourgon
left a comment
peterbourgon
left a comment
There was a problem hiding this comment.
Looks like it's on the right track.
transport/amqp/error_handler.go
Outdated
| // Usually this means logging the error. | ||
| type ErrorHandler interface { | ||
| Handle(err error) | ||
| } |
There was a problem hiding this comment.
You shouldn't need to define ErrorHandler in each individual transport package. Try defining it in the top-level transport package.
log/error_handler.go
Outdated
| // ErrorHandler is a transport error handler implementation which logs an error. | ||
| type ErrorHandler struct { | ||
| logger Logger | ||
| } |
There was a problem hiding this comment.
This belongs in package transport, not package log.
log/error_handler_test.go
Outdated
| var output []interface{} | ||
|
|
||
| logger := log.Logger(log.LoggerFunc(func(keyvals ...interface{}) error { | ||
| output = keyvals |
There was a problem hiding this comment.
Probably
output = append(output, keyvals...)
transport/grpc/server.go
Outdated
| } | ||
|
|
||
| // ServerErrorHandler is used to handle non-terminal errors. By default, no errors | ||
| // are handled. |
There was a problem hiding this comment.
I think it would be a little bit less ambiguous to say e.g. "By default, non-terminal errors are ignored."
transport/http/server.go
Outdated
| func ServerErrorLogger(logger log.Logger) ServerOption { | ||
| return func(s *Server) { s.logger = logger } | ||
| return func(s *Server) { | ||
| if s.errorHandler == nil { |
There was a problem hiding this comment.
Because the ErrorLogger should be considered deprecated and if someone uses an ErrorHandler, it should take precedence.
There was a problem hiding this comment.
IMO ServerErrorLogger should now just be a helper function that calls e.g.
func ServerErrorLogger(logger log.Logger) ServerOption {
return func(s *Server) { s.errorHandler = LoggingErrorHandler(logger) }
}No need for precedence. You can keep the deprecation warning.
|
Addressed all comments |
peterbourgon
left a comment
peterbourgon
left a comment
There was a problem hiding this comment.
A couple minor tweaks and this LGTM! Thank you very much, I think this is a great addition.
transport/error_handler.go
Outdated
| } | ||
|
|
||
| func (h *LogErrorHandler) Handle(err error) { | ||
| _ = h.logger.Log("err", err) |
|
If you rebase on master, the builds should be fixed. |
|
Thanks! Rebased and applied the requested changes. |
|
Should I update the examples in this PR or in a separate one? |
|
This one, if you don't mind. |
|
Not at all. I'm wondering if we should somehow consider adding support for something like requested in #792 |
|
Seems like a good idea. At first glance, if the errorHandler had a signature like |
|
I will add that as well then. |
|
Done and done. |
|
Anything else I can do here @peterbourgon ? |
|
Thanks, this is a great addition! |
|
Thanks! |
* Add error handler implementation to the log package * Add error handler to http transport * Add error handler to amqp transport * Add error handler to awslambda transport * Add error handler to grpc transport * Add error handler to nats transport * Move error handler interfaces to transport package * Move log error handler to transport package * Remove error logger precedence * Improve documentation wording * Adjust transport package documentation * Remove ignore error * Update examples * Add context to the error handler signature
2 participants
This is a first implementation for #861
I haven't provided any tests for now as the error loggers were not tested either, but I can write tests if necessary.
ToDo