WIP: Add support for additional flag-specified tracing categories.

Add support for additional flag-specified tracing categories.

This CL adds two flags:
 * --extra-chrome-categories specifies additional Chrome tracing
   categories to include when running the benchmark.
 * --extra-atrace-categories specifies additional atrace categories.

This CL also renames CreateTimelineBasedMeasurementOptions to
CreateBaseTimelineBasedMeasurementOptions.
CreateBaseTimelineBasedMeasurementOptions will be deleted after
migrating clients.

BUG=catapult:#3450

Review-Url: https://codereview.chromium.org/2965383002
Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapult/+/d19b49c7f57f09361a34d8717869833050ce113b

[sullivan, 2017-07-07 14:54:39]

Description was changed from

==========
Add support for additional flag-specified tracing categories.

This CL adds two flags:
 * --extra-chrome-categories specifies additional Chrome tracing
   categories to include when running the benchmark.
 * --extra-atrace-categories specifies additional atrace categories.

This CL also renames CreateTimelineBasedMeasurementOptions to
CreateBaseTimelineBasedMeasurementOptions.
CreateBaseTimelineBasedMeasurementOptions will be deleted after
migrating clients.

BUG=catapult:#3450
==========

to

==========
Add support for additional flag-specified tracing categories.

This CL adds two flags:
 * --extra-chrome-categories specifies additional Chrome tracing
   categories to include when running the benchmark.
 * --extra-atrace-categories specifies additional atrace categories.

This CL also renames CreateTimelineBasedMeasurementOptions to
CreateBaseTimelineBasedMeasurementOptions.
CreateBaseTimelineBasedMeasurementOptions will be deleted after
migrating clients.

BUG=catapult:#3450
==========

[sullivan, 2017-07-07 14:58:46]

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

[sullivan, 2017-07-07 14:58:47]

This is a WIP. I added a bunch of CL comments on things I'd like feedback on.

If this is headed in the right direction, I'd also like opinions on whether to
split this CL--I can imagine adding in the options as a separate CL on top of
renaming CreateBaseTimelineBasedMeasurementOptions, but I think the first CL
would implement the beginnings of _GetTimelineBasedMeasurementOptions so I am
not sure if it's worth it.

Unit tests I plan to add if this is the right direction:
* Temporary test to ensure that assertion about pagetest vs timeline options is
working correctly
* Temporary test to ensure we always pick the correct
CreateTimelineBasedMeasurementOptions or
CreateBaseTimelineBasedMeasurementOptions
* Permanent test to ensure setting chrome trace options works as expected (unit
test of _GetTimelineBasedMeasurementOptions)
* Permanent test to ensuer setting atrace options works as expected (unit test
of _GetTimelineBasedMeasurementOptions).

https://codereview.chromium.org/2965383002/diff/1/telemetry/telemetry/benchma...
File telemetry/telemetry/benchmark.py (right):

https://codereview.chromium.org/2965383002/diff/1/telemetry/telemetry/benchma...
telemetry/telemetry/benchmark.py:275: 'This benchmark does not support Chrome
tracing.')
Copied this from the bug comment. Big question I have here is why not just
enable Chrome tracing if this is specified?

https://codereview.chromium.org/2965383002/diff/1/telemetry/telemetry/benchma...
telemetry/telemetry/benchmark.py:276:
tbm_options.chrome_trace_config.category_filter.AddFilterString(
Also copied this from the bug comment. Any reason to make this a filter string
instead of having separate included categories, disabled_by_default_categories,
and possibly excluded_categories? Is there documentation on filter strings
somewhere? I don't see it here:
https://github.com/catapult-project/catapult/blob/master/telemetry/telemetry/...

https://codereview.chromium.org/2965383002/diff/1/telemetry/telemetry/benchma...
telemetry/telemetry/benchmark.py:280: 'This benchmark does not support atrace.')
Same question about enabling atrace.

https://codereview.chromium.org/2965383002/diff/1/telemetry/telemetry/interna...
File telemetry/telemetry/internal/browser/browser_options.py (right):

https://codereview.chromium.org/2965383002/diff/1/telemetry/telemetry/interna...
telemetry/telemetry/internal/browser/browser_options.py:119: help='Filter string
to enable additional chrome tracing categories.')
Again, documentation on filter strings would be awesome to add here.

https://codereview.chromium.org/2965383002/diff/1/telemetry/telemetry/interna...
telemetry/telemetry/internal/browser/browser_options.py:121:
'--extra-atrace-categories', dest='extra_atrace_categories',
Any ideas where to find documentation on available atrace categories?

[charliea (OOO until 10-5), 2017-07-07 20:06:58]

charliea@chromium.org changed reviewers:
+ charliea@chromium.org

[charliea (OOO until 10-5), 2017-07-07 20:07:00]

