[Predator] Add Clusterfuzz stacktrace parser.

[Predator] Add Clusterfuzz stacktrace parser.

BUG=

Review-Url: https://codereview.chromium.org/2593593003
Committed: https://chromium.googlesource.com/infra/infra/+/389001b9706230000fbba929fde751a7bd8a70a6

[Sharu Jiang, 2016-12-21 18:36:05]

Patchset #1 (id:1) has been deleted

[Sharu Jiang, 2016-12-21 18:48:37]

Patchset #1 (id:20001) has been deleted

[Sharu Jiang, 2016-12-21 18:50:10]

Patchset #1 (id:40001) has been deleted

[Sharu Jiang, 2016-12-21 18:51:46]

katesonia@chromium.org changed reviewers:
+ mbarbella@chromium.org, stgao@chromium.org, wrengr@chromium.org

[Sharu Jiang, 2016-12-21 18:51:47]

PTAL.

[Sharu Jiang, 2016-12-21 19:04:30]

Description was changed from

==========
Clusterfuzz parser.

BUG=
==========

to

==========
[Predator] Add Clusterfuzz stacktrace parser.

BUG=
==========

[Sharu Jiang, 2016-12-22 03:25:27]

Patchset #4 (id:120001) has been deleted

[Sharu Jiang, 2016-12-22 03:28:06]

Patchset #4 (id:140001) has been deleted

[Sharu Jiang, 2016-12-22 03:29:12]

Patchset #4 (id:160001) has been deleted

[Sharu Jiang, 2016-12-22 03:57:27]

Patchset #4 (id:180001) has been deleted

[wrengr, 2016-12-22 23:28:36]

lgtm with nits.

https://codereview.chromium.org/2593593003/diff/220001/appengine/findit/crash...
File appengine/findit/crash/callstack_detectors.py (right):

https://codereview.chromium.org/2593593003/diff/220001/appengine/findit/crash...
appengine/findit/crash/callstack_detectors.py:13: """Detector to detect the
start of a CallStack."""
"detector to detect" is awkward. Try something like "Class for detecting the
start of a particular sort of CallStack." (to make clear that the reason for
this class is to abstract over the particular sorts it's detecting)

https://codereview.chromium.org/2593593003/diff/220001/appengine/findit/crash...
appengine/findit/crash/callstack_detectors.py:19: line (str): The line to be
checked whether it is a start of new callstack
I'd drop the "whether..." part.

If retained, then "whether" -> "to see if" and "a start" -> "the start"

https://codereview.chromium.org/2593593003/diff/220001/appengine/findit/crash...
appengine/findit/crash/callstack_detectors.py:21: flags (FlagManager): flag
manager to manage flags needed to determine
"flag manager to manage flags" is awkward. Just say "manager for keeping track
of parsing flags" or similar

https://codereview.chromium.org/2593593003/diff/220001/appengine/findit/crash...
appengine/findit/crash/callstack_detectors.py:26: (is_new_callstack, priority,
format_type, language_type, metadata)
It'd help clarify things to break this out as a namedtuple class.

Moreover, since whenever is_new_callstack is False all the other parts are None,
we can enforce that invariant by returning None whenever it's not a new
callstack. Thus, when it is a new callstack we need only return the
namedtuple('StartOfCallStack', ['priority', 'format_type', 'language_type',
'metadata']) since the is_new_callstack bit is redundant.

Since this change is major/invasive, it should prolly be done in a separate CL;
but do be sure to open a ticket for it.

https://codereview.chromium.org/2593593003/diff/220001/appengine/findit/crash...
appengine/findit/crash/callstack_detectors.py:30: format_type
(CallStackFormatType): Format type of new callstack.
-> "the format of the new callstack (or None if there is no new callstack)"

https://codereview.chromium.org/2593593003/diff/220001/appengine/findit/crash...
appengine/findit/crash/callstack_detectors.py:31: language_type (LanguageType):
Language type of new callstack.
-> "the language of the new callstack (or None if there is no new callstack)"

https://codereview.chromium.org/2593593003/diff/220001/appengine/findit/crash...
appengine/findit/crash/callstack_detectors.py:32: metadata (dict): Dict of
metadata of new callstack,
"metadata of" -> "metadata for the"

