Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
26482003:
Geolocation: log the number of wifi access points used to determine location. (Closed)
|
Created:
7 years, 2 months ago by Michael van Ouwerkerk Modified:
7 years, 2 months ago CC:
chromium-reviews, jar (doing other things), jam, joi+watch-content_chromium.org, darin-cc_chromium.org, asvitkine+watch_chromium.org, Ilya Sherman, bulach, joth Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionGeolocation: log the number of wifi access points used to determine location.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=229024
Patch Set 1 #
Total comments: 6
Patch Set 2 : Rebase and cleanup. #Patch Set 3 : Use UMA_HISTOGRAM_CUSTOM_COUNTS. #
Total comments: 2
Patch Set 4 : Rebase and address review comments. #
Messages
Total messages: 15 (0 generated)
timvolodine for geolocation, isherman for histograms
https://codereview.chromium.org/26482003/diff/1/content/browser/geolocation/n... File content/browser/geolocation/network_location_request.cc (right): https://codereview.chromium.org/26482003/diff/1/content/browser/geolocation/n... content/browser/geolocation/network_location_request.cc:63: count); What sorts of counts are expected? Could you use a non-sparse histogram instead? A sparse histogram is likely to be less efficient for this use-case than something like UMA_HISTOGRAM_COUNTS. https://codereview.chromium.org/26482003/diff/1/tools/metrics/histograms/hist... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/26482003/diff/1/tools/metrics/histograms/hist... tools/metrics/histograms/histograms.xml:4863: +<histogram name="Geolocation.NetworkLocationRequest.AccessPoints" units="count"> nit: This units attribute seems pretty redundant; I'd either change the value to something like "Access points" or just omit it entirely.
https://codereview.chromium.org/26482003/diff/1/content/browser/geolocation/n... File content/browser/geolocation/network_location_request.cc (right): https://codereview.chromium.org/26482003/diff/1/content/browser/geolocation/n... content/browser/geolocation/network_location_request.cc:63: count); On 2013/10/08 23:51:03, Ilya Sherman wrote: > What sorts of counts are expected? Could you use a non-sparse histogram > instead? A sparse histogram is likely to be less efficient for this use-case > than something like UMA_HISTOGRAM_COUNTS. I expect counts of 0 to 10 but in some cases there will be more. I want to use buckets of size 1 for the range of 0 - 10. I shied away from the macros in histogram.h because the comments state 0 is not a possible value, it will be silently clamped to 1. Is this actually true? https://codereview.chromium.org/26482003/diff/1/tools/metrics/histograms/hist... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/26482003/diff/1/tools/metrics/histograms/hist... tools/metrics/histograms/histograms.xml:4863: +<histogram name="Geolocation.NetworkLocationRequest.AccessPoints" units="count"> On 2013/10/08 23:51:03, Ilya Sherman wrote: > nit: This units attribute seems pretty redundant; I'd either change the value to > something like "Access points" or just omit it entirely. Done.
https://codereview.chromium.org/26482003/diff/1/content/browser/geolocation/n... File content/browser/geolocation/network_location_request.cc (right): https://codereview.chromium.org/26482003/diff/1/content/browser/geolocation/n... content/browser/geolocation/network_location_request.cc:63: count); On 2013/10/09 13:09:57, Michael van Ouwerkerk wrote: > On 2013/10/08 23:51:03, Ilya Sherman wrote: > > What sorts of counts are expected? Could you use a non-sparse histogram > > instead? A sparse histogram is likely to be less efficient for this use-case > > than something like UMA_HISTOGRAM_COUNTS. > > I expect counts of 0 to 10 but in some cases there will be more. I want to use > buckets of size 1 for the range of 0 - 10. > > I shied away from the macros in histogram.h because the comments state 0 is not > a possible value, it will be silently clamped to 1. Is this actually true? I'm pretty sure this is not accurate -- 0 is a valid bucket. It's also the implicit underflow bucket, but that shouldn't be an issue for this histogram. You can double-check this locally by emitting a 0 to the histogram, and then navigate to chrome:histograms to see what values the histogram has.
Thanks Ilya! All done I think. https://codereview.chromium.org/26482003/diff/1/content/browser/geolocation/n... File content/browser/geolocation/network_location_request.cc (right): https://codereview.chromium.org/26482003/diff/1/content/browser/geolocation/n... content/browser/geolocation/network_location_request.cc:63: count); On 2013/10/09 22:47:40, Ilya Sherman wrote: > On 2013/10/09 13:09:57, Michael van Ouwerkerk wrote: > > On 2013/10/08 23:51:03, Ilya Sherman wrote: > > > What sorts of counts are expected? Could you use a non-sparse histogram > > > instead? A sparse histogram is likely to be less efficient for this > use-case > > > than something like UMA_HISTOGRAM_COUNTS. > > > > I expect counts of 0 to 10 but in some cases there will be more. I want to use > > buckets of size 1 for the range of 0 - 10. > > > > I shied away from the macros in histogram.h because the comments state 0 is > not > > a possible value, it will be silently clamped to 1. Is this actually true? > > I'm pretty sure this is not accurate -- 0 is a valid bucket. It's also the > implicit underflow bucket, but that shouldn't be an issue for this histogram. > You can double-check this locally by emitting a 0 to the histogram, and then > navigate to chrome:histograms to see what values the histogram has. Done.
https://codereview.chromium.org/26482003/diff/17001/content/browser/geolocati... File content/browser/geolocation/network_location_request.cc (right): https://codereview.chromium.org/26482003/diff/17001/content/browser/geolocati... content/browser/geolocation/network_location_request.cc:63: count, 0, 10, 11); could we use some constants here, like similar to NETWORK_LOCATION_REQUEST_EVENT_COUNT in the code above this method?
histograms LGTM, thanks.
Thanks Tim! Please take another look. https://codereview.chromium.org/26482003/diff/17001/content/browser/geolocati... File content/browser/geolocation/network_location_request.cc (right): https://codereview.chromium.org/26482003/diff/17001/content/browser/geolocati... content/browser/geolocation/network_location_request.cc:63: count, 0, 10, 11); On 2013/10/10 12:00:43, timvolodine wrote: > could we use some constants here, like similar to > NETWORK_LOCATION_REQUEST_EVENT_COUNT in the code above this method? Done.
lgtm lgtm, so if I understand correctly instances with access_point_data.size() > 10 will be bucketed into max bucket with index 10?
On 2013/10/16 15:17:02, timvolodine wrote: > lgtm > > lgtm, > so if I understand correctly instances with access_point_data.size() > 10 will > be bucketed into max bucket with index 10? Yes, that's the way I understand it. The documentation for histograms is sometimes vague or incorrect, but this part seems clear. Thanks for the quick review!
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mvanouwerkerk@chromium.org/26482003/29001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mvanouwerkerk@chromium.org/26482003/29001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mvanouwerkerk@chromium.org/26482003/29001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mvanouwerkerk@chromium.org/26482003/29001
Message was sent while issue was closed.
Change committed as 229024 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