https://codereview.chromium.org/2965383002/diff/1/telemetry/examples/benchmar...
File telemetry/examples/benchmarks/tbm_benchmark.py (right):

https://codereview.chromium.org/2965383002/diff/1/telemetry/examples/benchmar...
telemetry/examples/benchmarks/tbm_benchmark.py:14: def
CreateBaseTimelineBasedMeasurementOptions(self):
Maybe it's just me, but it feels like this name is getting long and hard to
understand for how easy a concept it is. I especially don't like the fact that
we have both "Base" and "Based" in the name and that they carry two different
meanings in it.

What about something like "CreateBaseTBMOptions"? Is this too acronym-y?

nednguyen@google.com, WDYT?

https://codereview.chromium.org/2965383002/diff/1/telemetry/telemetry/benchma...
File telemetry/telemetry/benchmark.py (right):

https://codereview.chromium.org/2965383002/diff/1/telemetry/telemetry/benchma...
telemetry/telemetry/benchmark.py:239: def
CreateBaseTimelineBasedMeasurementOptions(self):
I think this could use clarification on what "base
TimelineBasedMeasurementOptions" are. I can imagine stumbling into this code and
wondering that. Something like "Return the base TimelineBasedMeasurementOptions
for this benchmark. These base options are specified by the benchmarks, but can
be added to when rerunning the benchmark with additional tracing options, for
example."

https://codereview.chromium.org/2965383002/diff/1/telemetry/telemetry/benchma...
telemetry/telemetry/benchmark.py:275: 'This benchmark does not support Chrome
tracing.')
On 2017/07/07 14:58:47, sullivan wrote:
> Copied this from the bug comment. Big question I have here is why not just
> enable Chrome tracing if this is specified?

One thing that jumps out to me is that it's not clear what it means if Chrome's
not started in the benchmark and this flag is specified. In cases like that
(such as power.idle_platform), I'm not sure what would/should happen. Agree
though that I can definitely see cases where this would be frustrating to not be
able to do.

https://codereview.chromium.org/2965383002/diff/1/telemetry/telemetry/benchma...
telemetry/telemetry/benchmark.py:276:
tbm_options.chrome_trace_config.category_filter.AddFilterString(
On 2017/07/07 14:58:47, sullivan wrote:
> Also copied this from the bug comment. Any reason to make this a filter string
> instead of having separate included categories,
disabled_by_default_categories,
> and possibly excluded_categories? Is there documentation on filter strings
> somewhere? I don't see it here:
>
https://github.com/catapult-project/catapult/blob/master/telemetry/telemetry/...

My understanding was that the filter string was the common way of serializing
which filters to use and the logic was shared between the record dialog,
telemetry, and chrome:tracing. They're also devilishly complex for how simple of
an idea they are. I'm not quite sure what you mean by "having separate included
categories..."

https://codereview.chromium.org/2965383002/diff/1/telemetry/telemetry/interna...
File telemetry/telemetry/internal/browser/browser_options.py (right):

https://codereview.chromium.org/2965383002/diff/1/telemetry/telemetry/interna...
telemetry/telemetry/internal/browser/browser_options.py:119: help='Filter string
to enable additional chrome tracing categories.')
On 2017/07/07 14:58:47, sullivan wrote:
> Again, documentation on filter strings would be awesome to add here.

"Filter strings are 1) a way to specify which tracing categories to enable and
disable in Chrome and 2) your worst nightmare."

... maybe not what we're going for, though.

https://codereview.chromium.org/2965383002/diff/1/telemetry/telemetry/interna...
telemetry/telemetry/internal/browser/browser_options.py:121:
'--extra-atrace-categories', dest='extra_atrace_categories',
On 2017/07/07 14:58:47, sullivan wrote:
> Any ideas where to find documentation on available atrace categories?

Apparently you can get the documentation from atrace --list_categories

https://developer.android.com/reference/android/support/test/rule/logging/Atr...

[nednguyen, 2017-07-07 20:07:51]

https://codereview.chromium.org/2965383002/diff/1/telemetry/telemetry/benchma...
File telemetry/telemetry/benchmark.py (right):

https://codereview.chromium.org/2965383002/diff/1/telemetry/telemetry/benchma...
telemetry/telemetry/benchmark.py:275: 'This benchmark does not support Chrome
tracing.')
On 2017/07/07 14:58:47, sullivan wrote:
> Copied this from the bug comment. Big question I have here is why not just
> enable Chrome tracing if this is specified?

I think most benchmark that support Chrome tracing would have this enabled
already. So if user call this on a benchmark, it's usually a mistake.

I am fine either way. Though I don't remember what happen if you invoke Chrome
tracing on a benchmark that doesn't support Chrome tracing (like Randy's
idle_platform benchmark).

