Adding dumpsys based power monitor for android.

Adding dumpsys based power monitor for android.

The implementation is based on the results from 'dumpsys batterystats'.

R=tonyg@chromium.org,jeremy@chromium.org
BUG=314481
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=261425

[tonyg, 2014-04-02 17:15:52]

This is beautiful! Excellent work.

Some nits inline, but please take an lgtm in advance.

https://codereview.chromium.org/222413002/diff/30001/tools/telemetry/telemetr...
File
tools/telemetry/telemetry/core/platform/power_monitor/android_dumpsys_power_monitor.py
(right):

https://codereview.chromium.org/222413002/diff/30001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/platform/power_monitor/android_dumpsys_power_monitor.py:32:
result = self._adb.RunShellCommand('dumpsys batterystats -c')
I haven't looked into which args batterystats takes, but I've found that with
dumpsys, it is usually best to be as specific as possible in order to get it to
run quickly. Can we pass interested pids or anything?

https://codereview.chromium.org/222413002/diff/30001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/platform/power_monitor/android_dumpsys_power_monitor.py:46:
csvreader = csv.reader(dumpsys_output)
The csv package has a class that I think will make this more readable.
DictReader or something?

Then all the entries below will have names instead of numbers.

https://codereview.chromium.org/222413002/diff/30001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/platform/power_monitor/android_dumpsys_power_monitor.py:54:
assert len(entries_by_type[package]) == 1
Let's put error message on all these assertions.

https://codereview.chromium.org/222413002/diff/30001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/platform/power_monitor/android_dumpsys_power_monitor.py:57:
if entry[1] == uid and
This indentation is funky. I'd go with 4 spaces in from consumption_mha, or else
one space to the right of "[".

https://codereview.chromium.org/222413002/diff/30001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/platform/power_monitor/android_dumpsys_power_monitor.py:63:
consumption_mwh = consumption_mah * 3.7
Same value android uses? I've noticed some of my devices at more like 3.9.
Should we just pull voltage now or is an ideal value more appropriate here?

https://codereview.chromium.org/222413002/diff/30001/tools/telemetry/telemetr...
File
tools/telemetry/telemetry/core/platform/power_monitor/android_dumpsys_power_monitor_unittest.py
(right):

https://codereview.chromium.org/222413002/diff/30001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/platform/power_monitor/android_dumpsys_power_monitor_unittest.py:20:
self.assertEqual(results['energy_consumption_mwh'], 2.7047)
Should this be an assertAlmostEqual or are we guaranteed precision here.

[qsr, 2014-04-03 09:47:41]

https://chromiumcodereview.appspot.com/222413002/diff/30001/tools/telemetry/t...
File
tools/telemetry/telemetry/core/platform/power_monitor/android_dumpsys_power_monitor.py
(right):

https://chromiumcodereview.appspot.com/222413002/diff/30001/tools/telemetry/t...
tools/telemetry/telemetry/core/platform/power_monitor/android_dumpsys_power_monitor.py:32:
result = self._adb.RunShellCommand('dumpsys batterystats -c')
On 2014/04/02 17:15:52, tonyg wrote:
> I haven't looked into which args batterystats takes, but I've found that with
> dumpsys, it is usually best to be as specific as possible in order to get it
to
> run quickly. Can we pass interested pids or anything?

 The documentation says we can pass package name, so I updated with it, but my
experiment showed no difference.

 Moreover, if in the future, we try to collect overall power consumption, we
will need the fill dump.

https://chromiumcodereview.appspot.com/222413002/diff/30001/tools/telemetry/t...
tools/telemetry/telemetry/core/platform/power_monitor/android_dumpsys_power_monitor.py:46:
csvreader = csv.reader(dumpsys_output)
On 2014/04/02 17:15:52, tonyg wrote:
> The csv package has a class that I think will make this more readable.
> DictReader or something?
> 
> Then all the entries below will have names instead of numbers.

 Seems nice, except that the name of the columns depends on the type of the
