Add SkipValue to Telemetry.

Add SkipValue to Telemetry.

This adds logic for representing page skips as values in Telemetry, in the
spirit of how failures are to be handled with regard to the linked bug. The
goal for this CL is just to add the logic, and then once it's safely in the
codebase, we can wire it up and make sure Buildbot is happy with it.

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

[eakuefner, 2014-07-22 18:54:07]

For this CL I've followed Chris's approach in keeping the wiring logic out of
this CL.

[chrishenry, 2014-07-22 23:37:30]

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

https://codereview.chromium.org/409023002/diff/20001/tools/telemetry/telemetr...
tools/telemetry/telemetry/value/merge_values.py:118: should_continue =
(isinstance(value, failure.FailureValue) or
s/should_continue/should_skip_value/

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

https://codereview.chromium.org/409023002/diff/20001/tools/telemetry/telemetr...
tools/telemetry/telemetry/value/skip.py:23: page_name = None
I think skip should always have a page associated with it?

https://codereview.chromium.org/409023002/diff/20001/tools/telemetry/telemetr...
tools/telemetry/telemetry/value/skip.py:25: page_name, self._reason)
nit: can fit in 1 line

[eakuefner-google (do not use), 2014-07-22 23:58:25]

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

https://codereview.chromium.org/409023002/diff/20001/tools/telemetry/telemetr...
tools/telemetry/telemetry/value/merge_values.py:118: should_continue =
(isinstance(value, failure.FailureValue) or
On 2014/07/22 23:37:29, chrishenry wrote:
> s/should_continue/should_skip_value/

Done.

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

https://codereview.chromium.org/409023002/diff/20001/tools/telemetry/telemetr...
tools/telemetry/telemetry/value/skip.py:23: page_name = None
On 2014/07/22 23:37:30, chrishenry wrote:
> I think skip should always have a page associated with it?

Done.

https://codereview.chromium.org/409023002/diff/20001/tools/telemetry/telemetr...
tools/telemetry/telemetry/value/skip.py:25: page_name, self._reason)
On 2014/07/22 23:37:30, chrishenry wrote:
> nit: can fit in 1 line

Done.

[nednguyen, 2014-07-23 00:01:58]

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

https://codereview.chromium.org/409023002/diff/20001/tools/telemetry/telemetr...
tools/telemetry/telemetry/value/skip.py:14: reason: The reason the page was
skipped.
Can you add type information for these two params?

https://codereview.chromium.org/409023002/diff/20001/tools/telemetry/telemetr...
tools/telemetry/telemetry/value/skip.py:16: super(SkipValue,
self).__init__(page, 'skip', '', True)
How does the code that process the 'units' param deal with this? Would None or
'N/A' be the better value?

[eakuefner, 2014-07-23 00:33:03]

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

https://codereview.chromium.org/409023002/diff/20001/tools/telemetry/telemetr...
tools/telemetry/telemetry/value/skip.py:14: reason: The reason the page was
skipped.
On 2014/07/23 00:01:58, nednguyen wrote:
> Can you add type information for these two params?

Done.

https://codereview.chromium.org/409023002/diff/20001/tools/telemetry/telemetr...
tools/telemetry/telemetry/value/skip.py:16: super(SkipValue,
self).__init__(page, 'skip', '', True)
On 2014/07/23 00:01:58, nednguyen wrote:
> How does the code that process the 'units' param deal with this? Would None or
> 'N/A' be the better value?

As of now I think that there are places that expect units to be a string.
Perhaps we'll want to use N/A, I'll defer to chrishenry@ though since this
parallels his FailureValue CL.

[chrishenry, 2014-07-23 00:38:51]

On 2014/07/23 00:33:03, eakuefner wrote:
>
https://codereview.chromium.org/409023002/diff/20001/tools/telemetry/telemetr...
> tools/telemetry/telemetry/value/skip.py:16: super(SkipValue,
> self).__init__(page, 'skip', '', True)
> On 2014/07/23 00:01:58, nednguyen wrote:
> > How does the code that process the 'units' param deal with this? Would None
or
> > 'N/A' be the better value?
> 
> As of now I think that there are places that expect units to be a string.
> Perhaps we'll want to use N/A, I'll defer to chrishenry@ though since this
> parallels his FailureValue CL.

I think None (to represent N/A, rather than a special string 'N/A') would be ok,
but we'd have to audit the current usage (including the new JSON output). Do you
want to take a stab at it? If not let's file a bug to track.

[chrishenry, 2014-07-23 00:39:46]

lgtm

[eakuefner, 2014-07-23 00:45:28]

On 2014/07/23 00:38:51, chrishenry wrote:
> On 2014/07/23 00:33:03, eakuefner wrote:
> >
>
https://codereview.chromium.org/409023002/diff/20001/tools/telemetry/telemetr...
> > tools/telemetry/telemetry/value/skip.py:16: super(SkipValue,
> > self).__init__(page, 'skip', '', True)
> > On 2014/07/23 00:01:58, nednguyen wrote:
> > > How does the code that process the 'units' param deal with this? Would
None
> or
> > > 'N/A' be the better value?
> > 
> > As of now I think that there are places that expect units to be a string.
> > Perhaps we'll want to use N/A, I'll defer to chrishenry@ though since this
> > parallels his FailureValue CL.
> 
> I think None (to represent N/A, rather than a special string 'N/A') would be
ok,
> but we'd have to audit the current usage (including the new JSON output). Do
you
> want to take a stab at it? If not let's file a bug to track.