https://codereview.chromium.org/2593593003/diff/220001/appengine/findit/crash...
appengine/findit/crash/callstack_detectors.py:112: """Detects the start of a
ubsan callstack."""
"a ubsan" -> "an undefined-behavior sanitizer"

https://codereview.chromium.org/2593593003/diff/220001/appengine/findit/crash...
appengine/findit/crash/callstack_detectors.py:172:
asan_crash_callstack_start_regex1_match.group(1).strip())
I think it'd be clearer to return (True, 0, DEFAULT, CPP, {'pid': ...})
directly, rather than having the metadata variable and mutating it and testing
asan_crash_callstack_start_regex1_match twice

https://codereview.chromium.org/2593593003/diff/220001/appengine/findit/crash...
File appengine/findit/crash/clusterfuzz_parser.py (right):

https://codereview.chromium.org/2593593003/diff/220001/appengine/findit/crash...
appengine/findit/crash/clusterfuzz_parser.py:39: """Gets ``CallStackDetector``
instance to detect start of new callstack."""
-> "Returns a ``CallStackDetector`` for the particular sanitizer and job type."

https://codereview.chromium.org/2593593003/diff/220001/appengine/findit/crash...
appengine/findit/crash/clusterfuzz_parser.py:44: if sanitizer ==
SanitizerType.SYZYASAN:
fwiw, rather than doing a bunch of equality tests one at a time, you could jump
directly to the right thing by using a dict:

    # at the top level, so we only build it once.
    callstack_detector_classes = {
        SanitizerType.SYZYASAN: callstack_detectors.SyzyasanDetector,
        SanitizerType.THREAD_SANITIZER: callstack_detectors.TsanDetector,
        ...
    }

    # inside this method:
    try:
        return callstack_detector_classes[sanitizer]()
    except KeyError:
        return None

The performance difference isn't a problem yet, but if we start adding a whole
bunch of different CallStackDetectors then it'll be a lot faster.

https://codereview.chromium.org/2593593003/diff/220001/appengine/findit/crash...
appengine/findit/crash/clusterfuzz_parser.py:101: # Reset both stacktarce and
callstack flags.
"stacktarce" -> "stacktrace"

https://codereview.chromium.org/2593593003/diff/220001/appengine/findit/crash...
File appengine/findit/crash/test/callstack_detectors_test.py (right):

https://codereview.chromium.org/2593593003/diff/220001/appengine/findit/crash...
appengine/findit/crash/test/callstack_detectors_test.py:16: """Tests
``AndroidJobDetector`` detects android job callstack."""
for all these docstrings "tests ``foo`` detects" -> "tests that ``foo`` detects"

[Sharu Jiang, 2016-12-28 20:16:46]

https://codereview.chromium.org/2593593003/diff/220001/appengine/findit/crash...
File appengine/findit/crash/callstack_detectors.py (right):

https://codereview.chromium.org/2593593003/diff/220001/appengine/findit/crash...
appengine/findit/crash/callstack_detectors.py:13: """Detector to detect the
start of a CallStack."""
On 2016/12/22 23:28:36, wrengr wrote:
> "detector to detect" is awkward. Try something like "Class for detecting the
> start of a particular sort of CallStack." (to make clear that the reason for
> this class is to abstract over the particular sorts it's detecting)

good point. Done.

https://codereview.chromium.org/2593593003/diff/220001/appengine/findit/crash...
appengine/findit/crash/callstack_detectors.py:19: line (str): The line to be
checked whether it is a start of new callstack
On 2016/12/22 23:28:36, wrengr wrote:
> I'd drop the "whether..." part.
> 
> If retained, then "whether" -> "to see if" and "a start" -> "the start"

Done.

https://codereview.chromium.org/2593593003/diff/220001/appengine/findit/crash...
appengine/findit/crash/callstack_detectors.py:21: flags (FlagManager): flag
manager to manage flags needed to determine
On 2016/12/22 23:28:35, wrengr wrote:
> "flag manager to manage flags" is awkward. Just say "manager for keeping track
> of parsing flags" or similar

