[Telemetry] Fix 'important' field of certain Values being non-boolean

[Telemetry] Fix 'important' field of certain Values being non-boolean

This patch fixes a specific metric that was adding values with descriptions
accidentally being inserted in the important field's position. It also adds an
assert to prevent values with non-bool important fields from being constructed.

R=sullivan,nednguyen@google.com,dtu
BUG=458523
Committed: https://crrev.com/1af5584c413372d56601adfc53fd30de002e7e09
Cr-Commit-Position: refs/heads/master@{#316598}

[eakuefner, 2015-02-13 18:14:40]

I believe this fixes the specific metric that was causing the particular issue
described in the bug. If my assert breaks any benchmarks, let me know and we can
fix them as they come up.

[sullivan, 2015-02-13 18:18:12]

lgtm

[nednguyen, 2015-02-13 18:19:43]

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

https://codereview.chromium.org/914083005/diff/1/tools/telemetry/telemetry/va...
tools/telemetry/telemetry/value/__init__.py:63: assert isinstance(important,
bool)
What about these other fields?

[eakuefner, 2015-02-13 19:00:54]

New patchsets have been uploaded after l-g-t-m from sullivan@chromium.org

[eakuefner, 2015-02-13 19:06:29]

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

https://codereview.chromium.org/914083005/diff/1/tools/telemetry/telemetry/va...
tools/telemetry/telemetry/value/__init__.py:63: assert isinstance(important,
bool)
On 2015/02/13 18:19:43, nednguyen wrote:
> What about these other fields?

Done. One note is that I couldn't import the page module in value because of the
weirdness about the page module that eventually needs to be fixed (see
crbug.com/458605). Instead I'm checking the class name, which isn't optimal but
will probably do as a sanity check for now.

[nednguyen, 2015-02-13 19:09:21]

lgtm

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

https://codereview.chromium.org/914083005/diff/20001/tools/telemetry/telemetr...
tools/telemetry/telemetry/value/__init__.py:62: assert type(page).__name__ ==
'Page'
Actually, let's left page alone for now because the abstraction we have been
cheating here is 'user_story' :-(

https://codereview.chromium.org/914083005/diff/20001/tools/telemetry/telemetr...
tools/telemetry/telemetry/value/__init__.py:67: assert isinstance(description,
str)
assert (not description) or isinstance(description, str)

[nednguyen, 2015-02-13 19:13:26]

On 2015/02/13 19:09:21, nednguyen wrote:
> lgtm
> 
>
https://codereview.chromium.org/914083005/diff/20001/tools/telemetry/telemetr...
> File tools/telemetry/telemetry/value/__init__.py (right):
> 
>
https://codereview.chromium.org/914083005/diff/20001/tools/telemetry/telemetr...
> tools/telemetry/telemetry/value/__init__.py:62: assert type(page).__name__ ==
> 'Page'
> Actually, let's left page alone for now because the abstraction we have been
> cheating here is 'user_story' :-(

Context: https://code.google.com/p/chromium/issues/detail?id=442036

> 
>
https://codereview.chromium.org/914083005/diff/20001/tools/telemetry/telemetr...
> tools/telemetry/telemetry/value/__init__.py:67: assert isinstance(description,
> str)
> assert (not description) or isinstance(description, str)

[eakuefner, 2015-02-13 19:28:14]

New patchsets have been uploaded after l-g-t-m from nednguyen@google.com

[eakuefner, 2015-02-13 19:29:13]

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

https://codereview.chromium.org/914083005/diff/20001/tools/telemetry/telemetr...
tools/telemetry/telemetry/value/__init__.py:62: assert type(page).__name__ ==
'Page'
On 2015/02/13 at 19:09:21, nednguyen wrote:
> Actually, let's left page alone for now because the abstraction we have been
cheating here is 'user_story' :-(

Fair enough. I did some fiddling in the bug tracker and also put a ref to the
bug in question here.

https://codereview.chromium.org/914083005/diff/20001/tools/telemetry/telemetr...
tools/telemetry/telemetry/value/__init__.py:67: assert isinstance(description,
str)
On 2015/02/13 at 19:09:21, nednguyen wrote:
> assert (not description) or isinstance(description, str)

Done, except I think (not description) is not quite correct because that will be
True if description is False so I've gone for (description is None).

[nednguyen, 2015-02-13 19:58:35]

The CQ bit was checked by nednguyen@google.com

[commit-bot: I haz the power, 2015-02-13 19:59:25]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/914083005/40001

[commit-bot: I haz the power, 2015-02-13 20:02:19]

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

[commit-bot: I haz the power, 2015-02-13 20:02:20]

Try jobs failed on following builders:
  ios_dbg_simulator on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ng...)
  ios_rel_device_ninja_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)

[eakuefner, 2015-02-13 20:41:19]

The CQ bit was checked by eakuefner@chromium.org

[commit-bot: I haz the power, 2015-02-13 20:41:50]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/914083005/60001

[dtu, 2015-02-13 21:43:06]

lgtm % nit

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

https://codereview.chromium.org/914083005/diff/60001/tools/telemetry/telemetr...
tools/telemetry/telemetry/value/__init__.py:61: assert isinstance(name, str)
Technically should be ValueError.

[commit-bot: I haz the power, 2015-02-13 21:43:47]

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

[commit-bot: I haz the power, 2015-02-13 21:43:48]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[nednguyen, 2015-02-13 21:56:37]

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

https://codereview.chromium.org/914083005/diff/60001/tools/telemetry/telemetr...
tools/telemetry/telemetry/value/__init__.py:61: assert isinstance(name, str)
basestring instead of str. The unicode problem in python2 has bitten me one. :)

[eakuefner, 2015-02-17 16:37:59]

New patchsets have been uploaded after l-g-t-m from dtu@chromium.org

[eakuefner, 2015-02-17 16:38:45]

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

https://codereview.chromium.org/914083005/diff/60001/tools/telemetry/telemetr...
tools/telemetry/telemetry/value/__init__.py:61: assert isinstance(name, str)
On 2015/02/13 21:56:37, nednguyen wrote:
> basestring instead of str. The unicode problem in python2 has bitten me one.
:)

Done.

https://codereview.chromium.org/914083005/diff/60001/tools/telemetry/telemetr...
tools/telemetry/telemetry/value/__init__.py:61: assert isinstance(name, str)
On 2015/02/13 21:43:06, dtu wrote:
> Technically should be ValueError.

Done.

[eakuefner, 2015-02-17 16:38:55]

The CQ bit was checked by eakuefner@chromium.org

[commit-bot: I haz the power, 2015-02-17 16:39:10]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/914083005/80001

[commit-bot: I haz the power, 2015-02-17 17:36:19]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-02-17 17:37:10]

Patchset 5 (id:??) landed as
https://crrev.com/1af5584c413372d56601adfc53fd30de002e7e09
Cr-Commit-Position: refs/heads/master@{#316598}