[telemetry] Add Html2OutputFormatter for generating results2.html

[telemetry] Add Html2OutputFormatter for generating results2.html

Currently, HtmlOutputFormatter generates results.html using chart_json.

TBMv2 is a completely new value system with a new json schema and affords a new UI.

This CL adds a separate Html2OutputFormatter that generates a new results2.html
using TBMv2 values, with a new UI.

Existing benchmarks will continue to generate results(1).html;
TBMv2 benchmarks will generate results2.html.

The new UI uses the same value-set-view as trace viewer's Metrics side panel.
The value-set-view is currently just a table, but will grow over time.
We're still working on the value-set-view design doc:
https://docs.google.com/document/d/1F7A0WRVC-aYHSz_rDnEBzDpjEDJl0_1Kjb9IKrpAhok/edit

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

[benjhayden, 2016-05-09 22:15:16]

Description was changed from

==========
results2.html
==========

to

==========
Add Html2OutputFormatter results2.html to telemetry
==========

[benjhayden, 2016-05-09 22:15:42]

Description was changed from

==========
Add Html2OutputFormatter results2.html to telemetry
==========

to

==========
Add Html2OutputFormatter results2.html to telemetry

WIP
==========

[benjhayden, 2016-05-09 22:16:01]

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

[benjhayden, 2016-05-09 22:16:07]

Patchset #1 (id:1) has been deleted

[benjhayden, 2016-05-09 22:16:15]

Patchset #1 (id:20001) has been deleted

[benjhayden, 2016-05-09 22:16:18]

Patchset #1 (id:40001) has been deleted

[benjhayden, 2016-05-09 22:55:45]

https://codereview.chromium.org/1964663003/diff/60001/tracing/tracing/results...
File tracing/tracing/results2html.html (right):

https://codereview.chromium.org/1964663003/diff/60001/tracing/tracing/results...
tracing/tracing/results2html.html:17: // TODO visualize values using
results-table
We don't yet know whether we want to use a value-set-view or a
value-set-sequence-view here.
If value-set-view, then this needs to wait for
https://codereview.chromium.org/1927723002

[benjhayden, 2016-05-17 20:02:21]

PTAL?

[eakuefner, 2016-05-17 20:13:38]

On 2016/05/17 at 20:02:21, benjhayden wrote:
> PTAL?

There's a really big problem here, which is that we need to preserve the
ValueSet instead of using PageTestResults, which is going to involve some fairly
major plumbing as I see it. We'll probably need to modify the OutputFormatter
abstraction to make this possible, and maybe add some more specialcasing to
SharedPageState.

Ben, would you be interested in pair programming on this? I think we'd both
learn a lot.

[benjhayden, 2016-05-17 20:38:32]

On 2016/05/17 at 20:13:38, eakuefner wrote:
> On 2016/05/17 at 20:02:21, benjhayden wrote:
> > PTAL?
> 
> There's a really big problem here, which is that we need to preserve the
ValueSet instead of using PageTestResults, which is going to involve some fairly
major plumbing as I see it. We'll probably need to modify the OutputFormatter
abstraction to make this possible, and maybe add some more specialcasing to
SharedPageState.
> 
> Ben, would you be interested in pair programming on this? I think we'd both
learn a lot.

I certainly do have a lot to learn about telemetry, and I'd be interested in
pair programming, but I thought that Ned had me preserve the ValueSet through
PageTestResults?

[eakuefner, 2016-05-17 20:59:25]

On 2016/05/17 at 20:38:32, benjhayden wrote:
> On 2016/05/17 at 20:13:38, eakuefner wrote:
> > On 2016/05/17 at 20:02:21, benjhayden wrote:
> > > PTAL?
> > 
> > There's a really big problem here, which is that we need to preserve the
ValueSet instead of using PageTestResults, which is going to involve some fairly
major plumbing as I see it. We'll probably need to modify the OutputFormatter
abstraction to make this possible, and maybe add some more specialcasing to
SharedPageState.
> > 
> > Ben, would you be interested in pair programming on this? I think we'd both
learn a lot.
> 
> I certainly do have a lot to learn about telemetry, and I'd be interested in
pair programming, but I thought that Ned had me preserve the ValueSet through
PageTestResults?

