Telemetry: Sanitize a key and add description to speedindex metrics

Telemetry: Sanitize a key and add description to speedindex metrics

BUG=N/A
TEST=./tools/perf/run_benchmark --browser=android-chrome-shell page_cycler.top_10_mobile --pageset-repeat=1 --report-speed-index

Committed: https://crrev.com/20deb3a6dc4d6b128a5eb933578922096ba1beb3
Cr-Commit-Position: refs/heads/master@{#301313}

[shimazu, 2014-10-23 03:35:41]

shimazu@chromium.org changed reviewers:
+ qyearsley@chromium.org

[shimazu, 2014-10-23 03:35:42]

PTAL

[qyearsley, 2014-10-23 17:01:40]

Thanks for adding a description :-)
What do you mean by 'sanitize a key'?

https://codereview.chromium.org/671913004/diff/1/tools/perf/metrics/speedinde...
File tools/perf/metrics/speedindex.py (right):

https://codereview.chromium.org/671913004/diff/1/tools/perf/metrics/speedinde...
tools/perf/metrics/speedindex.py:56: results.current_page, '%s_speed_index' %
chart_name, 'ms', index,
This change from %s.speed_index to %s_speed_index was intentional, right?

https://codereview.chromium.org/671913004/diff/1/tools/perf/metrics/speedinde...
tools/perf/metrics/speedindex.py:57: description='Speed Index. This focuses on a
time when visible parts of '
a time -> time

https://codereview.chromium.org/671913004/diff/1/tools/perf/metrics/speedinde...
tools/perf/metrics/speedindex.py:59: 'first look is "almost" composed. If you
want to get more '
I'm not sure if this description is accurate. Speed index doesn't indicate the
time when any particular event happened -- it's just supposed to be a measure of
load time when is lower when most of the page loads sooner.

https://codereview.chromium.org/671913004/diff/1/tools/perf/metrics/speedinde...
tools/perf/metrics/speedindex.py:60: 'detail, please refer to
http://goo.gl/Rw3d5d . Currently '
No need for the space between the URL and the dot.

https://codereview.chromium.org/671913004/diff/1/tools/perf/metrics/speedinde...
tools/perf/metrics/speedindex.py:61: 'there are two implementation: for Android
and for '
implementation -> implementations

https://codereview.chromium.org/671913004/diff/1/tools/perf/metrics/speedinde...
tools/perf/metrics/speedindex.py:65: 'to catch paint events.'))
I wouldn't say the desktop version is "not good", but it is worth noting that it
it has extra overhead.

I would phrase this sentence as:

"The Android version uses video capture; the Desktop one uses paint events and
has extra overhead to catch paint events.

[shimazu, 2014-10-27 02:33:12]

Thanks for your reviews!
"Sanitize" means '.' is changed to '_'.

https://codereview.chromium.org/671913004/diff/1/tools/perf/metrics/speedinde...
File tools/perf/metrics/speedindex.py (right):

https://codereview.chromium.org/671913004/diff/1/tools/perf/metrics/speedinde...
tools/perf/metrics/speedindex.py:56: results.current_page, '%s_speed_index' %
chart_name, 'ms', index,
On 2014/10/23 17:01:39, qyearsley wrote:
> This change from %s.speed_index to %s_speed_index was intentional, right?

Yes, I saw the result of run_benchmark as 'yourChartName: speed_index' format
though 'yourChartName_speed_index: url' is our expectation. This is caused by
the dot, so I changed the dot to underscore.

https://codereview.chromium.org/671913004/diff/1/tools/perf/metrics/speedinde...
tools/perf/metrics/speedindex.py:57: description='Speed Index. This focuses on a
time when visible parts of '
On 2014/10/23 17:01:39, qyearsley wrote:
> a time -> time

Done.

https://codereview.chromium.org/671913004/diff/1/tools/perf/metrics/speedinde...
tools/perf/metrics/speedindex.py:59: 'first look is "almost" composed. If you
want to get more '
On 2014/10/23 17:01:39, qyearsley wrote:
> I'm not sure if this description is accurate. Speed index doesn't indicate the
> time when any particular event happened -- it's just supposed to be a measure
of
> load time when is lower when most of the page loads sooner.

I tried to add such explanation. I'm not sure how detail I have to write. Is it
okay or a bit too redundant?

https://codereview.chromium.org/671913004/diff/1/tools/perf/metrics/speedinde...
tools/perf/metrics/speedindex.py:60: 'detail, please refer to
http://goo.gl/Rw3d5d . Currently '
On 2014/10/23 17:01:39, qyearsley wrote:
> No need for the space between the URL and the dot.

Done.

https://codereview.chromium.org/671913004/diff/1/tools/perf/metrics/speedinde...
tools/perf/metrics/speedindex.py:61: 'there are two implementation: for Android
and for '
On 2014/10/23 17:01:39, qyearsley wrote:
> implementation -> implementations

Done.

https://codereview.chromium.org/671913004/diff/1/tools/perf/metrics/speedinde...
tools/perf/metrics/speedindex.py:65: 'to catch paint events.'))
On 2014/10/23 17:01:39, qyearsley wrote:
> I wouldn't say the desktop version is "not good", but it is worth noting that
it
> it has extra overhead.
> 
> I would phrase this sentence as:
> 
> "The Android version uses video capture; the Desktop one uses paint events and
> has extra overhead to catch paint events.

Done.

[qyearsley, 2014-10-27 02:55:47]

LGTM, with one note -- I think that some of the explanation could be removed
because the WebPagetest Speed Index site has more information. But it's also OK
as is.

https://codereview.chromium.org/671913004/diff/1/tools/perf/metrics/speedinde...
File tools/perf/metrics/speedindex.py (right):

https://codereview.chromium.org/671913004/diff/1/tools/perf/metrics/speedinde...
tools/perf/metrics/speedindex.py:59: 'first look is "almost" composed. If you
want to get more '
On 2014/10/27 02:33:11, makoto wrote:
> On 2014/10/23 17:01:39, qyearsley wrote:
> > I'm not sure if this description is accurate. Speed index doesn't indicate
the
> > time when any particular event happened -- it's just supposed to be a
measure
> of
> > load time when is lower when most of the page loads sooner.
> 
> I tried to add such explanation. I'm not sure how detail I have to write. Is
it
> okay or a bit too redundant?

I personally think it might be a bit redundant, because that link
(http://goo.gl/Rw3d5d) has more details.

[shimazu, 2014-10-27 03:06:16]

Thanks, and I'll commit as is.

[shimazu, 2014-10-27 03:06:22]

The CQ bit was checked by shimazu@chromium.org

[commit-bot: I haz the power, 2014-10-27 03:06:54]

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

[commit-bot: I haz the power, 2014-10-27 03:59:53]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2014-10-27 04:00:28]

Patchset 2 (id:??) landed as
https://crrev.com/20deb3a6dc4d6b128a5eb933578922096ba1beb3
Cr-Commit-Position: refs/heads/master@{#301313}