[Telemetry] Also merge across tir_labels for TBM

[Telemetry] Also merge across tir_labels for TBM

This CL updates the summarization behavior of chart JSON to summarize twice:
once by name and tir_label (which is the current behavior), and once by name
only (so that merging across tir_labels will occur). The reason we want to do
this is to fill in the summary values for story sets with story keys, which we
abuse the tir_label field to store.

It also updates the merging code for SummarizableValues, which previously
assumed that merging would only occur for lists of values which all had the
same tir_label.

Currently, in the presence of IR labels, we only output these summaries:
* Per story/tir_label
* Per tir_label

With this change, we will additionally output, for TBMv2 benchmarks:
* Per story, across all tir_labels
* Per value, across all stories and tir_labels

BUG=chromium:624522
Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapult/+/8e77a164018f4e2821a4fa37747365261cd0071c

[eakuefner, 2016-07-12 22:23:20]

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

[eakuefner, 2016-07-12 22:23:20]

PTAL. Please note that this change depends on
https://codereview.chromium.org/2144883002 but for some reason setting up the
dependency didn't work, so you'll see those (small) changes here.

[eakuefner, 2016-07-12 22:34:25]

nvm; figured it out in the latest patch set.

[nednguyen, 2016-07-13 14:54:04]

This worths making an announcement to people once this patch is ready to land

https://codereview.chromium.org/2142293002/diff/20001/telemetry/telemetry/int...
File telemetry/telemetry/internal/results/chart_json_output_formatter.py
(right):

https://codereview.chromium.org/2142293002/diff/20001/telemetry/telemetry/int...
telemetry/telemetry/internal/results/chart_json_output_formatter.py:14:
summary_values, should_flatten_tir_labels=False):
Why do we need this should_flatten_tir_labels param and not move this logic in
summary.interleaved_computed_per_page_values_and_summaries?

[eakuefner, 2016-07-13 17:46:16]

https://codereview.chromium.org/2142293002/diff/20001/telemetry/telemetry/int...
File telemetry/telemetry/internal/results/chart_json_output_formatter.py
(right):

https://codereview.chromium.org/2142293002/diff/20001/telemetry/telemetry/int...
telemetry/telemetry/internal/results/chart_json_output_formatter.py:14:
summary_values, should_flatten_tir_labels=False):
On 2016/07/13 at 14:54:04, nednguyen wrote:
> Why do we need this should_flatten_tir_labels param and not move this logic in
summary.interleaved_computed_per_page_values_and_summaries?

Based on the way Summary is currently architected, that would be a layering
violation. Summary doesn't know about Value internals, because merging is
something values specify. It also doesn't know about PageTestResults, because
Summary operates on a list of page specific values. It also doesn't prescribe
grouping by a certain key, because it takes a key function: you'll notice here,
the only thing we need to do is construct a second summary to which we pass a
custom key func.

There is one downside to the way this is all architected, which is that you need
to decide to output these extra values on a per-output-formatter basis. As
you'll notice, I haven't made any changes to BuildbotOutputFormatter, but that's
because it's deprecated; if existing BBOF stragglers want the new extra values
for TBMv2 metrics, they can switch to chart JSON.

[eakuefner, 2016-07-13 17:46:39]

I'll be sure to send an announcement to telemetry-announce, perf-sheriffs, and
the perf dashboard announce list once this is all done.

[nednguyen, 2016-07-13 18:01:28]

https://codereview.chromium.org/2142293002/diff/20001/telemetry/telemetry/int...
File telemetry/telemetry/internal/results/chart_json_output_formatter.py
(right):

https://codereview.chromium.org/2142293002/diff/20001/telemetry/telemetry/int...
telemetry/telemetry/internal/results/chart_json_output_formatter.py:14:
summary_values, should_flatten_tir_labels=False):
On 2016/07/13 17:46:16, eakuefner wrote:
> On 2016/07/13 at 14:54:04, nednguyen wrote:
> > Why do we need this should_flatten_tir_labels param and not move this logic
in
> summary.interleaved_computed_per_page_values_and_summaries?
> 
> Based on the way Summary is currently architected, that would be a layering
> violation. Summary doesn't know about Value internals, because merging is
> something values specify. It also doesn't know about PageTestResults, because
> Summary operates on a list of page specific values. It also doesn't prescribe
> grouping by a certain key, because it takes a key function: you'll notice
here,
> the only thing we need to do is construct a second summary to which we pass a
> custom key func.
> 
> There is one downside to the way this is all architected, which is that you
need
> to decide to output these extra values on a per-output-formatter basis. As
> you'll notice, I haven't made any changes to BuildbotOutputFormatter, but
that's
> because it's deprecated; if existing BBOF stragglers want the new extra values
> for TBMv2 metrics, they can switch to chart JSON.

