Geolocation: add UMA histograms for events and response codes.

Geolocation: add UMA histograms for events and response codes.

It is currently unknown whether NetworkLocationRequest is actually reliable.
This adds UMA histograms for the server response codes, and for some
significant events in the lifecycle of NetworkLocationRequest.

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

[Michael van Ouwerkerk, 2013-09-30 15:27:24]

mpearson as metrics owner
timvolodine as reviewer that is ramping up on geolocation :-)
bulach as geolocation owner (no longer strictly necessary)

[bulach, 2013-09-30 16:27:20]

lgtm % nit, +vadimt FYI

https://codereview.chromium.org/25262003/diff/2001/content/browser/geolocatio...
File content/browser/geolocation/network_location_request.cc (right):

https://codereview.chromium.org/25262003/diff/2001/content/browser/geolocatio...
content/browser/geolocation/network_location_request.cc:322: return;
nit: remove return

[timvolodine, 2013-09-30 16:38:49]

lgtm

https://codereview.chromium.org/25262003/diff/2001/content/browser/geolocatio...
File content/browser/geolocation/network_location_request.cc (right):

https://codereview.chromium.org/25262003/diff/2001/content/browser/geolocatio...
content/browser/geolocation/network_location_request.cc:36: // in
tools/metrics/histograms/histograms.xml to keep it in sync.
side-note: to slightly help this problem we could assign values to them like =0,
=1.. That way deleting entries would be ok. Also, it looks like a certain
numbering is given in the .xml file associated with this patch.

https://codereview.chromium.org/25262003/diff/2001/content/browser/geolocatio...
content/browser/geolocation/network_location_request.cc:52: event,
NETWORK_LOCATION_REQUEST_EVENT_COUNT);
indent

[Michael van Ouwerkerk, 2013-09-30 17:02:21]

Thanks for the quick review!

https://codereview.chromium.org/25262003/diff/2001/content/browser/geolocatio...
File content/browser/geolocation/network_location_request.cc (right):

https://codereview.chromium.org/25262003/diff/2001/content/browser/geolocatio...
content/browser/geolocation/network_location_request.cc:36: // in
tools/metrics/histograms/histograms.xml to keep it in sync.
On 2013/09/30 16:38:49, timvolodine wrote:
> side-note: to slightly help this problem we could assign values to them like
=0,
> =1.. That way deleting entries would be ok. Also, it looks like a certain
> numbering is given in the .xml file associated with this patch.

Done.

https://codereview.chromium.org/25262003/diff/2001/content/browser/geolocatio...
content/browser/geolocation/network_location_request.cc:52: event,
NETWORK_LOCATION_REQUEST_EVENT_COUNT);
On 2013/09/30 16:38:49, timvolodine wrote:
> indent

Done.

https://codereview.chromium.org/25262003/diff/2001/content/browser/geolocatio...
content/browser/geolocation/network_location_request.cc:322: return;
On 2013/09/30 16:27:21, bulach wrote:
> nit: remove return

Oops, experiment leftover. Done.

[Ilya Sherman, 2013-09-30 21:39:48]

I think Mark is OOO, so I'll take over for him on the histograms.xml review:

https://codereview.chromium.org/25262003/diff/12001/content/browser/geolocati...
File content/browser/geolocation/network_location_request.cc (right):

https://codereview.chromium.org/25262003/diff/12001/content/browser/geolocati...
content/browser/geolocation/network_location_request.cc:60: code,
kCodeRangeStart, kCodeRangeEnd, kCodeBuckets);
Please use a sparse histogram for this, via the UMA_HISTOGRAM_SPARSE_SLOWLY
macro.  Allocating 502 buckets is very inefficient, especially given that most
of those buckets will be empty most of the time.

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

https://codereview.chromium.org/25262003/diff/12001/tools/metrics/histograms/...
tools/metrics/histograms/histograms.xml:4861: +<histogram
name="Geolocation.NetworkLocationRequest.ResponseCode" units="code">
nit: I'd recommend either omitting the 'units' attribute or expanding it to
something like "HTTP response code".

[Michael van Ouwerkerk, 2013-10-01 10:28:36]

Thanks for the quick review Ilya!

https://codereview.chromium.org/25262003/diff/12001/content/browser/geolocati...
File content/browser/geolocation/network_location_request.cc (right):

https://codereview.chromium.org/25262003/diff/12001/content/browser/geolocati...
content/browser/geolocation/network_location_request.cc:60: code,
kCodeRangeStart, kCodeRangeEnd, kCodeBuckets);
On 2013/09/30 21:39:48, Ilya Sherman wrote:
> Please use a sparse histogram for this, via the UMA_HISTOGRAM_SPARSE_SLOWLY
> macro.  Allocating 502 buckets is very inefficient, especially given that most
> of those buckets will be empty most of the time.

Nice, thanks!

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

https://codereview.chromium.org/25262003/diff/12001/tools/metrics/histograms/...
tools/metrics/histograms/histograms.xml:4861: +<histogram
name="Geolocation.NetworkLocationRequest.ResponseCode" units="code">
On 2013/09/30 21:39:48, Ilya Sherman wrote:
> nit: I'd recommend either omitting the 'units' attribute or expanding it to
> something like "HTTP response code".

Done.

[Ilya Sherman, 2013-10-01 22:27:00]

histograms LGTM, thanks.

[commit-bot: I haz the power, 2013-10-02 10:12:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mvanouwerkerk@chromium.org/25262003/22001

[commit-bot: I haz the power, 2013-10-02 12:05:58]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_cl...

[commit-bot: I haz the power, 2013-10-02 12:45:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mvanouwerkerk@chromium.org/25262003/52001

[commit-bot: I haz the power, 2013-10-02 14:45:49]

Retried try job too often on win_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-10-02 14:46:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mvanouwerkerk@chromium.org/25262003/52001

[commit-bot: I haz the power, 2013-10-02 21:58:15]

Change committed as 226577