Added support for improvement_direction to relevant values, which is propogated to chartjson.

Added support for higher_is_better to relevant values, which is propogated to chartjson.

This should get automatically picked up by the perf dashboard, making unit-info.json unneccesary.

Also added higher_is_better to most tests in tools/perf.
BUG=

[sullivan, 2014-12-18 17:03:12]

sullivan@chromium.org changed reviewers:
+ tonyg@chromium.org

[sullivan, 2014-12-18 17:03:13]

I currently have this all in one pass. Would it be better to split up the
tools/telemetry changes into one CL and the tools/perf changes into a second?

[tonyg, 2014-12-18 17:31:28]

tonyg@chromium.org changed reviewers:
+ dtu@chromium.org

[tonyg, 2014-12-18 17:31:35]

https://codereview.chromium.org/809393002/diff/1/tools/perf/benchmarks/dom_pe...
File tools/perf/benchmarks/dom_perf.py (right):

https://codereview.chromium.org/809393002/diff/1/tools/perf/benchmarks/dom_pe...
tools/perf/benchmarks/dom_perf.py:55: higher_is_better=True))
For anything that is summarizable into a numeric value, I don't think we should
leave this parameter optional. There's no good way to check that we've
considered all of the cases in this CL and in the future, folks might forget to
specify.

That being said, we also don't want to have a naked boolean in the API. A couple
of ideas are:
- We could assert that the field is passed by name and continue to use a bool
- We could assert a different type besides bool that has a more readable name

https://codereview.chromium.org/809393002/diff/1/tools/telemetry/telemetry/va...
File tools/telemetry/telemetry/value/__init__.py (right):

https://codereview.chromium.org/809393002/diff/1/tools/telemetry/telemetry/va...
tools/telemetry/telemetry/value/__init__.py:61: regression (False). None if
unknown.
Thinking aloud here... not all Value subclasses have a strict numerical
representation -- for instance FailureValue, StringValue, TraceValue. So that
suggests that higher_is_better doesn't belong in the Value class. There's
probably a variety of solutions, but maybe we're missing a SummarizableValue
class in the hierarchy? The thing to figure out then is whether the stuff in
Summary moves into SummarizableValue or whether it asserts that it only operates
on SummarizableValues (I haven't explored the relationship enough to make a
suggestion).

[sullivan, 2014-12-23 22:48:03]

On 2014/12/18 17:31:35, tonyg (OOO til Jan 5) wrote:
>
https://codereview.chromium.org/809393002/diff/1/tools/perf/benchmarks/dom_pe...
> File tools/perf/benchmarks/dom_perf.py (right):
> 
>
https://codereview.chromium.org/809393002/diff/1/tools/perf/benchmarks/dom_pe...
> tools/perf/benchmarks/dom_perf.py:55: higher_is_better=True))
> For anything that is summarizable into a numeric value, I don't think we
should
> leave this parameter optional. There's no good way to check that we've
> considered all of the cases in this CL and in the future, folks might forget
to
> specify.
> 
> That being said, we also don't want to have a naked boolean in the API. A
couple
> of ideas are:
> - We could assert that the field is passed by name and continue to use a bool
> - We could assert a different type besides bool that has a more readable name
> 
>
https://codereview.chromium.org/809393002/diff/1/tools/telemetry/telemetry/va...
> File tools/telemetry/telemetry/value/__init__.py (right):
> 
>
https://codereview.chromium.org/809393002/diff/1/tools/telemetry/telemetry/va...
> tools/telemetry/telemetry/value/__init__.py:61: regression (False). None if
> unknown.
> Thinking aloud here... not all Value subclasses have a strict numerical
> representation -- for instance FailureValue, StringValue, TraceValue. So that
> suggests that higher_is_better doesn't belong in the Value class. There's
> probably a variety of solutions, but maybe we're missing a SummarizableValue
> class in the hierarchy? The thing to figure out then is whether the stuff in
> Summary moves into SummarizableValue or whether it asserts that it only
operates
> on SummarizableValues (I haven't explored the relationship enough to make a
> suggestion).

I uploaded a new patchset, which doesn't finish the implementation, but takes a
new approach:

* Adds a new SummarizableValue class, which summarizable values inherit from
* Adds an assertion in Summary that all values being summarized are
summarizable. I think Summary should still be separate from SummarizableValue
since Summary works on multiple values.
* Changes from a bool to an enum improvement_direction. The new enum name is so
long, and feels kind of clunky. I was wondering if there is a better way to do
an enum that fits in with telemetry style? Maybe just two constants in
summarizable module instead of a class?

Note: this implementation is not ready for full review, wanted to get feedback
on the above points before completing it.

[qyearsley, 2014-12-29 20:23:55]

On 2014/12/23 22:48:03, sullivan wrote:
> * Adds an assertion in Summary that all values being summarized are
> summarizable. I think Summary should still be separate from SummarizableValue
> since Summary works on multiple values.

