[Android] Add ability to generate test trace json for perf tests runs.

[Android] Add ability to generate test trace json for perf tests runs.

This adds the ability to set --trace-output <dir> and generate a json file that
can be opened in the chrome trace viewer that will display timelines of when
each test is run on which testing shard.

BUG=667470
Committed: https://crrev.com/293fc82cb952c4c0d0ef816b8b86b7e1a63ada9a
Cr-Commit-Position: refs/heads/master@{#439125}

[rnephew (Reviews Here), 2016-12-15 19:24:57]

rnephew@chromium.org changed reviewers:
+ jbudorick@chromium.org, mikecase@chromium.org

[rnephew (Reviews Here), 2016-12-15 19:24:58]

A sample json file this generates looks like this:
[{"category": "process_argv", "name": "process_argv", "args": {"argv":
["./test_runner.py", "perf", "--steps", "test_run.json", "-v", "--dry-run"]},
"pid": 28335, "ts": 267962068732.43402, "tid": 140153717864256, "ph": "M"}
,
{"category": "python", "name": "test 3", "args": {}, "pid": 28335, "ts":
267962070355.43298, "tid": 140153638999808, "ph": "B"},
{"category": "python", "name": "test 3", "args": {}, "pid": 28335, "ts":
267963085750.11902, "tid": 140153638999808, "ph": "E"},
{"category": "python", "name": "test 1", "args": {}, "pid": 28335, "ts":
267962356825.416, "tid": 140153622214400, "ph": "B"},
{"category": "python", "name": "test 1", "args": {}, "pid": 28335, "ts":
267963374081.983, "tid": 140153622214400, "ph": "E"},
{"category": "python", "name": "test 2", "args": {}, "pid": 28335, "ts":
267964392866.263, "tid": 140153622214400, "ph": "B"},
{"category": "python", "name": "test 2", "args": {}, "pid": 28335, "ts":
267965409468.98703, "tid": 140153622214400, "ph": "E"}

Save that to a file and open in about:tracing to see what this will look like.

https://codereview.chromium.org/2583613002/diff/1/build/android/pylib/local/d...
File build/android/pylib/local/device/local_device_perf_test_run.py (right):

https://codereview.chromium.org/2583613002/diff/1/build/android/pylib/local/d...
build/android/pylib/local/device/local_device_perf_test_run.py:32: from
py_trace_event import trace_event
I keep getting:
** Presubmit Warnings **
Pylint (128 files using ['--disable=cyclic-import'] on 12 cores) (4.40s) failed
************* Module pylib.local.device.local_device_perf_test_run
F: 32, 0: Unable to import 'py_trace_event' (import-error)

Even though it works locally. Do I need to add it to another __init__ file
somewhere so that pylint is appeased?

[rnephew (Reviews Here), 2016-12-15 19:26:05]

On 2016/12/15 19:24:58, rnephew (Reviews Here) wrote:
> A sample json file this generates looks like this:
> [{"category": "process_argv", "name": "process_argv", "args": {"argv":
> ["./test_runner.py", "perf", "--steps", "test_run.json", "-v", "--dry-run"]},
> "pid": 28335, "ts": 267962068732.43402, "tid": 140153717864256, "ph": "M"}
> ,
> {"category": "python", "name": "test 3", "args": {}, "pid": 28335, "ts":
> 267962070355.43298, "tid": 140153638999808, "ph": "B"},
> {"category": "python", "name": "test 3", "args": {}, "pid": 28335, "ts":
> 267963085750.11902, "tid": 140153638999808, "ph": "E"},
> {"category": "python", "name": "test 1", "args": {}, "pid": 28335, "ts":
> 267962356825.416, "tid": 140153622214400, "ph": "B"},
> {"category": "python", "name": "test 1", "args": {}, "pid": 28335, "ts":
> 267963374081.983, "tid": 140153622214400, "ph": "E"},
> {"category": "python", "name": "test 2", "args": {}, "pid": 28335, "ts":
> 267964392866.263, "tid": 140153622214400, "ph": "B"},
> {"category": "python", "name": "test 2", "args": {}, "pid": 28335, "ts":
> 267965409468.98703, "tid": 140153622214400, "ph": "E"}
> 
> Save that to a file and open in about:tracing to see what this will look like.
> 
>
https://codereview.chromium.org/2583613002/diff/1/build/android/pylib/local/d...
> File build/android/pylib/local/device/local_device_perf_test_run.py (right):
> 
>
https://codereview.chromium.org/2583613002/diff/1/build/android/pylib/local/d...
> build/android/pylib/local/device/local_device_perf_test_run.py:32: from
> py_trace_event import trace_event
> I keep getting:
> ** Presubmit Warnings **
> Pylint (128 files using ['--disable=cyclic-import'] on 12 cores) (4.40s)
failed
> ************* Module pylib.local.device.local_device_perf_test_run
> F: 32, 0: Unable to import 'py_trace_event' (import-error)
> 
> Even though it works locally. Do I need to add it to another __init__ file
> somewhere so that pylint is appeased?

P.S. Ignore the missing ] at the end of the file. Chrome trace viewer
automatically adds one at the end. This has to do with multithreaded trace
generation and not knowing which trace is the last to close.

[mikecase (-- gone --), 2016-12-15 19:46:48]

https://codereview.chromium.org/2583613002/diff/1/build/android/pylib/local/d...
File build/android/pylib/local/device/local_device_perf_test_run.py (right):

https://codereview.chromium.org/2583613002/diff/1/build/android/pylib/local/d...
build/android/pylib/local/device/local_device_perf_test_run.py:32: from
py_trace_event import trace_event
On 2016/12/15 at 19:24:58, rnephew (Reviews Here) wrote:
> I keep getting:
> ** Presubmit Warnings **
> Pylint (128 files using ['--disable=cyclic-import'] on 12 cores) (4.40s)
failed
> ************* Module pylib.local.device.local_device_perf_test_run
> F: 32, 0: Unable to import 'py_trace_event' (import-error)
> 
> Even though it works locally. Do I need to add it to another __init__ file
somewhere so that pylint is appeased?

Here I think.

https://cs.chromium.org/chromium/src/build/android/PRESUBMIT.py?q=PRESUBMIT+f...

[rnephew (Reviews Here), 2016-12-15 19:55:13]

https://codereview.chromium.org/2583613002/diff/1/build/android/pylib/local/d...
File build/android/pylib/local/device/local_device_perf_test_run.py (right):

https://codereview.chromium.org/2583613002/diff/1/build/android/pylib/local/d...
build/android/pylib/local/device/local_device_perf_test_run.py:32: from
py_trace_event import trace_event
On 2016/12/15 19:46:47, mikecase wrote:
> On 2016/12/15 at 19:24:58, rnephew (Reviews Here) wrote:
> > I keep getting:
> > ** Presubmit Warnings **
> > Pylint (128 files using ['--disable=cyclic-import'] on 12 cores) (4.40s)
> failed
> > ************* Module pylib.local.device.local_device_perf_test_run
> > F: 32, 0: Unable to import 'py_trace_event' (import-error)
> > 
> > Even though it works locally. Do I need to add it to another __init__ file
> somewhere so that pylint is appeased?
> 
> Here I think.
> 
>
https://cs.chromium.org/chromium/src/build/android/PRESUBMIT.py?q=PRESUBMIT+f...

Thanks.

[mikecase (-- gone --), 2016-12-15 19:56:02]

Looks good. Just have a couple comments.

https://codereview.chromium.org/2583613002/diff/1/build/android/pylib/local/d...
File build/android/pylib/local/device/local_device_perf_test_run.py (right):

https://codereview.chromium.org/2583613002/diff/1/build/android/pylib/local/d...
build/android/pylib/local/device/local_device_perf_test_run.py:421: assert
trace_event.trace_is_enabled(), 'Tracing didn\'t enable properly.'
nit: Maybe just didn't -> "did not enable" or just "not enabled" to remove need
to escape single-quote.


Also nit:

Are the...

assert trace_event.trace_is_enabled(), 'Tracing didn\'t enable properly.'

and final...

assert not trace_event.trace_is_enabled(), 'Tracing not disabled.'

really neccesarry. I get the other two, something else could be enabling tracing
which could screw us up. And somethinhg could disable tracing half-way into run.
Other two asserts dont seem neccessary.

[rnephew (Reviews Here), 2016-12-15 20:02:10]

https://codereview.chromium.org/2583613002/diff/1/build/android/pylib/local/d...
File build/android/pylib/local/device/local_device_perf_test_run.py (right):

https://codereview.chromium.org/2583613002/diff/1/build/android/pylib/local/d...
build/android/pylib/local/device/local_device_perf_test_run.py:421: assert
trace_event.trace_is_enabled(), 'Tracing didn\'t enable properly.'
On 2016/12/15 19:56:02, mikecase wrote:
> nit: Maybe just didn't -> "did not enable" or just "not enabled" to remove
need
> to escape single-quote.
> 
> 
> Also nit:
> 
> Are the...
> 
> assert trace_event.trace_is_enabled(), 'Tracing didn\'t enable properly.'
> 
> and final...
> 
> assert not trace_event.trace_is_enabled(), 'Tracing not disabled.'
> 
> really neccesarry. I get the other two, something else could be enabling
tracing
> which could screw us up. And somethinhg could disable tracing half-way into
run.
> Other two asserts dont seem neccessary.

Thats how I do it in telemetry when using trace_events. I dont recall why
exactly, but its probably safe to get rid of them.

[jbudorick, 2016-12-15 20:12:23]

https://codereview.chromium.org/2583613002/diff/20001/build/android/pylib/loc...
File build/android/pylib/local/device/local_device_perf_test_run.py (right):

https://codereview.chromium.org/2583613002/diff/20001/build/android/pylib/loc...
build/android/pylib/local/device/local_device_perf_test_run.py:101:
trace_event.trace_begin(test)
Isn't there a context manager version of this? Can we use that? As-is, we never
hit the trace end if the GetCmdStatusAndOutputWithTimeout call throws an
exception.

https://codereview.chromium.org/2583613002/diff/20001/build/android/pylib/loc...
build/android/pylib/local/device/local_device_perf_test_run.py:421: assert
trace_event.trace_is_enabled(), 'Tracing didn\'t enable properly.'
Double quotes around strings that contain single quotes.

[rnephew (Reviews Here), 2016-12-15 20:16:43]

https://codereview.chromium.org/2583613002/diff/20001/build/android/pylib/loc...
File build/android/pylib/local/device/local_device_perf_test_run.py (right):

https://codereview.chromium.org/2583613002/diff/20001/build/android/pylib/loc...
build/android/pylib/local/device/local_device_perf_test_run.py:101:
trace_event.trace_begin(test)
On 2016/12/15 20:12:22, jbudorick wrote:
> Isn't there a context manager version of this? Can we use that? As-is, we
never
> hit the trace end if the GetCmdStatusAndOutputWithTimeout call throws an
> exception.

I originally was using trace_event.trace(name) but moved to this instead.
Because of the option of not having trace_output set, I thought it was easier to
do it this way. I can switch it back to a context manager with a little extra
work, or I could do a try: finally: block.

https://codereview.chromium.org/2583613002/diff/20001/build/android/pylib/loc...
build/android/pylib/local/device/local_device_perf_test_run.py:421: assert
trace_event.trace_is_enabled(), 'Tracing didn\'t enable properly.'
On 2016/12/15 20:12:22, jbudorick wrote:
> Double quotes around strings that contain single quotes.

Acknowledged. This message is gone now.

[mikecase (-- gone --), 2016-12-15 20:19:00]

lgtm

https://codereview.chromium.org/2583613002/diff/40001/build/android/PRESUBMIT.py
File build/android/PRESUBMIT.py (right):

https://codereview.chromium.org/2583613002/diff/40001/build/android/PRESUBMIT...
build/android/PRESUBMIT.py:35: J('..', '..', 'third_party', 'catapult',
'common', 'py_trace_event')
nit: alphabetize.

[jbudorick, 2016-12-15 20:22:33]

https://codereview.chromium.org/2583613002/diff/40001/build/android/pylib/loc...
File build/android/pylib/local/device/local_device_perf_test_run.py (right):

https://codereview.chromium.org/2583613002/diff/40001/build/android/pylib/loc...
build/android/pylib/local/device/local_device_perf_test_run.py:100: if
self._test_instance.trace_output:
My vote would be for a context manager.

In related news, I need to get around to writing a conditional context manager
or something like that in catapult/common/py_utils.

[jbudorick, 2016-12-15 20:23:00]

https://codereview.chromium.org/2583613002/diff/40001/build/android/pylib/loc...
File build/android/pylib/local/device/local_device_perf_test_run.py (right):

https://codereview.chromium.org/2583613002/diff/40001/build/android/pylib/loc...
build/android/pylib/local/device/local_device_perf_test_run.py:100: if
self._test_instance.trace_output:
On 2016/12/15 20:22:33, jbudorick wrote:
> My vote would be for a context manager.
> 
> In related news, I need to get around to writing a conditional context manager
> or something like that in catapult/common/py_utils.

that and tempdir as a context manager.

[mikecase (-- gone --), 2016-12-15 20:25:59]

On 2016/12/15 at 20:22:33, jbudorick wrote:
>
https://codereview.chromium.org/2583613002/diff/40001/build/android/pylib/loc...
> File build/android/pylib/local/device/local_device_perf_test_run.py (right):
> 
>
https://codereview.chromium.org/2583613002/diff/40001/build/android/pylib/loc...
> build/android/pylib/local/device/local_device_perf_test_run.py:100: if
self._test_instance.trace_output:
> My vote would be for a context manager.
> 
> In related news, I need to get around to writing a conditional context manager
or something like that in catapult/common/py_utils.

+1

[rnephew (Reviews Here), 2016-12-15 21:24:05]

https://codereview.chromium.org/2583613002/diff/40001/build/android/PRESUBMIT.py
File build/android/PRESUBMIT.py (right):

https://codereview.chromium.org/2583613002/diff/40001/build/android/PRESUBMIT...
build/android/PRESUBMIT.py:35: J('..', '..', 'third_party', 'catapult',
'common', 'py_trace_event')
On 2016/12/15 20:19:00, mikecase wrote:
> nit: alphabetize.

Done.

https://codereview.chromium.org/2583613002/diff/40001/build/android/pylib/loc...
File build/android/pylib/local/device/local_device_perf_test_run.py (right):

https://codereview.chromium.org/2583613002/diff/40001/build/android/pylib/loc...
build/android/pylib/local/device/local_device_perf_test_run.py:100: if
self._test_instance.trace_output:
On 2016/12/15 20:22:59, jbudorick wrote:
> On 2016/12/15 20:22:33, jbudorick wrote:
> > My vote would be for a context manager.
> > 
> > In related news, I need to get around to writing a conditional context
manager
> > or something like that in catapult/common/py_utils.
> 
> that and tempdir as a context manager.

Moved to context manager.

[mikecase (-- gone --), 2016-12-15 21:44:52]

cool. still lgtm

[rnephew (Reviews Here), 2016-12-15 22:01:08]

The CQ bit was checked by rnephew@chromium.org

[commit-bot: I haz the power, 2016-12-15 22:02:32]

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-16 00:21:04]

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

[commit-bot: I haz the power, 2016-12-16 00:21:05]

Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[rnephew (Reviews Here), 2016-12-16 15:53:37]

The CQ bit was checked by rnephew@chromium.org

[commit-bot: I haz the power, 2016-12-16 15:53:52]

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-16 16:46:11]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1481903617889020,
"parent_rev": "e0015dab0d81210b6045a01c3e9b827e6ae1502f", "commit_rev":
"1ad376b841b45d3b52c10b84edf562580323ea8d"}

