Send ts_mon metrics from android device data.

Send ts_mon metrics from android device data.

BUG=608427
Committed: https://chromium.googlesource.com/infra/infra/+/a6f6037e0f6bc697b331bf1c0b54ec77ed634399

[ghost stip (do not use), 2016-05-16 07:37:00]

Description was changed from

==========
Initial implementation of getting ts_mon metrics from android device data.

BUG=608427
==========

to

==========
Initial implementation of getting ts_mon metrics from android device data.

BUG=608427
==========

[ghost stip (do not use), 2016-05-16 07:37:00]

stip@chromium.org changed reviewers:
+ bpastene@chromium.org, dsansome@chromium.org, sergeyberezin@chromium.org

[ghost stip (do not use), 2016-05-16 07:39:16]

ptal

this is not expected to work yet, I wanted to get a quick review of the approach
before I figure out how to test this

bpastene: whether the metrics line up with the current data
sergeyberezin / dsansome: whether this fits with the sysmon policy, and whether
the target flipping hack works or not

thanks!

https://codereview.chromium.org/1981853002/diff/40001/infra/services/sysmon/a...
File infra/services/sysmon/android_device_metrics.py (right):

https://codereview.chromium.org/1981853002/diff/40001/infra/services/sysmon/a...
infra/services/sysmon/android_device_metrics.py:20:
ANDROID_PREVIOUS_DEVICE_FILE_VERSION = ANDROID_DEVICE_FILE_VERSION - 1
not used right now, but version bumps + cipd is a nightmare unless you have
rollforward/rollback capability

