Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1308173006:
Create daemon to monitor android device temperatures (Closed)
|
Created:
5 years, 3 months ago by bpastene Modified:
5 years, 3 months ago CC:
chromium-reviews, infra-reviews+build_chromium.org, kjellander-cc_chromium.org, stip+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/tools/build.git@master Target Ref:
refs/heads/master Project:
build Visibility:
Public. |
DescriptionCreate daemon to monitor android device temperatures
This cl adds the start-up and shut-down scripts for a new daemon
that will monitor android device temperatures and upload them to
monarch using infra's ts_mon. This daemon is intended to be launced
right before the first test suite is ran and killed right after the
last suite.
BUG=519884
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=296614
Patch Set 1 #
Total comments: 22
Patch Set 2 : respond to comments #
Total comments: 38
Patch Set 3 : respond to stip-nits #
Total comments: 8
Patch Set 4 : Change metric name + nits #
Messages
Total messages: 19 (5 generated)
bpastene@chromium.org changed reviewers: + jbudorick@chromium.org, luqui@chromium.org
ptal when you get a chance
https://codereview.chromium.org/1308173006/diff/1/scripts/slave/recipe_module... File scripts/slave/recipe_modules/chromium_android/resources/shutdown_device_temp_monitor.py (right): https://codereview.chromium.org/1308173006/diff/1/scripts/slave/recipe_module... scripts/slave/recipe_modules/chromium_android/resources/shutdown_device_temp_monitor.py:13: def main(pid_file_path='/tmp/device_monitor_pid'): pidfiles usually end in .pid, rather than _pid. https://codereview.chromium.org/1308173006/diff/1/scripts/slave/recipe_module... scripts/slave/recipe_modules/chromium_android/resources/shutdown_device_temp_monitor.py:21: os.remove(pid_file_path) I think it should clean up its own pidfile with a signal handler, so that if it's killed in a nonstandard way (modulo SIGKILL) it won't leave junk. https://codereview.chromium.org/1308173006/diff/1/scripts/slave/recipe_module... File scripts/slave/recipe_modules/chromium_android/resources/spawn_device_temp_monitor.py (right): https://codereview.chromium.org/1308173006/diff/1/scripts/slave/recipe_module... scripts/slave/recipe_modules/chromium_android/resources/spawn_device_temp_monitor.py:22: _RUN_PY = '/opt/infra-python/run.py' We can't rely on this location. Probably have to take it as an arg or env var. https://codereview.chromium.org/1308173006/diff/1/scripts/slave/recipe_module... scripts/slave/recipe_modules/chromium_android/resources/spawn_device_temp_monitor.py:48: if pid_file_path: This whole block and the kill script have nothing to do with monitoring. Maybe you could roll this as a feature of daemonizer, to keep this script to the point and enable future things in its style. https://codereview.chromium.org/1308173006/diff/1/scripts/slave/recipe_module... scripts/slave/recipe_modules/chromium_android/resources/spawn_device_temp_monitor.py:72: 'grep "%s" /sys/class/thermal/thermal_zone*/type -l' Stylistically I usually see options first: grep -l ... https://codereview.chromium.org/1308173006/diff/1/scripts/slave/recipe_module... scripts/slave/recipe_modules/chromium_android/resources/spawn_device_temp_monitor.py:77: cpu_temp_file = cpu_temp_files.strip()[:-4] + "temp" This is opaque. Can you use re.sub() instead so I know what you're slicing off? https://codereview.chromium.org/1308173006/diff/1/scripts/slave/recipe_module... scripts/slave/recipe_modules/chromium_android/resources/spawn_device_temp_monitor.py:103: upload_cmd_args = upload_cmd_args + cpu_temp_args + bat_temp_args += It took me a while to realize the point of this loop is to build upload_cmd_args. Can you factor it out so it's: upload_cmd_args = [] for device in devices: upload_cmd_args += get_device_args(device) https://codereview.chromium.org/1308173006/diff/1/scripts/slave/recipe_module... File scripts/slave/recipe_modules/chromium_android/tests/spawn_device_temp_monitor_test.py (right): https://codereview.chromium.org/1308173006/diff/1/scripts/slave/recipe_module... scripts/slave/recipe_modules/chromium_android/tests/spawn_device_temp_monitor_test.py:70: return "temperature: 456" else: die horribly https://codereview.chromium.org/1308173006/diff/1/scripts/slave/recipe_module... scripts/slave/recipe_modules/chromium_android/tests/spawn_device_temp_monitor_test.py:82: self.assertEquals(6, len(self.send_ts_mon_call)) Woah that's a lot of asserts, why not just assert that they are equal as lists?
https://codereview.chromium.org/1308173006/diff/1/scripts/slave/recipe_module... File scripts/slave/recipe_modules/chromium_android/resources/spawn_device_temp_monitor.py (right): https://codereview.chromium.org/1308173006/diff/1/scripts/slave/recipe_module... scripts/slave/recipe_modules/chromium_android/resources/spawn_device_temp_monitor.py:48: if pid_file_path: On 2015/09/03 01:10:41, luqui wrote: > This whole block and the kill script have nothing to do with monitoring. Maybe > you could roll this as a feature of daemonizer, to keep this script to the point > and enable future things in its style. I was trying to emulate the style of the one script that uses daemonizer (adb_logcat_monitor), but I agree. It would be cleaner to move all that into daemonizer; I'll look into it.
https://codereview.chromium.org/1308173006/diff/1/scripts/slave/recipe_module... File scripts/slave/recipe_modules/chromium_android/resources/shutdown_device_temp_monitor.py (right): https://codereview.chromium.org/1308173006/diff/1/scripts/slave/recipe_module... scripts/slave/recipe_modules/chromium_android/resources/shutdown_device_temp_monitor.py:13: def main(pid_file_path='/tmp/device_monitor_pid'): I'd use argparse for command-line arguments to a script. https://codereview.chromium.org/1308173006/diff/1/scripts/slave/recipe_module... File scripts/slave/recipe_modules/chromium_android/resources/spawn_device_temp_monitor.py (right): https://codereview.chromium.org/1308173006/diff/1/scripts/slave/recipe_module... scripts/slave/recipe_modules/chromium_android/resources/spawn_device_temp_monitor.py:72: 'grep "%s" /sys/class/thermal/thermal_zone*/type -l' Is this true across all Nexus devices, or just specific models? We've seen variance in this type of thing across different models in the past. https://codereview.chromium.org/1308173006/diff/1/scripts/slave/recipe_module... scripts/slave/recipe_modules/chromium_android/resources/spawn_device_temp_monitor.py:78: cmd = [adb_path, '-s', device, 'shell', Someday I will get infra to use something other than raw, unchecked adb calls. That day is blocked on getting our more sane utilities somewhere that infra can use them, though, so for now, you're stuck with this. https://codereview.chromium.org/1308173006/diff/1/scripts/slave/recipe_module... scripts/slave/recipe_modules/chromium_android/resources/spawn_device_temp_monitor.py:85: # Dump system battery info and grab the temp and as an added bonus, you'd get this for free: https://code.google.com/p/chromium/codesearch#chromium/src/build/android/pyli...
https://codereview.chromium.org/1308173006/diff/1/scripts/slave/recipe_module... File scripts/slave/recipe_modules/chromium_android/resources/spawn_device_temp_monitor.py (right): https://codereview.chromium.org/1308173006/diff/1/scripts/slave/recipe_module... scripts/slave/recipe_modules/chromium_android/resources/spawn_device_temp_monitor.py:22: _RUN_PY = '/opt/infra-python/run.py' On 2015/09/03 01:10:41, luqui wrote: > We can't rely on this location. Probably have to take it as an arg or env var. Per offline discussion with Luke, resolving this would be too much of a hassle at the moment. Adding a todo once it becomes easier/necessary to change. https://codereview.chromium.org/1308173006/diff/1/scripts/slave/recipe_module... scripts/slave/recipe_modules/chromium_android/resources/spawn_device_temp_monitor.py:48: if pid_file_path: On 2015/09/03 17:36:00, bpastene wrote: > On 2015/09/03 01:10:41, luqui wrote: > > This whole block and the kill script have nothing to do with monitoring. > Maybe > > you could roll this as a feature of daemonizer, to keep this script to the > point > > and enable future things in its style. > > I was trying to emulate the style of the one script that uses daemonizer > (adb_logcat_monitor), but I agree. It would be cleaner to move all that into > daemonizer; I'll look into it. Uploaded a cl for the changes to daemonizer here: https://codereview.chromium.org/1328623004/ https://codereview.chromium.org/1308173006/diff/1/scripts/slave/recipe_module... scripts/slave/recipe_modules/chromium_android/resources/spawn_device_temp_monitor.py:72: 'grep "%s" /sys/class/thermal/thermal_zone*/type -l' On 2015/09/03 01:10:41, luqui wrote: > Stylistically I usually see options first: grep -l ... Done. https://codereview.chromium.org/1308173006/diff/1/scripts/slave/recipe_module... scripts/slave/recipe_modules/chromium_android/resources/spawn_device_temp_monitor.py:72: 'grep "%s" /sys/class/thermal/thermal_zone*/type -l' On 2015/09/03 17:37:31, jbudorick wrote: > Is this true across all Nexus devices, or just specific models? > > We've seen variance in this type of thing across different models in the past. I'm not sure. All the devices I've come across on the many waterfalls follow this layout & naming scheme. Is there a better way? If the device in question doesn't successfully report the cpu temp, we'd still get the battery temp (assuming 'dumpsys battery' hasn't changed too), so there'd still be thermal metrics to monitor/alarm on. https://codereview.chromium.org/1308173006/diff/1/scripts/slave/recipe_module... scripts/slave/recipe_modules/chromium_android/resources/spawn_device_temp_monitor.py:77: cpu_temp_file = cpu_temp_files.strip()[:-4] + "temp" On 2015/09/03 01:10:41, luqui wrote: > This is opaque. Can you use re.sub() instead so I know what you're slicing off? Done. https://codereview.chromium.org/1308173006/diff/1/scripts/slave/recipe_module... scripts/slave/recipe_modules/chromium_android/resources/spawn_device_temp_monitor.py:103: upload_cmd_args = upload_cmd_args + cpu_temp_args + bat_temp_args On 2015/09/03 01:10:41, luqui wrote: > += > > It took me a while to realize the point of this loop is to build > upload_cmd_args. Can you factor it out so it's: > > upload_cmd_args = [] > for device in devices: > upload_cmd_args += get_device_args(device) Done. https://codereview.chromium.org/1308173006/diff/1/scripts/slave/recipe_module... File scripts/slave/recipe_modules/chromium_android/tests/spawn_device_temp_monitor_test.py (right): https://codereview.chromium.org/1308173006/diff/1/scripts/slave/recipe_module... scripts/slave/recipe_modules/chromium_android/tests/spawn_device_temp_monitor_test.py:70: return "temperature: 456" On 2015/09/03 01:10:41, luqui wrote: > else: > die horribly Done. https://codereview.chromium.org/1308173006/diff/1/scripts/slave/recipe_module... scripts/slave/recipe_modules/chromium_android/tests/spawn_device_temp_monitor_test.py:82: self.assertEquals(6, len(self.send_ts_mon_call)) On 2015/09/03 01:10:41, luqui wrote: > Woah that's a lot of asserts, why not just assert that they are equal as lists? Done.
lgtm
stip@chromium.org changed reviewers: + stip@chromium.org
hella nits, and a question about PYTHONPATH manipulation other than that lgtm https://chromiumcodereview.appspot.com/1308173006/diff/20001/scripts/slave/re... File scripts/slave/recipe_modules/chromium_android/resources/spawn_device_temp_monitor.py (right): https://chromiumcodereview.appspot.com/1308173006/diff/20001/scripts/slave/re... scripts/slave/recipe_modules/chromium_android/resources/spawn_device_temp_monitor.py:1: #!/usr/bin/env python Add a license directive to the top. see other files. https://chromiumcodereview.appspot.com/1308173006/diff/20001/scripts/slave/re... scripts/slave/recipe_modules/chromium_android/resources/spawn_device_temp_monitor.py:3: """ """Single line description of the module, ending in a period. Multi-line description after a blank line. Block ends with triple-quotes on a separate line. """ https://chromiumcodereview.appspot.com/1308173006/diff/20001/scripts/slave/re... scripts/slave/recipe_modules/chromium_android/resources/spawn_device_temp_monitor.py:18: # Common name of sensor found in nexus devices to measure cpu (core0) temp comments end in a period. https://chromiumcodereview.appspot.com/1308173006/diff/20001/scripts/slave/re... scripts/slave/recipe_modules/chromium_android/resources/spawn_device_temp_monitor.py:22: # out repo slaves instead of a cipd managed package same https://chromiumcodereview.appspot.com/1308173006/diff/20001/scripts/slave/re... scripts/slave/recipe_modules/chromium_android/resources/spawn_device_temp_monitor.py:24: # Location of the infra-python package's run script same https://chromiumcodereview.appspot.com/1308173006/diff/20001/scripts/slave/re... scripts/slave/recipe_modules/chromium_android/resources/spawn_device_temp_monitor.py:26: two spaces for top-level definitions https://chromiumcodereview.appspot.com/1308173006/diff/20001/scripts/slave/re... scripts/slave/recipe_modules/chromium_android/resources/spawn_device_temp_monitor.py:30: space https://chromiumcodereview.appspot.com/1308173006/diff/20001/scripts/slave/re... scripts/slave/recipe_modules/chromium_android/resources/spawn_device_temp_monitor.py:34: # Search for the file that the _CPU_TEMP_SENSOR dumps to and cat it nit: add a . at the end of a comment. https://chromiumcodereview.appspot.com/1308173006/diff/20001/scripts/slave/re... scripts/slave/recipe_modules/chromium_android/resources/spawn_device_temp_monitor.py:49: # Dump system battery info and grab the temp nit: end in period. https://chromiumcodereview.appspot.com/1308173006/diff/20001/scripts/slave/re... scripts/slave/recipe_modules/chromium_android/resources/spawn_device_temp_monitor.py:68: space https://chromiumcodereview.appspot.com/1308173006/diff/20001/scripts/slave/re... scripts/slave/recipe_modules/chromium_android/resources/spawn_device_temp_monitor.py:72: """Polls the devices for their battery and cpu temperatures """One line description ending in a period. Fuller, multiline description after a blank line. The final quotes are on their own line. """ https://chromiumcodereview.appspot.com/1308173006/diff/20001/scripts/slave/re... scripts/slave/recipe_modules/chromium_android/resources/spawn_device_temp_monitor.py:83: def SigtermHandler(_signum, _unused_frame): delete lines 83 - 85 https://chromiumcodereview.appspot.com/1308173006/diff/20001/scripts/slave/re... scripts/slave/recipe_modules/chromium_android/resources/spawn_device_temp_monitor.py:101: except SigtermError: delete lines 100 - 104 https://chromiumcodereview.appspot.com/1308173006/diff/20001/scripts/slave/re... File scripts/slave/recipe_modules/chromium_android/tests/spawn_device_temp_monitor_test.py (right): https://chromiumcodereview.appspot.com/1308173006/diff/20001/scripts/slave/re... scripts/slave/recipe_modules/chromium_android/tests/spawn_device_temp_monitor_test.py:1: #!/usr/bin/env python you should append a LICENSE directive at the top. see other files https://chromiumcodereview.appspot.com/1308173006/diff/20001/scripts/slave/re... scripts/slave/recipe_modules/chromium_android/tests/spawn_device_temp_monitor_test.py:15: sys.path.insert( I wonder if you can use https://chromium.googlesource.com/chromium/tools/build/+/master/scripts/commo... # Import 'common.env' to load our Infra PYTHONPATH sys.path.insert(0, os.path.join( os.path.dirname(os.path.realpath(__file__)), os.pardir)) import common.env maybe not since these are tests https://chromiumcodereview.appspot.com/1308173006/diff/20001/scripts/slave/re... scripts/slave/recipe_modules/chromium_android/tests/spawn_device_temp_monitor_test.py:28: two spaces for top-level import https://chromiumcodereview.appspot.com/1308173006/diff/20001/scripts/slave/re... scripts/slave/recipe_modules/chromium_android/tests/spawn_device_temp_monitor_test.py:30: remove spaces https://chromiumcodereview.appspot.com/1308173006/diff/20001/scripts/slave/re... scripts/slave/recipe_modules/chromium_android/tests/spawn_device_temp_monitor_test.py:32: # Collect calls to 'subprocess.Popen', which calls send_ts_mon_values.py period after comment https://chromiumcodereview.appspot.com/1308173006/diff/20001/scripts/slave/re... scripts/slave/recipe_modules/chromium_android/tests/spawn_device_temp_monitor_test.py:117: two spaces for top-level declarations
https://codereview.chromium.org/1308173006/diff/20001/scripts/slave/recipe_mo... File scripts/slave/recipe_modules/chromium_android/resources/spawn_device_temp_monitor.py (right): https://codereview.chromium.org/1308173006/diff/20001/scripts/slave/recipe_mo... scripts/slave/recipe_modules/chromium_android/resources/spawn_device_temp_monitor.py:1: #!/usr/bin/env python On 2015/09/08 21:46:36, stip wrote: > Add a license directive to the top. see other files. Done. https://codereview.chromium.org/1308173006/diff/20001/scripts/slave/recipe_mo... scripts/slave/recipe_modules/chromium_android/resources/spawn_device_temp_monitor.py:3: """ On 2015/09/08 21:46:36, stip wrote: > """Single line description of the module, ending in a period. > > Multi-line description after a blank line. > Block ends with triple-quotes on a separate line. > """ Done. https://codereview.chromium.org/1308173006/diff/20001/scripts/slave/recipe_mo... scripts/slave/recipe_modules/chromium_android/resources/spawn_device_temp_monitor.py:18: # Common name of sensor found in nexus devices to measure cpu (core0) temp On 2015/09/08 21:46:36, stip wrote: > comments end in a period. Done. https://codereview.chromium.org/1308173006/diff/20001/scripts/slave/recipe_mo... scripts/slave/recipe_modules/chromium_android/resources/spawn_device_temp_monitor.py:22: # out repo slaves instead of a cipd managed package On 2015/09/08 21:46:36, stip wrote: > same Done. https://codereview.chromium.org/1308173006/diff/20001/scripts/slave/recipe_mo... scripts/slave/recipe_modules/chromium_android/resources/spawn_device_temp_monitor.py:24: # Location of the infra-python package's run script On 2015/09/08 21:46:36, stip wrote: > same Done. https://codereview.chromium.org/1308173006/diff/20001/scripts/slave/recipe_mo... scripts/slave/recipe_modules/chromium_android/resources/spawn_device_temp_monitor.py:26: On 2015/09/08 21:46:36, stip wrote: > two spaces for top-level definitions Done. https://codereview.chromium.org/1308173006/diff/20001/scripts/slave/recipe_mo... scripts/slave/recipe_modules/chromium_android/resources/spawn_device_temp_monitor.py:30: On 2015/09/08 21:46:36, stip wrote: > space Done. https://codereview.chromium.org/1308173006/diff/20001/scripts/slave/recipe_mo... scripts/slave/recipe_modules/chromium_android/resources/spawn_device_temp_monitor.py:34: # Search for the file that the _CPU_TEMP_SENSOR dumps to and cat it On 2015/09/08 21:46:36, stip wrote: > nit: add a . at the end of a comment. Done. https://codereview.chromium.org/1308173006/diff/20001/scripts/slave/recipe_mo... scripts/slave/recipe_modules/chromium_android/resources/spawn_device_temp_monitor.py:49: # Dump system battery info and grab the temp On 2015/09/08 21:46:36, stip wrote: > nit: end in period. Done. https://codereview.chromium.org/1308173006/diff/20001/scripts/slave/recipe_mo... scripts/slave/recipe_modules/chromium_android/resources/spawn_device_temp_monitor.py:68: On 2015/09/08 21:46:36, stip wrote: > space Done. https://codereview.chromium.org/1308173006/diff/20001/scripts/slave/recipe_mo... scripts/slave/recipe_modules/chromium_android/resources/spawn_device_temp_monitor.py:72: """Polls the devices for their battery and cpu temperatures On 2015/09/08 21:46:36, stip wrote: > """One line description ending in a period. > > Fuller, multiline description after a blank line. > The final quotes are on their own line. > """ Done. https://codereview.chromium.org/1308173006/diff/20001/scripts/slave/recipe_mo... scripts/slave/recipe_modules/chromium_android/resources/spawn_device_temp_monitor.py:83: def SigtermHandler(_signum, _unused_frame): On 2015/09/08 21:46:36, stip wrote: > delete lines 83 - 85 Done. https://codereview.chromium.org/1308173006/diff/20001/scripts/slave/recipe_mo... scripts/slave/recipe_modules/chromium_android/resources/spawn_device_temp_monitor.py:101: except SigtermError: On 2015/09/08 21:46:36, stip wrote: > delete lines 100 - 104 Done. https://codereview.chromium.org/1308173006/diff/20001/scripts/slave/recipe_mo... File scripts/slave/recipe_modules/chromium_android/tests/spawn_device_temp_monitor_test.py (right): https://codereview.chromium.org/1308173006/diff/20001/scripts/slave/recipe_mo... scripts/slave/recipe_modules/chromium_android/tests/spawn_device_temp_monitor_test.py:1: #!/usr/bin/env python On 2015/09/08 21:46:36, stip wrote: > you should append a LICENSE directive at the top. see other files Done. https://codereview.chromium.org/1308173006/diff/20001/scripts/slave/recipe_mo... scripts/slave/recipe_modules/chromium_android/tests/spawn_device_temp_monitor_test.py:15: sys.path.insert( On 2015/09/08 21:46:37, stip wrote: > I wonder if you can use > https://chromium.googlesource.com/chromium/tools/build/+/master/scripts/commo... > > # Import 'common.env' to load our Infra PYTHONPATH > sys.path.insert(0, os.path.join( > os.path.dirname(os.path.realpath(__file__)), os.pardir)) > import common.env > > maybe not since these are tests It couldn't find unittests/test_env https://codereview.chromium.org/1308173006/diff/20001/scripts/slave/recipe_mo... scripts/slave/recipe_modules/chromium_android/tests/spawn_device_temp_monitor_test.py:28: On 2015/09/08 21:46:37, stip wrote: > two spaces for top-level import Done. https://codereview.chromium.org/1308173006/diff/20001/scripts/slave/recipe_mo... scripts/slave/recipe_modules/chromium_android/tests/spawn_device_temp_monitor_test.py:30: On 2015/09/08 21:46:37, stip wrote: > remove spaces Done. https://codereview.chromium.org/1308173006/diff/20001/scripts/slave/recipe_mo... scripts/slave/recipe_modules/chromium_android/tests/spawn_device_temp_monitor_test.py:32: # Collect calls to 'subprocess.Popen', which calls send_ts_mon_values.py On 2015/09/08 21:46:36, stip wrote: > period after comment Done. https://codereview.chromium.org/1308173006/diff/20001/scripts/slave/recipe_mo... scripts/slave/recipe_modules/chromium_android/tests/spawn_device_temp_monitor_test.py:117: On 2015/09/08 21:46:37, stip wrote: > two spaces for top-level declarations Done.
bpastene@chromium.org changed reviewers: + sergeyberezin@chromium.org
Sergey, when you get a chance, can you take a quick look at the metrics being sent (right type of metric/how frequently/metric naming scheme)?
Major comment on the metric naming - let's sort it out before committing. It shouldn't change the code much though, which looks good to me. https://codereview.chromium.org/1308173006/diff/40001/scripts/slave/recipe_mo... File scripts/slave/recipe_modules/chromium_android/resources/spawn_device_temp_monitor.py (right): https://codereview.chromium.org/1308173006/diff/40001/scripts/slave/recipe_mo... scripts/slave/recipe_modules/chromium_android/resources/spawn_device_temp_monitor.py:9: temperatures every 30 seconds via adb and uploads them to monarch Please don't mention internal tools in external CLs. Just ts_mon or timeseries monitoring. https://codereview.chromium.org/1308173006/diff/40001/scripts/slave/recipe_mo... scripts/slave/recipe_modules/chromium_android/resources/spawn_device_temp_monitor.py:9: temperatures every 30 seconds via adb and uploads them to monarch nit: our retention policy starts with once a minute, so every 30 sec will generate (soft) errors on the backend API. Once a minute would be perfect. https://codereview.chromium.org/1308173006/diff/40001/scripts/slave/recipe_mo... scripts/slave/recipe_modules/chromium_android/resources/spawn_device_temp_monitor.py:27: # out repo on slaves instead of a cipd managed package. nit: don't rely on this happening; we might do away with deployment through checkout altogether. But at the moment, the future is uncertain. https://codereview.chromium.org/1308173006/diff/40001/scripts/slave/recipe_mo... scripts/slave/recipe_modules/chromium_android/resources/spawn_device_temp_monitor.py:80: /chrome/infra/${metric_prefix}/${device_serial}/(battery|cpu)_temp This is not the best metric name - you'll have hundreds of metrics that you won't be able to aggregate over. Instead, use a single metric name for temperature across all devices, e.g. /chrome/infra/dev/android/temperature Or, if it makes sense, you may split the device and battery temperature into two metrics: /chrome/infra/dev/battery/temperature Individual device IDs and possibly the slave name they are attached to, should be specified as metric fields. For a single metric, the type of temperature (device or battery) should also be a field. Let's talk offline about what's the best format; it depends on how you'll be processing it on the backend.
https://codereview.chromium.org/1308173006/diff/40001/scripts/slave/recipe_mo... File scripts/slave/recipe_modules/chromium_android/resources/spawn_device_temp_monitor.py (right): https://codereview.chromium.org/1308173006/diff/40001/scripts/slave/recipe_mo... scripts/slave/recipe_modules/chromium_android/resources/spawn_device_temp_monitor.py:9: temperatures every 30 seconds via adb and uploads them to monarch On 2015/09/09 16:47:13, Sergey Berezin wrote: > Please don't mention internal tools in external CLs. Just ts_mon or timeseries > monitoring. Done. https://codereview.chromium.org/1308173006/diff/40001/scripts/slave/recipe_mo... scripts/slave/recipe_modules/chromium_android/resources/spawn_device_temp_monitor.py:9: temperatures every 30 seconds via adb and uploads them to monarch On 2015/09/09 16:47:13, Sergey Berezin wrote: > nit: our retention policy starts with once a minute, so every 30 sec will > generate (soft) errors on the backend API. Once a minute would be perfect. Done. https://codereview.chromium.org/1308173006/diff/40001/scripts/slave/recipe_mo... scripts/slave/recipe_modules/chromium_android/resources/spawn_device_temp_monitor.py:27: # out repo on slaves instead of a cipd managed package. On 2015/09/09 16:47:14, Sergey Berezin wrote: > nit: don't rely on this happening; we might do away with deployment through > checkout altogether. But at the moment, the future is uncertain. Done. https://codereview.chromium.org/1308173006/diff/40001/scripts/slave/recipe_mo... scripts/slave/recipe_modules/chromium_android/resources/spawn_device_temp_monitor.py:80: /chrome/infra/${metric_prefix}/${device_serial}/(battery|cpu)_temp On 2015/09/09 16:47:13, Sergey Berezin wrote: > This is not the best metric name - you'll have hundreds of metrics that you > won't be able to aggregate over. Instead, use a single metric name for > temperature across all devices, e.g. > > /chrome/infra/dev/android/temperature > > Or, if it makes sense, you may split the device and battery temperature into two > metrics: > > /chrome/infra/dev/battery/temperature > > Individual device IDs and possibly the slave name they are attached to, should > be specified as metric fields. For a single metric, the type of temperature > (device or battery) should also be a field. > > Let's talk offline about what's the best format; it depends on how you'll be > processing it on the backend. Done. As per our discussion offline, metrics will be both /chrome/infra/dev/cpu/temperature and /chrome/infra/dev/battery/temperature with metric fields to specify devices and slaves.
LGTM. Thanks!
The CQ bit was checked by bpastene@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from luqui@chromium.org, stip@chromium.org Link to the patchset: https://codereview.chromium.org/1308173006/#ps60001 (title: "Change metric name + nits")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1308173006/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1308173006/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as http://src.chromium.org/viewvc/chrome?view=rev&revision=296614 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
