Telemetry: Add a new class FailureException.

Telemetry: Add a new exception Error.

This class is a common base class for all Telemetry exceptions.

BUG=460625
Committed: https://crrev.com/623562c7c293a3a58429947ce0269e3da97012b2
Cr-Commit-Position: refs/heads/master@{#318562}

[erikchen, 2015-02-27 18:26:32]

erikchen@chromium.org changed reviewers:
+ nednguyen@google.com

[erikchen, 2015-02-27 18:26:32]

nednguyen: Please review.

I thought for a long time about the best Exception class hierarchy. In the end I
decided to go with the smallest possible change that enables me to continue
improving Telemetry exception handling.

[nednguyen, 2015-02-27 18:54:53]

On 2015/02/27 18:26:32, erikchen wrote:
> nednguyen: Please review.
> 
> I thought for a long time about the best Exception class hierarchy. In the end
I
> decided to go with the smallest possible change that enables me to continue
> improving Telemetry exception handling.

I think the point of exceptions is that they either meant to be caught by client
code, or they provide some common functionality that's useful, e.g: print out
the stack trace for crashed app. I don't see how FailureException fit into this
yet. Maybe we can replace the Failure exception in page_test.PageTest with this?

There are some exception handling code in user_story_runner uses that
exceptions. slamm@ knows more details.

[nednguyen, 2015-02-27 18:55:35]

nednguyen@google.com changed reviewers:
+ slamm@chromium.org

[erikchen, 2015-02-27 19:02:15]

On 2015/02/27 18:54:53, nednguyen wrote:
> On 2015/02/27 18:26:32, erikchen wrote:
> > nednguyen: Please review.
> > 
> > I thought for a long time about the best Exception class hierarchy. In the
end
> I
> > decided to go with the smallest possible change that enables me to continue
> > improving Telemetry exception handling.
> 
> I think the point of exceptions is that they either meant to be caught by
client
> code, or they provide some common functionality that's useful, e.g: print out
> the stack trace for crashed app. I don't see how FailureException fit into
this
> yet. Maybe we can replace the Failure exception in page_test.PageTest with
this?
> 
> There are some exception handling code in user_story_runner uses that
> exceptions. slamm@ knows more details.

When a public method on tab, or inspector backend raises an exception, most
clients don't care what this exception is. They should catch FailureException.
If the client cares about the details, they can catch TimeoutException,
AppCrashException, etc.

[chromium-reviews, 2015-02-27 19:09:25]

OK, lgtm.

Please wait for slamm@
+slamm: do you think we can replace page_test's Failure exception with this
one?

On Fri, Feb 27, 2015, 11:02 AM null <erikchen@chromium.org> wrote:

> On 2015/02/27 18:54:53, nednguyen wrote:
> > On 2015/02/27 18:26:32, erikchen wrote:
> > > nednguyen: Please review.
> > >
> > > I thought for a long time about the best Exception class hierarchy. In
> > the
> end
> > I
> > > decided to go with the smallest possible change that enables me to
> > continue
> > > improving Telemetry exception handling.
>
> > I think the point of exceptions is that they either meant to be caught by
> client
> > code, or they provide some common functionality that's useful, e.g: print
> > out
> > the stack trace for crashed app. I don't see how FailureException fit
> into
> this
> > yet. Maybe we can replace the Failure exception in page_test.PageTest
> with
> this?
>
> > There are some exception handling code in user_story_runner uses that
> > exceptions. slamm@ knows more details.
>
> When a public method on tab, or inspector backend raises an exception, most
> clients don't care what this exception is. They should catch
> FailureException.
> If the client cares about the details, they can catch TimeoutException,
> AppCrashException, etc.
>
>
>
> https://codereview.chromium.org/962283002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[nednguyen, 2015-02-27 19:11:56]

lgtm

[slamm, 2015-02-27 19:30:41]

https://codereview.chromium.org/962283002/diff/1/tools/telemetry/telemetry/co...
File tools/telemetry/telemetry/core/exceptions.py (right):

https://codereview.chromium.org/962283002/diff/1/tools/telemetry/telemetry/co...
tools/telemetry/telemetry/core/exceptions.py:11: """The operation failed to
successfully complete."""
I am a little confused what "FailureException" means.
The doc strong seems to boil down to """There was an exception."""

What about something like

class Error(Exception):
  """Base class for Telemetry exceptions."""

Is that the intention of the new exception class?

[erikchen, 2015-02-27 19:40:41]

Patchset #2 (id:20001) has been deleted

[erikchen, 2015-02-27 19:45:56]

slamm: PTAL

https://codereview.chromium.org/962283002/diff/1/tools/telemetry/telemetry/co...
File tools/telemetry/telemetry/core/exceptions.py (right):

https://codereview.chromium.org/962283002/diff/1/tools/telemetry/telemetry/co...
tools/telemetry/telemetry/core/exceptions.py:11: """The operation failed to
successfully complete."""
On 2015/02/27 19:30:40, slamm wrote:
> I am a little confused what "FailureException" means.
> The doc strong seems to boil down to """There was an exception."""
> 
> What about something like
> 
> class Error(Exception):
>   """Base class for Telemetry exceptions."""
> 
> Is that the intention of the new exception class?

You have correctly interpreted the intention of the new exception class. I've
uploaded a new patch set with your suggested nomenclature.

[slamm, 2015-02-27 19:56:20]

lgtm

https://codereview.chromium.org/962283002/diff/40001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/core/exceptions.py (right):

https://codereview.chromium.org/962283002/diff/40001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/exceptions.py:11: """Base class for Telemetry
exceptions."""
Would you also add a comment with the plan? Perhaps a TODO. Otherwise, it may be
confusing why only some exception classes use Error and others use Exception.

[erikchen, 2015-02-27 20:57:47]

On 2015/02/27 19:56:20, slamm wrote:
> lgtm
> 
>
https://codereview.chromium.org/962283002/diff/40001/tools/telemetry/telemetr...
> File tools/telemetry/telemetry/core/exceptions.py (right):
> 
>
https://codereview.chromium.org/962283002/diff/40001/tools/telemetry/telemetr...
> tools/telemetry/telemetry/core/exceptions.py:11: """Base class for Telemetry
> exceptions."""
> Would you also add a comment with the plan? Perhaps a TODO. Otherwise, it may
be
> confusing why only some exception classes use Error and others use Exception.

What if I just went ahead and made all the Exceptions inherit from Error?

[achuithb, 2015-02-27 21:09:24]

On 2015/02/27 20:57:47, erikchen wrote:
> On 2015/02/27 19:56:20, slamm wrote:
> > lgtm
> > 
> >
>
https://codereview.chromium.org/962283002/diff/40001/tools/telemetry/telemetr...
> > File tools/telemetry/telemetry/core/exceptions.py (right):
> > 
> >
>
https://codereview.chromium.org/962283002/diff/40001/tools/telemetry/telemetr...
> > tools/telemetry/telemetry/core/exceptions.py:11: """Base class for Telemetry
> > exceptions."""
> > Would you also add a comment with the plan? Perhaps a TODO. Otherwise, it
may
> be
> > confusing why only some exception classes use Error and others use
Exception.
> 
> What if I just went ahead and made all the Exceptions inherit from Error?

This would be very helpful actually. Downstream code can just catch Error when
it doesn't care about the specific exception.

[erikchen, 2015-02-27 22:20:15]

slamm: PTAL

[slamm, 2015-02-27 22:26:36]

lgtm

Nice. You just went ahead and reparented all the exceptions. That is even better
than my suggestions.

[erikchen, 2015-02-27 22:28:17]

The CQ bit was checked by erikchen@chromium.org

[erikchen, 2015-02-27 22:28:17]

The patchset sent to the CQ was uploaded after l-g-t-m from nednguyen@google.com
Link to the patchset: https://codereview.chromium.org/962283002/#ps60001 (title:
"Reparent all exceptions.")

[commit-bot: I haz the power, 2015-02-27 22:30:44]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/962283002/60001

[commit-bot: I haz the power, 2015-02-28 00:44:18]

Committed patchset #3 (id:60001)

[commit-bot: I haz the power, 2015-02-28 00:44:45]

Patchset 3 (id:??) landed as
https://crrev.com/623562c7c293a3a58429947ce0269e3da97012b2
Cr-Commit-Position: refs/heads/master@{#318562}