Implement Timeline Based Measurement v2

Implement Timeline Based Measurement v2

BUG=catapult:#1599

Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapult/+/0f6704fc4f775a4f3840d0e08c284fcd0631709a

[nduca, 2016-02-10 19:37:13]

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

[nduca, 2016-02-10 19:37:13]

can some (much?) of this go in tracing/metrics?

[nduca, 2016-02-10 20:50:27]

some actual feedback

https://codereview.chromium.org/1685683003/diff/40001/perf_insights/perf_insi...
File perf_insights/perf_insights/tbm/metric_registry.html (right):

https://codereview.chromium.org/1685683003/diff/40001/perf_insights/perf_insi...
perf_insights/perf_insights/tbm/metric_registry.html:19: MetricRegistry.register
= function(metric) {
maybe a docstring on this would be good to explain what a Metric is.

https://codereview.chromium.org/1685683003/diff/40001/perf_insights/perf_insi...
perf_insights/perf_insights/tbm/metric_registry.html:28: };
Can you use base/extension_registry? TypeBased I think would work here.
Benjhayden can provide advice if you need it.

https://codereview.chromium.org/1685683003/diff/40001/perf_insights/perf_insi...
File perf_insights/perf_insights/tbm/sample_metric.html (right):

https://codereview.chromium.org/1685683003/diff/40001/perf_insights/perf_insi...
perf_insights/perf_insights/tbm/sample_metric.html:15: function
sampleMetric(metricResult, model) {
valueList? that way it conveys the dynamic type?

maybe this suggests that MetricResults.html was a better name from the beginnign

https://codereview.chromium.org/1685683003/diff/40001/telemetry/telemetry/web...
File telemetry/telemetry/web_perf/timeline_based_measurement.py (right):

https://codereview.chromium.org/1685683003/diff/40001/telemetry/telemetry/web...
telemetry/telemetry/web_perf/timeline_based_measurement.py:8: from
perf_insights.tbm import timeline_based_measurement as tbm_module
is this file in another patch?

[nduca, 2016-02-10 20:52:31]

https://codereview.chromium.org/1685683003/diff/40001/perf_insights/perf_insi...
File perf_insights/perf_insights/tbm/timeline_based_measurement.py (right):

https://codereview.chromium.org/1685683003/diff/40001/perf_insights/perf_insi...
perf_insights/perf_insights/tbm/timeline_based_measurement.py:18: assert
isinstance(result, value_list.ValueList)
bit confused how map_single_trace returns a ValueList automatically...

doesn't map_single_trace return some json blob that is a map result? then we
have to turn that into a ValueList here?

https://codereview.chromium.org/1685683003/diff/40001/telemetry/telemetry/web...
File telemetry/telemetry/web_perf/timeline_based_measurement.py (right):

https://codereview.chromium.org/1685683003/diff/40001/telemetry/telemetry/web...
telemetry/telemetry/web_perf/timeline_based_measurement.py:8: from
perf_insights.tbm import timeline_based_measurement as tbm_module
On 2016/02/10 at 20:50:27, nduca wrote:
> is this file in another patch?

oh, derp.

[benjhayden, 2016-02-10 21:24:34]

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

[benjhayden, 2016-02-10 21:24:35]

Yay!

Link to design doc?

Few nits.

https://codereview.chromium.org/1685683003/diff/40001/perf_insights/perf_insi...
File perf_insights/perf_insights/tbm/metric_registry.html (right):

https://codereview.chromium.org/1685683003/diff/40001/perf_insights/perf_insi...
perf_insights/perf_insights/tbm/metric_registry.html:15: allMetricsByName_: {},
Drop the 'all'? It's cleaner. :-)

https://codereview.chromium.org/1685683003/diff/40001/perf_insights/perf_insi...
perf_insights/perf_insights/tbm/metric_registry.html:20: if (typeof(metric) !==
'function') {
no curly braces necessary

https://codereview.chromium.org/1685683003/diff/40001/perf_insights/perf_insi...
perf_insights/perf_insights/tbm/metric_registry.html:26: throw new Error('Metric
named ' + metric.name + 'is already registered.');
Space before 'is'.

[eakuefner, 2016-02-22 17:58:02]

On 2016/02/10 at 19:37:13, nduca wrote:
> can some (much?) of this go in tracing/metrics?

Done.

[nduca, 2016-02-22 17:59:32]

lgtm

[nednguyen, 2016-02-25 16:44:16]

nednguyen@google.com changed reviewers:
+ nednguyen@google.com

[nednguyen, 2016-02-25 16:44:17]

lgtm with nits

Please add a smoke test to timeline_base_page_test with sample_metric before
landing this patch.

https://codereview.chromium.org/1685683003/diff/160001/telemetry/telemetry/va...
File telemetry/telemetry/value/merge_values.py (right):

https://codereview.chromium.org/1685683003/diff/160001/telemetry/telemetry/va...
telemetry/telemetry/value/merge_values.py:27: return (value.name,
value.tir_label) + grouping_keys_sorted
The tuple will looks s.t like:
('value_name', 'tir_label_name', ('keyA': valueA), ('keyB', valueB))

Make this:
 ('value_name': value.name, 'tir_label': value.tir_label) + grouping_keys_sorted

to make the tuple consistent & add a TODO to automatically appends value_name &
tir_label to grouping_key (using the same trick that we did in the js version).

[benjhayden, 2016-02-25 16:49:09]

benjhayden@chromium.org changed reviewers:
- nednguyen@google.com

[benjhayden, 2016-02-25 16:49:10]

Sorry, I didn't know you were waiting for my review!
I just have a few dumb questions and nits.

https://codereview.chromium.org/1685683003/diff/160001/perf_insights/perf_ins...
File perf_insights/perf_insights/map_single_trace.py (right):

https://codereview.chromium.org/1685683003/diff/160001/perf_insights/perf_ins...
perf_insights/perf_insights/map_single_trace.py:91: print all_source_paths
cruft?

https://codereview.chromium.org/1685683003/diff/160001/perf_insights/perf_ins...
File perf_insights/perf_insights/timeline_based_measurement/rendering_frame.html
(left):

https://codereview.chromium.org/1685683003/diff/160001/perf_insights/perf_ins...
perf_insights/perf_insights/timeline_based_measurement/rendering_frame.html:1:
<!DOCTYPE html>
is this deletion part of this CL?

https://codereview.chromium.org/1685683003/diff/160001/telemetry/telemetry/__...
File telemetry/telemetry/__init__.py (right):

https://codereview.chromium.org/1685683003/diff/160001/telemetry/telemetry/__...
telemetry/telemetry/__init__.py:38: _AddDirToPythonPath(_CATAPULT_DIR,
'perf_insights')
are we going to use mappers from perf_insights or metrics from
tracing/tracing/metrics?

https://codereview.chromium.org/1685683003/diff/160001/telemetry/telemetry/va...
File telemetry/telemetry/value/histogram.py (left):

https://codereview.chromium.org/1685683003/diff/160001/telemetry/telemetry/va...
telemetry/telemetry/value/histogram.py:114: if 'tir_label' in value_dict:
Sorry if this is a dumb comment, but is this part of this CL?

https://codereview.chromium.org/1685683003/diff/160001/telemetry/telemetry/we...
File telemetry/telemetry/web_perf/timeline_based_measurement.py (right):

https://codereview.chromium.org/1685683003/diff/160001/telemetry/telemetry/we...
telemetry/telemetry/web_perf/timeline_based_measurement.py:212: def
SetTimelineBasedMetrics(self, metrics):
what is metrics a list of?

https://codereview.chromium.org/1685683003/diff/160001/tracing/tracing/metric...
File tracing/tracing/metrics/metric_registry.html (right):

https://codereview.chromium.org/1685683003/diff/160001/tracing/tracing/metric...
tracing/tracing/metrics/metric_registry.html:22: }
curly braces not necessary here

https://codereview.chromium.org/1685683003/diff/160001/tracing/tracing/metric...
File tracing/tracing/metrics/metric_runner.py (right):

https://codereview.chromium.org/1685683003/diff/160001/tracing/tracing/metric...
tracing/tracing/metrics/metric_runner.py:34: return result.pairs['values']
Why shouldn't the metricMapFunction return the valueDicts directly so that this
function doesn't need to unpack the special 'values' pair?

[eakuefner, 2016-02-25 22:25:34]

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

[eakuefner, 2016-02-25 22:25:37]

https://codereview.chromium.org/1685683003/diff/40001/perf_insights/perf_insi...
File perf_insights/perf_insights/tbm/metric_registry.html (right):

https://codereview.chromium.org/1685683003/diff/40001/perf_insights/perf_insi...
perf_insights/perf_insights/tbm/metric_registry.html:19: MetricRegistry.register
= function(metric) {
On 2016/02/10 at 20:50:27, nduca wrote:
> maybe a docstring on this would be good to explain what a Metric is.

Done.

https://codereview.chromium.org/1685683003/diff/40001/perf_insights/perf_insi...
perf_insights/perf_insights/tbm/metric_registry.html:20: if (typeof(metric) !==
'function') {
On 2016/02/10 at 21:24:34, benjhayden_chromium wrote:
> no curly braces necessary

Done.

https://codereview.chromium.org/1685683003/diff/40001/perf_insights/perf_insi...
perf_insights/perf_insights/tbm/metric_registry.html:26: throw new Error('Metric
named ' + metric.name + 'is already registered.');
On 2016/02/10 at 21:24:34, benjhayden_chromium wrote:
> Space before 'is'.

Done.

https://codereview.chromium.org/1685683003/diff/40001/perf_insights/perf_insi...
perf_insights/perf_insights/tbm/metric_registry.html:28: };
On 2016/02/10 at 20:50:27, nduca wrote:
> Can you use base/extension_registry? TypeBased I think would work here.
Benjhayden can provide advice if you need it.

Punted per offline discussion.

https://codereview.chromium.org/1685683003/diff/40001/perf_insights/perf_insi...
File perf_insights/perf_insights/tbm/sample_metric.html (right):

https://codereview.chromium.org/1685683003/diff/40001/perf_insights/perf_insi...
perf_insights/perf_insights/tbm/sample_metric.html:15: function
sampleMetric(metricResult, model) {
On 2016/02/10 at 20:50:27, nduca wrote:
> valueList? that way it conveys the dynamic type?
> 
> maybe this suggests that MetricResults.html was a better name from the
beginnign

Changed to valueList.

https://codereview.chromium.org/1685683003/diff/40001/perf_insights/perf_insi...
File perf_insights/perf_insights/tbm/timeline_based_measurement.py (right):

https://codereview.chromium.org/1685683003/diff/40001/perf_insights/perf_insi...
perf_insights/perf_insights/tbm/timeline_based_measurement.py:18: assert
isinstance(result, value_list.ValueList)
On 2016/02/10 at 20:52:31, nduca wrote:
> bit confused how map_single_trace returns a ValueList automatically...
> 
> doesn't map_single_trace return some json blob that is a map result? then we
have to turn that into a ValueList here?

Changed this so now RunMetrics gives the raw MreResult (with the 'values' pair).
We'll fix this later, once Telemetry uses common values.

https://codereview.chromium.org/1685683003/diff/160001/perf_insights/perf_ins...
File perf_insights/perf_insights/map_single_trace.py (right):

https://codereview.chromium.org/1685683003/diff/160001/perf_insights/perf_ins...
perf_insights/perf_insights/map_single_trace.py:91: print all_source_paths
On 2016/02/25 at 16:49:10, benjhayden_chromium wrote:
> cruft?

Done.

https://codereview.chromium.org/1685683003/diff/160001/perf_insights/perf_ins...
File perf_insights/perf_insights/timeline_based_measurement/rendering_frame.html
(left):

https://codereview.chromium.org/1685683003/diff/160001/perf_insights/perf_ins...
perf_insights/perf_insights/timeline_based_measurement/rendering_frame.html:1:
<!DOCTYPE html>
On 2016/02/25 at 16:49:10, benjhayden_chromium wrote:
> is this deletion part of this CL?

Yes.

https://codereview.chromium.org/1685683003/diff/160001/telemetry/telemetry/__...
File telemetry/telemetry/__init__.py (right):

https://codereview.chromium.org/1685683003/diff/160001/telemetry/telemetry/__...
telemetry/telemetry/__init__.py:38: _AddDirToPythonPath(_CATAPULT_DIR,
'perf_insights')
On 2016/02/25 at 16:49:10, benjhayden_chromium wrote:
> are we going to use mappers from perf_insights or metrics from
tracing/tracing/metrics?

Metrics all belong in tracing/tracing/metrics. TBMv2 is based on the metrics
framework, so users of it implement metrics, rather than mappers. The
perf_insights directory is included here because MRE still lives in
perf_insights, and the metrics framework depends on MRE since metric_runner is
implemented using a mapper.

I have moved this path modification to tracing/tracing/metrics/__init__.py,
which is a more suitable place for it.

https://codereview.chromium.org/1685683003/diff/160001/telemetry/telemetry/va...
File telemetry/telemetry/value/histogram.py (left):

https://codereview.chromium.org/1685683003/diff/160001/telemetry/telemetry/va...
telemetry/telemetry/value/histogram.py:114: if 'tir_label' in value_dict:
On 2016/02/25 at 16:49:10, benjhayden_chromium wrote:
> Sorry if this is a dumb comment, but is this part of this CL?

Yes. This is redundant because tir_label is already inserted by
Value.GetConstructorKwArgs.

https://codereview.chromium.org/1685683003/diff/160001/telemetry/telemetry/we...
File telemetry/telemetry/web_perf/timeline_based_measurement.py (right):

https://codereview.chromium.org/1685683003/diff/160001/telemetry/telemetry/we...
telemetry/telemetry/web_perf/timeline_based_measurement.py:212: def
SetTimelineBasedMetrics(self, metrics):
On 2016/02/25 at 16:49:10, benjhayden_chromium wrote:
> what is metrics a list of?

Added docstring to clarify that this is a list of metric paths.

https://codereview.chromium.org/1685683003/diff/160001/tracing/tracing/metric...
File tracing/tracing/metrics/metric_registry.html (right):

https://codereview.chromium.org/1685683003/diff/160001/tracing/tracing/metric...
tracing/tracing/metrics/metric_registry.html:22: }
On 2016/02/25 at 16:49:10, benjhayden_chromium wrote:
> curly braces not necessary here

Done.

https://codereview.chromium.org/1685683003/diff/160001/tracing/tracing/metric...
File tracing/tracing/metrics/metric_runner.py (right):

https://codereview.chromium.org/1685683003/diff/160001/tracing/tracing/metric...
tracing/tracing/metrics/metric_runner.py:34: return result.pairs['values']
On 2016/02/25 at 16:49:10, benjhayden_chromium wrote:
> Why shouldn't the metricMapFunction return the valueDicts directly so that
this function doesn't need to unpack the special 'values' pair?

As I responded in Nat's similar comment, for now we simply now return the
MreResult here, to allow Telemetry to surface the MRE Failures as FailureValues.
We'll revisit this when Telemetry uses the common value system.

[eakuefner, 2016-02-25 22:25:58]

On 2016/02/25 at 16:44:17, nednguyen wrote:
> lgtm with nits
> 
> Please add a smoke test to timeline_base_page_test with sample_metric before
landing this patch.

Done.

> 
>
https://codereview.chromium.org/1685683003/diff/160001/telemetry/telemetry/va...
> File telemetry/telemetry/value/merge_values.py (right):
> 
>
https://codereview.chromium.org/1685683003/diff/160001/telemetry/telemetry/va...
> telemetry/telemetry/value/merge_values.py:27: return (value.name,
value.tir_label) + grouping_keys_sorted
> The tuple will looks s.t like:
> ('value_name', 'tir_label_name', ('keyA': valueA), ('keyB', valueB))
> 
> Make this:
>  ('value_name': value.name, 'tir_label': value.tir_label) +
grouping_keys_sorted
> 
> to make the tuple consistent & add a TODO to automatically appends value_name
& tir_label to grouping_key (using the same trick that we did in the js
version).

[benjhayden, 2016-02-25 22:43:13]

Maybe delete a couple prints, then lgtm, thanks!

https://codereview.chromium.org/1685683003/diff/240001/perf_insights/perf_ins...
File perf_insights/perf_insights/map_single_trace.py (right):

https://codereview.chromium.org/1685683003/diff/240001/perf_insights/perf_ins...
perf_insights/perf_insights/map_single_trace.py:91: print all_source_paths
did you mean to delete this?

https://codereview.chromium.org/1685683003/diff/240001/tracing/tracing/metric...
File tracing/tracing/metrics/metric_runner.py (right):

https://codereview.chromium.org/1685683003/diff/240001/tracing/tracing/metric...
tracing/tracing/metrics/metric_runner.py:21: print metric_paths
delete?

[eakuefner, 2016-02-25 23:17:33]

Description was changed from

==========
Implement Timeline Based Measurement v2
==========

to

==========
Implement Timeline Based Measurement v2

BUG=catapult:#1599
==========

[eakuefner-google (do not use), 2016-02-25 23:22:45]

The CQ bit was checked by eakuefner@google.com

[eakuefner-google (do not use), 2016-02-25 23:22:45]

The patchset sent to the CQ was uploaded after l-g-t-m from nduca@chromium.org,
nednguyen@google.com, benjhayden@chromium.org
Link to the patchset: https://codereview.chromium.org/1685683003/#ps260001
(title: "final round of comments")

[eakuefner-google (do not use), 2016-02-25 23:22:48]

The CQ bit was unchecked by eakuefner@google.com

[eakuefner, 2016-02-25 23:22:59]

The CQ bit was checked by eakuefner@chromium.org

[commit-bot: I haz the power, 2016-02-25 23:23:07]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1685683003/260001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1685683003/260001

[eakuefner, 2016-02-25 23:25:01]

https://codereview.chromium.org/1685683003/diff/160001/telemetry/telemetry/va...
File telemetry/telemetry/value/merge_values.py (right):

https://codereview.chromium.org/1685683003/diff/160001/telemetry/telemetry/va...
telemetry/telemetry/value/merge_values.py:27: return (value.name,
value.tir_label) + grouping_keys_sorted
On 2016/02/25 at 16:44:17, nednguyen wrote:
> The tuple will looks s.t like:
> ('value_name', 'tir_label_name', ('keyA': valueA), ('keyB', valueB))
> 
> Make this:
>  ('value_name': value.name, 'tir_label': value.tir_label) +
grouping_keys_sorted
> 
> to make the tuple consistent & add a TODO to automatically appends value_name
& tir_label to grouping_key (using the same trick that we did in the js
version).

Done.

https://codereview.chromium.org/1685683003/diff/240001/perf_insights/perf_ins...
File perf_insights/perf_insights/map_single_trace.py (right):

https://codereview.chromium.org/1685683003/diff/240001/perf_insights/perf_ins...
perf_insights/perf_insights/map_single_trace.py:91: print all_source_paths
On 2016/02/25 at 22:43:13, benjhayden_chromium wrote:
> did you mean to delete this?

Done.

https://codereview.chromium.org/1685683003/diff/240001/tracing/tracing/metric...
File tracing/tracing/metrics/metric_runner.py (right):

https://codereview.chromium.org/1685683003/diff/240001/tracing/tracing/metric...
tracing/tracing/metrics/metric_runner.py:21: print metric_paths
On 2016/02/25 at 22:43:13, benjhayden_chromium wrote:
> delete?

Done.

[eakuefner, 2016-02-25 23:32:41]

The CQ bit was unchecked by eakuefner@chromium.org

[eakuefner, 2016-02-25 23:35:09]

The CQ bit was checked by eakuefner@chromium.org

[eakuefner, 2016-02-25 23:35:10]

The patchset sent to the CQ was uploaded after l-g-t-m from
nednguyen@google.com, benjhayden@chromium.org, nduca@chromium.org
Link to the patchset: https://codereview.chromium.org/1685683003/#ps280001
(title: "Fix DefaultKeyFunc test")

[commit-bot: I haz the power, 2016-02-25 23:35:13]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1685683003/280001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1685683003/280001

[commit-bot: I haz the power, 2016-02-25 23:51:22]

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

[commit-bot: I haz the power, 2016-02-25 23:51:23]

Try jobs failed on following builders:
  Catapult Windows Tryserver on master.tryserver.client.catapult (JOB_FAILED,
https://build.chromium.org/p/tryserver.client.catapult/builders/Catapult%20Wi...)

[eakuefner, 2016-02-26 07:10:50]

The CQ bit was checked by eakuefner@chromium.org

[eakuefner, 2016-02-26 07:10:50]

The patchset sent to the CQ was uploaded after l-g-t-m from
nednguyen@google.com, benjhayden@chromium.org, nduca@chromium.org
Link to the patchset: https://codereview.chromium.org/1685683003/#ps300001
(title: "fix vinn tests")

[commit-bot: I haz the power, 2016-02-26 07:11:00]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1685683003/300001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1685683003/300001

[commit-bot: I haz the power, 2016-02-26 07:24:32]

Description was changed from

==========
Implement Timeline Based Measurement v2

BUG=catapult:#1599
==========

to

==========
Implement Timeline Based Measurement v2

BUG=catapult:#1599

Committed:
https://chromium.googlesource.com/external/github.com/catapult-project/catapu...
==========

[commit-bot: I haz the power, 2016-02-26 07:24:33]

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