Ah, somehow I totally missed that! Probably the right approach here.

[eakuefner, 2016-05-17 20:59:51]

Overall, I think this looks good. I don't really have a strong opinion on
whether we should land this before doing more UI work.

https://codereview.chromium.org/1964663003/diff/80001/telemetry/telemetry/int...
File telemetry/telemetry/internal/results/html2_output_formatter.py (right):

https://codereview.chromium.org/1964663003/diff/80001/telemetry/telemetry/int...
telemetry/telemetry/internal/results/html2_output_formatter.py:1: # Copyright
2014 The Chromium Authors. All rights reserved.
nit: 2016

https://codereview.chromium.org/1964663003/diff/80001/tracing/tracing/metrics...
File tracing/tracing/metrics/value_set.html (right):

https://codereview.chromium.org/1964663003/diff/80001/tracing/tracing/metrics...
tracing/tracing/metrics/value_set.html:33: addValueSetDicts: function(dicts) {
Should this be addValueDicts?

https://codereview.chromium.org/1964663003/diff/80001/tracing/tracing/results...
File tracing/tracing/results2html.html (right):

https://codereview.chromium.org/1964663003/diff/80001/tracing/tracing/results...
tracing/tracing/results2html.html:3: Copyright (c) 2014 The Chromium Authors.
All rights reserved.
nit: 2016, no (c)

[benjhayden, 2016-05-17 22:27:04]

Description was changed from

==========
Add Html2OutputFormatter results2.html to telemetry

WIP
==========

to

==========
Add Html2OutputFormatter results2.html to telemetry
==========

[benjhayden, 2016-05-17 22:28:49]

Thanks, PTAL.

Seems to me like UI work would be parallel to this?

[eakuefner, 2016-05-17 22:31:20]

lgtm

[nednguyen, 2016-05-18 02:44:43]

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

[nednguyen, 2016-05-18 02:44:44]

Now I think about it, I think it could be ok to let results2html live in
tracing/ for the reason that we may support the html view for
"tracing/bin/run_metrics" script as well.

If so, I think we should move all the logic related to vulcanization from
html2_output_formatter.py to tracing/tracing/metrics/results2html.py.

+Petr

https://codereview.chromium.org/1964663003/diff/120001/telemetry/telemetry/in...
File telemetry/telemetry/internal/results/results_options.py (right):

https://codereview.chromium.org/1964663003/diff/120001/telemetry/telemetry/in...
telemetry/telemetry/internal/results/results_options.py:101: if output_format ==
'html':
I believe this should be "if output_format == 'html' or output_format ==
'html2':"

https://codereview.chromium.org/1964663003/diff/120001/tracing/tracing/result...
File tracing/tracing/results2html.html (right):

https://codereview.chromium.org/1964663003/diff/120001/tracing/tracing/result...
tracing/tracing/results2html.html:24: Telemetry will append script tags that
import ValueSets into window.values.
Remove the "Telemetry" & replace this with "results_view_generator"

[petrcermak, 2016-05-18 10:43:21]

Looks good overall. Could you please:

* add a patch description and
* change the title to "[telemetry] Add Html2OutputFormatter for generating
results.html"?

Thanks,
Petr

https://codereview.chromium.org/1964663003/diff/120001/telemetry/telemetry/in...
File telemetry/telemetry/internal/results/html2_output_formatter.py (right):

https://codereview.chromium.org/1964663003/diff/120001/telemetry/telemetry/in...
telemetry/telemetry/internal/results/html2_output_formatter.py:10: from
catapult_base import cloud_storage  # pylint: disable=import-error
Why does this trigger an import error?

https://codereview.chromium.org/1964663003/diff/120001/telemetry/telemetry/in...
telemetry/telemetry/internal/results/html2_output_formatter.py:26:
self._output_filename = output_stream.name
Is there any point in storing this when you can access it via
self._output_stream.name? I would suggest adding a getter instead.

https://codereview.chromium.org/1964663003/diff/120001/telemetry/telemetry/in...
telemetry/telemetry/internal/results/html2_output_formatter.py:31: # TODO move
results2html to telemetry and use source paths magic from
nit: should be "TODO(username):" or at least "TODO: "

https://codereview.chromium.org/1964663003/diff/120001/telemetry/telemetry/in...
telemetry/telemetry/internal/results/html2_output_formatter.py:32: #
perf_insights_project
nit: Add a period

https://codereview.chromium.org/1964663003/diff/120001/telemetry/telemetry/in...
telemetry/telemetry/internal/results/html2_output_formatter.py:38: with
file(self._output_filename, 'a') as htmlfp:
out of curiosity, what does "htmlfp" abbreviate? Why not just use "html_file" or
simply "f"?

https://codereview.chromium.org/1964663003/diff/120001/telemetry/telemetry/in...
telemetry/telemetry/internal/results/html2_output_formatter.py:39:
htmlfp.write('\n<script>\nvalues.addValueDicts(' +
the following might be more readable:

htmlfp.write('\n'.join([
    '',
    '<script>',
    'values.addValueDicts(%s);' % json.dumps(...),
    '</script>',
    ''
]))

Alternatively, I would at least suggest using %s and %.

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

https://codereview.chromium.org/1964663003/diff/120001/telemetry/telemetry/in...
telemetry/telemetry/internal/results/page_test_results.py:65: self._value_set =
[]
You would expect a value set to be a set :-)

https://codereview.chromium.org/1964663003/diff/120001/telemetry/telemetry/va...
File telemetry/telemetry/value/trace.py (right):

https://codereview.chromium.org/1964663003/diff/120001/telemetry/telemetry/va...
telemetry/telemetry/value/trace.py:5: import StringIO
As far as I can tell, the sort order doesn't take case into account (I couldn't
find a single file in Catapult that would import StringIO before other modules).
Having said that, if the linter requires this, then I of course understand the
change.

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

