Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2922593004:
[telemetry] Provide some more specific exception classes (Closed)
Description[telemetry] Provide some more specific exception classes
Provide:
- StoryActionError
- TracingException
and use them where appropriate.
BUG=catapult:#3620
Review-Url: https://codereview.chromium.org/2922593004
Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapult/+/4cafad75c8313d6c14b47544574a20ecc49c253f
Patch Set 1 #
Messages
Total messages: 17 (8 generated)
perezju@chromium.org changed reviewers: + nednguyen@google.com
PTAL I also plan to use StoryActionError at [1] for the exception raised when Scrolling gets stuck. Only other remaining case of the generic "Error" is in [2]. Do you think it is OK to leave as is? Define a new exception for that case? Or reuse one of the existing exceptions? [1]: https://cs.chromium.org/chromium/src/tools/perf/page_sets/system_health/brows... [2]: https://cs.chromium.org/chromium/src/tools/perf/profile_creators/update_remot...
The CQ bit was checked by perezju@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/06/02 14:38:05, perezju wrote: > PTAL > > I also plan to use StoryActionError at [1] for the exception raised when > Scrolling gets stuck. > > Only other remaining case of the generic "Error" is in [2]. Do you think it is > OK to leave as is? Define a new exception for that case? Or reuse one of the > existing exceptions? > > [1]: > https://cs.chromium.org/chromium/src/tools/perf/page_sets/system_health/brows... > > [2]: > https://cs.chromium.org/chromium/src/tools/perf/profile_creators/update_remot... My general thinking toward exception is differentexception types are only useful if the handlers handle them differently. For the ease of understanding the error, people should rely on the exception's message.
On 2017/06/02 at 14:41:53, nednguyen wrote: > On 2017/06/02 14:38:05, perezju wrote: > > PTAL > > > > I also plan to use StoryActionError at [1] for the exception raised when > > Scrolling gets stuck. > > > > Only other remaining case of the generic "Error" is in [2]. Do you think it is > > OK to leave as is? Define a new exception for that case? Or reuse one of the > > existing exceptions? > > > > [1]: > > https://cs.chromium.org/chromium/src/tools/perf/page_sets/system_health/brows... > > > > [2]: > > https://cs.chromium.org/chromium/src/tools/perf/profile_creators/update_remot... > > My general thinking toward exception is differentexception types are only useful if the handlers handle them differently. For the ease of understanding the error, people should rely on the exception's message. My issue with that is that "Error" is too generic. When I scrape logs to figure out which kinds of exceptions are seen on benchmark runs I see things like "WebSocketConnectionClosedException", "AdbCommandFailedError", "DevtoolsTargetCrashException", which are great because in a short name they already convey a lot of information, and then the exception message can give even more useful details! Compare that to "Error" which is utterly meaningless. On the other hand the exception message is often too free form, it may or may not span several lines. And it's hard to know where it ends (both for humans and computers). See e.g. in https://goto.google.com/hehox the sort of acrobatics I have to do to extract useful information out of "boring" error names.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/06/02 14:54:15, perezju wrote: > On 2017/06/02 at 14:41:53, nednguyen wrote: > > On 2017/06/02 14:38:05, perezju wrote: > > > PTAL > > > > > > I also plan to use StoryActionError at [1] for the exception raised when > > > Scrolling gets stuck. > > > > > > Only other remaining case of the generic "Error" is in [2]. Do you think it > is > > > OK to leave as is? Define a new exception for that case? Or reuse one of the > > > existing exceptions? > > > > > > [1]: > > > > https://cs.chromium.org/chromium/src/tools/perf/page_sets/system_health/brows... > > > > > > [2]: > > > > https://cs.chromium.org/chromium/src/tools/perf/profile_creators/update_remot... > > > > My general thinking toward exception is differentexception types are only > useful if the handlers handle them differently. For the ease of understanding > the error, people should rely on the exception's message. > > My issue with that is that "Error" is too generic. When I scrape logs to figure > out which kinds of exceptions are seen on benchmark runs I see things like > "WebSocketConnectionClosedException", "AdbCommandFailedError", > "DevtoolsTargetCrashException", which are great because in a short name they > already > convey a lot of information, and then the exception message can give even more > useful details! Compare that to "Error" which is utterly meaningless. > > On the other hand the exception message is often too free form, it may or may > not span several lines. And it's hard to know where it ends (both > for humans and computers). > > See e.g. in https://goto.google.com/hehox the sort of acrobatics I have to do to > extract useful information out of "boring" error names. Error for everywhere is probably not right, but I generally want to minimize the number of exception types. Also we would later use flakiness dashboard for the error data mining use case?
On 2017/06/02 15:03:01, nednguyen wrote: > On 2017/06/02 14:54:15, perezju wrote: > > On 2017/06/02 at 14:41:53, nednguyen wrote: > > > On 2017/06/02 14:38:05, perezju wrote: > > > > PTAL > > > > > > > > I also plan to use StoryActionError at [1] for the exception raised when > > > > Scrolling gets stuck. > > > > > > > > Only other remaining case of the generic "Error" is in [2]. Do you think > it > > is > > > > OK to leave as is? Define a new exception for that case? Or reuse one of > the > > > > existing exceptions? > > > > > > > > [1]: > > > > > > > https://cs.chromium.org/chromium/src/tools/perf/page_sets/system_health/brows... > > > > > > > > [2]: > > > > > > > https://cs.chromium.org/chromium/src/tools/perf/profile_creators/update_remot... > > > > > > My general thinking toward exception is differentexception types are only > > useful if the handlers handle them differently. For the ease of understanding > > the error, people should rely on the exception's message. > > > > My issue with that is that "Error" is too generic. When I scrape logs to > figure > > out which kinds of exceptions are seen on benchmark runs I see things like > > "WebSocketConnectionClosedException", "AdbCommandFailedError", > > "DevtoolsTargetCrashException", which are great because in a short name they > > already > > convey a lot of information, and then the exception message can give even more > > useful details! Compare that to "Error" which is utterly meaningless. > > > > On the other hand the exception message is often too free form, it may or may > > not span several lines. And it's hard to know where it ends (both > > for humans and computers). > > > > See e.g. in https://goto.google.com/hehox the sort of acrobatics I have to do > to > > extract useful information out of "boring" error names. > > Error for everywhere is probably not right, but I generally want to minimize the > number of exception types. Also we would later use flakiness dashboard for the > error data mining use case? lgtm for these 2 extra exception types
On 2017/06/02 at 15:03:01, nednguyen wrote: > On 2017/06/02 14:54:15, perezju wrote: > > On 2017/06/02 at 14:41:53, nednguyen wrote: > > > On 2017/06/02 14:38:05, perezju wrote: > > > > PTAL > > > > > > > > I also plan to use StoryActionError at [1] for the exception raised when > > > > Scrolling gets stuck. > > > > > > > > Only other remaining case of the generic "Error" is in [2]. Do you think it > > is > > > > OK to leave as is? Define a new exception for that case? Or reuse one of the > > > > existing exceptions? > > > > > > > > [1]: > > > > > > https://cs.chromium.org/chromium/src/tools/perf/page_sets/system_health/brows... > > > > > > > > [2]: > > > > > > https://cs.chromium.org/chromium/src/tools/perf/profile_creators/update_remot... > > > > > > My general thinking toward exception is differentexception types are only > > useful if the handlers handle them differently. For the ease of understanding > > the error, people should rely on the exception's message. > > > > My issue with that is that "Error" is too generic. When I scrape logs to figure > > out which kinds of exceptions are seen on benchmark runs I see things like > > "WebSocketConnectionClosedException", "AdbCommandFailedError", > > "DevtoolsTargetCrashException", which are great because in a short name they > > already > > convey a lot of information, and then the exception message can give even more > > useful details! Compare that to "Error" which is utterly meaningless. > > > > On the other hand the exception message is often too free form, it may or may > > not span several lines. And it's hard to know where it ends (both > > for humans and computers). > > > > See e.g. in https://goto.google.com/hehox the sort of acrobatics I have to do to > > extract useful information out of "boring" error names. > > Error for everywhere is probably not right, but I generally want to minimize the number of exception types. Also we would later use flakiness dashboard for the error data mining use case? Yep, I would love to track the kinds of errors through flakiness dashboard; and also there, having more specific names will be super useful!
The CQ bit was checked by perezju@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 1, "attempt_start_ts": 1496416763821310, "parent_rev":
"5409eb23af2846b69c935a28b3270cfd416abe2e", "commit_rev":
"4cafad75c8313d6c14b47544574a20ecc49c253f"}
Message was sent while issue was closed.
Description was changed from ========== [telemetry] Provide some more specific exception classes Provide: - StoryActionError - TracingException and use them where appropriate. BUG=catapult:#3620 ========== to ========== [telemetry] Provide some more specific exception classes Provide: - StoryActionError - TracingException and use them where appropriate. BUG=catapult:#3620 Review-Url: https://codereview.chromium.org/2922593004 Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapu... ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1) as https://chromium.googlesource.com/external/github.com/catapult-project/catapu... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
