Add new UMA stats for parallelizable download

Add new UMA stats for parallelizable download

To study the effectiveness of parallel download,
we want to compare the bandwidth if parallel downloads are not parallelized.
Because parallel download has requirements on file size, so comparing with all
other downloads is be fair.
This CL records the UMA metrics for downloads that satisfy the requirement
of parallel download, while don't use parallel requests.

BUG=644352, 712892
Review-Url: https://codereview.chromium.org/2823273004
Cr-Commit-Position: refs/heads/master@{#466128}
Committed: https://chromium.googlesource.com/chromium/src/+/2269e30d4ea0874704bfbd4021c087ba960e7ea5

[qinmin, 2017-04-18 23:22:52]

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

[qinmin, 2017-04-18 23:22:53]

PTAL
+isherman@ for histogram
+alexmos@ for content/public/test/test_file_error_injector.cc

[alexmos, 2017-04-19 00:26:55]

content/public/test/ LGTM

[Ilya Sherman, 2017-04-19 00:29:41]

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

https://codereview.chromium.org/2823273004/diff/1/content/browser/download/do...
content/browser/download/download_stats.cc:779:
UMA_HISTOGRAM_ENUMERATION(histogram_name, type,
UMA_HISTOGRAM macros require a runtime-constant name.   Please use the function
UmaHistogramEnumeration instead.

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

https://codereview.chromium.org/2823273004/diff/1/tools/metrics/histograms/hi...
tools/metrics/histograms/histograms.xml:13489: -</histogram>
Please mark these as <obsolete> rather than deleting their metadata.

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

https://codereview.chromium.org/2823273004/diff/1/tools/metrics/histograms/hi...
tools/metrics/histograms/histograms.xml:123083: +</histogram_suffixes>
nit: Please combine this with the histogram_suffixes element below -- or am I
missing something?

https://codereview.chromium.org/2823273004/diff/1/tools/metrics/histograms/hi...
tools/metrics/histograms/histograms.xml:123094: +      label="Bandwidth without
using parallel requests."/>
How does this differ from ParallelizableDownload?

[xingliu, 2017-04-19 02:18:11]

In general looks good, some nits.

https://codereview.chromium.org/2823273004/diff/1/content/browser/download/do...
File content/browser/download/download_file.h (right):

https://codereview.chromium.org/2823273004/diff/1/content/browser/download/do...
content/browser/download/download_file.h:50: virtual void Initialize(const
InitializeCallback& callback,
FYI: sorry that I just modified this function, so you probably need to rebase.
It might cause some issue if we want to cherry pick this commit, we might need
to cherry pick another first.

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

https://codereview.chromium.org/2823273004/diff/1/content/browser/download/do...
content/browser/download/download_file_impl.cc:159: record_stream_bandwidth_ =
is_parallelizable;
Can we initialize this to false in the constructor?

https://codereview.chromium.org/2823273004/diff/1/content/browser/download/do...
File content/browser/download/download_job.h (right):

https://codereview.chromium.org/2823273004/diff/1/content/browser/download/do...
content/browser/download/download_job.h:45: virtual bool IsParallelizable()
const;
nit: Can we add a comment here.

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

https://codereview.chromium.org/2823273004/diff/1/content/browser/download/do...
content/browser/download/download_job_factory.cc:23: bool
ShouldUseParallelDownload(const DownloadCreateInfo& create_info) {
nit: Maybe also change the function name?

[qinmin, 2017-04-19 22:42:34]

https://codereview.chromium.org/2823273004/diff/1/content/browser/download/do...
File content/browser/download/download_file.h (right):

https://codereview.chromium.org/2823273004/diff/1/content/browser/download/do...
content/browser/download/download_file.h:50: virtual void Initialize(const
InitializeCallback& callback,
On 2017/04/19 02:18:11, xingliu wrote:
> FYI: sorry that I just modified this function, so you probably need to rebase.
> It might cause some issue if we want to cherry pick this commit, we might need
> to cherry pick another first.

rebased in the new PS

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

https://codereview.chromium.org/2823273004/diff/1/content/browser/download/do...
content/browser/download/download_file_impl.cc:159: record_stream_bandwidth_ =
is_parallelizable;
On 2017/04/19 02:18:11, xingliu wrote:
> Can we initialize this to false in the constructor? 

Done.

https://codereview.chromium.org/2823273004/diff/1/content/browser/download/do...
File content/browser/download/download_job.h (right):

https://codereview.chromium.org/2823273004/diff/1/content/browser/download/do...
content/browser/download/download_job.h:45: virtual bool IsParallelizable()
const;
On 2017/04/19 02:18:11, xingliu wrote:
> nit: Can we add a comment here.

Done.

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

https://codereview.chromium.org/2823273004/diff/1/content/browser/download/do...
content/browser/download/download_job_factory.cc:23: bool
ShouldUseParallelDownload(const DownloadCreateInfo& create_info) {
On 2017/04/19 02:18:11, xingliu wrote:
> nit: Maybe also change the function name?

Done.

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

https://codereview.chromium.org/2823273004/diff/1/content/browser/download/do...
content/browser/download/download_stats.cc:779:
UMA_HISTOGRAM_ENUMERATION(histogram_name, type,
On 2017/04/19 00:29:41, Ilya Sherman wrote:
> UMA_HISTOGRAM macros require a runtime-constant name.   Please use the
function
> UmaHistogramEnumeration instead.

Done.

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

https://codereview.chromium.org/2823273004/diff/1/tools/metrics/histograms/hi...
tools/metrics/histograms/histograms.xml:13489: -</histogram>
On 2017/04/19 00:29:41, Ilya Sherman wrote:
> Please mark these as <obsolete> rather than deleting their metadata.

Done.

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

https://codereview.chromium.org/2823273004/diff/1/tools/metrics/histograms/hi...
tools/metrics/histograms/histograms.xml:123083: +</histogram_suffixes>
On 2017/04/19 00:29:41, Ilya Sherman wrote:
> nit: Please combine this with the histogram_suffixes element below -- or am I
> missing something?

Sorry, the affected-histogram name should be Download.Counts, corrected.

https://codereview.chromium.org/2823273004/diff/1/tools/metrics/histograms/hi...
tools/metrics/histograms/histograms.xml:123094: +      label="Bandwidth without
using parallel requests."/>
On 2017/04/19 00:29:41, Ilya Sherman wrote:
> How does this differ from ParallelizableDownload?

Fixed the affected-histogram for ParallelizableDownload. This is for bandwidth
metrics, the ParallelizableDownload is for Download.counts.

[Ilya Sherman, 2017-04-19 22:45:59]

Metrics LGTM, thanks.

[David Trainor- moved to gerrit, 2017-04-19 22:56:28]

lgtm

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

https://codereview.chromium.org/2823273004/diff/20001/content/browser/downloa...
content/browser/download/download_job_factory.cc:52: ?
ParallelDownloadCreationEvent::STARTED_PARALLEL_DOWNLOAD
Is this still valid?  We didn't technically start a parallel download, we
started a parallelizable one.  Is this ok?

[xingliu, 2017-04-19 23:04:16]

lgtm.

[qinmin, 2017-04-20 17:00:28]

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

https://codereview.chromium.org/2823273004/diff/20001/content/browser/downloa...
content/browser/download/download_job_factory.cc:52: ?
ParallelDownloadCreationEvent::STARTED_PARALLEL_DOWNLOAD
On 2017/04/19 22:56:28, David Trainor-ping if over 24h wrote:
> Is this still valid?  We didn't technically start a parallel download, we
> started a parallelizable one.  Is this ok?

This is fine. The if statement on line 47 ensures that not all parallelizable
download will reach here. And if they reach this line, they belongs to parallel
download.

[qinmin, 2017-04-20 17:00:34]

The CQ bit was checked by qinmin@chromium.org

[qinmin, 2017-04-20 17:00:35]

The patchset sent to the CQ was uploaded after l-g-t-m from alexmos@chromium.org
Link to the patchset: https://codereview.chromium.org/2823273004/#ps20001
(title: "addressing comments")

[commit-bot: I haz the power, 2017-04-20 17:01:23]

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-04-20 17:12:12]

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

[commit-bot: I haz the power, 2017-04-20 17:12:13]

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

[qinmin, 2017-04-20 17:29:16]

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

[commit-bot: I haz the power, 2017-04-20 17:29:50]

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-04-20 17:50:57]

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

[commit-bot: I haz the power, 2017-04-20 17:50:58]

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

[qinmin, 2017-04-20 18:15:11]

Patchset #3 (id:40001) has been deleted

[qinmin, 2017-04-20 18:15:16]

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

[commit-bot: I haz the power, 2017-04-20 18:15:53]

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-04-20 18:38:24]

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

[commit-bot: I haz the power, 2017-04-20 18:38:26]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[qinmin, 2017-04-20 19:13:00]

Patchset #3 (id:60001) has been deleted

[qinmin, 2017-04-20 19:13:06]

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

[commit-bot: I haz the power, 2017-04-20 19:13:31]

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-04-20 20:57:21]

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

[commit-bot: I haz the power, 2017-04-20 20:57:22]

Dry run: This issue passed the CQ dry run.

[qinmin, 2017-04-20 20:57:39]

The CQ bit was checked by qinmin@chromium.org

[qinmin, 2017-04-20 20:57:39]

The patchset sent to the CQ was uploaded after l-g-t-m from
xingliu@chromium.org, isherman@chromium.org, dtrainor@chromium.org,
alexmos@chromium.org
Link to the patchset: https://codereview.chromium.org/2823273004/#ps80001
(title: "fix test")

[commit-bot: I haz the power, 2017-04-20 20:58:36]

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-04-20 21:04:56]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1492721859208020,
"parent_rev": "f32cf78bd306f2750fbc5deb14e33169c894be70", "commit_rev":
"2269e30d4ea0874704bfbd4021c087ba960e7ea5"}

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

Description was changed from

==========
Add new UMA stats for parallelizable download

To study the effectiveness of parallel download,
we want to compare the bandwidth if parallel downloads are not parallelized.
Because parallel download has requirements on file size, so comparing with all
other downloads is be fair.
This CL records the UMA metrics for downloads that satisfy the requirement
of parallel download, while don't use parallel requests.

BUG=644352,712892
==========

to

==========
Add new UMA stats for parallelizable download

To study the effectiveness of parallel download,
we want to compare the bandwidth if parallel downloads are not parallelized.
Because parallel download has requirements on file size, so comparing with all
other downloads is be fair.
This CL records the UMA metrics for downloads that satisfy the requirement
of parallel download, while don't use parallel requests.

BUG=644352,712892

Review-Url: https://codereview.chromium.org/2823273004
Cr-Commit-Position: refs/heads/master@{#466128}
Committed:
https://chromium.googlesource.com/chromium/src/+/2269e30d4ea0874704bfbd4021c0...
==========

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

Committed patchset #3 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/2269e30d4ea0874704bfbd4021c0...