Update parsing of dumpsys batterystats

Update parsing of dumpsys batterystats

The format of dumpsys batterystats has changed. This CL updates the
parsing.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=284659

[pasko, 2014-07-21 13:13:11]

https://codereview.chromium.org/402153002/diff/1/tools/telemetry/telemetry/co...
File
tools/telemetry/telemetry/core/platform/power_monitor/android_dumpsys_power_monitor.py
(right):

https://codereview.chromium.org/402153002/diff/1/tools/telemetry/telemetry/co...
tools/telemetry/telemetry/core/platform/power_monitor/android_dumpsys_power_monitor.py:58:
"""Parse output of 'dumpsys batterystats -c'
I could not easily find the description of the format. Could you provide a link
here?

https://codereview.chromium.org/402153002/diff/1/tools/telemetry/telemetry/co...
tools/telemetry/telemetry/core/platform/power_monitor/android_dumpsys_power_monitor.py:70:
COLUMN_TYPE_INDEX = 3
isn't it rather a row type?

https://codereview.chromium.org/402153002/diff/1/tools/telemetry/telemetry/co...
tools/telemetry/telemetry/core/platform/power_monitor/android_dumpsys_power_monitor.py:73:
PACKAGE_NAME_INDEX = 5
In the very first lines of batterystats_v8.csv I see some package name at index
4. This package name at index==5 seems to be some other kind of a package name.
Please put a more specific name for this constant to hint what package name
"category" it is about.

https://codereview.chromium.org/402153002/diff/1/tools/telemetry/telemetry/co...
tools/telemetry/telemetry/core/platform/power_monitor/android_dumpsys_power_monitor.py:75:
PWI_UID_INDEX = 1
what is PWI? Please describe in comments what possible values for PWI* fields
are and what possible values they can hold.

https://codereview.chromium.org/402153002/diff/1/tools/telemetry/telemetry/co...
tools/telemetry/telemetry/core/platform/power_monitor/android_dumpsys_power_monitor.py:85:
if len(entry) <= COLUMN_TYPE_INDEX:
strictly less?

https://codereview.chromium.org/402153002/diff/1/tools/telemetry/telemetry/co...
tools/telemetry/telemetry/core/platform/power_monitor/android_dumpsys_power_monitor.py:89:
assert current_package not in uid_entries
Please educate me:

AFAICT sandboxed processes can run with different UIDs, and AFAIR their package
names are different. No contradiction here, but I don't understand where the
guarantee is coming from. Is this the Android framework guarantee that the same
package cannot run as 2 UIDs simultaneously?

https://codereview.chromium.org/402153002/diff/1/tools/telemetry/telemetry/co...
tools/telemetry/telemetry/core/platform/power_monitor/android_dumpsys_power_monitor.py:99:
logging.warning('Unable to parse dumpsys output. Please upgrade the OS.')
the warning should probably instruct updating OS on the device and not on the
host, which might be not totally obvious to someone who runs this script.

[qsr, 2014-07-21 13:30:24]

https://codereview.chromium.org/402153002/diff/1/tools/telemetry/telemetry/co...
File
tools/telemetry/telemetry/core/platform/power_monitor/android_dumpsys_power_monitor.py
(right):

https://codereview.chromium.org/402153002/diff/1/tools/telemetry/telemetry/co...
tools/telemetry/telemetry/core/platform/power_monitor/android_dumpsys_power_monitor.py:58:
"""Parse output of 'dumpsys batterystats -c'
On 2014/07/21 13:13:11, pasko wrote:
> I could not easily find the description of the format. Could you provide a
link
> here?

I do not know of any description of this format, out of looking in the source:
https://android.googlesource.com/platform/frameworks/base/+/master/core/java/...

https://codereview.chromium.org/402153002/diff/1/tools/telemetry/telemetry/co...
tools/telemetry/telemetry/core/platform/power_monitor/android_dumpsys_power_monitor.py:70:
COLUMN_TYPE_INDEX = 3
On 2014/07/21 13:13:11, pasko wrote:
> isn't it rather a row type?

 This is the index of the column that contain the type of the row.

https://codereview.chromium.org/402153002/diff/1/tools/telemetry/telemetry/co...
tools/telemetry/telemetry/core/platform/power_monitor/android_dumpsys_power_monitor.py:73:
PACKAGE_NAME_INDEX = 5
On 2014/07/21 13:13:10, pasko wrote:
> In the very first lines of batterystats_v8.csv I see some package name at
index
> 4. This package name at index==5 seems to be some other kind of a package
name.
> Please put a more specific name for this constant to hint what package name
> "category" it is about.

 That is the same kind of package, but not taking from this type of row. Look
for with uid at index 3

https://codereview.chromium.org/402153002/diff/1/tools/telemetry/telemetry/co...
tools/telemetry/telemetry/core/platform/power_monitor/android_dumpsys_power_monitor.py:75:
PWI_UID_INDEX = 1
On 2014/07/21 13:13:11, pasko wrote:
> what is PWI? Please describe in comments what possible values for PWI* fields
> are and what possible values they can hold.

Done.

