Support None value for telemetry values.

Support None value for the following telemetry values: scalar, string, list_of_scalars, list_of_strings

See https://docs.google.com/a/google.com/document/d/1vzmYzpKK5ImV-ItSE9j2TpC2o0BLcKGaXvsnoUeBCno/
for more of the motivation behind this.

BUG=

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

[chrishenry, 2014-08-15 04:43:21]

I'd be more comfortable splitting the changes into 2 patches: changes to
telemetry.values and changes to smoothness metrics.

https://codereview.chromium.org/475503004/diff/100001/tools/telemetry/telemet...
File tools/telemetry/telemetry/value/list_of_scalar_values.py (right):

https://codereview.chromium.org/475503004/diff/100001/tools/telemetry/telemet...
tools/telemetry/telemetry/value/list_of_scalar_values.py:18:
same_page_merge_policy=value_module.CONCATENATE):
Can we add documentation? What is a possible value of none_value_reason? Add
assertion for that?

https://codereview.chromium.org/475503004/diff/100001/tools/telemetry/telemet...
tools/telemetry/telemetry/value/list_of_scalar_values.py:119: if(v.values is
None):
Remove parenthesis around conditional

https://codereview.chromium.org/475503004/diff/100001/tools/telemetry/telemet...
File tools/telemetry/telemetry/value/list_of_string_values.py (right):

https://codereview.chromium.org/475503004/diff/100001/tools/telemetry/telemet...
tools/telemetry/telemetry/value/list_of_string_values.py:17: assert
all(isinstance(v, basestring) for v in values)
Maybe a little more readable:
if values is not None:
  assert len(values) > 0
  assert isinstance(values, list)
  assert all(isinstance(v, basestring) for v in values)