https://codereview.chromium.org/1964663003/diff/120001/telemetry/telemetry/we...
telemetry/telemetry/web_perf/timeline_based_measurement.py:312: value_dicts =
mre_result.pairs.get('values', [])
No need to use |get| here anymore (if 'values' are not presont, this method will
fail anyway on line 305).

https://codereview.chromium.org/1964663003/diff/120001/tracing/tracing/result...
File tracing/tracing/results2html.html (right):

https://codereview.chromium.org/1964663003/diff/120001/tracing/tracing/result...
tracing/tracing/results2html.html:1: <!DOCTYPE html>
Why is this file called results2html.html? Wouldn't something like
results2_template.html make more sense?

https://codereview.chromium.org/1964663003/diff/120001/tracing/tracing/result...
tracing/tracing/results2html.html:16: window.values = new
tr.metrics.ValueList();
Shouldn't this be ValueSet?

https://codereview.chromium.org/1964663003/diff/120001/tracing/tracing/result...
tracing/tracing/results2html.html:20: valueSetView.values = values;
Please use |window.values| to make it explicit where the variable comes from.

[benjhayden, 2016-05-18 16:22:22]

Description was changed from

==========
Add Html2OutputFormatter results2.html to telemetry
==========

to

==========
[telemetry] Add Html2OutputFormatter for generating results2.html

Currently, HtmlOutputFormatter generates results.html using chart_json.

TBMv2 is a completely new value system with a new json schema and affords a new
UI.

This CL adds a separate Html2OutputFormatter that generates a new results2.html
using TBMv2 values, with a new UI.

Existing benchmarks will continue to generate results(1).html;
TBMv2 benchmarks will generate results2.html.

The new UI uses the same value-set-view as trace viewer's Metrics side panel.
The value-set-view is currently just a table, but will grow over time.
We're still working on the value-set-view design doc.
==========

[benjhayden, 2016-05-18 18:23:34]

Thanks, Petr! PTAL.

https://codereview.chromium.org/1964663003/diff/120001/telemetry/telemetry/in...
File telemetry/telemetry/internal/results/html2_output_formatter.py (right):