Done.

https://codereview.chromium.org/2593593003/diff/220001/appengine/findit/crash...
appengine/findit/crash/callstack_detectors.py:26: (is_new_callstack, priority,
format_type, language_type, metadata)
On 2016/12/22 23:28:36, wrengr wrote:
> It'd help clarify things to break this out as a namedtuple class.
> 
> Moreover, since whenever is_new_callstack is False all the other parts are
None,
> we can enforce that invariant by returning None whenever it's not a new
> callstack. Thus, when it is a new callstack we need only return the
> namedtuple('StartOfCallStack', ['priority', 'format_type', 'language_type',
> 'metadata']) since the is_new_callstack bit is redundant.
> 
> Since this change is major/invasive, it should prolly be done in a separate
CL;
> but do be sure to open a ticket for it.

Added a todo.

https://codereview.chromium.org/2593593003/diff/220001/appengine/findit/crash...
appengine/findit/crash/callstack_detectors.py:30: format_type
(CallStackFormatType): Format type of new callstack.
On 2016/12/22 23:28:35, wrengr wrote:
> -> "the format of the new callstack (or None if there is no new callstack)"

Done.

https://codereview.chromium.org/2593593003/diff/220001/appengine/findit/crash...
appengine/findit/crash/callstack_detectors.py:31: language_type (LanguageType):
Language type of new callstack.
On 2016/12/22 23:28:35, wrengr wrote:
> -> "the language of the new callstack (or None if there is no new callstack)"

Done.

https://codereview.chromium.org/2593593003/diff/220001/appengine/findit/crash...
appengine/findit/crash/callstack_detectors.py:32: metadata (dict): Dict of
metadata of new callstack,
On 2016/12/22 23:28:35, wrengr wrote:
> "metadata of" -> "metadata for the"

Done.

https://codereview.chromium.org/2593593003/diff/220001/appengine/findit/crash...
appengine/findit/crash/callstack_detectors.py:112: """Detects the start of a
ubsan callstack."""
On 2016/12/22 23:28:36, wrengr wrote:
> "a ubsan" -> "an undefined-behavior sanitizer"

Done.

https://codereview.chromium.org/2593593003/diff/220001/appengine/findit/crash...
appengine/findit/crash/callstack_detectors.py:172:
asan_crash_callstack_start_regex1_match.group(1).strip())
On 2016/12/22 23:28:36, wrengr wrote:
> I think it'd be clearer to return (True, 0, DEFAULT, CPP, {'pid': ...})
> directly, rather than having the metadata variable and mutating it and testing
> asan_crash_callstack_start_regex1_match twice

Done.

https://codereview.chromium.org/2593593003/diff/220001/appengine/findit/crash...
File appengine/findit/crash/clusterfuzz_parser.py (right):

https://codereview.chromium.org/2593593003/diff/220001/appengine/findit/crash...
appengine/findit/crash/clusterfuzz_parser.py:39: """Gets ``CallStackDetector``
instance to detect start of new callstack."""
On 2016/12/22 23:28:36, wrengr wrote:
> -> "Returns a ``CallStackDetector`` for the particular sanitizer and job
type."

Done.

https://codereview.chromium.org/2593593003/diff/220001/appengine/findit/crash...
appengine/findit/crash/clusterfuzz_parser.py:44: if sanitizer ==
SanitizerType.SYZYASAN:
On 2016/12/22 23:28:36, wrengr wrote:
> fwiw, rather than doing a bunch of equality tests one at a time, you could
jump
> directly to the right thing by using a dict:
> 
>     # at the top level, so we only build it once.
>     callstack_detector_classes = {
>         SanitizerType.SYZYASAN: callstack_detectors.SyzyasanDetector,
>         SanitizerType.THREAD_SANITIZER: callstack_detectors.TsanDetector,
>         ...
>     }
> 
>     # inside this method:
>     try:
>         return callstack_detector_classes[sanitizer]()
>     except KeyError:
>         return None
> 
> The performance difference isn't a problem yet, but if we start adding a whole
> bunch of different CallStackDetectors then it'll be a lot faster.

