Send fan hysteresis data to chromium as UMA histogram [chromiumos/platform/power_manager : master]

1 view
Skip to first unread message

Jon Kliegman (Code Review)

unread,
May 16, 2012, 3:14:56 PM5/16/12
to
Jon Kliegman has uploaded a new change for review.

Change subject: Send fan hysteresis data to chromium as UMA histogram
......................................................................

Send fan hysteresis data to chromium as UMA histogram

Collect and send data to Chromium to let us track how
useful the fan hysteresis code is.

BUG=chromium-os:30526
TEST=Observed new metrics in chrome://histograms
x86/amd64 paladin trybots
CQ-DEPEND=I07368e9260f69482e16674ddab826314bde7e22b

Change-Id: I57d69a6154a84b8dbbebe2a460230798d4ae16c6
---
M metrics_constants.cc
M metrics_constants.h
M powerd.h
M powerd_metrics.cc
M util.cc
M util.h
6 files changed, 76 insertions(+), 1 deletion(-)


git pull ssh://gerrit.chromium.org:29418/chromiumos/platform/power_manager refs/changes/44/22844/1
--
To view, visit https://gerrit.chromium.org/gerrit/22844
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: I57d69a6154a84b8dbbebe2a460230798d4ae16c6
Gerrit-PatchSet: 1
Gerrit-Project: chromiumos/platform/power_manager
Gerrit-Branch: master
Gerrit-Owner: Jon Kliegman <kli...@chromium.org>

Jon Kliegman (Code Review)

unread,
May 16, 2012, 3:20:26 PM5/16/12
to Sameer Nanda, Ryan Harrison, Simon Arscott
Jon Kliegman has posted comments on this change.

Change subject: Send fan hysteresis data to chromium as UMA histogram
......................................................................


Patch Set 2:

Patch sets #1 & #2 are the same, just different commit message.

--
To view, visit https://gerrit.chromium.org/gerrit/22844
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I57d69a6154a84b8dbbebe2a460230798d4ae16c6
Gerrit-PatchSet: 2
Gerrit-Project: chromiumos/platform/power_manager
Gerrit-Branch: master
Gerrit-Owner: Jon Kliegman <kli...@chromium.org>
Gerrit-Reviewer: Jon Kliegman <kli...@chromium.org>
Gerrit-Reviewer: Ryan Harrison <rhar...@chromium.org>
Gerrit-Reviewer: Sameer Nanda <sna...@chromium.org>
Gerrit-Reviewer: Simon Arscott <ars...@chromium.org>

Ryan Harrison (Code Review)

unread,
May 16, 2012, 3:56:11 PM5/16/12
to Jon Kliegman, Sameer Nanda, Simon Arscott
Ryan Harrison has posted comments on this change.

Change subject: Send fan hysteresis data to chromium as UMA histogram
......................................................................


Patch Set 2: I would prefer that you didn't submit this

(3 inline comments)

Please bring as much of the new code that you have written under test, given that metrics generation code should be side effect free.

....................................................
File metrics_constants.cc
Line 77: const char kMetricThermalAbortedFanTurnOnName[] =
You should organize this such that all of the constants for the same metric are in a block together instead of interleaving the different metrics.

....................................................
File metrics_constants.h
Line 60: extern const char kMetricThermalAbortedFanTurnOnName[];
Blocks not interleaving again.

....................................................
File powerd_metrics.cc
Line 315:
You might want to look at refactoring this method to make unit testing easier.

--
To view, visit https://gerrit.chromium.org/gerrit/22844
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I57d69a6154a84b8dbbebe2a460230798d4ae16c6
Gerrit-PatchSet: 2
Gerrit-Project: chromiumos/platform/power_manager
Gerrit-Branch: master
Gerrit-Owner: Jon Kliegman <kli...@chromium.org>

Jon Kliegman (Code Review)

unread,
May 16, 2012, 6:00:54 PM5/16/12
to Sameer Nanda, Ryan Harrison, Simon Arscott
Jon Kliegman has posted comments on this change.

Change subject: Send fan hysteresis data to chromium as UMA histogram
......................................................................




--
To view, visit https://gerrit.chromium.org/gerrit/22844
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I57d69a6154a84b8dbbebe2a460230798d4ae16c6
Gerrit-PatchSet: 3
Gerrit-Project: chromiumos/platform/power_manager
Gerrit-Branch: master
Gerrit-Owner: Jon Kliegman <kli...@chromium.org>

