Add a module for tracking categories that metrics depend on.

Add a module for tracking categories that metrics depend on.

Now we can specify category dependencies of metrics in one place and
use them in all benchmark configurations.

This patch generalizes page_cycler_v2.AugmentOptionsForLoadingMetrics.

BUG=

[ulan, 2017-02-24 19:46:13]

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

[ulan, 2017-02-24 19:46:13]

PTAL

[nednguyen, 2017-03-02 13:33:04]

Description was changed from

==========
Add a module for tracking categories that metrics depend on.

Now we can specify category dependencies of metrics in one place and
use them in all benchmark configurations.

This patch generalizes page_cycler_v2.AugmentOptionsForLoadingMetrics.

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

to

==========
Add a module for tracking categories that metrics depend on.

Now we can specify category dependencies of metrics in one place and
use them in all benchmark configurations.

This patch generalizes page_cycler_v2.AugmentOptionsForLoadingMetrics.

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

[nednguyen, 2017-03-02 13:33:04]

nednguyen@google.com changed reviewers:
+ benjhayden@chromium.org, eakuefner@chromium.org, tdresser@chromium.org

[nednguyen, 2017-03-02 13:36:00]

https://codereview.chromium.org/2717713003/diff/1/tools/perf/benchmarks/metri...
File tools/perf/benchmarks/metric_dependencies.py (right):

