Add histograms about files copied or movied files from or to Drive.

Add histograms about files copied or movied files from or to Drive.

This patch adds temporary histograms in order to investigate how to optimize
downloading and uploading multiple files.

NOTRY=true
TEST=Tested manually.
BUG=229650
Committed: https://crrev.com/b400464aca6815dfb4967b844103c37ae1b3dda9
Cr-Commit-Position: refs/heads/master@{#299445}

[mtomasz, 2014-10-08 03:27:07]

mtomasz@chromium.org changed reviewers:
+ kinaba@chromium.org

[mtomasz, 2014-10-08 03:27:16]

@kinaba: PTAL. Thanks.

[kinaba, 2014-10-08 14:13:29]

https://codereview.chromium.org/640543002/diff/1/chrome/browser/chromeos/driv...
File chrome/browser/chromeos/drive/job_scheduler.cc (right):

https://codereview.chromium.org/640543002/diff/1/chrome/browser/chromeos/driv...
chrome/browser/chromeos/drive/job_scheduler.cc:612:
CollectCopyHistogramSample("Drive.CopyFromDriveFileSizeKB",
Strictly speaking, this counts not only copying to other volumes but also mere
file open on Drive.
Ditto for "CopyToDrive" that also counts, for instance, uploading of modified
files on Drive.

What about renaming to DownloadFromDriveFileSize and UploadToDriveFileSize
resepctively?

[mtomasz, 2014-10-09 02:24:45]

https://codereview.chromium.org/640543002/diff/1/chrome/browser/chromeos/driv...
File chrome/browser/chromeos/drive/job_scheduler.cc (right):

https://codereview.chromium.org/640543002/diff/1/chrome/browser/chromeos/driv...
chrome/browser/chromeos/drive/job_scheduler.cc:612:
CollectCopyHistogramSample("Drive.CopyFromDriveFileSizeKB",
On 2014/10/08 14:13:28, kinaba wrote:
> Strictly speaking, this counts not only copying to other volumes but also mere
> file open on Drive.
> Ditto for "CopyToDrive" that also counts, for instance, uploading of modified
> files on Drive.
> 
> What about renaming to DownloadFromDriveFileSize and UploadToDriveFileSize
> resepctively?

SGTM. Done.

[kinaba, 2014-10-09 02:41:08]

lgtm

[mtomasz, 2014-10-09 02:41:40]

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

[mtomasz, 2014-10-09 02:41:52]

@isherman: PTAL at histograms. Thanks.

[Ilya Sherman, 2014-10-09 04:44:52]

https://codereview.chromium.org/640543002/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/drive/job_scheduler.cc (right):

https://codereview.chromium.org/640543002/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/drive/job_scheduler.cc:142: histogram_name, size / 1024,
1, 1024 * 1024 /* 1 GB */, 50);
Unfortunately, this won't work.  The UMA_HISTOGRAM_* macros require the
histogram name to be a run-time constant, as they declare a static local
variable to cache the constructed histogram.  You'll need to either define this
as a macro that wraps the UMA_HISTOGRAM_* macros, or make separate calls for
each differently named histogram.

https://codereview.chromium.org/640543002/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/drive/job_scheduler.cc:142: histogram_name, size / 1024,
1, 1024 * 1024 /* 1 GB */, 50);
Btw, did you consider using UMA_HISTOGRAM_MEMORY_MB?  I do realize that that
cuts off at 1MB rather than at 1KB, as you've defined here.

https://codereview.chromium.org/640543002/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/extensions/file_manager/private_api_file_system.cc
(right):

https://codereview.chromium.org/640543002/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/extensions/file_manager/private_api_file_system.cc:9:
#include "base/metrics/histogram.h"
nit: Is this needed?

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

https://codereview.chromium.org/640543002/diff/20001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:5817: +<histogram
name="Drive.DownloadFromDriveFileSizeKB">
nit: Please add a units attribute.

https://codereview.chromium.org/640543002/diff/20001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:5818: + 
<owner>mtomasz@google.com</owner>
nit: I suppose it's fine either way, but I think @chromium.org addresses are
preferred within the Chromium repository.

https://codereview.chromium.org/640543002/diff/20001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:5821: +    gathering data for
crbug.com/229650.
nit: Please prepend "http://" to the link, for increased likelihood of
autolinkification by tools.

[mtomasz, 2014-10-14 04:46:01]

https://codereview.chromium.org/640543002/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/drive/job_scheduler.cc (right):

https://codereview.chromium.org/640543002/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/drive/job_scheduler.cc:142: histogram_name, size / 1024,
1, 1024 * 1024 /* 1 GB */, 50);
On 2014/10/09 04:44:51, Ilya Sherman wrote:
> Btw, did you consider using UMA_HISTOGRAM_MEMORY_MB?  I do realize that that
> cuts off at 1MB rather than at 1KB, as you've defined here.

Yep, I considered, but the range is just not good for us here. We need to
register very small files as well as large files (1GB).

https://codereview.chromium.org/640543002/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/drive/job_scheduler.cc:142: histogram_name, size / 1024,
1, 1024 * 1024 /* 1 GB */, 50);
On 2014/10/09 04:44:51, Ilya Sherman wrote:
> Unfortunately, this won't work.  The UMA_HISTOGRAM_* macros require the
> histogram name to be a run-time constant, as they declare a static local
> variable to cache the constructed histogram.  You'll need to either define
this
> as a macro that wraps the UMA_HISTOGRAM_* macros, or make separate calls for
> each differently named histogram.

Done.

https://codereview.chromium.org/640543002/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/extensions/file_manager/private_api_file_system.cc
(right):

https://codereview.chromium.org/640543002/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/extensions/file_manager/private_api_file_system.cc:9:
#include "base/metrics/histogram.h"
On 2014/10/09 04:44:51, Ilya Sherman wrote:
> nit: Is this needed?

Done.

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

https://codereview.chromium.org/640543002/diff/20001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:5817: +<histogram
name="Drive.DownloadFromDriveFileSizeKB">
On 2014/10/09 04:44:52, Ilya Sherman wrote:
> nit: Please add a units attribute.

Done.

https://codereview.chromium.org/640543002/diff/20001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:5818: + 
<owner>mtomasz@google.com</owner>
On 2014/10/09 04:44:52, Ilya Sherman wrote:
> nit: I suppose it's fine either way, but I think @chromium.org addresses are
> preferred within the Chromium repository.

Done.

https://codereview.chromium.org/640543002/diff/20001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:5821: +    gathering data for
crbug.com/229650.
On 2014/10/09 04:44:52, Ilya Sherman wrote:
> nit: Please prepend "http://" to the link, for increased likelihood of
> autolinkification by tools.

Done.

[Ilya Sherman, 2014-10-14 06:35:46]

LGTM, thanks.

[mtomasz, 2014-10-14 06:37:12]

The CQ bit was checked by mtomasz@chromium.org

[commit-bot: I haz the power, 2014-10-14 06:37:31]

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

[commit-bot: I haz the power, 2014-10-14 06:44:24]

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

[commit-bot: I haz the power, 2014-10-14 06:44:26]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[mtomasz, 2014-10-14 07:40:21]

The CQ bit was checked by mtomasz@chromium.org

[commit-bot: I haz the power, 2014-10-14 07:41:01]

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

[commit-bot: I haz the power, 2014-10-14 07:41:28]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2014-10-14 08:15:24]

Patchset 3 (id:??) landed as
https://crrev.com/b400464aca6815dfb4967b844103c37ae1b3dda9
Cr-Commit-Position: refs/heads/master@{#299445}