Parallelizable download metrics.

Parallelizable download metrics.

Add average bandwidth, download time, and file size metrics for
parallelizable downloads.

Also add some suffixes to categorize these 3 metrics with file size.

BUG=723139
Review-Url: https://codereview.chromium.org/2885793004
Cr-Commit-Position: refs/heads/master@{#473033}
Committed: https://chromium.googlesource.com/chromium/src/+/5697152d0632f1924b5150132ea5d4d1ebb59752

[xingliu, 2017-05-17 00:32:24]

xingliu@chromium.org changed reviewers:
+ dtrainor@chromium.org, qinmin@chromium.org

[xingliu, 2017-05-17 00:32:24]

Hi, PTAL for general review, we're still discussing our approach.

[David Trainor- moved to gerrit, 2017-05-17 17:23:54]

lgtm % nits

https://codereview.chromium.org/2885793004/diff/1/content/browser/download/do...
File content/browser/download/download_stats.cc (right):

https://codereview.chromium.org/2885793004/diff/1/content/browser/download/do...
content/browser/download/download_stats.cc:898: for (size_t n =
arraysize(kParallelizableBucketKb); i < n; ++i) {
do we need n or can we just do ; i < arraysize(...); ++i?

https://codereview.chromium.org/2885793004/diff/1/tools/metrics/histograms/hi...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2885793004/diff/1/tools/metrics/histograms/hi...
tools/metrics/histograms/histograms.xml:14343: +<histogram base="true"
name="Download.Parallelizable.DownloadTime" units="ms">
Add something like the following?  I seem to see this sometimes.

<!-- Name completed by histogram_suffixes name="ParallelizableDownloadFileSize"
-->

https://codereview.chromium.org/2885793004/diff/1/tools/metrics/histograms/hi...
tools/metrics/histograms/histograms.xml:90212: +      label="Parallelizable
download with file size less or equal to 1MB."/>
between 0.5 and 1mb?  Same for all others?

[xingliu, 2017-05-17 18:02:44]

The CQ bit was checked by xingliu@chromium.org to run a CQ dry run

[xingliu, 2017-05-17 18:04:11]

xingliu@chromium.org changed reviewers:
+ isherman@chromium.org

[xingliu, 2017-05-17 18:04:12]

Thanks for the review, 

+isherman@ for histogram review, we're trying to associate some metrics with
another metric(file size) with suffixes.

https://codereview.chromium.org/2885793004/diff/1/content/browser/download/do...
File content/browser/download/download_stats.cc (right):

https://codereview.chromium.org/2885793004/diff/1/content/browser/download/do...
content/browser/download/download_stats.cc:898: for (size_t n =
arraysize(kParallelizableBucketKb); i < n; ++i) {
On 2017/05/17 17:23:53, David Trainor-ping if over 24h wrote:
> do we need n or can we just do ; i < arraysize(...); ++i?

Trying to avoid evaluate array size on every iteration here, not sure if
compiler can optimize this.

https://codereview.chromium.org/2885793004/diff/1/tools/metrics/histograms/hi...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2885793004/diff/1/tools/metrics/histograms/hi...
tools/metrics/histograms/histograms.xml:14343: +<histogram base="true"
name="Download.Parallelizable.DownloadTime" units="ms">
On 2017/05/17 17:23:53, David Trainor-ping if over 24h wrote:
> Add something like the following?  I seem to see this sometimes.
> 
> <!-- Name completed by histogram_suffixes
name="ParallelizableDownloadFileSize"
> -->

Done.

https://codereview.chromium.org/2885793004/diff/1/tools/metrics/histograms/hi...
tools/metrics/histograms/histograms.xml:90212: +      label="Parallelizable
download with file size less or equal to 1MB."/>
On 2017/05/17 17:23:53, David Trainor-ping if over 24h wrote:
> between 0.5 and 1mb?  Same for all others?

Done.

[commit-bot: I haz the power, 2017-05-17 18:05:02]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Ilya Sherman, 2017-05-17 19:13:07]

https://codereview.chromium.org/2885793004/diff/20001/content/browser/downloa...
File content/browser/download/download_stats.cc (right):

https://codereview.chromium.org/2885793004/diff/20001/content/browser/downloa...
content/browser/download/download_stats.cc:894: 256);
It looks like you are recording a histogram with 256 buckets.  That's quite
expensive!  Could you use fewer buckets?  The standard is to use 50 buckets. 
How much granularity do you need?

https://codereview.chromium.org/2885793004/diff/20001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2885793004/diff/20001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:90222: +      label="Parallelizable
download with file size greater to 50MB."/>
nit: s/greater to/greater than

[qinmin, 2017-05-17 19:43:26]

lgtm % ishermans's comments

https://codereview.chromium.org/2885793004/diff/20001/content/browser/downloa...
File content/browser/download/download_stats.cc (right):

https://codereview.chromium.org/2885793004/diff/20001/content/browser/downloa...
content/browser/download/download_stats.cc:891: int64_t max = 1024 * 1024 *
1024;
nit: make this a constant

[commit-bot: I haz the power, 2017-05-17 20:21:05]

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

[commit-bot: I haz the power, 2017-05-17 20:21:06]

Dry run: This issue passed the CQ dry run.

[xingliu, 2017-05-17 21:20:18]

Thanks for the review. PTAL.

https://codereview.chromium.org/2885793004/diff/20001/content/browser/downloa...
File content/browser/download/download_stats.cc (right):

https://codereview.chromium.org/2885793004/diff/20001/content/browser/downloa...
content/browser/download/download_stats.cc:891: int64_t max = 1024 * 1024 *
1024;
On 2017/05/17 19:43:26, qinmin wrote:
> nit: make this a constant

Done.

https://codereview.chromium.org/2885793004/diff/20001/content/browser/downloa...
content/browser/download/download_stats.cc:894: 256);
On 2017/05/17 19:13:07, Ilya Sherman wrote:
> It looks like you are recording a histogram with 256 buckets.  That's quite
> expensive!  Could you use fewer buckets?  The standard is to use 50 buckets. 
> How much granularity do you need?

