Add error handler class #302
Conversation
To handle setup exceptions, a method was added to each error handler. Said method is subsequently called when setup of the client fails. This is to ensure that a failed setup is not only logged, but also reported to e.g. sentry. Closes ruby-amqp#288
| include Logging | ||
|
|
||
| def handle(properties, payload, consumer, ex) | ||
| raise NotImplementedError.new |
There was a problem hiding this comment.
Unfortunately, NotImplementedError is subclassed from ScriptError which is subclassed from Exception.
I think it would be safer to use something that can be caught by StandardError. I'm assuming that most top-level error handlers do a generic rescue, which would only catch StandardError subclasses.
There was a problem hiding this comment.
There seems to be no standard way of doing this besides the above, and not throwing an Error, but relying on the no method error being thrown when a subclass doesn't implement the method.
Do you suggest to define a custom Error class that inherits from StandardError?
Please cf. #297 for links to discussions regarding the above error... I'm a newbie to ruby (heh, that rhymed), so I really don't feel confident to make a call, due to the different opinions I found...
Also, we should probably fix the ack base class as well if we mind the above.
There was a problem hiding this comment.
NotImplementedError is right on the money in terms of what we'd like to communicate, though.
There was a problem hiding this comment.
Again, I don't really know, not a seasoned ruby programmer, but: the docs for the NotImplementedError can be interpreted in such a way that it is only to be used for instanced where a feature is not implemented on the current platform, due to a hardware / software deficit of the underlying system, not a missing implementation in the ruby code.
Even the error message in error.c, where the error class is defined, sounds as follow:
[$FUNCTION_NAME() function is unimplemented on this machine](https://github.com/ruby/ruby/blob/1ae69a20645d7be556fd0ecd001b0bb5f3959330/error.c#L2338)
Of course I don't actually get that message when I raise the error in irb, just get the text NotImplementedError...
The above are just meant as hints - you guys should decide how to handle this problem : )
There was a problem hiding this comment.
I don't think this discussion has to delay this PR, it's great :)
I have only called this out because in April this year we had apps not working due to a missed implementation, and we did not get any Honeybadger reports since it was a NotImplementedError and zipped past through all layers of error reporting.
There was a problem hiding this comment.
@sldblog the fact that in Ruby a rescue without specific exceptions doesn't catch everything (match all Exception subclasses) not a problem library authors should worry about. Ruby designers made a mistake, using a rescue without an exception class is therefore is not a good idea. NotImplementedError is exactly the exception that should be used in this case.
| @@ -90,6 +90,9 @@ def start_work_loop | |||
| @worker.run | |||
| :success | |||
| rescue ConnectionError, AuthenticationError, WorkerSetupError => ex | |||
| Hutch::Config[:error_handlers].each do |backend| | |||
| backend.handle_setup_exception(ex) | |||
| end | |||
There was a problem hiding this comment.
One thing though, could this be tested in cli_spec, please?
|
I can’t say that I didn’t look at adding a test, but I was a wee bit
stumped as to what it might look like. You got me ^^
I’ll take another look and try adding a test : )
…On Wed, 6 Dec 2017 at 16:28, Michael Klishin ***@***.***> wrote:
***@***.**** requested changes on this pull request.
Please add a test case, as requested in the comments.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#302 (review)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/APMdBdy8YREX9HLVW7nKZJvMg-IL2Kcwks5s9rK4gaJpZM4Q2Z9z>
.
|
|
@valentin-krasontovitsch thank you. |
|
So I just pushed a commit that introduces a test case, making sure that if I'm not super happy with the test, as it relies on the implementation of the tested method: If you have any pointers or ideas about how this could be tested in a less brittle way, let me know. Maybe the coupling to the implementation is OK in this case... |
|
So I'm basically happy with the other merge request merged in, but just for completeness sake: Any new thought / comments on the test I wrote? |
|
@valentin-krasontovitsch sorry, this slipped through the cracks. What's the other PR you are referring to? Is this one still relevant and would you like us to review it? |
|
no worries : ) I guess as you've merged this one now, i don't need to answer your second question anymore ^^ |
To handle setup exceptions, a method was added to each error handler.
Said method is subsequently called when setup of the client fails.
This is to ensure that a failed setup is not only logged, but also
reported to e.g. sentry.
Furthermore, a base class indicating the interface for error handlers is added.
Closes #288