Add "description" attribute to Value.

Add "description" attribute to Value.

BUG=395787
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=285512

[qyearsley, 2014-07-17 18:24:29]

This is meant to be a preliminary CL before descriptions are added to individual
values in https://codereview.chromium.org/391183002/.

A few questions:

 - Which tests should be added if any?

 - What kinds of PageMeasurementResults should output the description? Later, I
assume that the JSON output format will; how about "block" format?

 - I think that later we'll also want to add other attributes to Value, like
"higher_is_better" "bigger_is_better", and "alert", and we'll may want to remove
"important". Is that right?

[eakuefner, 2014-07-21 18:36:27]

+me, +nednguyen

[eakuefner, 2014-07-21 19:27:34]

On 2014/07/17 18:24:29, qyearsley wrote:
> This is meant to be a preliminary CL before descriptions are added to
individual
> values in https://codereview.chromium.org/391183002/.
> 
> A few questions:
> 
>  - Which tests should be added if any?
The test that you have right now looks good, though I would add one for
WithoutDescription, like there currently is for Page (WithPage and WithoutPage),
to explicitly make sure that no description key is being inserted if it should
not be.
> 
>  - I think that later we'll also want to add other attributes to Value, like
> "higher_is_better" "bigger_is_better", and "alert", and we'll may want to
remove
> "important". Is that right?
Yes, this maybe represents a separate refactor somewhere down the line; we will
transition from values having an 'important' field to values having both
'bigger_is_better', a boolean, and 'alert', a string specifying an alerting
algo, as per the design doc.

[eakuefner, 2014-07-21 19:27:50]

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

https://codereview.chromium.org/399893003/diff/20001/tools/telemetry/telemetr...
tools/telemetry/telemetry/value/histogram.py:31: description=True):
I think this is probably the right approach, to make description mandatory in
the Value constructor but optional in the children as required, since certain
types of values might want to set the description in their own way.

nit: True is probably unsuitable as a default value for description.

[qyearsley, 2014-07-21 20:10:46]

On 2014/07/21 19:27:50, eakuefner wrote:
>
https://codereview.chromium.org/399893003/diff/20001/tools/telemetry/telemetr...
> File tools/telemetry/telemetry/value/histogram.py (right):
> 
>
https://codereview.chromium.org/399893003/diff/20001/tools/telemetry/telemetr...
> tools/telemetry/telemetry/value/histogram.py:31: description=True):
> I think this is probably the right approach, to make description mandatory in
> the Value constructor but optional in the children as required, since certain
> types of values might want to set the description in their own way.
> 
> nit: True is probably unsuitable as a default value for description.

Updated; any more nits?

[nednguyen, 2014-07-21 20:26:53]

Do you have a bug opened for migration the data from trace-info.json to Value? 

If not, can you please make one and assign the bug number to this patch?

https://codereview.chromium.org/399893003/diff/40001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/value/__init__.py (right):

https://codereview.chromium.org/399893003/diff/40001/tools/telemetry/telemetr...
tools/telemetry/telemetry/value/__init__.py:52: description: An string
describing what was measured.
"A string"

Also, Value also include FailureValue, which tells that the test has failed. So
you may want to change the description of 'description' too.

[qyearsley, 2014-07-21 21:31:58]

On 2014/07/21 20:26:53, nednguyen wrote:
> Do you have a bug opened for migration the data from trace-info.json to Value?

> 
> If not, can you please make one and assign the bug number to this patch?

Done.

[qyearsley, 2014-07-21 21:32:16]

https://codereview.chromium.org/399893003/diff/40001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/value/__init__.py (right):

https://codereview.chromium.org/399893003/diff/40001/tools/telemetry/telemetr...
tools/telemetry/telemetry/value/__init__.py:52: description: An string
describing what was measured.
On 2014/07/21 20:26:53, nednguyen wrote:
> "A string"
> 
> Also, Value also include FailureValue, which tells that the test has failed.
So
> you may want to change the description of 'description' too.

Updated comment. Where is FailureValue? I can't seem to find it.

[eakuefner, 2014-07-21 21:38:41]

On 2014/07/21 21:32:16, qyearsley wrote:
> Updated comment. Where is FailureValue? I can't seem to find it.

FailureValue was added by chrishenry@ in this CL
https://codereview.chromium.org/399263003/ that recently landed. You may need to
rebase.

[eakuefner, 2014-07-21 22:41:21]

On 2014/07/21 21:38:41, eakuefner wrote:
> On 2014/07/21 21:32:16, qyearsley wrote:
> > Updated comment. Where is FailureValue? I can't seem to find it.
> 
> FailureValue was added by chrishenry@ in this CL
> https://codereview.chromium.org/399263003/ that recently landed. You may need
to
> rebase.

According to conversation with Chris he is actually currently trying to re-land
that CL, but you should at least be able to use it as a reference for now.

[chrishenry, 2014-07-21 23:56:51]