I see, but just doing this for chartjson + results.html on tbmv2 data only is
bad because it makes the situation of value + results in telemetry even more
diverged & complex. 

To avoid copy this code across all the output_formatter, can you create a
helpler methods like merge_values.GenerateSummaryValues & have all the results
formatter use it? I would also want to avoid this logic being applied to tbm2
values only.

https://codereview.chromium.org/2142293002/diff/20001/telemetry/telemetry/int...
telemetry/telemetry/internal/results/chart_json_output_formatter.py:42:
summary2.interleaved_computed_per_page_values_and_summaries)
This seems like it will make the page_specific_values being duplicated twice

[eakuefner, 2016-07-13 18:11:16]

https://codereview.chromium.org/2142293002/diff/20001/telemetry/telemetry/int...
File telemetry/telemetry/internal/results/chart_json_output_formatter.py
(right):

https://codereview.chromium.org/2142293002/diff/20001/telemetry/telemetry/int...
telemetry/telemetry/internal/results/chart_json_output_formatter.py:14:
summary_values, should_flatten_tir_labels=False):
On 2016/07/13 at 18:01:28, nednguyen wrote:
> On 2016/07/13 17:46:16, eakuefner wrote:
> > On 2016/07/13 at 14:54:04, nednguyen wrote:
> > > Why do we need this should_flatten_tir_labels param and not move this
logic in
> > summary.interleaved_computed_per_page_values_and_summaries?
> > 
> > Based on the way Summary is currently architected, that would be a layering
> > violation. Summary doesn't know about Value internals, because merging is
> > something values specify. It also doesn't know about PageTestResults,
because
> > Summary operates on a list of page specific values. It also doesn't
prescribe
> > grouping by a certain key, because it takes a key function: you'll notice
here,
> > the only thing we need to do is construct a second summary to which we pass
a
> > custom key func.
> > 
> > There is one downside to the way this is all architected, which is that you
need
> > to decide to output these extra values on a per-output-formatter basis. As
> > you'll notice, I haven't made any changes to BuildbotOutputFormatter, but
that's
> > because it's deprecated; if existing BBOF stragglers want the new extra
values
> > for TBMv2 metrics, they can switch to chart JSON.
> 
> I see, but just doing this for chartjson + results.html on tbmv2 data only is
bad because it makes the situation of value + results in telemetry even more
diverged & complex. 
> 
> To avoid copy this code across all the output_formatter, can you create a
helpler methods like merge_values.GenerateSummaryValues & have all the results
formatter use it? I would also want to avoid this logic being applied to tbm2
values only.

Yeah, that sounds fine. Please confirm that the new plan is:

1. Encapsulate the code in a helper that's easy to call from the various OFs,
and
2. Apply this logic also for TBMv1.

For what it's worth, based on a search yesterday, it looks like we have few if
any purely TBMv1-based benchmarks remaining and monitored, so I think just doing
this as a global change to the behavior of timeline-based benchmark results is
totally fine. Smoothness, and the associated alerting, should not be affected by
this change. The potential for spurious alerts was one of the reasons I was
afraid to apply this for TBMv1, but based on this reasoning I don't think there
should be a problem.

https://codereview.chromium.org/2142293002/diff/20001/telemetry/telemetry/int...
telemetry/telemetry/internal/results/chart_json_output_formatter.py:42:
summary2.interleaved_computed_per_page_values_and_summaries)
On /07/13 at 18:01:28, nednguyen wrote:
> This seems like it will make the page_specific_values being duplicated twice

Actually, that's not quite correct, because this means "page specific values
_per merge key_" -- it's a subtle point, but in the first summary, it means you
get page specific values for each IR, and in the second summary, it means you
get page specific values across all IRs.

[nednguyen, 2016-07-13 18:35:55]

https://codereview.chromium.org/2142293002/diff/20001/telemetry/telemetry/int...
File telemetry/telemetry/internal/results/chart_json_output_formatter.py
(right):

