[Android][Telemetry] Add support for multiple power monitors

[Android][Telemetry] Add support for multiple power monitors

BUG=513004
Committed: https://crrev.com/a36acb95c375d4de17277cadbe8d56b45a80d5ce
Cr-Commit-Position: refs/heads/master@{#340208}

[rnephew (Reviews Here), 2015-07-23 19:14:01]

rnephew@chromium.org changed reviewers:
+ jdduke@chromium.org, nednguyen@google.com

[jdduke (slow), 2015-07-23 19:24:47]

https://codereview.chromium.org/1255673002/diff/1/tools/perf/metrics/power.py
File tools/perf/metrics/power.py (right):

https://codereview.chromium.org/1255673002/diff/1/tools/perf/metrics/power.py...
tools/perf/metrics/power.py:107: application_energy_consumption_mwh =
self._results.get(APP_POWER_LABEL)
Maybe someday we can make this a loop :).

https://codereview.chromium.org/1255673002/diff/1/tools/telemetry/telemetry/i...
File
tools/telemetry/telemetry/internal/platform/power_monitor/power_monitor_controller.py
(right):

https://codereview.chromium.org/1255673002/diff/1/tools/telemetry/telemetry/i...
tools/telemetry/telemetry/internal/platform/power_monitor/power_monitor_controller.py:30:
self._active_monitors.append(monitor)
Hmm, not sure about these side effects in the CanMonitor method, any downside to
modifying _active_monitors in the StartMonitor method, asserting that it's empty
at first, and non-empty after adding potential monitors?

https://codereview.chromium.org/1255673002/diff/1/tools/telemetry/telemetry/i...
File
tools/telemetry/telemetry/internal/platform/power_monitor/sysfs_power_monitor.py
(right):

https://codereview.chromium.org/1255673002/diff/1/tools/telemetry/telemetry/i...
tools/telemetry/telemetry/internal/platform/power_monitor/sysfs_power_monitor.py:26:
standalone: If it is not wrapping another monitor, set to True.
I see, should we add a TODO to convert other platforms (Cros in particular) to
use PowerMonitorController so this can always be standalone?

[rnephew (Reviews Here), 2015-07-23 19:34:46]

https://codereview.chromium.org/1255673002/diff/1/tools/telemetry/telemetry/i...
File
tools/telemetry/telemetry/internal/platform/power_monitor/power_monitor_controller.py
(right):

https://codereview.chromium.org/1255673002/diff/1/tools/telemetry/telemetry/i...
tools/telemetry/telemetry/internal/platform/power_monitor/power_monitor_controller.py:30:
self._active_monitors.append(monitor)
On 2015/07/23 19:24:46, jdduke wrote:
> Hmm, not sure about these side effects in the CanMonitor method, any downside
to
> modifying _active_monitors in the StartMonitor method, asserting that it's
empty
> at first, and non-empty after adding potential monitors?

Done.

https://codereview.chromium.org/1255673002/diff/1/tools/telemetry/telemetry/i...
File
tools/telemetry/telemetry/internal/platform/power_monitor/sysfs_power_monitor.py
(right):

https://codereview.chromium.org/1255673002/diff/1/tools/telemetry/telemetry/i...
tools/telemetry/telemetry/internal/platform/power_monitor/sysfs_power_monitor.py:26:
standalone: If it is not wrapping another monitor, set to True.
On 2015/07/23 19:24:46, jdduke wrote:
> I see, should we add a TODO to convert other platforms (Cros in particular) to
> use PowerMonitorController so this can always be standalone?

Done.

[jdduke (slow), 2015-07-23 19:39:38]

Thanks, I'll leave final review for Ned.

[nednguyen, 2015-07-23 20:26:07]

https://codereview.chromium.org/1255673002/diff/20001/tools/telemetry/telemet...
File
tools/telemetry/telemetry/internal/platform/power_monitor/power_monitor_controller.py
(right):

https://codereview.chromium.org/1255673002/diff/20001/tools/telemetry/telemet...
tools/telemetry/telemetry/internal/platform/power_monitor/power_monitor_controller.py:21:
self._canidate_power_monitors = power_monitors
self._candidate_power_monitors

https://codereview.chromium.org/1255673002/diff/20001/tools/telemetry/telemet...
tools/telemetry/telemetry/internal/platform/power_monitor/power_monitor_controller.py:30:
return False
return any(m.CanMonitorPower() for m in self._candidate_power_monitors)

https://codereview.chromium.org/1255673002/diff/20001/tools/telemetry/telemet...
tools/telemetry/telemetry/internal/platform/power_monitor/power_monitor_controller.py:36:
self._active_monitors.append(monitor)
self._active_monitors = [m for m self._candidate_power_monitors if
m.CanMonitorPower()]