Done. Also lower the maximum size.

[Ilya Sherman, 2017-05-17 21:28:05]

Metrics LGTM, thanks.

[xingliu, 2017-05-17 21:30:45]

The CQ bit was checked by xingliu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-17 21:32:35]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-18 04:28:26]

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

[commit-bot: I haz the power, 2017-05-18 04:28:27]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[xingliu, 2017-05-18 20:48:39]

The CQ bit was checked by xingliu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-18 20:50:11]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[xingliu, 2017-05-18 20:52:14]

Thanks for all the review.

According to the discussion in today's parallel download meeting, suffixes for
bucketing by file size have been removed. This CL will be submitted to CQ around
5pm today.

[Ilya Sherman, 2017-05-18 21:45:01]

Ceteris paribus, I'm all for recording fewer histograms, so still LGTM  =)

[commit-bot: I haz the power, 2017-05-18 22:29:31]

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

[commit-bot: I haz the power, 2017-05-18 22:29:32]

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

[xingliu, 2017-05-18 23:55:23]

The CQ bit was checked by xingliu@chromium.org

[xingliu, 2017-05-18 23:55:24]

The patchset sent to the CQ was uploaded after l-g-t-m from
dtrainor@chromium.org, qinmin@chromium.org
Link to the patchset: https://codereview.chromium.org/2885793004/#ps80001
(title: "Some change according to meeting.")

[commit-bot: I haz the power, 2017-05-18 23:56:28]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-19 02:16:40]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1495151723982150,
"parent_rev": "eb6e744f08cc8087eb7836753d98d599c28a80f7", "commit_rev":
"5697152d0632f1924b5150132ea5d4d1ebb59752"}

[commit-bot: I haz the power, 2017-05-19 02:16:53]

Description was changed from

==========
Parallelizable download metrics.

Add average bandwidth, download time, and file size metrics for
parallelizable downloads.

Also add some suffixes to categorize these 3 metrics with file size.

BUG=723139
==========

to

==========
Parallelizable download metrics.

Add average bandwidth, download time, and file size metrics for
parallelizable downloads.

Also add some suffixes to categorize these 3 metrics with file size.

BUG=723139

Review-Url: https://codereview.chromium.org/2885793004
Cr-Commit-Position: refs/heads/master@{#473033}
Committed:
https://chromium.googlesource.com/chromium/src/+/5697152d0632f1924b5150132ea5...
==========

[commit-bot: I haz the power, 2017-05-19 02:16:55]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/5697152d0632f1924b5150132ea5...