https://codereview.chromium.org/1964663003/diff/120001/telemetry/telemetry/in...
telemetry/telemetry/internal/results/html2_output_formatter.py:10: from
catapult_base import cloud_storage  # pylint: disable=import-error
On 2016/05/18 at 10:43:20, petrcermak wrote:
> Why does this trigger an import error?

Copy-pasta. presubmit seems ok without it.

https://codereview.chromium.org/1964663003/diff/120001/telemetry/telemetry/in...
telemetry/telemetry/internal/results/html2_output_formatter.py:26:
self._output_filename = output_stream.name
On 2016/05/18 at 10:43:20, petrcermak wrote:
> Is there any point in storing this when you can access it via
self._output_stream.name? I would suggest adding a getter instead.

This is a bit of a work-around for how this output_stream is constructed.
This could be simplified if the existing HtmlOutputFormatter is removed.

https://codereview.chromium.org/1964663003/diff/120001/telemetry/telemetry/in...
telemetry/telemetry/internal/results/html2_output_formatter.py:31: # TODO move
results2html to telemetry and use source paths magic from
On 2016/05/18 at 10:43:20, petrcermak wrote:
> nit: should be "TODO(username):" or at least "TODO: "

I'm not sure who will do this or when.

https://codereview.chromium.org/1964663003/diff/120001/telemetry/telemetry/in...
telemetry/telemetry/internal/results/html2_output_formatter.py:32: #
perf_insights_project
On 2016/05/18 at 10:43:20, petrcermak wrote:
> nit: Add a period

Done.

https://codereview.chromium.org/1964663003/diff/120001/telemetry/telemetry/in...
telemetry/telemetry/internal/results/html2_output_formatter.py:38: with
file(self._output_filename, 'a') as htmlfp:
On 2016/05/18 at 10:43:20, petrcermak wrote:
> out of curiosity, what does "htmlfp" abbreviate? Why not just use "html_file"
or simply "f"?

"file pointer" from C, but "f" sgtm.

https://codereview.chromium.org/1964663003/diff/120001/telemetry/telemetry/in...
telemetry/telemetry/internal/results/html2_output_formatter.py:39:
htmlfp.write('\n<script>\nvalues.addValueDicts(' +
On 2016/05/18 at 10:43:20, petrcermak wrote:
> the following might be more readable:
> 
> htmlfp.write('\n'.join([
>     '',
>     '<script>',
>     'values.addValueDicts(%s);' % json.dumps(...),
>     '</script>',
>     ''
> ]))
> 
> Alternatively, I would at least suggest using %s and %.

Done.

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

https://codereview.chromium.org/1964663003/diff/120001/telemetry/telemetry/in...
telemetry/telemetry/internal/results/page_test_results.py:65: self._value_set =
[]
On 2016/05/18 at 10:43:20, petrcermak wrote:
> You would expect a value set to be a set :-)

commented.

https://codereview.chromium.org/1964663003/diff/120001/telemetry/telemetry/va...
File telemetry/telemetry/value/trace.py (right):

https://codereview.chromium.org/1964663003/diff/120001/telemetry/telemetry/va...
telemetry/telemetry/value/trace.py:5: import StringIO
On 2016/05/18 at 10:43:20, petrcermak wrote:
> As far as I can tell, the sort order doesn't take case into account (I
couldn't find a single file in Catapult that would import StringIO before other
modules). Having said that, if the linter requires this, then I of course
understand the change.

cruft from an unrelated change.

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

https://codereview.chromium.org/1964663003/diff/120001/telemetry/telemetry/we...
telemetry/telemetry/web_perf/timeline_based_measurement.py:312: value_dicts =
mre_result.pairs.get('values', [])
On 2016/05/18 at 10:43:20, petrcermak wrote:
> No need to use |get| here anymore (if 'values' are not presont, this method
will fail anyway on line 305).

I think that's a bug in the change.

https://codereview.chromium.org/1964663003/diff/120001/tracing/tracing/result...
File tracing/tracing/results2html.html (right):

https://codereview.chromium.org/1964663003/diff/120001/tracing/tracing/result...
tracing/tracing/results2html.html:1: <!DOCTYPE html>
On 2016/05/18 at 10:43:20, petrcermak wrote:
> Why is this file called results2html.html? Wouldn't something like
results2_template.html make more sense?