https://codereview.chromium.org/1255673002/diff/20001/tools/telemetry/telemet...
File
tools/telemetry/telemetry/internal/platform/power_monitor/sysfs_power_monitor.py
(right):

https://codereview.chromium.org/1255673002/diff/20001/tools/telemetry/telemet...
tools/telemetry/telemetry/internal/platform/power_monitor/sysfs_power_monitor.py:21:
# TODO(rnephew): Convert all platforms to use standalone power monitors.
Can you file a bug for this? It looks like a substantial amount work.

[rnephew (Reviews Here), 2015-07-23 20:41:20]

https://codereview.chromium.org/1255673002/diff/20001/tools/telemetry/telemet...
File
tools/telemetry/telemetry/internal/platform/power_monitor/power_monitor_controller.py
(right):

https://codereview.chromium.org/1255673002/diff/20001/tools/telemetry/telemet...
tools/telemetry/telemetry/internal/platform/power_monitor/power_monitor_controller.py:21:
self._canidate_power_monitors = power_monitors
On 2015/07/23 20:26:07, nednguyen wrote:
> self._candidate_power_monitors

Done.

https://codereview.chromium.org/1255673002/diff/20001/tools/telemetry/telemet...
tools/telemetry/telemetry/internal/platform/power_monitor/power_monitor_controller.py:30:
return False
On 2015/07/23 20:26:07, nednguyen wrote:
> return any(m.CanMonitorPower() for m in self._candidate_power_monitors)

Done.

https://codereview.chromium.org/1255673002/diff/20001/tools/telemetry/telemet...
tools/telemetry/telemetry/internal/platform/power_monitor/power_monitor_controller.py:36:
self._active_monitors.append(monitor)
On 2015/07/23 20:26:07, nednguyen wrote:
> self._active_monitors = [m for m self._candidate_power_monitors if
> m.CanMonitorPower()]

Done.

https://codereview.chromium.org/1255673002/diff/20001/tools/telemetry/telemet...
File
tools/telemetry/telemetry/internal/platform/power_monitor/sysfs_power_monitor.py
(right):

https://codereview.chromium.org/1255673002/diff/20001/tools/telemetry/telemet...
tools/telemetry/telemetry/internal/platform/power_monitor/sysfs_power_monitor.py:21:
# TODO(rnephew): Convert all platforms to use standalone power monitors.
On 2015/07/23 20:26:07, nednguyen wrote:
> Can you file a bug for this? It looks like a substantial amount work. 

crbug.com/513453

[nednguyen, 2015-07-23 21:01:35]

lgtm

https://codereview.chromium.org/1255673002/diff/40001/tools/telemetry/telemet...
File
tools/telemetry/telemetry/internal/platform/power_monitor/sysfs_power_monitor.py
(right):

https://codereview.chromium.org/1255673002/diff/40001/tools/telemetry/telemet...
tools/telemetry/telemetry/internal/platform/power_monitor/sysfs_power_monitor.py:21:
# TODO(rnephew): Convert all platforms to use standalone power monitors.
Nits: put the bug number in this TODO

[rnephew (Reviews Here), 2015-07-23 21:05:18]

https://codereview.chromium.org/1255673002/diff/40001/tools/telemetry/telemet...
File
tools/telemetry/telemetry/internal/platform/power_monitor/sysfs_power_monitor.py
(right):

https://codereview.chromium.org/1255673002/diff/40001/tools/telemetry/telemet...
tools/telemetry/telemetry/internal/platform/power_monitor/sysfs_power_monitor.py:21:
# TODO(rnephew): Convert all platforms to use standalone power monitors.
On 2015/07/23 21:01:34, nednguyen wrote:
> Nits: put the bug number in this TODO

Done.

[rnephew (Reviews Here), 2015-07-23 21:05:37]

The CQ bit was checked by rnephew@chromium.org

[rnephew (Reviews Here), 2015-07-23 21:05:37]

The patchset sent to the CQ was uploaded after l-g-t-m from nednguyen@google.com
Link to the patchset: https://codereview.chromium.org/1255673002/#ps60001
(title: " ")

[commit-bot: I haz the power, 2015-07-23 21:06:13]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1255673002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1255673002/60001

[commit-bot: I haz the power, 2015-07-23 23:22:54]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-07-23 23:23:59]

Patchset 4 (id:??) landed as
https://crrev.com/a36acb95c375d4de17277cadbe8d56b45a80d5ce
Cr-Commit-Position: refs/heads/master@{#340208}

[jbudorick, 2015-07-24 14:03:44]

A revert of this CL (patchset #4 id:60001) has been created in
https://codereview.chromium.org/1252923003/ by jbudorick@chromium.org.

The reason for reverting is: speculative revert: this is the prime suspect for
the telemetry_perf_unittests failure on
http://build.chromium.org/p/chromium.linux/builders/Android%20Tests%20(dbg).