Ryan Harrison (Code Review)

unread,
May 17, 2012, 10:36:39 AM5/17/12
to Jon Kliegman, Sameer Nanda, Simon Arscott
Ryan Harrison has posted comments on this change.

Change subject: Send fan hysteresis data to chromium as UMA histogram
......................................................................


Patch Set 3: I would prefer that you didn't submit this

(3 inline comments)

....................................................
File powerd_metrics.cc
Line 313: void Daemon::SendThermalMetrics(int aborted, int turned_on, int multiple) {
It is possible for aborted and turned_on to be 0 or aborted == - turned_on? This would give you a total value of 0, which you are using in the denominator.

You should catch this condition and log it. I don't think this should be a fatal issue, since metric generation is not essential to the usage of the device.

Line 330:
Negative values make no sense for these variables, correct? Should you change these to unsigned ints or be trimming and logging negative values here? I don't think it makes sense to have GeIntFromFile to trim, since it isn't obvious that a method with that name would have that side effect.

....................................................
File powerd_unittest.cc
Line 832: TEST_F(DaemonTest, SendThermalMetrics) {
You don't appear to be testing weird/odd values. The two cases that I have alluded to in other comments would be the total being 0 and some of these values being negative.

--
To view, visit https://gerrit.chromium.org/gerrit/22844
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I57d69a6154a84b8dbbebe2a460230798d4ae16c6
Gerrit-PatchSet: 3
Gerrit-Project: chromiumos/platform/power_manager
Gerrit-Branch: master
Gerrit-Owner: Jon Kliegman <kli...@chromium.org>

Jon Kliegman (Code Review)

unread,
May 17, 2012, 11:00:08 AM5/17/12
to Sameer Nanda, Ryan Harrison, Simon Arscott
Jon Kliegman has posted comments on this change.

Change subject: Send fan hysteresis data to chromium as UMA histogram
......................................................................


Patch Set 3: (3 inline comments)

....................................................
File powerd_metrics.cc
Line 313: void Daemon::SendThermalMetrics(int aborted, int turned_on, int multiple) {
0 should never happen in our current configuration but its good to check for so I'll add that in. A future board might treat fans differently as well.

Line 330:
Correct, these values should never be negative. They're uint32 in the kernel so I'll change to unsigned int here.

....................................................
File powerd_unittest.cc
Line 832: TEST_F(DaemonTest, SendThermalMetrics) {
will add more tests

--
To view, visit https://gerrit.chromium.org/gerrit/22844
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I57d69a6154a84b8dbbebe2a460230798d4ae16c6
Gerrit-PatchSet: 3
Gerrit-Project: chromiumos/platform/power_manager
Gerrit-Branch: master
Gerrit-Owner: Jon Kliegman <kli...@chromium.org>

Ryan Harrison (Code Review)

unread,
May 17, 2012, 11:03:48 AM5/17/12
to Jon Kliegman, Sameer Nanda, Simon Arscott
Ryan Harrison has posted comments on this change.

Change subject: Send fan hysteresis data to chromium as UMA histogram
......................................................................


Patch Set 3: (2 inline comments)

....................................................
File powerd_metrics.cc
Line 315: int aborted_percent = static_cast<int>(round(100 * aborted / total));
The cast and round are not useful here, since the inner operation is integer based, thus should be chopping off any values after the decimal point.

....................................................
File util.cc
Line 90: bool GetIntFromFile(const char* filename, int* value) {
It seems reasonable to bring this under test, since you can control the file that it is accessing.

--
To view, visit https://gerrit.chromium.org/gerrit/22844
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I57d69a6154a84b8dbbebe2a460230798d4ae16c6
Gerrit-PatchSet: 3
Gerrit-Project: chromiumos/platform/power_manager
Gerrit-Branch: master
Gerrit-Owner: Jon Kliegman <kli...@chromium.org>

Jon Kliegman (Code Review)

unread,
May 17, 2012, 11:54:51 AM5/17/12
to Sameer Nanda, Ryan Harrison, Simon Arscott
Jon Kliegman has posted comments on this change.

Change subject: Send fan hysteresis data to chromium as UMA histogram
......................................................................


Patch Set 3: (5 inline comments)

....................................................
File powerd_metrics.cc
Line 313: void Daemon::SendThermalMetrics(int aborted, int turned_on, int multiple) {
Done

Line 315: int aborted_percent = static_cast<int>(round(100 * aborted / total));
Done

Line 330:
Done

....................................................
File powerd_unittest.cc
Line 832: TEST_F(DaemonTest, SendThermalMetrics) {
Not done - will add more tests in next patch.

....................................................
File util.cc
Line 90: bool GetIntFromFile(const char* filename, int* value) {
Agreed. Will soonish.

--
To view, visit https://gerrit.chromium.org/gerrit/22844
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I57d69a6154a84b8dbbebe2a460230798d4ae16c6
Gerrit-PatchSet: 3
Gerrit-Project: chromiumos/platform/power_manager
Gerrit-Branch: master
Gerrit-Owner: Jon Kliegman <kli...@chromium.org>

Sameer Nanda (Code Review)

unread,
May 17, 2012, 2:01:26 PM5/17/12
to Jon Kliegman, Sameer Nanda, Ryan Harrison, Simon Arscott
Sameer Nanda has posted comments on this change.

Change subject: Send fan hysteresis data to chromium as UMA histogram
......................................................................


Patch Set 4: (3 inline comments)

....................................................
Commit Message
Line 23: TEST=Observed new metrics in chrome://histograms
nit: TEST is for telling the testers how to verify your changes and not how you tested it.

....................................................
File powerd_metrics.cc
Line 318: << aborted << ", turnd_on = " << turned_on << ")";
Doesn't seem like an ERROR. If the system never got hot enough, the fans wouldn't need to be turned on. Change to INFO?

Line 340: !util::GetUintFromFile(kMetricThermalMultipleFanFilename, &multiple))
Lets do an ERROR log here.

--
To view, visit https://gerrit.chromium.org/gerrit/22844
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I57d69a6154a84b8dbbebe2a460230798d4ae16c6
Gerrit-PatchSet: 4
Gerrit-Project: chromiumos/platform/power_manager
Gerrit-Branch: master
Gerrit-Owner: Jon Kliegman <kli...@chromium.org>

Jon Kliegman (Code Review)

unread,
May 17, 2012, 2:19:04 PM5/17/12
to Sameer Nanda, Ryan Harrison, Simon Arscott
Jon Kliegman has posted comments on this change.

Change subject: Send fan hysteresis data to chromium as UMA histogram
......................................................................


Patch Set 4: (2 inline comments)

....................................................
File powerd_metrics.cc
Line 318: << aborted << ", turnd_on = " << turned_on << ")";
On current systems we have this would never be the case (there's a 0% fan that turns on at 0 degrees) so it would indicate an error in current platforms.

Line 340: !util::GetUintFromFile(kMetricThermalMultipleFanFilename, &multiple))
I was relying on the errors from GetUintFromFile which would be more descriptive (giving specific filename and what was read). Adding another one here would only be useful in case the logging was removed from it.

--
To view, visit https://gerrit.chromium.org/gerrit/22844
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I57d69a6154a84b8dbbebe2a460230798d4ae16c6
Gerrit-PatchSet: 4
Gerrit-Project: chromiumos/platform/power_manager
Gerrit-Branch: master
Gerrit-Owner: Jon Kliegman <kli...@chromium.org>

Sameer Nanda (Code Review)

unread,
May 17, 2012, 2:24:56 PM5/17/12
to Jon Kliegman, Sameer Nanda, Ryan Harrison, Simon Arscott
Sameer Nanda has posted comments on this change.

Change subject: Send fan hysteresis data to chromium as UMA histogram
......................................................................


Patch Set 4: (2 inline comments)

....................................................
File powerd_metrics.cc
Line 318: << aborted << ", turnd_on = " << turned_on << ")";
This will likely not be true of daisy. But I am ok with this as-is.

Line 340: !util::GetUintFromFile(kMetricThermalMultipleFanFilename, &multiple))
GetUintFromFile can potentially be called from multiple places so it may be a bit hard to disambiguate the error (although the file name is being printed there.

--
To view, visit https://gerrit.chromium.org/gerrit/22844
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I57d69a6154a84b8dbbebe2a460230798d4ae16c6
Gerrit-PatchSet: 4
Gerrit-Project: chromiumos/platform/power_manager
Gerrit-Branch: master
Gerrit-Owner: Jon Kliegman <kli...@chromium.org>

Jon Kliegman (Code Review)

unread,
May 17, 2012, 3:38:06 PM5/17/12
to Sameer Nanda, Ryan Harrison, Simon Arscott
Jon Kliegman has posted comments on this change.

Change subject: Send fan hysteresis data to chromium as UMA histogram
......................................................................


Patch Set 4: (2 inline comments)

Added a new util_unittest.cc which checks GetUintFromFile()

Updated SendThermalMetrics unittest to test 0's being sent.

Filed crbug.com/128596 to track StringToUint treating negative values as valid.

....................................................
File powerd_metrics.cc
Line 318: << aborted << ", turnd_on = " << turned_on << ")";
Done

Line 340: !util::GetUintFromFile(kMetricThermalMultipleFanFilename, &multiple))
Done

--
To view, visit https://gerrit.chromium.org/gerrit/22844
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I57d69a6154a84b8dbbebe2a460230798d4ae16c6
Gerrit-PatchSet: 4
Gerrit-Project: chromiumos/platform/power_manager
Gerrit-Branch: master
Gerrit-Owner: Jon Kliegman <kli...@chromium.org>

Ryan Harrison (Code Review)

unread,
May 17, 2012, 3:47:38 PM5/17/12
to Jon Kliegman, Sameer Nanda, Simon Arscott
Ryan Harrison has posted comments on this change.

Change subject: Send fan hysteresis data to chromium as UMA histogram
......................................................................


Patch Set 5: (1 inline comment)

....................................................
File Makefile
Line 406: UTIL_UNITTEST_FLAGS = $(GLIB_FLAGS)
Could you use LIBUTIL_* versions as the base point here, since that is the target this source file is from?

--
To view, visit https://gerrit.chromium.org/gerrit/22844
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I57d69a6154a84b8dbbebe2a460230798d4ae16c6
Gerrit-PatchSet: 5
Gerrit-Project: chromiumos/platform/power_manager
Gerrit-Branch: master
Gerrit-Owner: Jon Kliegman <kli...@chromium.org>

Jon Kliegman (Code Review)

unread,
May 17, 2012, 3:58:49 PM5/17/12
to Sameer Nanda, Ryan Harrison, Simon Arscott
Jon Kliegman has posted comments on this change.

Change subject: Send fan hysteresis data to chromium as UMA histogram
......................................................................


Patch Set 5: (1 inline comment)

....................................................
File Makefile
Line 406: UTIL_UNITTEST_FLAGS = $(GLIB_FLAGS)
Done

--
To view, visit https://gerrit.chromium.org/gerrit/22844
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I57d69a6154a84b8dbbebe2a460230798d4ae16c6
Gerrit-PatchSet: 5
Gerrit-Project: chromiumos/platform/power_manager
Gerrit-Branch: master
Gerrit-Owner: Jon Kliegman <kli...@chromium.org>

Sameer Nanda (Code Review)

unread,
May 17, 2012, 4:10:55 PM5/17/12
to Jon Kliegman, Sameer Nanda, Ryan Harrison, Simon Arscott
Sameer Nanda has posted comments on this change.

Change subject: Send fan hysteresis data to chromium as UMA histogram
......................................................................


Patch Set 6: Looks good to me, approved

--
To view, visit https://gerrit.chromium.org/gerrit/22844
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I57d69a6154a84b8dbbebe2a460230798d4ae16c6
Gerrit-PatchSet: 6
Gerrit-Project: chromiumos/platform/power_manager
Gerrit-Branch: master
Gerrit-Owner: Jon Kliegman <kli...@chromium.org>

Jon Kliegman (Code Review)

unread,
May 17, 2012, 4:18:20 PM5/17/12
to Sameer Nanda, Ryan Harrison, Simon Arscott
Jon Kliegman has posted comments on this change.

Change subject: Send fan hysteresis data to chromium as UMA histogram
......................................................................


Patch Set 6: I would prefer that you didn't submit this

Trybot is failing on unit test - investigating:

power_manager-0.0.1-r402: make[1]: *** No rule to make target `util_unittest.o', needed by `CXX_BINARY(util_unittest)'. Stop.

--
To view, visit https://gerrit.chromium.org/gerrit/22844
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I57d69a6154a84b8dbbebe2a460230798d4ae16c6
Gerrit-PatchSet: 6
Gerrit-Project: chromiumos/platform/power_manager
Gerrit-Branch: master
Gerrit-Owner: Jon Kliegman <kli...@chromium.org>

Jon Kliegman (Code Review)

unread,
May 17, 2012, 4:22:16 PM5/17/12
to Sameer Nanda, Ryan Harrison, Simon Arscott
Jon Kliegman has posted comments on this change.

Change subject: Send fan hysteresis data to chromium as UMA histogram
......................................................................


Patch Set 7:

Too much 'git -a --amend' and forgot new files need a manual 'git add foo'. Coupled with the 'out/' dir not being in .gitignore I was used to ignoring the 'unchanged files' rule.

I'll update .gitignore in a separate cl

--
To view, visit https://gerrit.chromium.org/gerrit/22844
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I57d69a6154a84b8dbbebe2a460230798d4ae16c6
Gerrit-PatchSet: 7
Gerrit-Project: chromiumos/platform/power_manager
Gerrit-Branch: master
Gerrit-Owner: Jon Kliegman <kli...@chromium.org>

Jon Kliegman (Code Review)

unread,
May 17, 2012, 4:55:42 PM5/17/12
to Sameer Nanda, Ryan Harrison, Simon Arscott
Jon Kliegman has posted comments on this change.

Change subject: Send fan hysteresis data to chromium as UMA histogram
......................................................................


Patch Set 7: Verified

PTA(final)L

Trybots are happy now with all the files up. Only change from previous patch is util_unittest.cc added.

Thanks for the quick reviews and feedback

--
To view, visit https://gerrit.chromium.org/gerrit/22844
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I57d69a6154a84b8dbbebe2a460230798d4ae16c6
Gerrit-PatchSet: 7
Gerrit-Project: chromiumos/platform/power_manager
Gerrit-Branch: master
Gerrit-Owner: Jon Kliegman <kli...@chromium.org>

Sameer Nanda (Code Review)

unread,
May 17, 2012, 4:57:19 PM5/17/12
to Jon Kliegman, Sameer Nanda, Ryan Harrison, Simon Arscott
Sameer Nanda has posted comments on this change.

Change subject: Send fan hysteresis data to chromium as UMA histogram
......................................................................


Patch Set 7: Looks good to me, approved

--
To view, visit https://gerrit.chromium.org/gerrit/22844
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I57d69a6154a84b8dbbebe2a460230798d4ae16c6
Gerrit-PatchSet: 7
Gerrit-Project: chromiumos/platform/power_manager
Gerrit-Branch: master
Gerrit-Owner: Jon Kliegman <kli...@chromium.org>

Ryan Harrison (Code Review)

unread,
May 17, 2012, 4:57:38 PM5/17/12
to Jon Kliegman, Sameer Nanda, Simon Arscott
Ryan Harrison has posted comments on this change.

Change subject: Send fan hysteresis data to chromium as UMA histogram
......................................................................


Patch Set 7: Looks good to me, approved

--
To view, visit https://gerrit.chromium.org/gerrit/22844
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I57d69a6154a84b8dbbebe2a460230798d4ae16c6
Gerrit-PatchSet: 7
Gerrit-Project: chromiumos/platform/power_manager
Gerrit-Branch: master
Gerrit-Owner: Jon Kliegman <kli...@chromium.org>

Jon Kliegman (Code Review)

unread,
May 18, 2012, 11:07:54 AM5/18/12
to Sameer Nanda, Ryan Harrison, Simon Arscott
Jon Kliegman has posted comments on this change.

Change subject: Send fan hysteresis data to chromium as UMA histogram
......................................................................


Patch Set 7: Ready

kernel change has landed

--
To view, visit https://gerrit.chromium.org/gerrit/22844
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I57d69a6154a84b8dbbebe2a460230798d4ae16c6
Gerrit-PatchSet: 7
Gerrit-Project: chromiumos/platform/power_manager
Gerrit-Branch: master
Gerrit-Owner: Jon Kliegman <kli...@chromium.org>
Reply all
Reply to author
Forward
0 new messages