Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
480113003:
Battery Status API: add UMA logging for Linux. (Closed)
|
Created:
6 years, 4 months ago by timvolodine Modified:
6 years, 3 months ago CC:
chromium-reviews, darin-cc_chromium.org, jam, asvitkine+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionBattery Status API: add UMA logging for Linux.
Adds UMA logging to track how many batteries are reported
by the system at the start of Battery Status API.
BUG=
Committed: https://crrev.com/ab7555f4979feef26b9c9c4f9f74aa462ce9927f
Cr-Commit-Position: refs/heads/master@{#292398}
Patch Set 1 #
Total comments: 2
Patch Set 2 : switched to UMA_HISTOGRAM_CUSTOM_COUNTS #
Total comments: 1
Patch Set 3 : added enum for "5+" label #
Messages
Total messages: 16 (0 generated)
https://codereview.chromium.org/480113003/diff/1/content/browser/battery_stat... File content/browser/battery_status/battery_status_manager_linux.cc (right): https://codereview.chromium.org/480113003/diff/1/content/browser/battery_stat... content/browser/battery_status/battery_status_manager_linux.cc:47: enum NumberBatteriesType { This seems kind of over-engineered. Can't we use a simple UMA_HISTOGRAM_CUSTOM_COUNTS like for Net.Wifi.InterfaceCount ? Its results are here: https://uma.googleplex.com/histograms/?dayCount=1&endDate=08-20-2014&version=...
https://codereview.chromium.org/480113003/diff/1/content/browser/battery_stat... File content/browser/battery_status/battery_status_manager_linux.cc (right): https://codereview.chromium.org/480113003/diff/1/content/browser/battery_stat... content/browser/battery_status/battery_status_manager_linux.cc:47: enum NumberBatteriesType { On 2014/08/21 09:28:18, Michael van Ouwerkerk wrote: > This seems kind of over-engineered. Can't we use a simple > UMA_HISTOGRAM_CUSTOM_COUNTS like for Net.Wifi.InterfaceCount ? > > Its results are here: > https://uma.googleplex.com/histograms/?dayCount=1&endDate=08-20-2014&version=... sure, the CUSTOM_COUNTS are a bit difficult to use though. Note that the referred Net.Wifi.InterfaceCount example does it incorrectly and declares wrong number of buckets. Done.
lgtm
+ isherman@ : histograms.xml
LGTM with a suggestion: https://codereview.chromium.org/480113003/diff/20001/tools/metrics/histograms... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/480113003/diff/20001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:1936: + (corresponding to the value 5). nit: You could associate an <enum> with this histogram, and label the overflow bucket as "5+".
On 2014/08/21 17:29:56, Ilya Sherman wrote: > LGTM with a suggestion: > > https://codereview.chromium.org/480113003/diff/20001/tools/metrics/histograms... > File tools/metrics/histograms/histograms.xml (right): > > https://codereview.chromium.org/480113003/diff/20001/tools/metrics/histograms... > tools/metrics/histograms/histograms.xml:1936: + (corresponding to the value > 5). > nit: You could associate an <enum> with this histogram, and label the overflow > bucket as "5+". yes that's what my 1st patch was about, but Michael didn't like it..
On 2014/08/21 17:43:18, timvolodine wrote: > On 2014/08/21 17:29:56, Ilya Sherman wrote: > > LGTM with a suggestion: > > > > > https://codereview.chromium.org/480113003/diff/20001/tools/metrics/histograms... > > File tools/metrics/histograms/histograms.xml (right): > > > > > https://codereview.chromium.org/480113003/diff/20001/tools/metrics/histograms... > > tools/metrics/histograms/histograms.xml:1936: + (corresponding to the value > > 5). > > nit: You could associate an <enum> with this histogram, and label the overflow > > bucket as "5+". > > yes that's what my 1st patch was about, but Michael didn't like it.. I'll defer to Ilya. My comment was a suggestion, I'm happy to be convinced otherwise.
On 2014/08/21 18:13:47, Michael van Ouwerkerk wrote: > On 2014/08/21 17:43:18, timvolodine wrote: > > On 2014/08/21 17:29:56, Ilya Sherman wrote: > > > LGTM with a suggestion: > > > > > > > > > https://codereview.chromium.org/480113003/diff/20001/tools/metrics/histograms... > > > File tools/metrics/histograms/histograms.xml (right): > > > > > > > > > https://codereview.chromium.org/480113003/diff/20001/tools/metrics/histograms... > > > tools/metrics/histograms/histograms.xml:1936: + (corresponding to the > value > > > 5). > > > nit: You could associate an <enum> with this histogram, and label the > overflow > > > bucket as "5+". > > > > yes that's what my 1st patch was about, but Michael didn't like it.. > > I'll defer to Ilya. My comment was a suggestion, I'm happy to be convinced > otherwise. My suggestion is to keep the C++ code as it is, and to simply tweak histograms.xml by adding an enum that looks like so: <enum name="BatteryStatusNumberBatteriesLinux" type="int"> <int value="5" label="5+"/> </enum> (and updating the histogram to reference it).
On 2014/08/22 03:34:04, Ilya Sherman wrote: > On 2014/08/21 18:13:47, Michael van Ouwerkerk wrote: > > On 2014/08/21 17:43:18, timvolodine wrote: > > > On 2014/08/21 17:29:56, Ilya Sherman wrote: > > > > LGTM with a suggestion: > > > > > > > > > > > > > > https://codereview.chromium.org/480113003/diff/20001/tools/metrics/histograms... > > > > File tools/metrics/histograms/histograms.xml (right): > > > > > > > > > > > > > > https://codereview.chromium.org/480113003/diff/20001/tools/metrics/histograms... > > > > tools/metrics/histograms/histograms.xml:1936: + (corresponding to the > > value > > > > 5). > > > > nit: You could associate an <enum> with this histogram, and label the > > overflow > > > > bucket as "5+". > > > > > > yes that's what my 1st patch was about, but Michael didn't like it.. > > > > I'll defer to Ilya. My comment was a suggestion, I'm happy to be convinced > > otherwise. > > My suggestion is to keep the C++ code as it is, and to simply tweak > histograms.xml by adding an enum that looks like so: > > <enum name="BatteryStatusNumberBatteriesLinux" type="int"> > <int value="5" label="5+"/> > </enum> > > (and updating the histogram to reference it). ah I see, done. thanks for the suggestion.
(Still lgtm)
On 2014/08/22 20:29:32, Ilya Sherman wrote: > (Still lgtm) :) thanks!
The CQ bit was checked by timvolodine@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/timvolodine@chromium.org/480113003/40001
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as 4bb71039118874ef0ba08281ac5ee0b84e7fb79d
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/ab7555f4979feef26b9c9c4f9f74aa462ce9927f Cr-Commit-Position: refs/heads/master@{#292398} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