Is it the case that a SummarizableValue can't necessarily be converted to a
scalar by itself, but it at least has to be combinable with other values (of the
same time) to be converted to a scalar?

> * Changes from a bool to an enum improvement_direction. The new enum name is
so
> long, and feels kind of clunky. I was wondering if there is a better way to do
> an enum that fits in with telemetry style? Maybe just two constants in
> summarizable module instead of a class?

One possiblity: Make a separate (very very small) module named
improvement_diction which just contains two public constants.

[qyearsley, 2014-12-29 20:25:02]

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

[qyearsley, 2014-12-29 20:25:04]

https://codereview.chromium.org/809393002/diff/20001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/results/buildbot_output_formatter_unittest.py
(right):

https://codereview.chromium.org/809393002/diff/20001/tools/telemetry/telemetr...
tools/telemetry/telemetry/results/buildbot_output_formatter_unittest.py:31: def
test_basic_summary(self):
Unrelated to this CL:

Method names for a few of the unit tests are inconsistent with the rest of the
method names in Telemetry.

Most of Telemetry (including most unit tests) uses CamelCase, but this module
and chart_json_output_formatter_unittest.py use lowercase_with_underscore.

https://codereview.chromium.org/809393002/diff/20001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/results/json_output_formatter_unittest.py
(right):

https://codereview.chromium.org/809393002/diff/20001/tools/telemetry/telemetr...
tools/telemetry/telemetry/results/json_output_formatter_unittest.py:104: 
Extra blank lines.

https://codereview.chromium.org/809393002/diff/20001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/value/histogram.py (right):

https://codereview.chromium.org/809393002/diff/20001/tools/telemetry/telemetr...
tools/telemetry/telemetry/value/histogram.py:9: from telemetry.value import
summarizable as summarizable_module
I think it's probably unnecessary to import summarizable with a different name
here.

https://codereview.chromium.org/809393002/diff/20001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/value/summarizable.py (right):

https://codereview.chromium.org/809393002/diff/20001/tools/telemetry/telemetr...
tools/telemetry/telemetry/value/summarizable.py:6: 
Could add another blank line before first top-level class.

https://codereview.chromium.org/809393002/diff/20001/tools/telemetry/telemetr...
tools/telemetry/telemetry/value/summarizable.py:9: DOWN = 'down'
I guess it's possible that later we might want to support some kind of "none"
improvement direction, where it's not clear where higher is better or worse.

But that could be added later if we need it, and it's good to leave this as-is
for now.

[tonyg, 2015-01-05 17:37:21]

New approach looks great. PLMK when it's ready for another round.

https://codereview.chromium.org/809393002/diff/20001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/value/summarizable.py (right):

https://codereview.chromium.org/809393002/diff/20001/tools/telemetry/telemetr...
tools/telemetry/telemetry/value/summarizable.py:7: class ImprovementDirection:
One idea for fixing the callsite verbosity is to move these to a new file and
inline them (like core/wpr_modes.py). Then the callsite would simply be
"improvement_direction.DOWN"

[dtu, 2015-01-06 01:47:30]

https://codereview.chromium.org/809393002/diff/20001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/value/summarizable.py (right):

https://codereview.chromium.org/809393002/diff/20001/tools/telemetry/telemetr...
tools/telemetry/telemetry/value/summarizable.py:7: class ImprovementDirection:
On 2015/01/05 17:37:21, tonyg wrote:
> One idea for fixing the callsite verbosity is to move these to a new file and
> inline them (like core/wpr_modes.py). Then the callsite would simply be
> "improvement_direction.DOWN"

sgtm. It's apparent from the name (summarizable.ImprovementDirection.DOWN) that
it doesn't make sense in this module anyway.

https://codereview.chromium.org/809393002/diff/20001/tools/telemetry/telemetr...
tools/telemetry/telemetry/value/summarizable.py:12: class
SummarizableValue(value_module.Value):
I don't know that much about the Values system, but having SummarizableValue
seems pretty clean to me.

[sullivan, 2015-01-06 23:04:23]

Another pass at this, it's not quite complete but brought up a few more
questions:

1) I get test failures because I need to make TraceValue summarizable. Does a
TraceValue have an improvement_direction?

2) How should I find the improvement_direction in blink_perf.py -- modify the
blink_perf tests in blink?

3) Not sure if I got the direction right in:
fast_metric.py
rasterize_and_record.py
screenshot.py
skpicture_printer.py

[sullivan, 2015-01-07 21:16:52]

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

[sullivan, 2015-01-07 21:16:53]

+nednguyen

Ned, this is a super-long CL, but the big changes are:

Add a summarizable value class and make it have an improvement direction:
tools/telemetry/telemetry/value/summarizable.py

Assert that only summarizable values are passed into Summary:
tools/telemetry/telemetry/value/summary.py