https://codereview.chromium.org/2142293002/diff/20001/telemetry/telemetry/int...
telemetry/telemetry/internal/results/chart_json_output_formatter.py:14:
summary_values, should_flatten_tir_labels=False):
On 2016/07/13 18:11:16, eakuefner wrote:
> On 2016/07/13 at 18:01:28, nednguyen wrote:
> > On 2016/07/13 17:46:16, eakuefner wrote:
> > > On 2016/07/13 at 14:54:04, nednguyen wrote:
> > > > Why do we need this should_flatten_tir_labels param and not move this
> logic in
> > > summary.interleaved_computed_per_page_values_and_summaries?
> > > 
> > > Based on the way Summary is currently architected, that would be a
layering
> > > violation. Summary doesn't know about Value internals, because merging is
> > > something values specify. It also doesn't know about PageTestResults,
> because
> > > Summary operates on a list of page specific values. It also doesn't
> prescribe
> > > grouping by a certain key, because it takes a key function: you'll notice
> here,
> > > the only thing we need to do is construct a second summary to which we
pass
> a
> > > custom key func.
> > > 
> > > There is one downside to the way this is all architected, which is that
you
> need
> > > to decide to output these extra values on a per-output-formatter basis. As
> > > you'll notice, I haven't made any changes to BuildbotOutputFormatter, but
> that's
> > > because it's deprecated; if existing BBOF stragglers want the new extra
> values
> > > for TBMv2 metrics, they can switch to chart JSON.
> > 
> > I see, but just doing this for chartjson + results.html on tbmv2 data only
is
> bad because it makes the situation of value + results in telemetry even more
> diverged & complex. 
> > 
> > To avoid copy this code across all the output_formatter, can you create a
> helpler methods like merge_values.GenerateSummaryValues & have all the results
> formatter use it? I would also want to avoid this logic being applied to tbm2
> values only.
> 
> Yeah, that sounds fine. Please confirm that the new plan is:
> 
> 1. Encapsulate the code in a helper that's easy to call from the various OFs,
> and
Yes
> 2. Apply this logic also for TBMv1.
Yes
> 
> For what it's worth, based on a search yesterday, it looks like we have few if
> any purely TBMv1-based benchmarks remaining and monitored, so I think just
doing
> this as a global change to the behavior of timeline-based benchmark results is
> totally fine. Smoothness, and the associated alerting, should not be affected
by
> this change. The potential for spurious alerts was one of the reasons I was
> afraid to apply this for TBMv1, but based on this reasoning I don't think
there
> should be a problem.

Sounds great!

https://codereview.chromium.org/2142293002/diff/20001/telemetry/telemetry/int...
telemetry/telemetry/internal/results/chart_json_output_formatter.py:42:
summary2.interleaved_computed_per_page_values_and_summaries)
On 2016/07/13 18:11:16, eakuefner wrote:
> On /07/13 at 18:01:28, nednguyen wrote:
> > This seems like it will make the page_specific_values being duplicated twice
> 
> Actually, that's not quite correct, because this means "page specific values
> _per merge key_" -- it's a subtle point, but in the first summary, it means
you
> get page specific values for each IR, and in the second summary, it means you
> get page specific values across all IRs.

Oh, I see. Seems like it should be renamed to
interleaved_computed_per_merged_key_values_and_summaries 
 
I forgot that we never keep the original values when outputting them. ~_~

[eakuefner, 2016-08-12 23:45:11]

Ned, please take another look. I have addressed your comments by:

1. Making the summarize-twice code a function (static) in the output_formatter
module, and
2. Not making this code apply only to TBMv2.

[nednguyen, 2016-08-15 13:40:57]

https://codereview.chromium.org/2142293002/diff/100001/telemetry/telemetry/in...
File telemetry/telemetry/internal/results/chart_json_output_formatter.py
(right):

https://codereview.chromium.org/2142293002/diff/100001/telemetry/telemetry/in...
telemetry/telemetry/internal/results/chart_json_output_formatter.py:34:
output_formatter.SummarizeTimelineBasedResults(page_specific_values),
you mean  SummarizePageSpecificValues?

https://codereview.chromium.org/2142293002/diff/100001/telemetry/telemetry/in...
File telemetry/telemetry/internal/results/output_formatter.py (right):

