Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
15734015:
chromeos: Add descriptions for power manager metrics. (Closed)
|
Created:
7 years, 7 months ago by Daniel Erat Modified:
7 years, 7 months ago Reviewers:
Daniel Erat, Alexei Svitkine (slow), Ilya Sherman, jar (doing other things), commit-bot: I haz the power CC:
chromium-reviews, MAD, Ilya Sherman, jar (doing other things), Steve Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
Descriptionchromeos: Add descriptions for power manager metrics.
This adds descriptions for a few histograms that I'm adding
to the power manager:
Power.BatteryDischargeRateWhileSuspended
Power.KeyboardBacklightLevel
It also removes Power.BrightnessAdjustOnAC and
Power.BrightnessAdjustOnBattery. These are being removed from
the power manager; they should be reported as user actions
rather than histograms, and Chrome already reports them as
such.
BUG=218397, 220993
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=202294
Patch Set 1 #
Total comments: 4
Patch Set 2 : apply review feedback #
Total comments: 2
Patch Set 3 : more feedback #
Total comments: 2
Patch Set 4 : remove brightness adjustment metrics #Patch Set 5 : restore deprecated histograms #Messages
Total messages: 22 (0 generated)
leaving for vacation this instant; please find a different reviewer if you want this in before next week. On Wed, May 22, 2013 at 5:41 PM, <derat@chromium.org> wrote: > Reviewers: Mark P, > > Description: > chromeos: Add descriptions for power manager metrics. > > This adds descriptions for a few histograms that I'm adding > to the power manager: > > Power.**BatteryDischargeRateWhileSuspe**nded > Power.KeyboardBacklightLevel > Power.KeyboardBrightnessAdjust > > BUG=218397,220993 > > Please review this at https://codereview.chromium.**org/15734015/<https://codereview.chromium.org/1... > > SVN Base: svn://svn.chromium.org/chrome/**trunk/src<http://svn.chromium.org/chrome/trunk/src> > > Affected files: > M tools/metrics/histograms/**histograms.xml > > > Index: tools/metrics/histograms/**histograms.xml > diff --git a/tools/metrics/histograms/**histograms.xml > b/tools/metrics/histograms/**histograms.xml > index 22ab0b304a14ee9ed1da3920f74429**a981173668..** > 09bc86dff93b452ec38425e5da367f**f349fc52dc 100644 > --- a/tools/metrics/histograms/**histograms.xml > +++ b/tools/metrics/histograms/**histograms.xml > @@ -6302,6 +6302,15 @@ other types of suffix sets. > </summary> > </histogram> > > +<histogram name="Power.**BatteryDischargeRateWhileSuspe**nded"> > + <summary> > + Chrome OS battery discharge rate in mW while the system was suspended, > + sampled at resume. Only reported if the system was on battery power > both > + before suspending and after resuming and if the energy level didn't > increase > + while suspended (which would indicate that an AC adapter was > connected). > + </summary> > +</histogram> > + > <histogram name="Power.BatteryInfoSample" enum="BatteryInfoSampleResult"* > *> > <summary> > Counts the number of times we have read the battery status from sysfs > and if > @@ -6480,6 +6489,17 @@ other types of suffix sets. > </summary> > </histogram> > > +<histogram name="Power.**KeyboardBacklightLevel" units="%"> > + <summary>The level of the keyboard backlight as a percentage.</summary> > +</histogram> > + > +<histogram name="Power.**KeyboardBrightnessAdjust" > enum="PowerBrightnessAdjust"> > + <summary> > + Number of times the user has adjusted keyboard backlight brightness > up and > + down. > + </summary> > +</histogram> > + > <histogram name="Power.LengthOfSession" units="seconds"> > <summary> > The length of time, in seconds, that a user spent in a single session. > > >
Will do. Have fun! On Wed, May 22, 2013 at 5:47 PM, Mark Pearson <mpearson@chromium.org> wrote: > leaving for vacation this instant; please find a different reviewer if you > want this in before next week. > > > On Wed, May 22, 2013 at 5:41 PM, <derat@chromium.org> wrote: > >> Reviewers: Mark P, >> >> Description: >> chromeos: Add descriptions for power manager metrics. >> >> This adds descriptions for a few histograms that I'm adding >> to the power manager: >> >> Power.**BatteryDischargeRateWhileSuspe**nded >> Power.KeyboardBacklightLevel >> Power.KeyboardBrightnessAdjust >> >> BUG=218397,220993 >> >> Please review this at https://codereview.chromium.**org/15734015/<https://codereview.chromium.org/1... >> >> SVN Base: svn://svn.chromium.org/chrome/**trunk/src<http://svn.chromium.org/chrome/trunk/src> >> >> Affected files: >> M tools/metrics/histograms/**histograms.xml >> >> >> Index: tools/metrics/histograms/**histograms.xml >> diff --git a/tools/metrics/histograms/**histograms.xml >> b/tools/metrics/histograms/**histograms.xml >> index 22ab0b304a14ee9ed1da3920f74429**a981173668..** >> 09bc86dff93b452ec38425e5da367f**f349fc52dc 100644 >> --- a/tools/metrics/histograms/**histograms.xml >> +++ b/tools/metrics/histograms/**histograms.xml >> @@ -6302,6 +6302,15 @@ other types of suffix sets. >> </summary> >> </histogram> >> >> +<histogram name="Power.**BatteryDischargeRateWhileSuspe**nded"> >> + <summary> >> + Chrome OS battery discharge rate in mW while the system was >> suspended, >> + sampled at resume. Only reported if the system was on battery power >> both >> + before suspending and after resuming and if the energy level didn't >> increase >> + while suspended (which would indicate that an AC adapter was >> connected). >> + </summary> >> +</histogram> >> + >> <histogram name="Power.BatteryInfoSample" enum="BatteryInfoSampleResult" >> **> >> <summary> >> Counts the number of times we have read the battery status from >> sysfs and if >> @@ -6480,6 +6489,17 @@ other types of suffix sets. >> </summary> >> </histogram> >> >> +<histogram name="Power.**KeyboardBacklightLevel" units="%"> >> + <summary>The level of the keyboard backlight as a percentage.</summary> >> +</histogram> >> + >> +<histogram name="Power.**KeyboardBrightnessAdjust" >> enum="PowerBrightnessAdjust"> >> + <summary> >> + Number of times the user has adjusted keyboard backlight brightness >> up and >> + down. >> + </summary> >> +</histogram> >> + >> <histogram name="Power.LengthOfSession" units="seconds"> >> <summary> >> The length of time, in seconds, that a user spent in a single >> session. >> >> >> >
Also leaving for vacation, sorry. Alexei, could you take a look?
Alexei/SteveT: Can one of you review/comment on this first?
https://codereview.chromium.org/15734015/diff/1/tools/metrics/histograms/hist... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/15734015/diff/1/tools/metrics/histograms/hist... tools/metrics/histograms/histograms.xml:6307: Chrome OS battery discharge rate in mW while the system was suspended, Please specify units="mW" above, to get the right label in the graph. Also do this for the histogram above. https://codereview.chromium.org/15734015/diff/1/tools/metrics/histograms/hist... tools/metrics/histograms/histograms.xml:6493: <summary>The level of the keyboard backlight as a percentage.</summary> Please mention when/how this is sampled. Is it periodic, on startup / wake, etc?
Thanks, please take another look. https://codereview.chromium.org/15734015/diff/1/tools/metrics/histograms/hist... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/15734015/diff/1/tools/metrics/histograms/hist... tools/metrics/histograms/histograms.xml:6307: Chrome OS battery discharge rate in mW while the system was suspended, On 2013/05/23 14:30:30, Alexei Svitkine wrote: > Please specify units="mW" above, to get the right label in the graph. Also do > this for the histogram above. Done. https://codereview.chromium.org/15734015/diff/1/tools/metrics/histograms/hist... tools/metrics/histograms/histograms.xml:6493: <summary>The level of the keyboard backlight as a percentage.</summary> On 2013/05/23 14:30:30, Alexei Svitkine wrote: > Please mention when/how this is sampled. > > Is it periodic, on startup / wake, etc? Done.
lgtm with one final nit https://codereview.chromium.org/15734015/diff/10001/tools/metrics/histograms/... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/15734015/diff/10001/tools/metrics/histograms/... tools/metrics/histograms/histograms.xml:6282: periodically. Nit: Mention how frequently. Here and in the other ones.
https://codereview.chromium.org/15734015/diff/10001/tools/metrics/histograms/... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/15734015/diff/10001/tools/metrics/histograms/... tools/metrics/histograms/histograms.xml:6282: periodically. On 2013/05/24 14:48:47, Alexei Svitkine wrote: > Nit: Mention how frequently. Here and in the other ones. Sure (although I'm concerned about the likelihood of something like this getting out-of-date).
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/derat@chromium.org/15734015/15001
Retried try job too often on chromium_presubmit for step(s) presubmit http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...
Consider adding to the prose below.... then LGTM. https://codereview.chromium.org/15734015/diff/15001/tools/metrics/histograms/... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/15734015/diff/15001/tools/metrics/histograms/... tools/metrics/histograms/histograms.xml:6539: Number of times the user has adjusted keyboard backlight brightness up and When is this sampled? At shutdown??? Each time the the system sleeps?
https://codereview.chromium.org/15734015/diff/15001/tools/metrics/histograms/... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/15734015/diff/15001/tools/metrics/histograms/... tools/metrics/histograms/histograms.xml:6539: Number of times the user has adjusted keyboard backlight brightness up and On 2013/05/24 17:09:03, jar wrote: > When is this sampled? At shutdown??? Each time the the system sleeps? As discussed over IM, I've removed this histogram -- Chrome already reports these adjustments as user actions. I'm also removing the Power.BacklightLevel* histograms in conjunction with a power manager change to stop reporting them. Is it acceptable to remove the descriptions for them in this change, or should the descriptions be retained indefinitely (since they have previously been reported)?
On 2013/05/24 22:02:28, Daniel Erat wrote: > https://codereview.chromium.org/15734015/diff/15001/tools/metrics/histograms/... > File tools/metrics/histograms/histograms.xml (right): > > https://codereview.chromium.org/15734015/diff/15001/tools/metrics/histograms/... > tools/metrics/histograms/histograms.xml:6539: Number of times the user has > adjusted keyboard backlight brightness up and > On 2013/05/24 17:09:03, jar wrote: > > When is this sampled? At shutdown??? Each time the the system sleeps? > > As discussed over IM, I've removed this histogram -- Chrome already reports > these adjustments as user actions. > > I'm also removing the Power.BacklightLevel* histograms in conjunction with a > power manager change to stop reporting them. Is it acceptable to remove the > descriptions for them in this change, or should the descriptions be retained > indefinitely (since they have previously been reported)? Sorry, misspoke here: I meant that I'm also remove the Power.BrightnessAdjust* histograms. Power.BacklightLevel* are still needed.
The descriptions should be retained indefinitely, but you can mark them as obsolete via an attribute. On Fri, May 24, 2013 at 6:03 PM, <derat@chromium.org> wrote: > On 2013/05/24 22:02:28, Daniel Erat wrote: > > https://codereview.chromium.org/15734015/diff/15001/tools/metrics/histograms/... >> >> File tools/metrics/histograms/histograms.xml (right): > > > > https://codereview.chromium.org/15734015/diff/15001/tools/metrics/histograms/... >> >> tools/metrics/histograms/histograms.xml:6539: Number of times the user has >> adjusted keyboard backlight brightness up and >> On 2013/05/24 17:09:03, jar wrote: >> > When is this sampled? At shutdown??? Each time the the system sleeps? > > >> As discussed over IM, I've removed this histogram -- Chrome already >> reports >> these adjustments as user actions. > > >> I'm also removing the Power.BacklightLevel* histograms in conjunction with >> a >> power manager change to stop reporting them. Is it acceptable to remove >> the >> descriptions for them in this change, or should the descriptions be >> retained >> indefinitely (since they have previously been reported)? > > > Sorry, misspoke here: I meant that I'm also remove the > Power.BrightnessAdjust* > histograms. Power.BacklightLevel* are still needed. > > https://codereview.chromium.org/15734015/
Thanks, one more look?
lgtm On Fri, May 24, 2013 at 9:20 PM, <derat@chromium.org> wrote: > Thanks, one more look? > > https://codereview.chromium.org/15734015/
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/derat@chromium.org/15734015/28001
Retried try job too often on linux_chromeos for step(s) browser_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/derat@chromium.org/15734015/28001
Message was sent while issue was closed.
Change committed as 202294 |