Done.

https://codereview.chromium.org/2593593003/diff/220001/appengine/findit/crash...
appengine/findit/crash/clusterfuzz_parser.py:101: # Reset both stacktarce and
callstack flags.
On 2016/12/22 23:28:36, wrengr wrote:
> "stacktarce" -> "stacktrace"

Done.

https://codereview.chromium.org/2593593003/diff/220001/appengine/findit/crash...
File appengine/findit/crash/test/callstack_detectors_test.py (right):

https://codereview.chromium.org/2593593003/diff/220001/appengine/findit/crash...
appengine/findit/crash/test/callstack_detectors_test.py:16: """Tests
``AndroidJobDetector`` detects android job callstack."""
On 2016/12/22 23:28:36, wrengr wrote:
> for all these docstrings "tests ``foo`` detects" -> "tests that ``foo``
detects"

Done.

[wrengr, 2016-12-28 23:47:52]

lgtm

https://codereview.chromium.org/2593593003/diff/240001/appengine/findit/crash...
File appengine/findit/crash/clusterfuzz_parser.py (right):

https://codereview.chromium.org/2593593003/diff/240001/appengine/findit/crash...
appengine/findit/crash/clusterfuzz_parser.py:77: """ Updates metadata with
callstack flags. Returns updated stack buffer."""
extraneous space at start of docstring

[Sharu Jiang, 2016-12-29 18:29:22]

https://codereview.chromium.org/2593593003/diff/240001/appengine/findit/crash...
File appengine/findit/crash/clusterfuzz_parser.py (right):

https://codereview.chromium.org/2593593003/diff/240001/appengine/findit/crash...
appengine/findit/crash/clusterfuzz_parser.py:77: """ Updates metadata with
callstack flags. Returns updated stack buffer."""
On 2016/12/28 23:47:52, wrengr wrote:
> extraneous space at start of docstring

Done.

[Sharu Jiang, 2016-12-29 18:29:25]

The CQ bit was checked by katesonia@chromium.org

[Sharu Jiang, 2016-12-29 18:29:25]

The patchset sent to the CQ was uploaded after l-g-t-m from wrengr@chromium.org
Link to the patchset: https://codereview.chromium.org/2593593003/#ps260001
(title: "Rebase and fix nits.")

[commit-bot: I haz the power, 2016-12-29 18:29:34]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-29 18:29:35]

This CL has an open dependency (Issue 2593093003 Patch 60001). Please resolve
the dependency and try again.
If you are sure that there is no real dependency, please use one of the options
listed in https://goo.gl/9Es4OR to land the CL.

[Sharu Jiang, 2016-12-29 19:01:21]

Patchset #7 (id:260001) has been deleted

[Sharu Jiang, 2016-12-29 19:01:26]

The CQ bit was checked by katesonia@chromium.org

[Sharu Jiang, 2016-12-29 19:01:26]

The patchset sent to the CQ was uploaded after l-g-t-m from wrengr@chromium.org
Link to the patchset: https://codereview.chromium.org/2593593003/#ps280001
(title: "Rebase and fix nit.")

[commit-bot: I haz the power, 2016-12-29 19:01:31]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-29 19:11:07]

CQ is committing da patch.

Bot data: {"patchset_id": 280001, "attempt_start_ts": 1483038086552380,
"parent_rev": "cb4c9bdf1f73ecca55f275d5c8190c53cb13258f", "commit_rev":
"389001b9706230000fbba929fde751a7bd8a70a6"}

[commit-bot: I haz the power, 2016-12-29 19:11:10]

Description was changed from

==========
[Predator] Add Clusterfuzz stacktrace parser.

BUG=
==========

to

==========
[Predator] Add Clusterfuzz stacktrace parser.

BUG=

Review-Url: https://codereview.chromium.org/2593593003
Committed:
https://chromium.googlesource.com/infra/infra/+/389001b9706230000fbba929fde75...
==========

[commit-bot: I haz the power, 2016-12-29 19:11:11]

Committed patchset #7 (id:280001) as
https://chromium.googlesource.com/infra/infra/+/389001b9706230000fbba929fde75...