I was following the lead of trace2html.html. I'm not sure how to figure out what
the name should be.
I'd think that "_template.html" would contain layout slugs to be replaced (which
this does not), and would not need vulcanization (which this does).
But if you're sure that "results2_template.html" is the right name, then I'd be
happy to change it. :-)

https://codereview.chromium.org/1964663003/diff/120001/tracing/tracing/result...
tracing/tracing/results2html.html:16: window.values = new
tr.metrics.ValueList();
On 2016/05/18 at 10:43:21, petrcermak wrote:
> Shouldn't this be ValueSet?

Done.

https://codereview.chromium.org/1964663003/diff/120001/tracing/tracing/result...
tracing/tracing/results2html.html:20: valueSetView.values = values;
On 2016/05/18 at 10:43:20, petrcermak wrote:
> Please use |window.values| to make it explicit where the variable comes from.

Done.

https://codereview.chromium.org/1964663003/diff/120001/tracing/tracing/result...
tracing/tracing/results2html.html:24: Telemetry will append script tags that
import ValueSets into window.values.
On 2016/05/18 at 02:44:44, nednguyen wrote:
> Remove the "Telemetry" & replace this with "results_view_generator"

I couldn't find "results_view_generator". Html2OutputFormatter?

[eakuefner, 2016-05-18 18:39:55]

https://codereview.chromium.org/1964663003/diff/120001/tracing/tracing/result...
File tracing/tracing/results2html.html (right):

https://codereview.chromium.org/1964663003/diff/120001/tracing/tracing/result...
tracing/tracing/results2html.html:1: <!DOCTYPE html>
On 2016/05/18 at 18:23:33, benjhayden_chromium wrote:
> On 2016/05/18 at 10:43:20, petrcermak wrote:
> > Why is this file called results2html.html? Wouldn't something like
results2_template.html make more sense?
> 
> I was following the lead of trace2html.html. I'm not sure how to figure out
what the name should be.
> I'd think that "_template.html" would contain layout slugs to be replaced
(which this does not), and would not need vulcanization (which this does).
> But if you're sure that "results2_template.html" is the right name, then I'd
be happy to change it. :-)

I, too, find the name results2html.html confusing. I'd prefer
results2_template.html.

https://codereview.chromium.org/1964663003/diff/120001/tracing/tracing/result...
tracing/tracing/results2html.html:24: Telemetry will append script tags that
import ValueSets into window.values.
On 2016/05/18 at 18:23:33, benjhayden_chromium wrote:
> On 2016/05/18 at 02:44:44, nednguyen wrote:
> > Remove the "Telemetry" & replace this with "results_view_generator"
> 
> I couldn't find "results_view_generator". Html2OutputFormatter?

Actually, this points at a broader design issue -- we should consider exposing
the Html2OutputFormatter logic through run_metric as well. Maybe add a module to
tracing/metrics (results_view_builder maybe better than results_view_generator?)
and then make sure that it's possible to call that from run_metric?

[petrcermak, 2016-05-18 19:05:41]

LGTM with two more comments (as long as others are happy).

Please add a link to the design doc to the patch description.

Thanks,
Petr

https://codereview.chromium.org/1964663003/diff/140001/telemetry/telemetry/in...
File telemetry/telemetry/internal/results/html2_output_formatter.py (right):

https://codereview.chromium.org/1964663003/diff/140001/telemetry/telemetry/in...
telemetry/telemetry/internal/results/html2_output_formatter.py:46: '',
nit: I think this should be indented 4 spaces (+2)

https://codereview.chromium.org/1964663003/diff/140001/telemetry/telemetry/in...
telemetry/telemetry/internal/results/html2_output_formatter.py:49:
'</script>']))
FYI: Unless you add another empty string to the end of the list, there will NOT
be a newline character appended to the end.

[benjhayden, 2016-05-18 19:25:40]

Description was changed from

==========
[telemetry] Add Html2OutputFormatter for generating results2.html

Currently, HtmlOutputFormatter generates results.html using chart_json.

