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

telemetry/telemetry/benchmark.py

 # Copyright 2014 The Chromium Authors. All rights reserved.
 # Use of this source code is governed by a BSD-style license that can be
 # found in the LICENSE file.
 
 import optparse
 
 from telemetry import decorators
 from telemetry.internal import story_runner
 from telemetry.internal.util import command_line
 from telemetry.page import legacy_page_test
@@ skipping 54 matching lines @@
     """Creates a new Benchmark.
 
     Args:
       max_failures: The number of story run's failures before bailing
           from executing subsequent page runs. If None, we never bail.
     """
     self._expectations = None
     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
   def ShouldDisable(cls, possible_browser):
     """Override this method to disable a benchmark under specific conditions.
 
      Supports logic too complex for simple Enabled and Disabled decorators.
      Decorators are still respected in cases where this function returns False.
      """
     return False
@@ skipping 130 matching lines @@
 
   def GetOwnership(self):
     """Returns an Ownership Diagnostic containing the benchmark's information.
 
     Returns:
       Diagnostic with the benchmark's owners' e-mails and component name
     """
     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:
       options: a browser_options.BrowserFinderOptions instance
     Returns:
       |test()| if |test| is a PageTest class.
       Otherwise, a TimelineBasedMeasurement instance.
     """
     is_page_test = issubclass(self.test, legacy_page_test.LegacyPageTest)
     is_tbm = self.test == timeline_based_measurement.TimelineBasedMeasurement
     if not is_page_test and not is_tbm:
       raise TypeError('"%s" is not a PageTest or a TimelineBasedMeasurement.' %
                       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)
 
   def CreateStorySet(self, options):
     """Creates the instance of StorySet used to run the benchmark.
 
     Can be overridden by subclasses.
     """
     del options  # unused
     # TODO(aiolos, nednguyen, eakufner): replace class attribute page_set with
@@ skipping 29 matching lines @@
     """
     return expectations.StoryExpectations()
 
 
 def AddCommandLineArgs(parser):
   story_runner.AddCommandLineArgs(parser)
 
 
 def ProcessCommandLineArgs(parser, args):
   story_runner.ProcessCommandLineArgs(parser, args)