https://codereview.chromium.org/2142293002/diff/100001/telemetry/telemetry/in...
telemetry/telemetry/internal/results/output_formatter.py:54: summary =
summary_module.Summary(page_specific_values)
For readability, maybe add a comment about by default, we summarize by both name
& tir_label.

https://codereview.chromium.org/2142293002/diff/100001/telemetry/telemetry/in...
telemetry/telemetry/internal/results/output_formatter.py:60: summaryByNameOnly =
summary_module.Summary(
summary_by_name_only, switching between python & js is tough ;)

https://codereview.chromium.org/2142293002/diff/100001/telemetry/telemetry/in...
telemetry/telemetry/internal/results/output_formatter.py:63: value_lists.append(
nits: you can use extend instead of relying on itertools.chain

https://codereview.chromium.org/2142293002/diff/100001/telemetry/telemetry/in...
telemetry/telemetry/internal/results/output_formatter.py:67: 
nits: remove the extra blank lines

https://codereview.chromium.org/2142293002/diff/100001/telemetry/telemetry/in...
File telemetry/telemetry/internal/results/page_test_results.py (right):

https://codereview.chromium.org/2142293002/diff/100001/telemetry/telemetry/in...
telemetry/telemetry/internal/results/page_test_results.py:160: def
has_tbmv2_results(self):
Do we still need to use this anywhere?

https://codereview.chromium.org/2142293002/diff/100001/telemetry/telemetry/va...
File telemetry/telemetry/value/__init__.py (right):

https://codereview.chromium.org/2142293002/diff/100001/telemetry/telemetry/va...
telemetry/telemetry/value/__init__.py:317: """Gives the tir_label that should be
applied to a merge of values.
s/Gives/Returns

[eakuefner, 2016-08-15 17:42:28]

Ned, PTAL.

https://codereview.chromium.org/2142293002/diff/100001/telemetry/telemetry/in...
File telemetry/telemetry/internal/results/chart_json_output_formatter.py
(right):

https://codereview.chromium.org/2142293002/diff/100001/telemetry/telemetry/in...
telemetry/telemetry/internal/results/chart_json_output_formatter.py:34:
output_formatter.SummarizeTimelineBasedResults(page_specific_values),
On 2016/08/15 at 13:40:57, nednguyen (ooo til 8-13) wrote:
> you mean  SummarizePageSpecificValues?

Done.

https://codereview.chromium.org/2142293002/diff/100001/telemetry/telemetry/in...
File telemetry/telemetry/internal/results/output_formatter.py (right):

https://codereview.chromium.org/2142293002/diff/100001/telemetry/telemetry/in...
telemetry/telemetry/internal/results/output_formatter.py:54: summary =
summary_module.Summary(page_specific_values)
On 2016/08/15 at 13:40:57, nednguyen (ooo til 8-13) wrote:
> For readability, maybe add a comment about by default, we summarize by both
name & tir_label.

Done.

https://codereview.chromium.org/2142293002/diff/100001/telemetry/telemetry/in...
telemetry/telemetry/internal/results/output_formatter.py:60: summaryByNameOnly =
summary_module.Summary(
On 2016/08/15 at 13:40:57, nednguyen (ooo til 8-13) wrote:
> summary_by_name_only, switching between python & js is tough ;)

Done. ~_~

https://codereview.chromium.org/2142293002/diff/100001/telemetry/telemetry/in...
telemetry/telemetry/internal/results/output_formatter.py:63: value_lists.append(
On 2016/08/15 at 13:40:57, nednguyen (ooo til 8-13) wrote:
> nits: you can use extend instead of relying on itertools.chain

Done.

https://codereview.chromium.org/2142293002/diff/100001/telemetry/telemetry/in...
telemetry/telemetry/internal/results/output_formatter.py:67: 
On 2016/08/15 at 13:40:57, nednguyen (ooo til 8-13) wrote:
> nits: remove the extra blank lines

Done.

https://codereview.chromium.org/2142293002/diff/100001/telemetry/telemetry/in...
File telemetry/telemetry/internal/results/page_test_results.py (right):

https://codereview.chromium.org/2142293002/diff/100001/telemetry/telemetry/in...
telemetry/telemetry/internal/results/page_test_results.py:160: def
has_tbmv2_results(self):
On 2016/08/15 at 13:40:57, nednguyen (ooo til 8-13) wrote:
> Do we still need to use this anywhere?

No, removed.

https://codereview.chromium.org/2142293002/diff/100001/telemetry/telemetry/va...
File telemetry/telemetry/value/__init__.py (right):

https://codereview.chromium.org/2142293002/diff/100001/telemetry/telemetry/va...
telemetry/telemetry/value/__init__.py:317: """Gives the tir_label that should be
applied to a merge of values.
On 2016/08/15 at 13:40:57, nednguyen (ooo til 8-13) wrote:
> s/Gives/Returns

Done.

[eakuefner, 2016-08-15 17:43:29]

Description was changed from

==========
[Telmetry] Also merge across tir_labels for TBMv2 benchmarks

This CL updates the summarization behavior of chart JSON to summarize twice:
once by name and tir_label (which is the current behavior), and once by name
only (so that merging across tir_labels will occur). The reason we want to do
this is to fill in the summary values for story sets with story keys, which we
abuse the tir_label field to store.

It also updates the merging code for SummarizableValues, which previously
assumed that merging would only occur for lists of values which all had the
same tir_label.

Currently, in the presence of IR labels, we only output these summaries:
* Per story/tir_label
* Per tir_label

With this change, we will additionally output, for TBMv2 benchmarks:
* Per story, across all tir_labels
* Per value, across all stories and tir_labels

BUG=chromium:624522
==========

to

==========
[Telemetry] Also merge across tir_labels for TBM

This CL updates the summarization behavior of chart JSON to summarize twice:
once by name and tir_label (which is the current behavior), and once by name
only (so that merging across tir_labels will occur). The reason we want to do
this is to fill in the summary values for story sets with story keys, which we
abuse the tir_label field to store.

It also updates the merging code for SummarizableValues, which previously
assumed that merging would only occur for lists of values which all had the
same tir_label.

Currently, in the presence of IR labels, we only output these summaries:
* Per story/tir_label
* Per tir_label

With this change, we will additionally output, for TBMv2 benchmarks:
* Per story, across all tir_labels
* Per value, across all stories and tir_labels

BUG=chromium:624522
==========

[nednguyen, 2016-08-15 17:46:28]

lgtm

[eakuefner, 2016-08-15 17:46:59]

The CQ bit was checked by eakuefner@chromium.org

[commit-bot: I haz the power, 2016-08-15 17:47:07]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[eakuefner, 2016-08-15 18:10:57]

The CQ bit was unchecked by eakuefner@chromium.org

[eakuefner, 2016-08-15 18:15:27]

The CQ bit was checked by eakuefner@chromium.org

[eakuefner, 2016-08-15 18:15:28]

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/2142293002/#ps140001
(title: "Fix failing tests")

[commit-bot: I haz the power, 2016-08-15 18:15:38]

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, 2016-08-15 19:17:27]