TBMv2 is a completely new value system with a new json schema and affords a new
UI.

This CL adds a separate Html2OutputFormatter that generates a new results2.html
using TBMv2 values, with a new UI.

Existing benchmarks will continue to generate results(1).html;
TBMv2 benchmarks will generate results2.html.

The new UI uses the same value-set-view as trace viewer's Metrics side panel.
The value-set-view is currently just a table, but will grow over time.
We're still working on the value-set-view design doc.
==========

to

==========
[telemetry] Add Html2OutputFormatter for generating results2.html

Currently, HtmlOutputFormatter generates results.html using chart_json.

TBMv2 is a completely new value system with a new json schema and affords a new
UI.

This CL adds a separate Html2OutputFormatter that generates a new results2.html
using TBMv2 values, with a new UI.

Existing benchmarks will continue to generate results(1).html;
TBMv2 benchmarks will generate results2.html.

The new UI uses the same value-set-view as trace viewer's Metrics side panel.
The value-set-view is currently just a table, but will grow over time.
We're still working on the value-set-view design doc:
https://docs.google.com/document/d/1F7A0WRVC-aYHSz_rDnEBzDpjEDJl0_1Kjb9IKrpAh...
==========

[benjhayden, 2016-05-18 19:28:44]

Thanks!

https://codereview.chromium.org/1964663003/diff/140001/telemetry/telemetry/in...
File telemetry/telemetry/internal/results/html2_output_formatter.py (right):

https://codereview.chromium.org/1964663003/diff/140001/telemetry/telemetry/in...
telemetry/telemetry/internal/results/html2_output_formatter.py:46: '',
On 2016/05/18 at 19:05:40, petrcermak wrote:
> nit: I think this should be indented 4 spaces (+2)

Done.

https://codereview.chromium.org/1964663003/diff/140001/telemetry/telemetry/in...
telemetry/telemetry/internal/results/html2_output_formatter.py:49:
'</script>']))
On 2016/05/18 at 19:05:40, petrcermak wrote:
> FYI: Unless you add another empty string to the end of the list, there will
NOT be a newline character appended to the end.

Done.

[eakuefner, 2016-05-18 19:34:48]

https://codereview.chromium.org/1964663003/diff/120001/telemetry/telemetry/in...
File telemetry/telemetry/internal/results/html2_output_formatter.py (right):

https://codereview.chromium.org/1964663003/diff/120001/telemetry/telemetry/in...
telemetry/telemetry/internal/results/html2_output_formatter.py:31: # TODO move
results2html to telemetry and use source paths magic from
On 2016/05/18 at 18:23:33, benjhayden_chromium wrote:
> On 2016/05/18 at 10:43:20, petrcermak wrote:
> > nit: should be "TODO(username):" or at least "TODO: "
> 
> I'm not sure who will do this or when.

In this case I'd usually do TODO(#bug).

[benjhayden, 2016-05-18 20:03:55]

https://codereview.chromium.org/1964663003/diff/120001/telemetry/telemetry/in...
File telemetry/telemetry/internal/results/html2_output_formatter.py (right):

https://codereview.chromium.org/1964663003/diff/120001/telemetry/telemetry/in...
telemetry/telemetry/internal/results/html2_output_formatter.py:31: # TODO move
results2html to telemetry and use source paths magic from
On 2016/05/18 at 19:34:48, eakuefner wrote:
> On 2016/05/18 at 18:23:33, benjhayden_chromium wrote:
> > On 2016/05/18 at 10:43:20, petrcermak wrote:
> > > nit: should be "TODO(username):" or at least "TODO: "
> > 
> > I'm not sure who will do this or when.
> 
> In this case I'd usually do TODO(#bug).

Done.

[nednguyen, 2016-05-18 20:55:11]

lgtm

[nednguyen, 2016-05-18 20:56:05]

https://codereview.chromium.org/1964663003/diff/220001/telemetry/telemetry/in...
File telemetry/telemetry/internal/results/html2_output_formatter.py (right):

