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

telemetry/telemetry/benchmark.py

Index: telemetry/telemetry/benchmark.py
diff --git a/telemetry/telemetry/benchmark.py b/telemetry/telemetry/benchmark.py
index a04d6b2f2cd14f3a2d20381772e19f6495f511f7..239c823ea1d584220e6f073dc9bb2ebca0e37840 100644
--- a/telemetry/telemetry/benchmark.py
+++ b/telemetry/telemetry/benchmark.py
@@ -72,12 +72,14 @@ class Benchmark(command_line.Command):
     self._max_failures = max_failures
     self._has_original_tbm_options = (
         self.CreateTimelineBasedMeasurementOptions.__func__ ==
-        Benchmark.CreateTimelineBasedMeasurementOptions.__func__)
+        Benchmark.CreateTimelineBasedMeasurementOptions.__func__ or
+        self.CreateBaseTimelineBasedMeasurementOptions.__func__ ==
+        Benchmark.CreateBaseTimelineBasedMeasurementOptions.__func__)
     has_original_create_page_test = (
         self.CreatePageTest.__func__ == Benchmark.CreatePageTest.__func__)
     assert self._has_original_tbm_options or has_original_create_page_test, (
         'Cannot override both CreatePageTest and '
-        'CreateTimelineBasedMeasurementOptions.')
+        'CreateBaseTimelineBasedMeasurementOptions.')
 
   # pylint: disable=unused-argument
   @classmethod
@@ -228,20 +230,64 @@ class Benchmark(command_line.Command):
     return histogram.Ownership(decorators.GetEmails(self),
                                decorators.GetComponent(self))
 
+  @decorators.Deprecated(
+      2017, 9, 1, 'Use CreateBaseTimelineBasedMeasurementOptions instead.')
   def CreateTimelineBasedMeasurementOptions(self):
-    """Return the TimelineBasedMeasurementOptions for this Benchmark.
+    """See CreateBaseTimelineBasedMeasurementOptions."""
+    return self.CreateBaseTimelineBasedMeasurementOptions()
 
-    Override this method to configure a TimelineBasedMeasurement benchmark.
-    Otherwise, override CreatePageTest for PageTest tests. Do not override
-    both methods.
+  def CreateBaseTimelineBasedMeasurementOptions(self):

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

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."

+    """Return the base TimelineBasedMeasurementOptions for this Benchmark.
+
+    Additional chrome and atrace categories can be appended when running the
+    benchmark with the --extra-chrome-categories and --extra-atrace-categories
+    flags.
+
+    Override this method to configure a TimelineBasedMeasurement benchmark. If
+    this is not a TimelineBasedMeasurement benchmark, override CreatePageTest
+    for PageTest tests. Do not override both methods.
     """
     return timeline_based_measurement.Options()
 
+  def _GetTimelineBasedMeasurementOptions(self, options):
+    """Return all timeline based measurements for the curren benchmark run.
+
+    This includes the benchmark-configured measurements in
+    CreateBaseTimelineBasedMeasurementOptions as well as the user-flag-
+    configured options from --extra-chrome-categories and
+    --extra-atrace-categories.
+    """
+    # TODO(sullivan): the benchmark options should all be configured in
+    # CreateBaseTimelineBasedMeasurementOptions. Remove references to
+    # CreateTimelineBasedMeasurementOptions when it is fully deprecated.
+    # In the short term, if the benchmark overrides
+    # CreateTimelineBasedMeasurementOptions use the overridden version,
+    # otherwise call CreateBaseTimelineBasedMeasurementOptions.
+    # https://github.com/catapult-project/catapult/issues/3450
+    tbm_options = None
+    if (self.CreateTimelineBasedMeasurementOptions.__func__ !=
+        Benchmark.CreateTimelineBasedMeasurementOptions.__func__):
+      tbm_options = self.CreateTimelineBasedMeasurementOptions()
+    else:
+      tbm_options = self.CreateBaseTimelineBasedMeasurementOptions()
+    if options.extra_chrome_categories:
+      assert tbm_options.enable_chrome_trace, (
+        'This benchmark does not support Chrome tracing.')

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

Copied this from the bug comment. Big question I have here is why not just
enable Chrome tracing if this is specified?

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

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.

[nednguyen, 2017/07/07 20:07:50]

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).

+      tbm_options.chrome_trace_config.category_filter.AddFilterString(

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

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/...

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

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..."

[nednguyen, 2017/07/07 20:07:50]

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
Lemme look around...

+          options.extra_chrome_categories)
+    if options.extra_atrace_categories:
+      assert tbm_options.enable_atrace_trace, (
+          'This benchmark does not support atrace.')

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

Same question about enabling atrace.

[nednguyen, 2017/07/07 20:07:50]

Same as above

+      tbm_options.atrace_config.categories.append(
+          options.extra_atrace_categories.split(','))
+    return tbm_options
+
+
   def CreatePageTest(self, options):  # pylint: disable=unused-argument
     """Return the PageTest for this Benchmark.
 
     Override this method for PageTest tests.
-    Override, override CreateTimelineBasedMeasurementOptions to configure
+    Override, CreateBaseTimelineBasedMeasurementOptions to configure
     TimelineBasedMeasurement tests. Do not override both methods.
 
     Args:
@@ -257,11 +303,11 @@ class Benchmark(command_line.Command):
                       self.test.__name__)
     if is_page_test:
       assert self._has_original_tbm_options, (
-          'Cannot override CreateTimelineBasedMeasurementOptions '
+          'Cannot override CreateBaseTimelineBasedMeasurementOptions '
           'with a PageTest.')
       return self.test()  # pylint: disable=no-value-for-parameter
 
-    opts = self.CreateTimelineBasedMeasurementOptions()
+    opts = self._GetTimelineBasedMeasurementOptions(options)
     self.SetupTraceRerunOptions(options, opts)
     return timeline_based_measurement.TimelineBasedMeasurement(opts)