row... Makes it less useful to name columns :( I will add constants to allow
nice reads of this code.

https://chromiumcodereview.appspot.com/222413002/diff/30001/tools/telemetry/t...
tools/telemetry/telemetry/core/platform/power_monitor/android_dumpsys_power_monitor.py:54:
assert len(entries_by_type[package]) == 1
On 2014/04/02 17:15:52, tonyg wrote:
> Let's put error message on all these assertions.

Done.

https://chromiumcodereview.appspot.com/222413002/diff/30001/tools/telemetry/t...
tools/telemetry/telemetry/core/platform/power_monitor/android_dumpsys_power_monitor.py:57:
if entry[1] == uid and
On 2014/04/02 17:15:52, tonyg wrote:
> This indentation is funky. I'd go with 4 spaces in from consumption_mha, or
else
> one space to the right of "[".

Done.

https://chromiumcodereview.appspot.com/222413002/diff/30001/tools/telemetry/t...
tools/telemetry/telemetry/core/platform/power_monitor/android_dumpsys_power_monitor.py:63:
consumption_mwh = consumption_mah * 3.7
On 2014/04/02 17:15:52, tonyg wrote:
> Same value android uses? I've noticed some of my devices at more like 3.9.
> Should we just pull voltage now or is an ideal value more appropriate here?

I do not want to put voltage_now, because the heuristic is based on the
operation of the CPU, while the change of voltage is only due to the battery
charging space, so I think using this will just make the result depends on the
battery state.

 I chose 3.7, as I've seen it is the nominal value for Li-ion battery. Will
change to 4.0V as this is the value we set the monsoon to, so that might help
get comparable results. Another idea would be to measure Ah instead of Wh, as
that will give us more correct results for monsoon, fuel gauge and this, but I
do not know for mac -> jeremy, any idea?

https://chromiumcodereview.appspot.com/222413002/diff/30001/tools/telemetry/t...
File
tools/telemetry/telemetry/core/platform/power_monitor/android_dumpsys_power_monitor_unittest.py
(right):

https://chromiumcodereview.appspot.com/222413002/diff/30001/tools/telemetry/t...
tools/telemetry/telemetry/core/platform/power_monitor/android_dumpsys_power_monitor_unittest.py:20:
self.assertEqual(results['energy_consumption_mwh'], 2.7047)
On 2014/04/02 17:15:52, tonyg wrote:
> Should this be an assertAlmostEqual or are we guaranteed precision here.

 Given that this is a test where we control all parameters, the exact comparison
works, but agreed that almost equals is the right concept.

[jeremy, 2014-04-03 11:16:30]

Great stuff!!

Thanks for cleaning up the Sync interfaces!!

https://codereview.chromium.org/222413002/diff/50001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/core/platform/__init__.py (right):

https://codereview.chromium.org/222413002/diff/50001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/platform/__init__.py:186: """Stops monitoring
power utilization and returns collects stats
While you're in here :)
s/returns collects/returns/

https://codereview.chromium.org/222413002/diff/50001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/core/platform/android_platform_backend.py
(right):

https://codereview.chromium.org/222413002/diff/50001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/platform/android_platform_backend.py:57:
android_dumpsys_power_monitor.DumpsysPowerMonitor(adb),
nit: trailing ,

https://codereview.chromium.org/222413002/diff/50001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/core/platform/platform_backend_unittest.py
(left):

https://codereview.chromium.org/222413002/diff/50001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/platform/platform_backend_unittest.py:12: def
testPowerMonitoringSync(self):
Please don't delete this unit test - this is the only test that actually does an
end-end test of power measurement.
If this doesn't make sense for Android then you can just move this to be Mac
only.

https://codereview.chromium.org/222413002/diff/50001/tools/telemetry/telemetr...
File
tools/telemetry/telemetry/core/platform/power_monitor/android_dumpsys_power_monitor.py
(right):

https://codereview.chromium.org/222413002/diff/50001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/platform/power_monitor/android_dumpsys_power_monitor.py:12:
class DumpsysPowerMonitor(power_monitor.PowerMonitor):
Can you add a header comment, especially interesting is the guarantees and
precision of this power monitor vs other types of power monitoring.

https://codereview.chromium.org/222413002/diff/50001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/platform/power_monitor/android_dumpsys_power_monitor.py:13:
def __init__(self, adb):
Comment input parameters

https://codereview.chromium.org/222413002/diff/50001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/platform/power_monitor/android_dumpsys_power_monitor.py:25:
self._adb.DisableUsbCharging()
What does this line do exactly? Can you add a comment?

https://codereview.chromium.org/222413002/diff/50001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/platform/power_monitor/android_dumpsys_power_monitor.py:33:
result = self._adb.RunShellCommand('dumpsys batterystats -c %s' % package)
I take it that dumpsys measures from disable->enable ?

https://codereview.chromium.org/222413002/diff/50001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/platform/power_monitor/android_dumpsys_power_monitor.py:34:
assert result, 'DumpSys produced no output'
nit: Dumpsys per capitalization in
http://source.android.com/devices/tech/input/dumpsys.html .

https://codereview.chromium.org/222413002/diff/50001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/platform/power_monitor/android_dumpsys_power_monitor.py:40:
def ParseSamplingOutput(package, dumpsys_output):
Do you also get any kind of stat which could give an indication of how hard the
GPU is working?

https://codereview.chromium.org/222413002/diff/50001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/platform/power_monitor/android_dumpsys_power_monitor.py:46:
# cvs columns
nit: s/cvs/csv/

https://codereview.chromium.org/222413002/diff/50001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/platform/power_monitor/android_dumpsys_power_monitor.py:60:
# Find the uid of for the given package
nit: .

https://codereview.chromium.org/222413002/diff/50001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/platform/power_monitor/android_dumpsys_power_monitor.py:64:
consumptions_mha = [float(entry[PWI_POWER_COMSUMPTION_INDEX])
_mah ?

https://codereview.chromium.org/222413002/diff/50001/tools/telemetry/telemetr...
File
tools/telemetry/telemetry/core/platform/power_monitor/android_dumpsys_power_monitor_unittest.py
(right):

https://codereview.chromium.org/222413002/diff/50001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/platform/power_monitor/android_dumpsys_power_monitor_unittest.py:20:
self.assertAlmostEqual(results['energy_consumption_mwh'], 2.924)
Can you check the 'identifier' field here too?

https://codereview.chromium.org/222413002/diff/50001/tools/telemetry/unittest...
File tools/telemetry/unittest_data/batterystats.csv (right):

https://codereview.chromium.org/222413002/diff/50001/tools/telemetry/unittest...
tools/telemetry/unittest_data/batterystats.csv:1:
7,hsp,0,android.process.acore,10003
Can you rename this file to contain the device name it was taken from - up to
you, if you think that's an interesting piece of information.

[qsr, 2014-04-03 12:02:43]

https://codereview.chromium.org/222413002/diff/50001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/core/platform/__init__.py (right):

https://codereview.chromium.org/222413002/diff/50001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/platform/__init__.py:186: """Stops monitoring
power utilization and returns collects stats
On 2014/04/03 11:16:30, jeremy wrote:
> While you're in here :)
> s/returns collects/returns/

Done.

https://codereview.chromium.org/222413002/diff/50001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/core/platform/android_platform_backend.py
(right):

https://codereview.chromium.org/222413002/diff/50001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/platform/android_platform_backend.py:57:
android_dumpsys_power_monitor.DumpsysPowerMonitor(adb),
On 2014/04/03 11:16:30, jeremy wrote:
> nit: trailing ,

 Is voluntary, helps when extending/shrinking this list.

https://codereview.chromium.org/222413002/diff/50001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/core/platform/platform_backend_unittest.py
(left):

https://codereview.chromium.org/222413002/diff/50001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/platform/platform_backend_unittest.py:12: def
testPowerMonitoringSync(self):
On 2014/04/03 11:16:30, jeremy wrote:
> Please don't delete this unit test - this is the only test that actually does
an
> end-end test of power measurement.
> If this doesn't make sense for Android then you can just move this to be Mac
> only.

Done.

https://codereview.chromium.org/222413002/diff/50001/tools/telemetry/telemetr...
File
tools/telemetry/telemetry/core/platform/power_monitor/android_dumpsys_power_monitor.py
(right):

https://codereview.chromium.org/222413002/diff/50001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/platform/power_monitor/android_dumpsys_power_monitor.py:12:
class DumpsysPowerMonitor(power_monitor.PowerMonitor):
On 2014/04/03 11:16:30, jeremy wrote:
> Can you add a header comment, especially interesting is the guarantees and
> precision of this power monitor vs other types of power monitoring.

Done.

https://codereview.chromium.org/222413002/diff/50001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/platform/power_monitor/android_dumpsys_power_monitor.py:13:
def __init__(self, adb):
On 2014/04/03 11:16:30, jeremy wrote:
> Comment input parameters

Done.

https://codereview.chromium.org/222413002/diff/50001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/platform/power_monitor/android_dumpsys_power_monitor.py:25:
self._adb.DisableUsbCharging()
On 2014/04/03 11:16:30, jeremy wrote:
> What does this line do exactly? Can you add a comment?

Done.

https://codereview.chromium.org/222413002/diff/50001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/platform/power_monitor/android_dumpsys_power_monitor.py:33:
result = self._adb.RunShellCommand('dumpsys batterystats -c %s' % package)
On 2014/04/03 11:16:30, jeremy wrote:
> I take it that dumpsys measures from disable->enable ?

Done.

https://codereview.chromium.org/222413002/diff/50001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/platform/power_monitor/android_dumpsys_power_monitor.py:34:
assert result, 'DumpSys produced no output'
On 2014/04/03 11:16:30, jeremy wrote:
> nit: Dumpsys per capitalization in
> http://source.android.com/devices/tech/input/dumpsys.html .

Done.

https://codereview.chromium.org/222413002/diff/50001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/platform/power_monitor/android_dumpsys_power_monitor.py:40:
def ParseSamplingOutput(package, dumpsys_output):
On 2014/04/03 11:16:30, jeremy wrote:
> Do you also get any kind of stat which could give an indication of how hard
the
> GPU is working?

 Not at this moment. When/If android starts collecting this data, we should get
it for free.

https://codereview.chromium.org/222413002/diff/50001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/platform/power_monitor/android_dumpsys_power_monitor.py:46:
# cvs columns
On 2014/04/03 11:16:30, jeremy wrote:
> nit: s/cvs/csv/

Done.

https://codereview.chromium.org/222413002/diff/50001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/platform/power_monitor/android_dumpsys_power_monitor.py:60:
# Find the uid of for the given package
On 2014/04/03 11:16:30, jeremy wrote:
> nit: .

Done.

https://codereview.chromium.org/222413002/diff/50001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/platform/power_monitor/android_dumpsys_power_monitor.py:64:
consumptions_mha = [float(entry[PWI_POWER_COMSUMPTION_INDEX])
On 2014/04/03 11:16:30, jeremy wrote:
> _mah ?

Done.

https://codereview.chromium.org/222413002/diff/50001/tools/telemetry/telemetr...
File
tools/telemetry/telemetry/core/platform/power_monitor/android_dumpsys_power_monitor_unittest.py
(right):

https://codereview.chromium.org/222413002/diff/50001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/platform/power_monitor/android_dumpsys_power_monitor_unittest.py:20:
self.assertAlmostEqual(results['energy_consumption_mwh'], 2.924)
On 2014/04/03 11:16:30, jeremy wrote:
> Can you check the 'identifier' field here too?

Done.

https://codereview.chromium.org/222413002/diff/50001/tools/telemetry/unittest...
File tools/telemetry/unittest_data/batterystats.csv (right):

https://codereview.chromium.org/222413002/diff/50001/tools/telemetry/unittest...
tools/telemetry/unittest_data/batterystats.csv:1:
7,hsp,0,android.process.acore,10003
On 2014/04/03 11:16:30, jeremy wrote:
> Can you rename this file to contain the device name it was taken from - up to
> you, if you think that's an interesting piece of information.

 Not relevant now, but might be in the future if multiple format appear. I'll
prefix with the dump version.

[jeremy, 2014-04-03 12:11:42]

LGTM with nits

https://codereview.chromium.org/222413002/diff/70001/tools/telemetry/telemetr...
File
tools/telemetry/telemetry/core/platform/power_monitor/android_dumpsys_power_monitor.py
(right):

https://codereview.chromium.org/222413002/diff/70001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/platform/power_monitor/android_dumpsys_power_monitor.py:14:
consumption of a single android application. This measure is using heuristic
nit: s/is using/uses a/

https://codereview.chromium.org/222413002/diff/70001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/platform/power_monitor/android_dumpsys_power_monitor.py:45:
# By default, 'dumpsys batterystats' measure power consumption during the
nit:measures

[qsr, 2014-04-03 12:19:06]

https://codereview.chromium.org/222413002/diff/70001/tools/telemetry/telemetr...
File
tools/telemetry/telemetry/core/platform/power_monitor/android_dumpsys_power_monitor.py
(right):

https://codereview.chromium.org/222413002/diff/70001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/platform/power_monitor/android_dumpsys_power_monitor.py:14:
consumption of a single android application. This measure is using heuristic
On 2014/04/03 12:11:42, jeremy wrote:
> nit: s/is using/uses a/

Done.

https://codereview.chromium.org/222413002/diff/70001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/platform/power_monitor/android_dumpsys_power_monitor.py:45:
# By default, 'dumpsys batterystats' measure power consumption during the
On 2014/04/03 12:11:42, jeremy wrote:
> nit:measures

Done.

[qsr, 2014-04-03 12:19:11]

The CQ bit was checked by qsr@chromium.org

[commit-bot: I haz the power, 2014-04-03 12:20:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/qsr@chromium.org/222413002/90001

[qsr, 2014-04-03 13:45:28]

The CQ bit was checked by qsr@chromium.org

[commit-bot: I haz the power, 2014-04-03 13:45:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/qsr@chromium.org/222413002/80018

[commit-bot: I haz the power, 2014-04-03 15:56:29]

Change committed as 261425