Report connection errors to sentry

[valentin-krasontovitsch]

When the hutch runner fails due to a connection, authentication or worker
setup error, the exception is only logged.

With this change the exception is also reported to sentry, if sentry is
available.

Closes #288

[michaelklishin]

The right thing to do here is to report this using an existing reporting backend API.

[valentin-krasontovitsch]

I amended the commit - now we're using the error handler backends.

Due to the implementation of the error handlers (they expect to be in the context of a message, having available such things as properties, payload, and a consumer) I had to hackatihack add a dummy properties object.

An alternative would be to change the implementation of (potentially all) handle functions to make the first three arguments optional, or make the functions tolerate that the arguments are nil (more precisely: that the argument properties does not have a method message_id) - cf. the files in lib/hutch/error_handlers/, for instance this line in the sentry handler which refers to message_id.

[michaelklishin]

@valentin-krasontovitsch we can extend the interface, e.g. introduce a new method, instead of passing in dummy objects.

[michaelklishin]

Amending existing function to allow nils where normally they should not be allowed sounds risky to me.

[michaelklishin]

The current state of this PR can only be described as a nasty hack.

We should introduce a new interface method for this purpose.

I'm also not entirely sure if creating a new class per invocation is necessary. Are classes garbage collected the way that's implied here?

[valentin-krasontovitsch]

It is a nasty hack indeed : D Should have probably marked this PR as work in progress, I definitely appreciate your feedback and input!

I'm a wee bit clueless about ruby, so perhaps you could point me in the right direction - what plays the role of the interface for the error handlers?

Might I add a method in the Logging module, which at the moment is included in each error handler class?

Or would I rather (a bit shaky on the details right now, sorry for the lack of precise terms) add something to the Errorhandlers module?

Gonna try to read up a bit about modules and classes in ruby in the meanwhile : )

[michaelklishin]

I think at this point having a base class for error reports (ErrorHandler?) is the best option. The base version of all methods then can throw a NotImplementedError to hint that subclasses must override.

Then ErrorHandler#handle_setup_exception (other naming suggestions are welcome) would be used
to handle the scenarios you are after.

This will involve updating all implementations with this new method but hopefully it's going to be similar enough to the #handle method that's already there.

How does that sound?

[valentin-krasontovitsch]

That sounds perfect, thanks for the help! Hopefully I'll get to it today : )

…

On Mon, Nov 13, 2017 at 12:27 AM, Michael Klishin ***@***.***> wrote: I think at this point having a base class for error reports (ErrorHandler?) is the best option. The base version of all methods then can throw a NotImplementedError to hint that subclasses must override. Then ErrorHandler#handle_setup_exception (other naming suggestions are welcome) would be used to handle the scenarios you are after. This will involve updating all implementations with this new method but hopefully it's going to be similar enough to the #handle method that's already there. How does that sound? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#297 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/APMdBVgU1mOakROld05xBpb_1K8nMEggks5s137HgaJpZM4QWGcQ> .

[michaelklishin]

Any progress on this one?

[valentin-krasontovitsch]

unfortunately I got sidetracked : ( I'm still on it though, gonna in'shallah push something soon ^^

…

On Tue, Nov 28, 2017 at 4:28 AM, Michael Klishin ***@***.***> wrote: Any progress on this one? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#297 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/APMdBSmZjonbDqDidPpAlR0eUxui-52eks5s633fgaJpZM4QWGcQ> .

[valentin-krasontovitsch]

So I just pushed and created two versions: #301 just adds a method to each class that handles setup exceptions, and calls that method in the appropriate place.

#302 Does the same as above, and also adds an interface as you described above.

I couldn't help myself and started googling about abstract classes / interfaces and NotImplementedError in ruby, found different opinions about the subject (a blog saying "ruby has duck typing, so don't use NotImplementedError, use documentation instead, a library for abstract classes where the docs imply not to use them, and a discussion in the style guide regarding when to use NotImplementedError, so I didn't quite know what to do, and decided to leave the choice to you : )