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

infra/services/sysmon/__main__.py

Index: infra/services/sysmon/__main__.py
diff --git a/infra/services/sysmon/__main__.py b/infra/services/sysmon/__main__.py
index 806104407ed2c07342999648251b3482d13aa1e5..a7cc23eb394dba06343b366aefa5c7d878b09346 100644
--- a/infra/services/sysmon/__main__.py
+++ b/infra/services/sysmon/__main__.py
@@ -20,6 +20,28 @@ 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)
 
@@ -39,6 +61,7 @@ class SysMon(outer_loop.Application):
     )
 
   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()
@@ -46,10 +69,17 @@ class SysMon(outer_loop.Application):
       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