https://codereview.chromium.org/2717713003/diff/1/tools/perf/benchmarks/metri...
tools/perf/benchmarks/metric_dependencies.py:11: _CATEGORIES_FOR_METRICS = {
ulan: instead of this, I think we should make a all_metrics.json file in
catapult/tracing/tracing/metrics that list out all the metrics & their dependent
tracing categories. 

The reason for this is it would allow the metric side panel in about://tracing
to also use this dependency map, so we can do things like: not displaying
certain metrics in the panel if the trace does not have the required categories,
show people that they will need to enable which categories to get the metric.

[tdresser, 2017-03-02 13:41:51]

Can we track this with a bug?

[benjhayden, 2017-03-02 16:52:44]

On 2017/03/02 at 13:36:00, nednguyen wrote:
>
https://codereview.chromium.org/2717713003/diff/1/tools/perf/benchmarks/metri...
> File tools/perf/benchmarks/metric_dependencies.py (right):
> 
>
https://codereview.chromium.org/2717713003/diff/1/tools/perf/benchmarks/metri...
> tools/perf/benchmarks/metric_dependencies.py:11: _CATEGORIES_FOR_METRICS = {
> ulan: instead of this, I think we should make a all_metrics.json file in
catapult/tracing/tracing/metrics that list out all the metrics & their dependent
tracing categories. 
> 
> The reason for this is it would allow the metric side panel in about://tracing
to also use this dependency map, so we can do things like: not displaying
certain metrics in the panel if the trace does not have the required categories,
show people that they will need to enable which categories to get the metric.

Instead of creating a new kind of json file, can we register the categories as
metadata when the metrics are registered in the MetricRegistry like how
supportsRangeOfInterest is registered?
MetricRegistry can require metrics to specify requiredCategories in their
metadata, as part of the existing all_metrics presubmit, instead of creating a
new presubmit and new infrastructure.

[benjhayden, 2017-03-02 16:53:16]

On 2017/03/02 at 16:52:44, benjhayden wrote:
> On 2017/03/02 at 13:36:00, nednguyen wrote:
> >
https://codereview.chromium.org/2717713003/diff/1/tools/perf/benchmarks/metri...
> > File tools/perf/benchmarks/metric_dependencies.py (right):
> > 
> >
https://codereview.chromium.org/2717713003/diff/1/tools/perf/benchmarks/metri...
> > tools/perf/benchmarks/metric_dependencies.py:11: _CATEGORIES_FOR_METRICS = {
> > ulan: instead of this, I think we should make a all_metrics.json file in
catapult/tracing/tracing/metrics that list out all the metrics & their dependent
tracing categories. 
> > 
> > The reason for this is it would allow the metric side panel in
about://tracing to also use this dependency map, so we can do things like: not
displaying certain metrics in the panel if the trace does not have the required
categories, show people that they will need to enable which categories to get
the metric.
> 
> Instead of creating a new kind of json file, can we register the categories as
metadata when the metrics are registered in the MetricRegistry like how
supportsRangeOfInterest is registered?
> MetricRegistry can require metrics to specify requiredCategories in their
metadata, as part of the existing all_metrics presubmit, instead of creating a
new presubmit and new infrastructure.

https://github.com/catapult-project/catapult/issues/1818

[benjhayden, 2017-03-02 16:54:35]

On 2017/03/02 at 16:53:16, benjhayden wrote:
> On 2017/03/02 at 16:52:44, benjhayden wrote:
> > On 2017/03/02 at 13:36:00, nednguyen wrote:
> > >
https://codereview.chromium.org/2717713003/diff/1/tools/perf/benchmarks/metri...
> > > File tools/perf/benchmarks/metric_dependencies.py (right):
> > > 
> > >
https://codereview.chromium.org/2717713003/diff/1/tools/perf/benchmarks/metri...
> > > tools/perf/benchmarks/metric_dependencies.py:11: _CATEGORIES_FOR_METRICS =
{
> > > ulan: instead of this, I think we should make a all_metrics.json file in
catapult/tracing/tracing/metrics that list out all the metrics & their dependent
tracing categories. 
> > > 
> > > The reason for this is it would allow the metric side panel in
about://tracing to also use this dependency map, so we can do things like: not
displaying certain metrics in the panel if the trace does not have the required
categories, show people that they will need to enable which categories to get
the metric.
> > 
> > Instead of creating a new kind of json file, can we register the categories
as metadata when the metrics are registered in the MetricRegistry like how
supportsRangeOfInterest is registered?
> > MetricRegistry can require metrics to specify requiredCategories in their
metadata, as part of the existing all_metrics presubmit, instead of creating a
new presubmit and new infrastructure.
> 
> https://github.com/catapult-project/catapult/issues/1818

Drive-by nit: can you either specify a BUG= or remove the line from the CL
description?

[nednguyen, 2017-03-02 17:00:23]

On 2017/03/02 16:52:44, benjhayden wrote:
> On 2017/03/02 at 13:36:00, nednguyen wrote:
> >
>
https://codereview.chromium.org/2717713003/diff/1/tools/perf/benchmarks/metri...
> > File tools/perf/benchmarks/metric_dependencies.py (right):
> > 
> >
>
https://codereview.chromium.org/2717713003/diff/1/tools/perf/benchmarks/metri...
> > tools/perf/benchmarks/metric_dependencies.py:11: _CATEGORIES_FOR_METRICS = {
> > ulan: instead of this, I think we should make a all_metrics.json file in
> catapult/tracing/tracing/metrics that list out all the metrics & their
dependent
> tracing categories. 
> > 
> > The reason for this is it would allow the metric side panel in
about://tracing
> to also use this dependency map, so we can do things like: not displaying
> certain metrics in the panel if the trace does not have the required
categories,
> show people that they will need to enable which categories to get the metric.
> 
> Instead of creating a new kind of json file, can we register the categories as
> metadata when the metrics are registered in the MetricRegistry like how
> supportsRangeOfInterest is registered?
> MetricRegistry can require metrics to specify requiredCategories in their
> metadata, as part of the existing all_metrics presubmit, instead of creating a
> new presubmit and new infrastructure.

The reason is prefer the JSON file is:
1) the dependencies will be needed by both JS (tracing) & Telemetry (Python). If
that is configured by pure JS, we need another way to invoke d8 to extract the
categories, which seems less ideal.
2) We already talk about the need for a centralized all_metrics.json file to
track all the metrics, historgrams & making sure that they are uniquely
identified. So if such a file is needed anyway, why not also use it for tracking
the categories? We can make s.t like:
{ 
   'metric_foo': {
      'histograms': {'foo_1':.., 'foo_2',...':..,}
      'required_trace_categories': [...],
    },
   'metric_bar': {...},
}

[benjhayden, 2017-03-02 21:33:35]

Is it ok if we continue the discussion about all_metrics.json in the context of
the bug about that feature?
https://github.com/catapult-project/catapult/issues/1818/

https://codereview.chromium.org/2717713003/diff/1/tools/perf/benchmarks/metri...
File tools/perf/benchmarks/metric_dependencies_unittest.py (right):

https://codereview.chromium.org/2717713003/diff/1/tools/perf/benchmarks/metri...
tools/perf/benchmarks/metric_dependencies_unittest.py:1: # Copyright 2016 The
Chromium Authors. All rights reserved.
Drive-by nit: 2017

[ulan, 2017-03-03 08:46:38]

On 2017/03/02 21:33:35, benjhayden wrote:
> Is it ok if we continue the discussion about all_metrics.json in the context
of
> the bug about that feature?
> https://github.com/catapult-project/catapult/issues/1818/
> 
>
https://codereview.chromium.org/2717713003/diff/1/tools/perf/benchmarks/metri...
> File tools/perf/benchmarks/metric_dependencies_unittest.py (right):
> 
>
https://codereview.chromium.org/2717713003/diff/1/tools/perf/benchmarks/metri...
> tools/perf/benchmarks/metric_dependencies_unittest.py:1: # Copyright 2016 The
> Chromium Authors. All rights reserved.
> Drive-by nit: 2017

Thanks all for the comments. As Ben suggested, let's continue discussion in
https://github.com/catapult-project/catapult/issues/1818/

I am going to close this CL and create a new one in catapult that uses either
all_metrics.json or metric.requireCategories.