https://codereview.chromium.org/2965383002/diff/1/telemetry/telemetry/benchma...
telemetry/telemetry/benchmark.py:276:
tbm_options.chrome_trace_config.category_filter.AddFilterString(
On 2017/07/07 14:58:47, sullivan wrote:
> Also copied this from the bug comment. Any reason to make this a filter string
> instead of having separate included categories,
disabled_by_default_categories,
> and possibly excluded_categories?
 
To avoid explosion number of flags. I think the perf dashboard UI can have
include categories, disabled_by_default_categories, ... , fields separately,
then merge them together as a single string before sending to Telemetry

> Is there documentation on filter strings
> somewhere? I don't see it here:
>
https://github.com/catapult-project/catapult/blob/master/telemetry/telemetry/...

Lemme look around...

https://codereview.chromium.org/2965383002/diff/1/telemetry/telemetry/benchma...
telemetry/telemetry/benchmark.py:280: 'This benchmark does not support atrace.')
On 2017/07/07 14:58:47, sullivan wrote:
> Same question about enabling atrace.

Same as above

[nednguyen, 2017-07-07 20:14:09]

On 2017/07/07 20:07:51, nednguyen wrote:
>
https://codereview.chromium.org/2965383002/diff/1/telemetry/telemetry/benchma...
> File telemetry/telemetry/benchmark.py (right):
> 
>
https://codereview.chromium.org/2965383002/diff/1/telemetry/telemetry/benchma...
> telemetry/telemetry/benchmark.py:275: 'This benchmark does not support Chrome
> tracing.')
> On 2017/07/07 14:58:47, sullivan wrote:
> > Copied this from the bug comment. Big question I have here is why not just
> > enable Chrome tracing if this is specified?
> 
> I think most benchmark that support Chrome tracing would have this enabled
> already. So if user call this on a benchmark, it's usually a mistake.
> 
> I am fine either way. Though I don't remember what happen if you invoke Chrome
> tracing on a benchmark that doesn't support Chrome tracing (like Randy's
> idle_platform benchmark).
> 
>
https://codereview.chromium.org/2965383002/diff/1/telemetry/telemetry/benchma...
> telemetry/telemetry/benchmark.py:276:
> tbm_options.chrome_trace_config.category_filter.AddFilterString(
> On 2017/07/07 14:58:47, sullivan wrote:
> > Also copied this from the bug comment. Any reason to make this a filter
string
> > instead of having separate included categories,
> disabled_by_default_categories,
> > and possibly excluded_categories?
>  
> To avoid explosion number of flags. I think the perf dashboard UI can have
> include categories, disabled_by_default_categories, ... , fields separately,
> then merge them together as a single string before sending to Telemetry
> 
> > Is there documentation on filter strings
> > somewhere? I don't see it here:
> >
>
https://github.com/catapult-project/catapult/blob/master/telemetry/telemetry/...
> 
> Lemme look around...
> 
>
https://codereview.chromium.org/2965383002/diff/1/telemetry/telemetry/benchma...
> telemetry/telemetry/benchmark.py:280: 'This benchmark does not support
atrace.')
> On 2017/07/07 14:58:47, sullivan wrote:
> > Same question about enabling atrace.
> 
> Same as above

Found the documentation of filter string in
https://cs.chromium.org/chromium/src/base/trace_event/trace_config.h?rcl=c8db...

[sullivan, 2017-07-13 14:47:27]

Added tests and documentation. This is ready for full review. PTAL.

Note: the more I look at this, the more I dislike the repeating of 'Base' in
CreateBaseTimelineBasedMeasurementOptions. Does anyone like 'Required' or
'Default'?

[sullivan, 2017-07-13 15:24:48]

The CQ bit was checked by sullivan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-07-13 15:24:58]

Dry run: 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, 2017-07-13 15:52:10]

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

[commit-bot: I haz the power, 2017-07-13 15:52:11]

Dry run: This issue passed the CQ dry run.

[nednguyen, 2017-07-13 18:12:41]

lgtm with some comments

https://codereview.chromium.org/2965383002/diff/40001/telemetry/telemetry/ben...
File telemetry/telemetry/benchmark.py (right):

https://codereview.chromium.org/2965383002/diff/40001/telemetry/telemetry/ben...
telemetry/telemetry/benchmark.py:288: categories =
tbm_options.config.atrace_config.categories
If the option is restricted to string, we can be sure to always use categores =
categories.split(',')

Otherwise I think the way you specify the list through command line is:

--flag-foo=item1 --flag-foo=item2...

Which can be ugly in this case if people do:
--flag-foo="item1,item3" --flag-foo=item2...

https://codereview.chromium.org/2965383002/diff/40001/telemetry/telemetry/ben...
File telemetry/telemetry/benchmark_unittest.py (right):

https://codereview.chromium.org/2965383002/diff/40001/telemetry/telemetry/ben...
telemetry/telemetry/benchmark_unittest.py:169: def
testGetTBMOptionsPrefersLegacyName(self):
We should not allow people be able to specify both

