adding os metrics (os name and version) being collected every hour

infra/services/sysmon/__main__.py

 #!/usr/bin/env python
 # Copyright (c) 2014 The Chromium Authors. All rights reserved.
 # Use of this source code is governed by a BSD-style license that can be
 # found in the LICENSE file.
 
 """Send system monitoring data to the timeseries monitoring API."""
 
 import random
 import time
 
 import psutil
 
 from infra.libs.service_utils import outer_loop
 from infra.services.sysmon import android_device_metrics
 from infra.services.sysmon import cipd_metrics
 from infra.services.sysmon import puppet_metrics
 from infra.services.sysmon import root_setup
 from infra.services.sysmon import system_metrics
 from infra_libs import ts_mon
 
 
 class SysMon(outer_loop.Application):
+  def __init__(self):
+    # make sure we call our super's init
+    super(SysMon, self).__init__()
+
+    # SysMon.task is called every minute

[dsansome, 2016/06/29 06:21:29]

Wrap these comments to 80 characters?

[chrishall, 2016/07/01 04:27:16]

Done.

+    # we want to collect some metrics (e.g. os_info)
+    # only once per hour, so here we count the minutes
+    # within the hour
+    self._call_count = -1

[dsansome, 2016/06/29 06:21:29]

I'd start at 0 and move the += 1 into the finally block in task()

[chrishall, 2016/07/01 04:27:17]

Done.

+
+  # should be called everytime self.task is called
+  def count_call(self):
+    # mark that we were called
+    self._call_count += 1
+
+    # roll over at 60, only care about minutes within hour
+    self._call_count %= 60

[dsansome, 2016/06/29 06:21:29]

If this is named call_count I think it makes sense to just have it be a raw
count, and do the % 60 in is_hour instead

[chrishall, 2016/07/01 04:27:17]

Done.

+
+  # check if this call is on the hour
+  def is_hour(self):
+    return self._call_count == 0

[ddoman, 2016/06/29 07:57:54]

Doesn't self.opts.interval determine how often task() gets called?

[dsansome, 2016/06/30 04:25:46]

You should add a comment here explaining why we're only doing this once an hour.

[chrishall, 2016/07/01 04:27:17]

Done.

+
   def add_argparse_options(self, parser):
     super(SysMon, self).add_argparse_options(parser)
 
     parser.add_argument(
         '--interval',
         default=10, type=int,
         help='time (in seconds) between sampling system metrics')
     parser.add_argument(
         '--root-setup',
         action='store_true',
         help='if this is set sysmon will run once to initialise configs in '
              '/etc and then exit immediately.  Used on GCE bots to bootstrap '
              'sysmon')
 
     parser.set_defaults(
         ts_mon_flush='manual',
     )
 
   def task(self):
+    self.count_call()

[dsansome, 2016/06/29 06:21:29]

I'm not sure hiding the implementations of these inside methods is any more
readable than just having self._call_count += 1 here, and (self._call_count %
60) == 0 below.

[chrishall, 2016/07/01 04:27:17]

I disagree, I wanted to give them names rather than have 'random math' in the
try/finally below.

     try:
       system_metrics.get_uptime()
       system_metrics.get_cpu_info()
       system_metrics.get_disk_info()
       system_metrics.get_mem_info()
       system_metrics.get_net_info()
       system_metrics.get_proc_info()
+      if self.is_hour():
+        # collect once per hour
+        system_metrics.get_os_info()
+      else:
+        # clear on all other minutes
+        system_metrics.clear_os_info()
       puppet_metrics.get_puppet_summary()
       cipd_metrics.get_cipd_summary()
       android_device_metrics.get_device_statuses()
       system_metrics.get_unix_time() # must be the last in the list
+
     finally:
       ts_mon.flush()
     return True
 
   def sleep_timeout(self):
     return self.opts.interval
 
   def main(self, opts):
     if opts.root_setup:
       return root_setup.root_setup()
 
     # This returns a 0 value the first time it's called.  Call it now and
     # discard the return value.
     psutil.cpu_times_percent()
 
     # Wait a random amount of time before starting the loop in case sysmon is
     # started at exactly the same time on all machines.
     time.sleep(random.uniform(0, opts.interval))
 
     return super(SysMon, self).main(opts)
 
 
 if __name__ == '__main__':
   SysMon().run()