UMA stats for MTP file system in Chrome OS Files.app.

UMA stats for MTP file system in Chrome OS Files.app.

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

[kinaba, 2014-05-01 03:38:45]

+hirono: file_manager/*
+isherman: histogram.xml

PTAL. Thanks!

[hirono, 2014-05-01 03:52:25]

lgtm!

[Ilya Sherman, 2014-05-01 21:15:24]

https://codereview.chromium.org/267573005/diff/1/chrome/browser/chromeos/file...
File chrome/browser/chromeos/file_manager/volume_manager.cc (right):

https://codereview.chromium.org/267573005/diff/1/chrome/browser/chromeos/file...
chrome/browser/chromeos/file_manager/volume_manager.cc:732:
UMA_HISTOGRAM_COUNTS("FileBrowser.MTPMount", 1);
You're currently allocating space for 50 buckets, and only using one of them. 
Please only allocate as many buckets as you need.

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

https://codereview.chromium.org/267573005/diff/1/tools/metrics/histograms/his...
tools/metrics/histograms/histograms.xml:6882: +    Chrome OS File Browser:
counts the number of times MTP devices are inserted
What is "MTP"?  Please spell out acronyms (it's fine to keep the acronym as
well, if you think it's useful).

https://codereview.chromium.org/267573005/diff/1/tools/metrics/histograms/his...
tools/metrics/histograms/histograms.xml:6885: +</histogram>
I strongly encourage you to design your metric with some sort of built-in
baseline.  It's much harder to interpret a raw number like "10,000" MTP devices
were mounted, vs. 0.01% or 10% of all mounted devices were MTP devices.

[kinaba, 2014-05-08 04:15:41]

https://codereview.chromium.org/267573005/diff/1/chrome/browser/chromeos/file...
File chrome/browser/chromeos/file_manager/volume_manager.cc (right):

https://codereview.chromium.org/267573005/diff/1/chrome/browser/chromeos/file...
chrome/browser/chromeos/file_manager/volume_manager.cc:732:
UMA_HISTOGRAM_COUNTS("FileBrowser.MTPMount", 1);
On 2014/05/01 21:15:24, Ilya Sherman wrote:
> You're currently allocating space for 50 buckets, and only using one of them. 
> Please only allocate as many buckets as you need.

Done.

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

https://codereview.chromium.org/267573005/diff/1/tools/metrics/histograms/his...
tools/metrics/histograms/histograms.xml:6882: +    Chrome OS File Browser:
counts the number of times MTP devices are inserted
On 2014/05/01 21:15:24, Ilya Sherman wrote:
> What is "MTP"?  Please spell out acronyms (it's fine to keep the acronym as
> well, if you think it's useful).

Done.

https://codereview.chromium.org/267573005/diff/1/tools/metrics/histograms/his...
tools/metrics/histograms/histograms.xml:6885: +</histogram>
On 2014/05/01 21:15:24, Ilya Sherman wrote:
> I strongly encourage you to design your metric with some sort of built-in
> baseline.  It's much harder to interpret a raw number like "10,000" MTP
devices
> were mounted, vs. 0.01% or 10% of all mounted devices were MTP devices.

Done. Changed to record the counts for all volume types.

[kinaba, 2014-05-08 04:17:49]

On 2014/05/08 04:15:41, kinaba wrote:
> Done. Changed to record the counts for all volume types.

Sorry for my slow response (I was in vacation for a while), could you take a
look once again?
I update the CL to record stats for all volume types not only MTP, to make them
comparable.

[Ilya Sherman, 2014-05-08 21:16:10]

LGTM, thanks.

https://codereview.chromium.org/267573005/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/file_manager/volume_manager.h (right):

https://codereview.chromium.org/267573005/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/file_manager/volume_manager.h:54: VOLUME_TYPE_TESTING, 
// Used only in tests.
nit: Since enums used for histograms should be append-only, I'd recommend making
this be the first item in the enum rather than the last.  That way, you don't
end up with gaps in terms of which items are relevant for histograms.xml.

(I'd also add a quick comment to document that this enum should be treated as
append-only.)

[kinaba, 2014-05-09 02:09:26]

https://codereview.chromium.org/267573005/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/file_manager/volume_manager.h (right):

https://codereview.chromium.org/267573005/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/file_manager/volume_manager.h:54: VOLUME_TYPE_TESTING, 
// Used only in tests.
On 2014/05/08 21:16:11, Ilya Sherman wrote:
> nit: Since enums used for histograms should be append-only, I'd recommend
making
> this be the first item in the enum rather than the last.  That way, you don't
> end up with gaps in terms of which items are relevant for histograms.xml.
> 
> (I'd also add a quick comment to document that this enum should be treated as
> append-only.)

Thanks for the suggestion. Done.

[kinaba, 2014-05-09 02:09:51]

The CQ bit was checked by kinaba@chromium.org

[commit-bot: I haz the power, 2014-05-09 02:18:35]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/kinaba@chromium.org/267573005/60001

[commit-bot: I haz the power, 2014-05-10 03:08:14]

Change committed as 269510