[commit-bot: I haz the power, 2016-12-16 16:46:37]

Description was changed from

==========
[Android] Add ability to generate test trace json for perf tests runs.

This adds the ability to set --trace-output <dir> and generate a json file that
can be opened in the chrome trace viewer that will display timelines of when
each test is run on which testing shard.

BUG=667470
==========

to

==========
[Android] Add ability to generate test trace json for perf tests runs.

This adds the ability to set --trace-output <dir> and generate a json file that
can be opened in the chrome trace viewer that will display timelines of when
each test is run on which testing shard.

BUG=667470

Review-Url: https://codereview.chromium.org/2583613002
==========

[commit-bot: I haz the power, 2016-12-16 16:46:38]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-12-16 16:49:55]

Description was changed from

==========
[Android] Add ability to generate test trace json for perf tests runs.

This adds the ability to set --trace-output <dir> and generate a json file that
can be opened in the chrome trace viewer that will display timelines of when
each test is run on which testing shard.

BUG=667470

Review-Url: https://codereview.chromium.org/2583613002
==========

to

==========
[Android] Add ability to generate test trace json for perf tests runs.

This adds the ability to set --trace-output <dir> and generate a json file that
can be opened in the chrome trace viewer that will display timelines of when
each test is run on which testing shard.

BUG=667470

Committed: https://crrev.com/293fc82cb952c4c0d0ef816b8b86b7e1a63ada9a
Cr-Commit-Position: refs/heads/master@{#439125}
==========

[commit-bot: I haz the power, 2016-12-16 16:49:56]

Patchset 4 (id:??) landed as
https://crrev.com/293fc82cb952c4c0d0ef816b8b86b7e1a63ada9a
Cr-Commit-Position: refs/heads/master@{#439125}