https://codereview.chromium.org/1964663003/diff/220001/telemetry/telemetry/in...
telemetry/telemetry/internal/results/html2_output_formatter.py:35: project =
tracing_project.TracingProject()
We should move this vulcanization code to tracing/ but this can be done in
subsequent patch

[benjhayden, 2016-05-18 20:56:14]

On 2016/05/18 at 20:03:55, benjhayden_chromium wrote:
>
https://codereview.chromium.org/1964663003/diff/120001/telemetry/telemetry/in...
> File telemetry/telemetry/internal/results/html2_output_formatter.py (right):
> 
>
https://codereview.chromium.org/1964663003/diff/120001/telemetry/telemetry/in...
> telemetry/telemetry/internal/results/html2_output_formatter.py:31: # TODO move
results2html to telemetry and use source paths magic from
> On 2016/05/18 at 19:34:48, eakuefner wrote:
> > On 2016/05/18 at 18:23:33, benjhayden_chromium wrote:
> > > On 2016/05/18 at 10:43:20, petrcermak wrote:
> > > > nit: should be "TODO(username):" or at least "TODO: "
> > > 
> > > I'm not sure who will do this or when.
> > 
> > In this case I'd usually do TODO(#bug).
> 
> Done.

Ethan suggested that maybe results2_template.html should not live in telemetry,
since it could also be used by run_metric.
So I removed the TODO and closed the bug. Feel free to reopen it if you still
think it belongs in telemetry.

[benjhayden, 2016-05-19 17:26:02]

I did some debugging and fixed a few small things here and there that don't seem
worth splitting to another CL. I asked Ethan to take a look, so the rest of you
feel free to either look again or carry on as you wish.

[eakuefner, 2016-05-19 17:30:56]

https://codereview.chromium.org/1964663003/diff/340001/telemetry/telemetry/va...
File telemetry/telemetry/value/scalar.py (right):

https://codereview.chromium.org/1964663003/diff/340001/telemetry/telemetry/va...
telemetry/telemetry/value/scalar.py:73: # Infinity and NaN are left out of JSON
for security reasons that do not
In Telemetry, we don't support string-valued scalars like this, and this is
likely to cause 400s on the dashboard. Instead you set value to None and set
none_value_reason to something like 'Value was inf/-inf/nan'.

This is, as I understand apocryphally, based on a mishearing of "NaN" once upon
a time as "None", so now this value system has these "NoneValues" when really we
should have just developed a scheme for handling nan/inf like you propose now.

Sigh.

https://codereview.chromium.org/1964663003/diff/340001/telemetry/telemetry/va...
telemetry/telemetry/value/scalar.py:93: # Infinity and NaN are left out of JSON
for security reasons that do not
Here I'd write an additional comment that says 'TBMv2 metrics can emit these
strings to describe infinity and nan' to show that this is a hack to accommodate
tbmv2.

https://codereview.chromium.org/1964663003/diff/340001/telemetry/telemetry/va...
telemetry/telemetry/value/scalar.py:95: if value_dict['value'] == 'Infinity':
And flatten all these cases to this block:

if ...:
  kwargs['value'] = None
  kwargs['none_value_reason'] = 'Value was ' + value_dict['value']

[eakuefner, 2016-05-19 17:32:09]

https://codereview.chromium.org/1964663003/diff/340001/telemetry/telemetry/va...
File telemetry/telemetry/value/scalar.py (right):

https://codereview.chromium.org/1964663003/diff/340001/telemetry/telemetry/va...
telemetry/telemetry/value/scalar.py:93: # Infinity and NaN are left out of JSON
for security reasons that do not
On 2016/05/19 at 17:30:56, eakuefner wrote:
> Here I'd write an additional comment that says 'TBMv2 metrics can emit these
strings to describe infinity and nan' to show that this is a hack to accommodate
tbmv2.

And to be clear, I'm not married to the wording; please feel free to innovate on
grammar and spelling.

[eakuefner, 2016-05-19 17:33:04]

https://codereview.chromium.org/1964663003/diff/340001/telemetry/telemetry/va...
File telemetry/telemetry/value/scalar.py (right):