https://codereview.chromium.org/402153002/diff/1/tools/telemetry/telemetry/co...
tools/telemetry/telemetry/core/platform/power_monitor/android_dumpsys_power_monitor.py:85:
if len(entry) <= COLUMN_TYPE_INDEX:
On 2014/07/21 13:13:11, pasko wrote:
> strictly less?

 Nope. If I have 5 entry in a row, I cannot use the entry at index 5. Reversed
to make it clearer.

https://codereview.chromium.org/402153002/diff/1/tools/telemetry/telemetry/co...
tools/telemetry/telemetry/core/platform/power_monitor/android_dumpsys_power_monitor.py:89:
assert current_package not in uid_entries
On 2014/07/21 13:13:11, pasko wrote:
> Please educate me:
> 
> AFAICT sandboxed processes can run with different UIDs, and AFAIR their
package
> names are different. No contradiction here, but I don't understand where the
> guarantee is coming from. Is this the Android framework guarantee that the
same
> package cannot run as 2 UIDs simultaneously?

 The uid there is an unique identifier for battery statistics. The OS aggregate
all the consumption of the different processes.

https://codereview.chromium.org/402153002/diff/1/tools/telemetry/telemetry/co...
tools/telemetry/telemetry/core/platform/power_monitor/android_dumpsys_power_monitor.py:99:
logging.warning('Unable to parse dumpsys output. Please upgrade the OS.')
On 2014/07/21 13:13:10, pasko wrote:
> the warning should probably instruct updating OS on the device and not on the
> host, which might be not totally obvious to someone who runs this script.

Done.

[pasko, 2014-07-21 13:52:02]

lgtm with a few more tweaks in comments.

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

https://codereview.chromium.org/402153002/diff/20001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/platform/power_monitor/android_dumpsys_power_monitor.py:61:
https://android.googlesource.com/platform/frameworks/base/+/master/core/java/...
thanks, that's helpful

How about the link to the precise commit, not just tip of master?

For example:
https://android.googlesource.com/platform/frameworks/base/+/c3eef19047b897bfb...

By the way, I don't see anything about 'pwi' there, is it in another file?

https://codereview.chromium.org/402153002/diff/20001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/platform/power_monitor/android_dumpsys_power_monitor.py:71:
# csv columns
nit: comments should be full sentences, like:
"The list of useful CSV columns."

https://codereview.chromium.org/402153002/diff/20001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/platform/power_monitor/android_dumpsys_power_monitor.py:74:
# Index of the column containing the type of the column.
s/type of the column/type of the row/

Since other constants don't have the word "COLUMN" in them, even though they
reference column indexes, it would be more consistent to name it ROW_TYPE_INDEX.

https://codereview.chromium.org/402153002/diff/20001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/platform/power_monitor/android_dumpsys_power_monitor.py:77:
# Index for columns of type 'uid'
This confused me once. Please mention that UID is a Uinque Idendifier (and not a
UserId).

[qsr, 2014-07-21 14:03:53]

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

https://codereview.chromium.org/402153002/diff/20001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/platform/power_monitor/android_dumpsys_power_monitor.py:61:
https://android.googlesource.com/platform/frameworks/base/+/master/core/java/...
 Given that this will have to evolve with the different versions of this file,
and that we will probably have to maintain multiple version in the future, I'm
not sure pointing to an exact commit is that useful.

https://codereview.chromium.org/402153002/diff/20001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/platform/power_monitor/android_dumpsys_power_monitor.py:71:
# csv columns
On 2014/07/21 13:52:02, pasko wrote:
> nit: comments should be full sentences, like:
> "The list of useful CSV columns."

Done.

https://codereview.chromium.org/402153002/diff/20001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/platform/power_monitor/android_dumpsys_power_monitor.py:74:
# Index of the column containing the type of the column.
On 2014/07/21 13:52:01, pasko wrote:
> s/type of the column/type of the row/
> 
> Since other constants don't have the word "COLUMN" in them, even though they
> reference column indexes, it would be more consistent to name it
ROW_TYPE_INDEX.

Done.

https://codereview.chromium.org/402153002/diff/20001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/platform/power_monitor/android_dumpsys_power_monitor.py:77:
# Index for columns of type 'uid'
On 2014/07/21 13:52:01, pasko wrote:
> This confused me once. Please mention that UID is a Uinque Idendifier (and not
a
> UserId).

Done.

[tonyg, 2014-07-22 03:12:51]

lgtm

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

https://codereview.chromium.org/402153002/diff/40001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/platform/power_monitor/android_dumpsys_power_monitor.py:97:
break
Should we logging.warn here?

[qsr, 2014-07-22 07:51:23]

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

https://codereview.chromium.org/402153002/diff/40001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/platform/power_monitor/android_dumpsys_power_monitor.py:97:
break
On 2014/07/22 03:12:50, tonyg wrote:
> Should we logging.warn here?

 We will warn line 112 if we reach this.

[qsr, 2014-07-22 07:51:26]

The CQ bit was checked by qsr@chromium.org

[commit-bot: I haz the power, 2014-07-22 07:52:04]

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

[commit-bot: I haz the power, 2014-07-22 08:48:09]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)

[commit-bot: I haz the power, 2014-07-22 10:32:51]

Change committed as 284659