Android / Telemetry: make surface_stats_collector more robust.

Android / Telemetry: make surface_stats_collector more robust.

Rather than failing the entire test suite, report empty data when
surface stats is unavailable.

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

[bulach, 2013-06-05 11:11:30]

ptal.. note that the underlying cause needs more investigation, but I suppose
this class returning a consistent set of results is a good move anyways..

[Tom Hudson, 2013-06-05 11:20:05]

What effect is reporting a bunch of 0s going to have on the bots / GASP? Could
that be seen as an improvement rather than a regression?

[bulach, 2013-06-05 11:28:39]

the graphs would have sharp dips.. afaict, gasp is smart enough to disregard
outliers.

right now, this is a hard-failure that discards all results , not just surface
stats, for all pages..

I suppose having 0s, but other results, would do more good than harm... as
mentioned in the bug, I agree there shouldn't be any 0s in the first place :)
however, at this level we have two options:
- return the sporadic empty list, and let higher up layers decide what to do.
- return a "constant" set of metrics so that higher up layers can at least plot
the graphs and do something useful...

wdyt?

[Tom Hudson, 2013-06-05 11:31:36]

If this test fails 10 times in a row, will 0s still be seen as outliers, or will
they be the new normal?

It'd be nice if we had a "NO DATA" value, like real grown-up datasets do, but I
don't think we do?
Or would that come in at a later stage in the pipeline?

Sure, 0s LGTM-ish, I'm just over-thinking to anticipate future problems. Let's
fix those when we come to them, if there isn't something obvious enough to make
us reconsider this now.

[bulach, 2013-06-05 13:00:30]

yeah, NO DATA would be nice! :)

just some points to consider, and questions:
- at this level, sending None rather the 0 is trivial.
- higher up at Telemetry, it expects:
-- all pages have the same set of metrics
-- all metrics are numeric
- I don't know about the dashboards.

so the questions:
-- I can change Telemetry to relax the numeric requirement, and actually treat
as a "failure" there, which is less drastic than a test suite crash. I'll try a
quick patchset here in a sec to show what this would mean, but please let me
know if it doesn't make sense :)

-- would the dashboards be happy with "None" rather than "0" ?

[Sami, 2013-06-05 14:04:10]

lgtm. Like we discussed, let's land this zero-reporting method now to unblock
the tests and implement some kind of DATA MISSING feature at the upper levels.

[bulach, 2013-06-05 14:10:02]

ok, I'll go ahead with this one, and start the alternatives on a separate
thread. thanks!

[commit-bot: I haz the power, 2013-06-05 14:10:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bulach@chromium.org/16438003/1

[bulach, 2013-06-05 14:43:23]

ok, sorry about the churn :)
chatted with tom and sami, the latest patch:
- makes surface_stats_collector.py more consistent. it'll always return a result
object, and then the values may be None.
- tiny change to the standalone utility.
- on telemetry, it seems preferrable to have a "MeasurementFailure", so whilst
it doesn't break the entire suite, the specific page will be flagged and its
result will be ignored.

ptal

[Tom Hudson, 2013-06-05 14:44:34]

lgtm

[commit-bot: I haz the power, 2013-06-05 14:45:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bulach@chromium.org/16438003/4005

[commit-bot: I haz the power, 2013-06-05 14:56:13]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[Sami, 2013-06-05 15:27:24]

https://codereview.chromium.org/16438003/diff/4005/build/android/surface_stat...
File build/android/surface_stats.py (right):

https://codereview.chromium.org/16438003/diff/4005/build/android/surface_stat...
build/android/surface_stats.py:32: if (fields != ['all'] and not result.name in
fields) or not result.value:
The last part should be "or result.value is None" since 0 is a valid value for
some of the metrics :)

[bulach, 2013-06-05 15:38:01]

a-ha! good point, thanks sami! ptal

https://codereview.chromium.org/16438003/diff/4005/build/android/surface_stat...
File build/android/surface_stats.py (right):

https://codereview.chromium.org/16438003/diff/4005/build/android/surface_stat...
build/android/surface_stats.py:32: if (fields != ['all'] and not result.name in
fields) or not result.value:
On 2013/06/05 15:27:25, Sami wrote:
> The last part should be "or result.value is None" since 0 is a valid value for
> some of the metrics :)

Done.

[Sami, 2013-06-05 15:57:44]

lgtm, thanks!

[commit-bot: I haz the power, 2013-06-06 09:40:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bulach@chromium.org/16438003/2005

[commit-bot: I haz the power, 2013-06-06 09:49:58]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[bulach, 2013-06-06 12:50:56]

+marja for OWNERS on tools/perf/perf_tools/smoothness_measurement.py

[marja, 2013-06-06 13:03:32]

I didn't write smoothness benchmark, but looks kinda sane, so lgtm.

https://chromiumcodereview.appspot.com/16438003/diff/2005/build/android/surfa...
File build/android/surface_stats.py (right):

https://chromiumcodereview.appspot.com/16438003/diff/2005/build/android/surfa...
build/android/surface_stats.py:33: result.value is None):
-> "or not result.value"

[Sami, 2013-06-06 13:10:03]

https://chromiumcodereview.appspot.com/16438003/diff/2005/build/android/surfa...
File build/android/surface_stats.py (right):

https://chromiumcodereview.appspot.com/16438003/diff/2005/build/android/surfa...
build/android/surface_stats.py:33: result.value is None):
On 2013/06/06 13:03:32, marja wrote:
> -> "or not result.value"

See above; 0 is a valid result.value but None isn't :)

[commit-bot: I haz the power, 2013-06-06 13:30:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bulach@chromium.org/16438003/2005

[bulach, 2013-06-06 13:30:47]

thanks marja! given sami's explanation, CQing. :)

[marja, 2013-06-06 13:31:44]

https://chromiumcodereview.appspot.com/16438003/diff/2005/build/android/surfa...
File build/android/surface_stats.py (right):

https://chromiumcodereview.appspot.com/16438003/diff/2005/build/android/surfa...
build/android/surface_stats.py:33: result.value is None):
On 2013/06/06 13:10:03, Sami wrote:
> On 2013/06/06 13:03:32, marja wrote:
> > -> "or not result.value"
> 
> See above; 0 is a valid result.value but None isn't :)

Ah, alright :)

[commit-bot: I haz the power, 2013-06-06 16:28:01]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-06-06 18:13:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bulach@chromium.org/16438003/2005

[commit-bot: I haz the power, 2013-06-06 21:10:37]

Change committed as 204593