https://codereview.chromium.org/1964663003/diff/340001/telemetry/telemetry/va...
telemetry/telemetry/value/scalar.py:73: # Infinity and NaN are left out of JSON
for security reasons that do not
On 2016/05/19 at 17:30:56, eakuefner wrote:
> In Telemetry, we don't support string-valued scalars like this, and this is
likely to cause 400s on the dashboard. Instead you set value to None and set
none_value_reason to something like 'Value was inf/-inf/nan'.
> 
> This is, as I understand apocryphally, based on a mishearing of "NaN" once
upon a time as "None", so now this value system has these "NoneValues" when
really we should have just developed a scheme for handling nan/inf like you
propose now.
> 
> Sigh.

Sorry, to be even more clear, I mean that you should probably delete this code
in light of the hack that I'm asking you to introduce in FromDict.

[benjhayden, 2016-05-19 21:53:06]

On 2016/05/19 at 17:33:04, eakuefner wrote:
>
https://codereview.chromium.org/1964663003/diff/340001/telemetry/telemetry/va...
> File telemetry/telemetry/value/scalar.py (right):
> 
>
https://codereview.chromium.org/1964663003/diff/340001/telemetry/telemetry/va...
> telemetry/telemetry/value/scalar.py:73: # Infinity and NaN are left out of
JSON for security reasons that do not
> On 2016/05/19 at 17:30:56, eakuefner wrote:
> > In Telemetry, we don't support string-valued scalars like this, and this is
likely to cause 400s on the dashboard. Instead you set value to None and set
none_value_reason to something like 'Value was inf/-inf/nan'.
> > 
> > This is, as I understand apocryphally, based on a mishearing of "NaN" once
upon a time as "None", so now this value system has these "NoneValues" when
really we should have just developed a scheme for handling nan/inf like you
propose now.
> > 
> > Sigh.
> 
> Sorry, to be even more clear, I mean that you should probably delete this code
in light of the hack that I'm asking you to introduce in FromDict.

Thanks! PTAL?

[eakuefner, 2016-05-19 22:03:25]

lgtm

[benjhayden, 2016-05-19 22:28:36]

The CQ bit was checked by benjhayden@chromium.org

[benjhayden, 2016-05-19 22:28:37]

The patchset sent to the CQ was uploaded after l-g-t-m from
petrcermak@chromium.org, nednguyen@google.com
Link to the patchset: https://codereview.chromium.org/1964663003/#ps360001
(title: "comments")

[commit-bot: I haz the power, 2016-05-19 22:28:45]

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

[commit-bot: I haz the power, 2016-05-19 22:51:13]

Description was changed from

==========
[telemetry] Add Html2OutputFormatter for generating results2.html

Currently, HtmlOutputFormatter generates results.html using chart_json.

TBMv2 is a completely new value system with a new json schema and affords a new
UI.

This CL adds a separate Html2OutputFormatter that generates a new results2.html
using TBMv2 values, with a new UI.

Existing benchmarks will continue to generate results(1).html;
TBMv2 benchmarks will generate results2.html.

The new UI uses the same value-set-view as trace viewer's Metrics side panel.
The value-set-view is currently just a table, but will grow over time.
We're still working on the value-set-view design doc:
https://docs.google.com/document/d/1F7A0WRVC-aYHSz_rDnEBzDpjEDJl0_1Kjb9IKrpAh...
==========

to

==========
[telemetry] Add Html2OutputFormatter for generating results2.html

Currently, HtmlOutputFormatter generates results.html using chart_json.

TBMv2 is a completely new value system with a new json schema and affords a new
UI.

This CL adds a separate Html2OutputFormatter that generates a new results2.html
using TBMv2 values, with a new UI.

Existing benchmarks will continue to generate results(1).html;
TBMv2 benchmarks will generate results2.html.

The new UI uses the same value-set-view as trace viewer's Metrics side panel.
The value-set-view is currently just a table, but will grow over time.
We're still working on the value-set-view design doc:
https://docs.google.com/document/d/1F7A0WRVC-aYHSz_rDnEBzDpjEDJl0_1Kjb9IKrpAh...

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

[commit-bot: I haz the power, 2016-05-19 22:51:14]

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