I'm going to land a partial change first (adding FailureValue without wiring it
- https://codereview.chromium.org/405283002/). Trying to isolate the buildbot
failure (that I'm not able to trigger locally).

[qyearsley, 2014-07-22 00:01:12]

On 2014/07/21 23:56:51, chrishenry wrote:
> I'm going to land a partial change first (adding FailureValue without wiring
it
> - https://codereview.chromium.org/405283002/). Trying to isolate the buildbot
> failure (that I'm not able to trigger locally).

Alright, after that's landed then I'll update this CL adding the description
attribute to FailureValue. :-)

[chrishenry, 2014-07-23 01:00:23]

On 2014/07/22 00:01:12, qyearsley wrote:
> On 2014/07/21 23:56:51, chrishenry wrote:
> > I'm going to land a partial change first (adding FailureValue without wiring
> it
> > - https://codereview.chromium.org/405283002/). Trying to isolate the
buildbot
> > failure (that I'm not able to trigger locally).
> 
> Alright, after that's landed then I'll update this CL adding the description
> attribute to FailureValue. :-)

The partial patch landed this morning as
https://codereview.chromium.org/405283002/, looks safe for now since previously
failing test (on the bot) still passed. I think this is sufficient to unblock?
Otherwise, the remainder of the patch will land tomorrow or day after
(https://codereview.chromium.org/408913002/).

[nednguyen, 2014-07-23 21:38:16]

lgtm

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

https://codereview.chromium.org/399893003/diff/60001/tools/telemetry/telemetr...
tools/telemetry/telemetry/value/__init__.py:53: no description.
Value system will not just be values obtained by measuring. I think "A string"
is enough.

[qyearsley, 2014-07-23 21:59:35]

On 2014/07/23 21:38:16, nednguyen wrote:
> lgtm
> 
>
https://codereview.chromium.org/399893003/diff/60001/tools/telemetry/telemetr...
> File tools/telemetry/telemetry/value/__init__.py (right):
> 
>
https://codereview.chromium.org/399893003/diff/60001/tools/telemetry/telemetr...
> tools/telemetry/telemetry/value/__init__.py:53: no description.
> Value system will not just be values obtained by measuring. I think "A string"
> is enough.

Alright, I've now included FailureValue and changed the docstring for
"description" to "A string".

[chrishenry, 2014-07-23 22:23:54]

Quick dumb questions:
What happen if a particular description changes? Would perf dashboard picks up
the latest description for the series?

p.s. Ethan, which design doc has the value metadata stuffs you're talking about
(higher_is_better, alert)? Can you share it with me? Thanks!

[eakuefner, 2014-07-23 22:25:25]

On 2014/07/23 22:23:54, chrishenry wrote:
> p.s. Ethan, which design doc has the value metadata stuffs you're talking
about
> (higher_is_better, alert)? Can you share it with me? Thanks!

https://docs.google.com/a/google.com/document/d/1HZv6v5hFPpey5N2DsjQ0KEIvR8B-...

[qyearsley, 2014-07-23 22:40:18]

On 2014/07/23 22:25:25, eakuefner wrote:
> On 2014/07/23 22:23:54, chrishenry wrote:
> > p.s. Ethan, which design doc has the value metadata stuffs you're talking
> about
> > (higher_is_better, alert)? Can you share it with me? Thanks!
> 
>
https://docs.google.com/a/google.com/document/d/1HZv6v5hFPpey5N2DsjQ0KEIvR8B-...

The plan is that the dashboard will pick up the latest description for the
series, overwriting any previous description (this is what it currently does
with units).

However, using the "buildbot" output method there's no clean way of adding
metadata like description (besides units, which are included in the format), so
this won't be used until the bots which are running the tests can get and parse
a JSON output format from Telemetry.

[nduca, 2014-07-24 17:41:50]

lgtm with nits. tldr, dont make stupid doc strings

https://codereview.chromium.org/399893003/diff/80001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/value/__init__.py (right):

https://codereview.chromium.org/399893003/diff/80001/tools/telemetry/telemetr...
tools/telemetry/telemetry/value/__init__.py:51: important: A boolean.
important: Causes the value to trigger alerts, if possible, and appear by
default in downstream UIs.

https://codereview.chromium.org/399893003/diff/80001/tools/telemetry/telemetr...
tools/telemetry/telemetry/value/__init__.py:52: description: A string.
description: A string explaining in human-understandable terms what this value
represents.

[qyearsley, 2014-07-24 17:51:34]

https://codereview.chromium.org/399893003/diff/80001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/value/__init__.py (right):

https://codereview.chromium.org/399893003/diff/80001/tools/telemetry/telemetr...
tools/telemetry/telemetry/value/__init__.py:51: important: A boolean.
On 2014/07/24 17:41:50, nduca wrote:
> important: Causes the value to trigger alerts, if possible, and appear by
> default in downstream UIs.

Done. Note, "important" still doesn't control monitoring behavior on the perf
dashboard, and I think we may want to replace this attribute with an "alert"
property later.

https://codereview.chromium.org/399893003/diff/80001/tools/telemetry/telemetr...
tools/telemetry/telemetry/value/__init__.py:52: description: A string.
On 2014/07/24 17:41:50, nduca wrote:
> description: A string explaining in human-understandable terms what this value
> represents.

Done.

[qyearsley, 2014-07-24 17:51:40]

The CQ bit was checked by qyearsley@chromium.org

[commit-bot: I haz the power, 2014-07-24 17:53:29]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/qyearsley@chromium.org/399893003/100001

[commit-bot: I haz the power, 2014-07-24 20:58:42]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_rel_swarming on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel_sw...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)

[commit-bot: I haz the power, 2014-07-24 21:37:20]

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

[commit-bot: I haz the power, 2014-07-24 21:37:22]

Try jobs failed on following builders:
  linux_chromium_rel_swarming on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel_sw...)

[qyearsley, 2014-07-24 23:52:16]

The CQ bit was checked by qyearsley@chromium.org

[commit-bot: I haz the power, 2014-07-24 23:55:53]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/qyearsley@chromium.org/399893003/120001

[commit-bot: I haz the power, 2014-07-25 07:03:52]

Change committed as 285512