https://codereview.chromium.org/1981853002/diff/40001/infra/services/sysmon/a...
infra/services/sysmon/android_device_metrics.py:21: ANDROID_DEVICE_FILE =
os.path.join(os.path.expanduser('~'),
as of https://chromereviews.googleplex.com/426027013/ this file is dropped on
every android swarming bot

https://codereview.chromium.org/1981853002/diff/40001/infra/services/sysmon/a...
infra/services/sysmon/android_device_metrics.py:28: cpu_temp =
ts_mon.FloatMetric('dev/cpu/temperature',
I tried to match these to
https://chromium.googlesource.com/chromium/tools/build/+/d648ab88761f198c77ac...

https://codereview.chromium.org/1981853002/diff/40001/infra/services/sysmon/a...
infra/services/sysmon/android_device_metrics.py:57: interface.state.target =
targets.DeviceTarget(
I don't know if you can just *do* this, leaving it up to dsansome@ and
sergeyberezin@

https://codereview.chromium.org/1981853002/diff/40001/infra/services/sysmon/a...
infra/services/sysmon/android_device_metrics.py:102: except IOError:
I feel like I'm missing another Error here... maybe for windows?

[ghost stip (do not use), 2016-05-16 07:42:29]

stip@chromium.org changed reviewers:
+ maruel@chromium.org

[ghost stip (do not use), 2016-05-16 07:42:30]

added maruel for the CPU temperature sensor question

https://codereview.chromium.org/1981853002/diff/40001/infra/services/sysmon/a...
File infra/services/sysmon/android_device_metrics.py (right):

https://codereview.chromium.org/1981853002/diff/40001/infra/services/sysmon/a...
infra/services/sysmon/android_device_metrics.py:87: (cpu_temp, cpu_temps[0] if
cpu_temps else None),
maruel: I'm hoping that swarming is smart enough in
https://goto.google.com/cyjvj to report the 'useful' temperature sensor first.
if that isn't the case, I'd like to export the blacklist out from swarming
somehow

[Sergey Berezin, 2016-05-16 19:01:43]

Most of the code looks good to me, except for the target field hack.

https://codereview.chromium.org/1981853002/diff/40001/infra/services/sysmon/a...
File infra/services/sysmon/android_device_metrics.py (right):

https://codereview.chromium.org/1981853002/diff/40001/infra/services/sysmon/a...
infra/services/sysmon/android_device_metrics.py:57: interface.state.target =
targets.DeviceTarget(
On 2016/05/16 07:39:16, stip wrote:
> I don't know if you can just *do* this, leaving it up to dsansome@ and
> sergeyberezin@

Nope, this is effectively a dangerous no-op. The default target is used at the
time of metric flush, which runs in an async thread, and what you're doing is
not thread-safe.

Instead, if you really want to change the target fields, use target_fields={...}
in the metric's .set() method.

But you probably don't want to modify target fields anyway, because why would
you? You already have your own metric names. I don't think we rely on this value
in the old metrics. The only reason it was there was to avoid clashing with
sysmon on presence/up metric. But now that we send it from sysmon itself, there
is no clash. So, I'd say don't modify the target fields at all.

[M-A Ruel, 2016-05-16 23:01:13]

https://codereview.chromium.org/1981853002/diff/40001/infra/services/sysmon/a...
File infra/services/sysmon/android_device_metrics.py (right):

https://codereview.chromium.org/1981853002/diff/40001/infra/services/sysmon/a...
infra/services/sysmon/android_device_metrics.py:51: # From
https://goo.gl/WuJDSu.
not a fan of using url shortener

https://codereview.chromium.org/1981853002/diff/40001/infra/services/sysmon/a...
infra/services/sysmon/android_device_metrics.py:87: (cpu_temp, cpu_temps[0] if
cpu_temps else None),
On 2016/05/16 07:42:30, stip wrote:
> maruel: I'm hoping that swarming is smart enough in
> https://goto.google.com/cyjvj to report the 'useful' temperature sensor first.
> if that isn't the case, I'd like to export the blacklist out from swarming
> somehow

Update the bot_config as you need if you find anything wrong.

https://codereview.chromium.org/1981853002/diff/40001/infra/services/sysmon/a...
infra/services/sysmon/android_device_metrics.py:102: except IOError:
On 2016/05/16 07:39:16, stip wrote:
> I feel like I'm missing another Error here... maybe for windows?

(IOError, OSError)

[ghost stip (do not use), 2016-05-17 22:32:13]

ptal

https://codereview.chromium.org/1981853002/diff/40001/infra/services/sysmon/a...
File infra/services/sysmon/android_device_metrics.py (right):

https://codereview.chromium.org/1981853002/diff/40001/infra/services/sysmon/a...
infra/services/sysmon/android_device_metrics.py:51: # From
https://goo.gl/WuJDSu.
On 2016/05/16 23:01:13, M-A Ruel wrote:
> not a fan of using url shortener

Done.

https://codereview.chromium.org/1981853002/diff/40001/infra/services/sysmon/a...
infra/services/sysmon/android_device_metrics.py:57: interface.state.target =
targets.DeviceTarget(
On 2016/05/16 19:01:43, Sergey Berezin wrote:
> On 2016/05/16 07:39:16, stip wrote:
> > I don't know if you can just *do* this, leaving it up to dsansome@ and
> > sergeyberezin@
> 
> Nope, this is effectively a dangerous no-op. The default target is used at the
> time of metric flush, which runs in an async thread, and what you're doing is
> not thread-safe.
> 
> Instead, if you really want to change the target fields, use
target_fields={...}
> in the metric's .set() method.
> 
> But you probably don't want to modify target fields anyway, because why would
> you? You already have your own metric names. I don't think we rely on this
value
> in the old metrics. The only reason it was there was to avoid clashing with
> sysmon on presence/up metric. But now that we send it from sysmon itself,
there
> is no clash. So, I'd say don't modify the target fields at all.

Done.

https://codereview.chromium.org/1981853002/diff/40001/infra/services/sysmon/a...
infra/services/sysmon/android_device_metrics.py:87: (cpu_temp, cpu_temps[0] if
cpu_temps else None),
On 2016/05/16 23:01:13, M-A Ruel wrote:
> On 2016/05/16 07:42:30, stip wrote:
> > maruel: I'm hoping that swarming is smart enough in
> > https://goto.google.com/cyjvj to report the 'useful' temperature sensor
first.
> > if that isn't the case, I'd like to export the blacklist out from swarming
> > somehow
> 
> Update the bot_config as you need if you find anything wrong.

Acknowledged.

https://codereview.chromium.org/1981853002/diff/40001/infra/services/sysmon/a...
infra/services/sysmon/android_device_metrics.py:102: except IOError:
On 2016/05/16 23:01:13, M-A Ruel wrote:
> On 2016/05/16 07:39:16, stip wrote:
> > I feel like I'm missing another Error here... maybe for windows?
> 
> (IOError, OSError)

Done.

[Sergey Berezin, 2016-05-17 22:48:48]

Looks good, will wait for tests.

In your test setUp(), use ts_mon.reset_for_unittest(disable=True)

Check your metric values with <metric>.get(flags=<flag_dict>)

See
https://code.google.com/p/chromium/codesearch#chromium/infra/infra_libs/ts_mo...

[bpastene, 2016-05-19 18:36:44]

https://codereview.chromium.org/1981853002/diff/60001/infra/services/sysmon/a...
File infra/services/sysmon/android_device_metrics.py (right):

https://codereview.chromium.org/1981853002/diff/60001/infra/services/sysmon/a...
infra/services/sysmon/android_device_metrics.py:40: metric_read_status =
ts_mon.StringMetric(
What are your plans for this metric?

https://codereview.chromium.org/1981853002/diff/60001/infra/services/sysmon/a...
infra/services/sysmon/android_device_metrics.py:47: if not success:
Nit: Remove success from the return and replace this line with
'if not devices:' ? IDK; how do I python?

https://codereview.chromium.org/1981853002/diff/60001/infra/services/sysmon/a...
infra/services/sysmon/android_device_metrics.py:54: cpu_temps =
device.get('temp', [])
I don't think this is a list as reported by swarming. Looks like a dict to me. A
sample test file should pick this up.

https://codereview.chromium.org/1981853002/diff/60001/infra/services/sysmon/a...
infra/services/sysmon/android_device_metrics.py:57: battery_temp = battery_temp
/ 10.0 if battery_temp else None
I've been burned repeatedly with some devices reporting millicelsius, some
reporting decicelsius, and some reporting celsius. Hopefully bot_config
normalizes all this.

[ghost stip (do not use), 2016-05-20 21:12:50]

should be ready for review, ptal!

https://codereview.chromium.org/1981853002/diff/60001/infra/services/sysmon/a...
File infra/services/sysmon/android_device_metrics.py (right):

https://codereview.chromium.org/1981853002/diff/60001/infra/services/sysmon/a...
infra/services/sysmon/android_device_metrics.py:40: metric_read_status =
ts_mon.StringMetric(
On 2016/05/19 18:36:44, bpastene wrote:
> What are your plans for this metric?

potential debugging, also a quick way to see if a bot is swarming or not

https://codereview.chromium.org/1981853002/diff/60001/infra/services/sysmon/a...
infra/services/sysmon/android_device_metrics.py:47: if not success:
On 2016/05/19 18:36:44, bpastene wrote:
> Nit: Remove success from the return and replace this line with
> 'if not devices:' ? IDK; how do I python?

sure, why not

https://codereview.chromium.org/1981853002/diff/60001/infra/services/sysmon/a...
infra/services/sysmon/android_device_metrics.py:54: cpu_temps =
device.get('temp', [])
On 2016/05/19 18:36:44, bpastene wrote:
> I don't think this is a list as reported by swarming. Looks like a dict to me.
A
> sample test file should pick this up.

you're right

https://codereview.chromium.org/1981853002/diff/60001/infra/services/sysmon/a...
infra/services/sysmon/android_device_metrics.py:57: battery_temp = battery_temp
/ 10.0 if battery_temp else None
On 2016/05/19 18:36:44, bpastene wrote:
> I've been burned repeatedly with some devices reporting millicelsius, some
> reporting decicelsius, and some reporting celsius. Hopefully bot_config
> normalizes all this.

yikes

[ghost stip (do not use), 2016-05-20 21:15:26]

bpastene: whether the metrics fit with what
https://chromium.googlesource.com/chromium/tools/build/+/master/scripts/slave...
send

dsansome/sergeyberezin: sysmon review / OWNERS

[ghost stip (do not use), 2016-05-20 21:15:54]

Description was changed from

==========
Initial implementation of getting ts_mon metrics from android device data.

BUG=608427
==========

to

==========
Send ts_mon metrics from android device data.

BUG=608427
==========

[dsansome, 2016-05-20 21:16:56]

lgtm

[Sergey Berezin, 2016-05-20 21:39:23]

LGTM + a comment. Thanks!

https://codereview.chromium.org/1981853002/diff/100001/infra/services/sysmon/...
File infra/services/sysmon/test/android_device_metric_test.py (right):

https://codereview.chromium.org/1981853002/diff/100001/infra/services/sysmon/...
infra/services/sysmon/test/android_device_metric_test.py:39: if device_id is not
None:
fields must be set everywhere unconditionally, or we'd lose metrics. I'd remove
this condition from the test and always have fields populated.

Also, the way you're testing for absence of metrics doesn't make sense if you
skip the fields - the test will pass even if the metrics are present.

[bpastene, 2016-05-20 21:50:08]

lgtm

[ghost stip (do not use), 2016-05-20 22:44:24]

The CQ bit was checked by stip@chromium.org

[ghost stip (do not use), 2016-05-20 22:44:24]

The patchset sent to the CQ was uploaded after l-g-t-m from
sergeyberezin@chromium.org, dsansome@chromium.org, bpastene@chromium.org
Link to the patchset: https://codereview.chromium.org/1981853002/#ps140001
(title: "Address final comments, fix coverage.")

[commit-bot: I haz the power, 2016-05-20 22:44:36]

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

[commit-bot: I haz the power, 2016-05-20 22:48:44]

Description was changed from

==========
Send ts_mon metrics from android device data.

BUG=608427
==========

to

==========
Send ts_mon metrics from android device data.

BUG=608427

Committed:
https://chromium.googlesource.com/infra/infra/+/a6f6037e0f6bc697b331bf1c0b54e...
==========

[commit-bot: I haz the power, 2016-05-20 22:48:46]

Committed patchset #8 (id:140001) as
https://chromium.googlesource.com/infra/infra/+/a6f6037e0f6bc697b331bf1c0b54e...