Description was changed from

==========
[Telemetry] Also merge across tir_labels for TBM

This CL updates the summarization behavior of chart JSON to summarize twice:
once by name and tir_label (which is the current behavior), and once by name
only (so that merging across tir_labels will occur). The reason we want to do
this is to fill in the summary values for story sets with story keys, which we
abuse the tir_label field to store.

It also updates the merging code for SummarizableValues, which previously
assumed that merging would only occur for lists of values which all had the
same tir_label.

Currently, in the presence of IR labels, we only output these summaries:
* Per story/tir_label
* Per tir_label

With this change, we will additionally output, for TBMv2 benchmarks:
* Per story, across all tir_labels
* Per value, across all stories and tir_labels

BUG=chromium:624522
==========

to

==========
[Telemetry] Also merge across tir_labels for TBM

This CL updates the summarization behavior of chart JSON to summarize twice:
once by name and tir_label (which is the current behavior), and once by name
only (so that merging across tir_labels will occur). The reason we want to do
this is to fill in the summary values for story sets with story keys, which we
abuse the tir_label field to store.

It also updates the merging code for SummarizableValues, which previously
assumed that merging would only occur for lists of values which all had the
same tir_label.

Currently, in the presence of IR labels, we only output these summaries:
* Per story/tir_label
* Per tir_label

With this change, we will additionally output, for TBMv2 benchmarks:
* Per story, across all tir_labels
* Per value, across all stories and tir_labels

BUG=chromium:624522

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

[commit-bot: I haz the power, 2016-08-15 19:17:28]

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