(similarly for the other class

https://codereview.chromium.org/475503004/diff/100001/tools/telemetry/telemet...
tools/telemetry/telemetry/value/list_of_string_values.py:113: if(v.values is
None):
Remove parenthesis around conditional

https://codereview.chromium.org/475503004/diff/100001/tools/telemetry/telemet...
tools/telemetry/telemetry/value/list_of_string_values.py:117:
all_values.extend(v.values)
Let's rename all_values to merged_values? Then None would make a bit more sense.

https://codereview.chromium.org/475503004/diff/100001/tools/telemetry/telemet...
File tools/telemetry/telemetry/value/none_values.py (right):

https://codereview.chromium.org/475503004/diff/100001/tools/telemetry/telemet...
tools/telemetry/telemetry/value/none_values.py:5: MERGE_FAILURE_REASON = "Unable
to merge values with None value."
The merge is successful tho, so maybe "Merging values containing None value
results in a None value" (or a better phrasing of that)

https://codereview.chromium.org/475503004/diff/100001/tools/telemetry/telemet...
tools/telemetry/telemetry/value/none_values.py:17: raise
ValueMustHaveNoneValue()
Should this just be part of __init__.py? Thoughts?

https://codereview.chromium.org/475503004/diff/100001/tools/telemetry/telemet...
File tools/telemetry/telemetry/web_perf/metrics/smoothness.py (right):

https://codereview.chromium.org/475503004/diff/100001/tools/telemetry/telemet...
tools/telemetry/telemetry/web_perf/metrics/smoothness.py:47: def
GetNoneValueReason(self, list_of_frame_timestamp_lists):
Maybe make this return boolean and name it something like CheckHasEnoughFrames.

https://codereview.chromium.org/475503004/diff/100001/tools/telemetry/telemet...
tools/telemetry/telemetry/web_perf/metrics/smoothness.py:58: not_enough_frames =
any(len(s) < 2 for s in list_of_frame_timestamp_lists)
So we aren't going to try to compute anything if any of the interaction record
(in a repeatable set) has <2 frames? Let's document this. Maybe as part of
SmoothnessMetric documentation.

https://codereview.chromium.org/475503004/diff/100001/tools/telemetry/telemet...
tools/telemetry/telemetry/web_perf/metrics/smoothness.py:60: return "Not enough
frames for smoothness metrics."
Single quote string.

https://codereview.chromium.org/475503004/diff/100001/tools/telemetry/telemet...
tools/telemetry/telemetry/web_perf/metrics/smoothness.py:63: def
GetLatencyMetricValues(self, page, name, list_of_latency_lists,
ComputeLatencyMetric

https://codereview.chromium.org/475503004/diff/100001/tools/telemetry/telemet...
tools/telemetry/telemetry/web_perf/metrics/smoothness.py:64: none_value_reason):
Instead of passing none_value_reason, maybe better if we instead do:

if self.HasEnoughFrames():
  latency_list = ...
  ...
else:
  none_value_reason = _NOT_ENOUGH_FRAMES

That way, the function signature will be much cleaner, and the code will be
about the same.

https://codereview.chromium.org/475503004/diff/100001/tools/telemetry/telemet...
tools/telemetry/telemetry/web_perf/metrics/smoothness.py:86: def
GetQueueingDurationValue(self, page, list_of_queueing_durations_lists,
ComputeQueueingDuration

[nednguyen, 2014-08-15 14:06:52]

Yes, let's put changes to web_perf/ to a different patch for easier reviewing.

https://codereview.chromium.org/475503004/diff/100001/tools/telemetry/telemet...
File tools/telemetry/telemetry/value/list_of_string_values.py (right):

https://codereview.chromium.org/475503004/diff/100001/tools/telemetry/telemet...
tools/telemetry/telemetry/value/list_of_string_values.py:4: 
I think we should kill string & list_of_string_values. It's only used for
specifying trace's file path, which will soon replaced by TracedValue.
Really, we don't have any real world case of string value, and I can't come up
with one that string value is the only solution.

[nednguyen, 2014-08-15 15:50:04]

lgtm

https://codereview.chromium.org/475503004/diff/100001/tools/telemetry/telemet...
File tools/telemetry/telemetry/value/list_of_scalar_values.py (right):

https://codereview.chromium.org/475503004/diff/100001/tools/telemetry/telemet...
tools/telemetry/telemetry/value/list_of_scalar_values.py:23: assert
all(isinstance(v, numbers.Number) for v in values)
assert non_value_reason is None

https://codereview.chromium.org/475503004/diff/100001/tools/telemetry/telemet...
tools/telemetry/telemetry/value/list_of_scalar_values.py:94: if
v0.same_page_merge_policy == value_module.PICK_FIRST:
@Nat: Do we have any use case of this PICK_FIRST policy? Why would you ever want
to use the first value as the representative summary value?

https://codereview.chromium.org/475503004/diff/100001/tools/telemetry/telemet...
File tools/telemetry/telemetry/value/none_values.py (right):

https://codereview.chromium.org/475503004/diff/100001/tools/telemetry/telemet...
tools/telemetry/telemetry/value/none_values.py:13: def ValidateValue(value,
none_value_reason):
Maybe ValidateNoneAbleValue.

https://codereview.chromium.org/475503004/diff/100001/tools/telemetry/telemet...
tools/telemetry/telemetry/value/none_values.py:17: raise
ValueMustHaveNoneValue()
On 2014/08/15 04:43:20, chrishenry wrote:
> Should this just be part of __init__.py? Thoughts?

I find the situation of partly "adding the new field at leaves" + partly "adding
the validation at root" quite awkward.

[nednguyen, 2014-08-15 15:50:32]

On 2014/08/15 15:50:10, nednguyen wrote:

not lgtm yet. Sorry, I clicked the button by mistake :(

[chrishenry, 2014-08-15 16:32:40]

https://codereview.chromium.org/475503004/diff/100001/tools/telemetry/telemet...
File tools/telemetry/telemetry/value/none_values.py (right):

https://codereview.chromium.org/475503004/diff/100001/tools/telemetry/telemet...
tools/telemetry/telemetry/value/none_values.py:17: raise
ValueMustHaveNoneValue()
On 2014/08/15 15:50:04, nednguyen wrote:
> On 2014/08/15 04:43:20, chrishenry wrote:
> > Should this just be part of __init__.py? Thoughts?
> 
> I find the situation of partly "adding the new field at leaves" + partly
"adding
> the validation at root" quite awkward.

Some raw thoughts from my end on this. I don't think we should pursue it in this
patch, but worth mulling over.

Maybe we can formalize the concept of value trait for this: NoneableValueTrait.
We can either make it a base class or a composable trait. Not sure if it's going
to look ok or not.

Something like this:

class NoneableValueTrait(ValueTrait):
  def __init__(self, value=None, none_value_reason=None):
    self._ValidateNoneTrait(value, none_value_reason)
    self._none_value_reason = none_value_reason

  @property
  def none_value_reason(): ...

  def FillDict(dict):
    base_obj['none_value_reason'] = self.none_value_reason

  @classmethod
  def ReviveFromDict(dict, value_class):
    if dict['none_value_reason'] and
value_class.IsTraitSupported(NoneValueTrait)
    return new NoneValueTrait(dict)

  def IsMergable(value): ...

  def MergeLikeValues(...): ?????? <- don't know how this looks like or how this
is called, maybe we can pair and discuss briefly later.

class Value(object):
  def __init__(..., traits=None):
    self._traits = traits if traits is not None else []

  def AsDict():
    base_dict = {...}
    for trait in self._traits:
      trait.FillDict(base_dict)

  etc...

[ariblue, 2014-08-15 22:10:26]

I'll go ahead and split this up into two patches per Chris's recommendation in
the comments.

https://codereview.chromium.org/475503004/diff/100001/tools/telemetry/telemet...
File tools/telemetry/telemetry/value/list_of_scalar_values.py (right):

https://codereview.chromium.org/475503004/diff/100001/tools/telemetry/telemet...
tools/telemetry/telemetry/value/list_of_scalar_values.py:23: assert
all(isinstance(v, numbers.Number) for v in values)
On 2014/08/15 15:50:04, nednguyen wrote:
> assert non_value_reason is None

This is handled in the none_values.ValidateValue().

https://codereview.chromium.org/475503004/diff/100001/tools/telemetry/telemet...
tools/telemetry/telemetry/value/list_of_scalar_values.py:119: if(v.values is
None):
On 2014/08/15 04:43:20, chrishenry wrote:
> Remove parenthesis around conditional

Done.

https://codereview.chromium.org/475503004/diff/100001/tools/telemetry/telemet...
File tools/telemetry/telemetry/value/list_of_string_values.py (right):

https://codereview.chromium.org/475503004/diff/100001/tools/telemetry/telemet...
tools/telemetry/telemetry/value/list_of_string_values.py:17: assert
all(isinstance(v, basestring) for v in values)
On 2014/08/15 04:43:20, chrishenry wrote:
> Maybe a little more readable:
> if values is not None:
>   assert len(values) > 0
>   assert isinstance(values, list)
>   assert all(isinstance(v, basestring) for v in values)
> 
> (similarly for the other class

Good call. Done.

https://codereview.chromium.org/475503004/diff/100001/tools/telemetry/telemet...
tools/telemetry/telemetry/value/list_of_string_values.py:113: if(v.values is
None):
On 2014/08/15 04:43:20, chrishenry wrote:
> Remove parenthesis around conditional

Done.

https://codereview.chromium.org/475503004/diff/100001/tools/telemetry/telemet...
tools/telemetry/telemetry/value/list_of_string_values.py:117:
all_values.extend(v.values)
On 2014/08/15 04:43:20, chrishenry wrote:
> Let's rename all_values to merged_values? Then None would make a bit more
sense.

Done.

https://codereview.chromium.org/475503004/diff/100001/tools/telemetry/telemet...
File tools/telemetry/telemetry/value/none_values.py (right):

https://codereview.chromium.org/475503004/diff/100001/tools/telemetry/telemet...
tools/telemetry/telemetry/value/none_values.py:5: MERGE_FAILURE_REASON = "Unable
to merge values with None value."
On 2014/08/15 04:43:20, chrishenry wrote:
> The merge is successful tho, so maybe "Merging values containing None value
> results in a None value" (or a better phrasing of that)

Done.

https://codereview.chromium.org/475503004/diff/100001/tools/telemetry/telemet...
tools/telemetry/telemetry/value/none_values.py:13: def ValidateValue(value,
none_value_reason):
On 2014/08/15 15:50:04, nednguyen wrote:
> Maybe ValidateNoneAbleValue. 

How about ValidateNullableValue?

https://codereview.chromium.org/475503004/diff/100001/tools/telemetry/telemet...
tools/telemetry/telemetry/value/none_values.py:17: raise
ValueMustHaveNoneValue()
On 2014/08/15 16:32:40, chrishenry wrote:
> On 2014/08/15 15:50:04, nednguyen wrote:
> > On 2014/08/15 04:43:20, chrishenry wrote:
> > > Should this just be part of __init__.py? Thoughts?
> > 
> > I find the situation of partly "adding the new field at leaves" + partly
> "adding
> > the validation at root" quite awkward.
> 
> Some raw thoughts from my end on this. I don't think we should pursue it in
this
> patch, but worth mulling over.
> 
> Maybe we can formalize the concept of value trait for this:
NoneableValueTrait.
> We can either make it a base class or a composable trait. Not sure if it's
going
> to look ok or not.
> 
> Something like this:
> 
> class NoneableValueTrait(ValueTrait):
>   def __init__(self, value=None, none_value_reason=None):
>     self._ValidateNoneTrait(value, none_value_reason)
>     self._none_value_reason = none_value_reason
> 
>   @property
>   def none_value_reason(): ...
> 
>   def FillDict(dict):
>     base_obj['none_value_reason'] = self.none_value_reason
> 
>   @classmethod
>   def ReviveFromDict(dict, value_class):
>     if dict['none_value_reason'] and
> value_class.IsTraitSupported(NoneValueTrait)
>     return new NoneValueTrait(dict)
> 
>   def IsMergable(value): ...
> 
>   def MergeLikeValues(...): ?????? <- don't know how this looks like or how
this
> is called, maybe we can pair and discuss briefly later.
> 
> class Value(object):
>   def __init__(..., traits=None):
>     self._traits = traits if traits is not None else []
> 
>   def AsDict():
>     base_dict = {...}
>     for trait in self._traits:
>       trait.FillDict(base_dict)
> 
>   etc...

Acknowledged.

https://codereview.chromium.org/475503004/diff/100001/tools/telemetry/telemet...
File tools/telemetry/telemetry/web_perf/metrics/smoothness.py (right):

https://codereview.chromium.org/475503004/diff/100001/tools/telemetry/telemet...
tools/telemetry/telemetry/web_perf/metrics/smoothness.py:47: def
GetNoneValueReason(self, list_of_frame_timestamp_lists):
On 2014/08/15 04:43:21, chrishenry wrote:
> Maybe make this return boolean and name it something like
CheckHasEnoughFrames.

Done.

https://codereview.chromium.org/475503004/diff/100001/tools/telemetry/telemet...
tools/telemetry/telemetry/web_perf/metrics/smoothness.py:58: not_enough_frames =
any(len(s) < 2 for s in list_of_frame_timestamp_lists)
On 2014/08/15 04:43:21, chrishenry wrote:
> So we aren't going to try to compute anything if any of the interaction record
> (in a repeatable set) has <2 frames? Let's document this. Maybe as part of
> SmoothnessMetric documentation.

Done. Added a doc for the SmoothnessMetric Class.

https://codereview.chromium.org/475503004/diff/100001/tools/telemetry/telemet...
tools/telemetry/telemetry/web_perf/metrics/smoothness.py:60: return "Not enough
frames for smoothness metrics."
On 2014/08/15 04:43:20, chrishenry wrote:
> Single quote string.

Done.

https://codereview.chromium.org/475503004/diff/100001/tools/telemetry/telemet...
tools/telemetry/telemetry/web_perf/metrics/smoothness.py:63: def
GetLatencyMetricValues(self, page, name, list_of_latency_lists,
On 2014/08/15 04:43:21, chrishenry wrote:
> ComputeLatencyMetric

Done.

https://codereview.chromium.org/475503004/diff/100001/tools/telemetry/telemet...
tools/telemetry/telemetry/web_perf/metrics/smoothness.py:64: none_value_reason):
On 2014/08/15 04:43:21, chrishenry wrote:
> Instead of passing none_value_reason, maybe better if we instead do:
> 
> if self.HasEnoughFrames():
>   latency_list = ...
>   ...
> else:
>   none_value_reason = _NOT_ENOUGH_FRAMES
> 
> That way, the function signature will be much cleaner, and the code will be
> about the same.

Done.

https://codereview.chromium.org/475503004/diff/100001/tools/telemetry/telemet...
tools/telemetry/telemetry/web_perf/metrics/smoothness.py:86: def
GetQueueingDurationValue(self, page, list_of_queueing_durations_lists,
On 2014/08/15 04:43:21, chrishenry wrote:
> ComputeQueueingDuration

Done.

[nednguyen, 2014-08-18 06:54:57]

LGTM

https://codereview.chromium.org/475503004/diff/140001/tools/telemetry/telemet...
File tools/telemetry/telemetry/value/list_of_scalar_values.py (right):

https://codereview.chromium.org/475503004/diff/140001/tools/telemetry/telemet...
tools/telemetry/telemetry/value/list_of_scalar_values.py:23: assert
isinstance(values, list)
Put this assertion first?

https://codereview.chromium.org/475503004/diff/140001/tools/telemetry/telemet...
File tools/telemetry/telemetry/value/list_of_scalar_values_unittest.py (right):

https://codereview.chromium.org/475503004/diff/140001/tools/telemetry/telemet...
tools/telemetry/telemetry/value/list_of_scalar_values_unittest.py:63: def
testListDifferentPageMerging(self):
Can you add testListSamePageMerging do we don't lose test cover on the left?

https://codereview.chromium.org/475503004/diff/140001/tools/telemetry/telemet...
File tools/telemetry/telemetry/value/none_values.py (right):

https://codereview.chromium.org/475503004/diff/140001/tools/telemetry/telemet...
tools/telemetry/telemetry/value/none_values.py:14: def
ValidateNullableValue(value, none_value_reason):
Validate none_value_reason's type since the AsDict method can apply for it?

[ariblue, 2014-08-18 17:56:45]

https://codereview.chromium.org/475503004/diff/140001/tools/telemetry/telemet...
File tools/telemetry/telemetry/value/list_of_scalar_values.py (right):

https://codereview.chromium.org/475503004/diff/140001/tools/telemetry/telemet...
tools/telemetry/telemetry/value/list_of_scalar_values.py:23: assert
isinstance(values, list)
On 2014/08/18 06:54:57, nednguyen wrote:
> Put this assertion first?

Done.

https://codereview.chromium.org/475503004/diff/140001/tools/telemetry/telemet...
File tools/telemetry/telemetry/value/list_of_scalar_values_unittest.py (right):

https://codereview.chromium.org/475503004/diff/140001/tools/telemetry/telemet...
tools/telemetry/telemetry/value/list_of_scalar_values_unittest.py:63: def
testListDifferentPageMerging(self):
On 2014/08/18 06:54:57, nednguyen wrote:
> Can you add testListSamePageMerging do we don't lose test cover on the left?

There are already the following tests:
* testListSamePageMergingWithSamePageConcatenatePolicy
* testListSamePageMergingWithPickFirstPolicy

https://codereview.chromium.org/475503004/diff/140001/tools/telemetry/telemet...
File tools/telemetry/telemetry/value/none_values.py (right):

https://codereview.chromium.org/475503004/diff/140001/tools/telemetry/telemet...
tools/telemetry/telemetry/value/none_values.py:14: def
ValidateNullableValue(value, none_value_reason):
On 2014/08/18 06:54:57, nednguyen wrote:
> Validate none_value_reason's type since the AsDict method can apply for it?

Done.

[chrishenry, 2014-08-18 18:46:04]

lgtm

https://codereview.chromium.org/475503004/diff/160001/tools/telemetry/telemet...
File tools/telemetry/telemetry/value/none_values.py (right):

https://codereview.chromium.org/475503004/diff/160001/tools/telemetry/telemet...
tools/telemetry/telemetry/value/none_values.py:6: 'Merging values containing a
None value results in a None value')
nit: Add fullstop.

https://codereview.chromium.org/475503004/diff/160001/tools/telemetry/telemet...
tools/telemetry/telemetry/value/none_values.py:14: def
ValidateNullableValue(value, none_value_reason):
Hmm, nullable? Maybe just ValidateNoneValueReason. Can you also add a short doc?

[ariblue, 2014-08-18 21:22:35]

https://codereview.chromium.org/475503004/diff/160001/tools/telemetry/telemet...
File tools/telemetry/telemetry/value/none_values.py (right):

https://codereview.chromium.org/475503004/diff/160001/tools/telemetry/telemet...
tools/telemetry/telemetry/value/none_values.py:6: 'Merging values containing a
None value results in a None value')
On 2014/08/18 18:46:03, chrishenry wrote:
> nit: Add fullstop.

Done.

https://codereview.chromium.org/475503004/diff/160001/tools/telemetry/telemet...
tools/telemetry/telemetry/value/none_values.py:14: def
ValidateNullableValue(value, none_value_reason):
On 2014/08/18 18:46:03, chrishenry wrote:
> Hmm, nullable? Maybe just ValidateNoneValueReason. Can you also add a short
doc?

Done.

[ariblue, 2014-08-19 19:58:35]

The CQ bit was checked by ariblue@google.com

[commit-bot: I haz the power, 2014-08-19 19:59:42]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/ariblue@google.com/475503004/180001

[commit-bot: I haz the power, 2014-08-19 21:32:45]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_clang_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)
  mac_chromium_rel_swarming on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
  win_chromium_compile_dbg on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[commit-bot: I haz the power, 2014-08-19 22:08:53]

Committed patchset #6 (180001) as 290675