982043 - Improve report sending code for handling all kind of HTTP failures

Status: RESOLVED WONTFIX

(Testing Graveyard :: Mozmill, defect)

Description

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Over on bug 970820 we fixed the case for socket timeouts. But the list of exceptions to catch is kinda long. We should come up with a single type of exception we could through in such a case, and which can be handled by calling code.

I also wonder if we should better make use of the Python requests (http://docs.python-requests.org/en/latest/) package to send reports. Dave, what do you think?

Sayan, would you like to work on this bug?

Comment 1

[Dave Hunt [:davehunt] [he/him] ⌚BST]

(In reply to Henrik Skupin (:whimboo) from comment #0)
> I also wonder if we should better make use of the Python requests
> (http://docs.python-requests.org/en/latest/) package to send reports. Dave,
> what do you think?

Requests has a nicer API, but I would say unless it's needed we should avoid adding the extra dependency.

Comment 2

[Sayan Paul]

I would like to work on this bug.

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+1]]

It's great to hear that. So lets drop the requests idea for now and make it that we raise a proper error in such cases. Therefor we should create our own errors.py module, which should hold all the different error (exception) classes.

Comment 4

[Sayan Paul]

There will be basically the following errors :
1. HTTP error 
   .1 Value Error
2. URL Error
Anything else ?

Comment 5

[Henrik Skupin [:whimboo][⌚️UTC+1]]

The errors we have to check for you have updated with your last patch. What this code should do now, is raising a general error e.g. SendReportError subclassed from Exception. Calling code would have to handle that appropriately, so we don't have all the print lines in report.py.

Comment 6

[Sayan Paul]

Sorry for the delay, but what all classes will the 'errors.py' file have ?
One will be the SendReportError class , What all functions or arguments should be in it ?

Comment 7

[Henrik Skupin [:whimboo][⌚️UTC+1]]

You may want to check the other modules in mozmill and give us a listing of used Exception/Error classes. With that info we can build up a list of errors the modules in the mozmill package can throw. For example have a look at bug 975068 where I'm doing it for jsbridge.

Comment 8

[Sayan Paul]

(In reply to Henrik Skupin (:whimboo) from comment #7)
> You may want to check the other modules in mozmill and give us a listing of
> used Exception/Error classes. With that info we can build up a list of
> errors the modules in the mozmill package can throw.

Are u referring to different modules in the report.py file or different modules in the whole package itself.

Comment 9

[Henrik Skupin [:whimboo][⌚️UTC+1]]

The term module references a specific .py file, while a package would be mozmill or jsbridge. So here I refer to the files used by mozmill. Bug 975068 gives an example for jsbridge, how I created a shared module for error classes.

Comment 10

[Sayan Paul]

These were the Exceptions/Error classes used in mozmill modules:
report.py 
      HandlerMatchException         L42
      file open error               L103
      HTTPError                     L131
      ValueError                    L136
      URLError                      L139
      Exception                     L142

python_callbacks.py
      BaseException                 L33

logger.py
      Does not raise exceptions but handles using simple try - except blocks

handlers.py
      HandlerMatchException defined and used here

__init__.py
     KeyError                           L111
     NotImplementedError                L113
     Exception                          L128
     JSBridgeDisconnectError L373 ,L436,L458
     BaseException

Comment 11

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to Sayan Paul from comment #10)
> These were the Exceptions/Error classes used in mozmill modules:
> report.py 
>       HandlerMatchException         L42

See below.

>       file open error               L103

We do not re-throw anything here, but simply print out the warning that we failed to create the report file. Nothing we should expose here.

>       HTTPError                     L131
>       ValueError                    L136
>       URLError                      L139
>       Exception                     L142

This whole block needs to be refactored, so we can throw our own exception with the proper failure message. 

> python_callbacks.py
>       BaseException                 L33

Callbacks are not used at the moment. What we could do here is to raise our own CallbackException.

> handlers.py
>       HandlerMatchException defined and used here

Perfect for the errors.py module. Also there are a couple of Exception classes raised in load_handler. Those should also be transformed into our own e.g. HandlerLoadError.

> __init__.py
>      KeyError                           L111
>      NotImplementedError                L113

KeyError is not getting raised again. And the latter is a basic Python exception. So we are fine here.

>      Exception                          L128

This exception type doesn't look appropriate. There should be a better one regarding invalid argument. We should change that. But I don't see needs to create our own class.

>      JSBridgeDisconnectError L373 ,L436,L458

I will touch this with my patch on bug 975068. No need for you to change anything here.

>      BaseException

This can be updated with the exception which gets thrown in handler.py.

Comment 12

[Sayan Paul]

So we need to make changes all the modules in mozmill and errors.py as a common shared Exception module containing all the respective exception classes

Comment 13

[Joel Maher ( :jmaher ) (UTC -8)]

:whimboo, please follow up here.

Comment 14

[Henrik Skupin [:whimboo][⌚️UTC+1]]

There is nothing I have to follow-up. Comment 12 is a statement of what to do on this bug, right? Sayan, will you have the time and interest to continue on this bug? I would appreciate that.

Comment 15

[Sayan Paul]

I am interested to work on this but i'm not being able to comprehend what is exactly required from me.

Comment 16

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Ok, so I missread it. Sorry. When reading the through the bug, I can see that I already gave all the informatin. So lets combine all in a single comment:

* Create an error.py module, which will contain all the error classes (see my patch on bug 975068 as example)
* Create a SendReportError class derived from Exception (maybe we could also inject a MozmillError class in the middle)
* Change the exception handling of the send_report method so it throws a single SendReportError exception in case of a failure. Therefore all print lines can be removed.
* Update the calling method to handle the exception by printing out the failure details

Please let me know if something is unclear. Also as for now I would drop all the other kind of exceptions, and only focus on the single one above.

Comment 17

[Joel Maher ( :jmaher ) (UTC -8)]

Hi Sayan, I am just a friendly bugzilla troll making sure mentored bugs have proper information.  :whimboo is away on vacation for the next week, but I would like to make sure you have what you need to work on this bug and are still interested in it?

Comment 18

[Sayan Paul]

Sorry i've been busy for a long time and couldn't work on it. This should've been over a long time ago. I will start working on it asap.

Comment 19

[Joel Maher ( :jmaher ) (UTC -8)]

Oh very cool! Please don't hesitate to ask any questions in the bug or on irc!

Comment 20

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Mozmill will reach its end of life soon. We are currently working on getting all the tests for Firefox ported to Marionette. For status updates please see bug 1080766.