I'll take a look at this tomorrow and if I can't reach a conclusion I'll file a
bug. I think for now we should be fine to land this, and then once we establish
a convention for values that don't have units (since this is a new concept), we
can fix skip and failure at the same time.

[eakuefner, 2014-07-23 00:45:40]

The CQ bit was checked by eakuefner@chromium.org

[commit-bot: I haz the power, 2014-07-23 00:46:48]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/eakuefner@chromium.org/409023002/60001

[commit-bot: I haz the power, 2014-07-23 00:46:50]

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

[commit-bot: I haz the power, 2014-07-23 00:46:51]

No LGTM from a valid reviewer yet. Only full committers are accepted.
Even if an LGTM may have been provided, it was from a non-committer or
a provisional committer, _not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[chrishenry, 2014-07-23 00:53:36]

On 2014/07/23 00:46:51, I haz the power (commit-bot) wrote:
> No LGTM from a valid reviewer yet. Only full committers are accepted.
> Even if an LGTM may have been provided, it was from a non-committer or
> a provisional committer, _not_ a full super star committer.
> See http://www.chromium.org/getting-involved/become-a-committer
> Note that this has nothing to do with OWNERS files.

Oops sorry about this. I should really send Nat my 6 patches so my lgtm can
start holding some water. I'll dig some tomorrow. Meanwhile, get Ned/Nat's lgtm.

[chrishenry, 2014-07-23 00:53:56]

On 2014/07/23 00:45:28, eakuefner wrote:
> On 2014/07/23 00:38:51, chrishenry wrote:
> > On 2014/07/23 00:33:03, eakuefner wrote:
> > >
> >
>
https://codereview.chromium.org/409023002/diff/20001/tools/telemetry/telemetr...
> > > tools/telemetry/telemetry/value/skip.py:16: super(SkipValue,
> > > self).__init__(page, 'skip', '', True)
> > > On 2014/07/23 00:01:58, nednguyen wrote:
> > > > How does the code that process the 'units' param deal with this? Would
> None
> > or
> > > > 'N/A' be the better value?
> > > 
> > > As of now I think that there are places that expect units to be a string.
> > > Perhaps we'll want to use N/A, I'll defer to chrishenry@ though since this
> > > parallels his FailureValue CL.
> > 
> > I think None (to represent N/A, rather than a special string 'N/A') would be
> ok,
> > but we'd have to audit the current usage (including the new JSON output). Do
> you
> > want to take a stab at it? If not let's file a bug to track.
> 
> I'll take a look at this tomorrow and if I can't reach a conclusion I'll file
a
> bug. I think for now we should be fine to land this, and then once we
establish
> a convention for values that don't have units (since this is a new concept),
we
> can fix skip and failure at the same time.

Sounds good.

[nednguyen, 2014-07-23 02:13:02]

On 2014/07/23 00:53:56, chrishenry wrote:
> On 2014/07/23 00:45:28, eakuefner wrote:
> > On 2014/07/23 00:38:51, chrishenry wrote:
> > > On 2014/07/23 00:33:03, eakuefner wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/409023002/diff/20001/tools/telemetry/telemetr...
> > > > tools/telemetry/telemetry/value/skip.py:16: super(SkipValue,
> > > > self).__init__(page, 'skip', '', True)
> > > > On 2014/07/23 00:01:58, nednguyen wrote:
> > > > > How does the code that process the 'units' param deal with this? Would
> > None
> > > or
> > > > > 'N/A' be the better value?
> > > > 
> > > > As of now I think that there are places that expect units to be a
string.
> > > > Perhaps we'll want to use N/A, I'll defer to chrishenry@ though since
this
> > > > parallels his FailureValue CL.
> > > 
> > > I think None (to represent N/A, rather than a special string 'N/A') would
be
> > ok,
> > > but we'd have to audit the current usage (including the new JSON output).
Do
> > you
> > > want to take a stab at it? If not let's file a bug to track.
> > 
> > I'll take a look at this tomorrow and if I can't reach a conclusion I'll
file
> a
> > bug. I think for now we should be fine to land this, and then once we
> establish
> > a convention for values that don't have units (since this is a new concept),
> we
> > can fix skip and failure at the same time.
> 
> Sounds good.

LGTM

[eakuefner, 2014-07-23 02:22:06]

The CQ bit was checked by eakuefner@chromium.org

[commit-bot: I haz the power, 2014-07-23 02:22:34]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/eakuefner@chromium.org/409023002/60001

[commit-bot: I haz the power, 2014-07-23 05:52:33]

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

[commit-bot: I haz the power, 2014-07-23 08:45:23]

Change committed as 284882