https://codereview.chromium.org/2965383002/diff/40001/telemetry/telemetry/int...
File telemetry/telemetry/internal/browser/browser_options.py (right):

https://codereview.chromium.org/2965383002/diff/40001/telemetry/telemetry/int...
telemetry/telemetry/internal/browser/browser_options.py:129:
'c8db6c6371ca047c24d41f3972d5819bc83d83ae&l=125')
nits: we can add type="string" here

https://codereview.chromium.org/2965383002/diff/40001/telemetry/telemetry/int...
telemetry/telemetry/internal/browser/browser_options.py:130: group.add_option(
I think we should restrict the type of this to string only

[sullivan, 2017-07-13 18:33:15]

https://codereview.chromium.org/2965383002/diff/40001/telemetry/telemetry/ben...
File telemetry/telemetry/benchmark.py (right):

https://codereview.chromium.org/2965383002/diff/40001/telemetry/telemetry/ben...
telemetry/telemetry/benchmark.py:288: categories =
tbm_options.config.atrace_config.categories
On 2017/07/13 18:12:41, nednguyen wrote:
> If the option is restricted to string, we can be sure to always use categores
=
> categories.split(',')
> 
> Otherwise I think the way you specify the list through command line is:
> 
> --flag-foo=item1 --flag-foo=item2...
> 
> Which can be ugly in this case if people do:
> --flag-foo="item1,item3" --flag-foo=item2...

I restricted the --enable-atrace-categories flag to a string, but I'm leaving
https://github.com/catapult-project/catapult/issues/3712 open for
tbm_options.config.atrace_config.categories since it's pretty out of scope for
this CL.

https://codereview.chromium.org/2965383002/diff/40001/telemetry/telemetry/ben...
File telemetry/telemetry/benchmark_unittest.py (right):

https://codereview.chromium.org/2965383002/diff/40001/telemetry/telemetry/ben...
telemetry/telemetry/benchmark_unittest.py:169: def
testGetTBMOptionsPrefersLegacyName(self):
On 2017/07/13 18:12:41, nednguyen wrote:
> We should not allow people be able to specify both

Done.

https://codereview.chromium.org/2965383002/diff/40001/telemetry/telemetry/int...
File telemetry/telemetry/internal/browser/browser_options.py (right):

https://codereview.chromium.org/2965383002/diff/40001/telemetry/telemetry/int...
telemetry/telemetry/internal/browser/browser_options.py:129:
'c8db6c6371ca047c24d41f3972d5819bc83d83ae&l=125')
On 2017/07/13 18:12:41, nednguyen wrote:
> nits: we can add type="string" here

Done.

https://codereview.chromium.org/2965383002/diff/40001/telemetry/telemetry/int...
telemetry/telemetry/internal/browser/browser_options.py:130: group.add_option(
On 2017/07/13 18:12:41, nednguyen wrote:
> I think we should restrict the type of this to string only

Done.

[sullivan, 2017-07-13 18:38:48]

The CQ bit was checked by sullivan@chromium.org

[sullivan, 2017-07-13 18:38:49]

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/2965383002/#ps60001
(title: "addressed review comments")

[commit-bot: I haz the power, 2017-07-13 18:38:58]

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, 2017-07-13 19:06:10]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1499971128930950,
"parent_rev": "843d521c32580b668662863e414e7fbb30c13f3c", "commit_rev":
"d19b49c7f57f09361a34d8717869833050ce113b"}

[commit-bot: I haz the power, 2017-07-13 19:06:14]

Description was changed from

==========
Add support for additional flag-specified tracing categories.

This CL adds two flags:
 * --extra-chrome-categories specifies additional Chrome tracing
   categories to include when running the benchmark.
 * --extra-atrace-categories specifies additional atrace categories.

This CL also renames CreateTimelineBasedMeasurementOptions to
CreateBaseTimelineBasedMeasurementOptions.
CreateBaseTimelineBasedMeasurementOptions will be deleted after
migrating clients.

BUG=catapult:#3450
==========

to

==========
Add support for additional flag-specified tracing categories.

This CL adds two flags:
 * --extra-chrome-categories specifies additional Chrome tracing
   categories to include when running the benchmark.
 * --extra-atrace-categories specifies additional atrace categories.

This CL also renames CreateTimelineBasedMeasurementOptions to
CreateBaseTimelineBasedMeasurementOptions.
CreateBaseTimelineBasedMeasurementOptions will be deleted after
migrating clients.

BUG=catapult:#3450

Review-Url: https://codereview.chromium.org/2965383002
Committed:
https://chromium.googlesource.com/external/github.com/catapult-project/catapu...
==========

[commit-bot: I haz the power, 2017-07-13 19:06:15]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/external/github.com/catapult-project/catapu...