The thought was that summarizable values represent scalar values, and scalar
values should have an improvement direction. But as I was working I realized
that maybe this isn't right. Specific concerns:

1) TraceValue is summariz(ed) in tools/perf/unittests, and it doesn't have a
scalar value/improvement direction IIUC

2) StringValue/ListOfStringValue are summarizable, but I don't think have an
improvement_direction.

3) I'm not sure if there's any special handling necessary for merging failures.

So I think we should take a step back and understand better how this *should*
work.

a) Is the SummarizableValue class still a good idea? If so, I think I should
move the merging methods from Value into SummarizableValue, and make
StringValue/ListOfStringValue also inherit SummarizableValue

b) Is inheritance the right way to share code for having an
improvement_direction? I think what I want to do is assert that ScalarValue,
ListOfScalarValues, and HistogramValue have an improvement direction specified.
(Let me know if I got that wrong!)

[nednguyen, 2015-01-08 18:07:42]

Hi Annie,
Looks like there are 2 output_formatters that are using summary:
chart_json_output_formatter & buildbot_output_formatter.

When I look at the code of chart_json_output_formatter, it looks to me that it
feeds all the page_test_results.all_page_specific_values to the summary
constructor.

This can be a problem if all_page_specific_values contain the trace value.

https://codereview.chromium.org/809393002/diff/60001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/value/scalar.py (right):

https://codereview.chromium.org/809393002/diff/60001/tools/telemetry/telemetr...
tools/telemetry/telemetry/value/scalar.py:110:
improvement_direction=v0.improvement_direction)
This is the ugliness that I mentioned. How do we guarantee that all values to be
merge have the same improvement_direction? :-(

It's fine if you want to punt this issue.

[nednguyen, 2015-01-08 18:08:03]

On 2015/01/08 18:07:42, nednguyen wrote:
> Hi Annie,
> Looks like there are 2 output_formatters that are using summary:
> chart_json_output_formatter & buildbot_output_formatter.
> 
> When I look at the code of chart_json_output_formatter, it looks to me that it
> feeds all the page_test_results.all_page_specific_values to the summary
> constructor.
> 
> This can be a problem if all_page_specific_values contain the trace value.
> 
>
https://codereview.chromium.org/809393002/diff/60001/tools/telemetry/telemetr...
> File tools/telemetry/telemetry/value/scalar.py (right):
> 
>
https://codereview.chromium.org/809393002/diff/60001/tools/telemetry/telemetr...
> tools/telemetry/telemetry/value/scalar.py:110:
> improvement_direction=v0.improvement_direction)
> This is the ugliness that I mentioned. How do we guarantee that all values to
be
> merge have the same improvement_direction? :-(
> 
> It's fine if you want to punt this issue.

The overall approach lg2me btw.

[tonyg, 2015-01-26 19:19:04]

tonyg@chromium.org changed reviewers:
- tonyg@chromium.org

[qyearsley, 2015-01-27 00:50:46]

https://codereview.chromium.org/809393002/diff/60001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/value/scalar.py (right):

https://codereview.chromium.org/809393002/diff/60001/tools/telemetry/telemetr...
tools/telemetry/telemetry/value/scalar.py:110:
improvement_direction=v0.improvement_direction)
On 2015/01/08 18:07:41, nednguyen wrote:
> This is the ugliness that I mentioned. How do we guarantee that all values to
be
> merge have the same improvement_direction? :-(
> 
> It's fine if you want to punt this issue.

What does "like values" mean in this context? Maybe based on what's meant here,
it does make sense to assume that they'll all always have the same improvement
direction (e.g. if they all represent the same type of measurement?) If so, then
a comment could be added to explain this.

https://codereview.chromium.org/809393002/diff/60001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/value/summarizable.py (right):

https://codereview.chromium.org/809393002/diff/60001/tools/telemetry/telemetr...
tools/telemetry/telemetry/value/summarizable.py:6: from telemetry.value import
improvement_direction as improvement_direction_module
Just barely over 80 columns :-/ If linter doesn't complain, then leave as is.

https://codereview.chromium.org/809393002/diff/60001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/value/summary.py (right):

https://codereview.chromium.org/809393002/diff/60001/tools/telemetry/telemetr...
tools/telemetry/telemetry/value/summary.py:73: assert isinstance(v,
summarizable.SummarizableValue), "Not summarizable: %r" % v
Looks like it's just over 80 chars. Also, I think single quotes are preferred
over double, quotes, so, this could be formatted as:

assert isinstance(v, summarizable.SummarizableValue), ('Not summariz'
                                                       'able: %r' % v)

or

is_summarizable = isinstance(v, summarizable.SummarizableValue)
assert is_summarizable, 'Not summarizable: %r' % v

or, maybe it's appropriate use a different kind of exception here?

[eakuefner, 2015-02-10 20:56:43]

Adding myself on CC since I might be picking this up.