gclient: Start reporting metrics.

Bug: 832386
Change-Id: I6d1167802f077bcd67bf004ccc417661d8fff3c7
Reviewed-on: https://chromium-review.googlesource.com/1135903
Reviewed-by: Aaron Gable <agable@chromium.org>
Commit-Queue: Edward Lesmes <ehmaldonado@chromium.org>

gclient.py

@@ -1916,6 +1916,7 @@ class CipdDependency(Dependency):
 
 
 @subcommand.usage('[command] [args ...]')
+@metrics.collector.collect_metrics('gclient recurse')
 def CMDrecurse(parser, args):
   """Operates [command args ...] on all the dependencies.
 
@@ -1960,6 +1961,7 @@ def CMDrecurse(parser, args):
 
 
 @subcommand.usage('[args ...]')
+@metrics.collector.collect_metrics('gclient fetch')
 def CMDfetch(parser, args):
   """Fetches upstream commits for all modules.
 
@@ -2123,6 +2125,7 @@ class Flattener(object):
         self._flatten_dep(d)
 
 
+@metrics.collector.collect_metrics('gclient flatten')
 def CMDflatten(parser, args):
   """Flattens the solutions into a single DEPS file."""
   parser.add_option('--output-deps', help='Path to the output DEPS file')
@@ -2291,6 +2294,7 @@ def _VarsToLines(variables):
   return s
 
 
+@metrics.collector.collect_metrics('gclient grep')
 def CMDgrep(parser, args):
   """Greps through git repos managed by gclient.
 
@@ -2320,6 +2324,7 @@ def CMDgrep(parser, args):
                   'git', 'grep', '--null', '--color=Always'] + args)
 
 
+@metrics.collector.collect_metrics('gclient root')
 def CMDroot(parser, args):
   """Outputs the solution root (or current dir if there isn't one)."""
   (options, args) = parser.parse_args(args)
@@ -2331,6 +2336,7 @@ def CMDroot(parser, args):
 
 
 @subcommand.usage('[url]')
+@metrics.collector.collect_metrics('gclient config')
 def CMDconfig(parser, args):
   """Creates a .gclient file in the current directory.
 
@@ -2416,6 +2422,7 @@ def CMDconfig(parser, args):
   gclient pack > patch.txt
     generate simple patch for configured client and dependences
 """)
+@metrics.collector.collect_metrics('gclient pack')
 def CMDpack(parser, args):
   """Generates a patch which can be applied at the root of the tree.
 
@@ -2440,6 +2447,7 @@ def CMDpack(parser, args):
   return client.RunOnDeps('pack', args)
 
 
+@metrics.collector.collect_metrics('gclient status')
 def CMDstatus(parser, args):
   """Shows modification status for every dependencies."""
   parser.add_option('--deps', dest='deps_os', metavar='OS_LIST',
@@ -2480,6 +2488,7 @@ os_deps, etc.)
   }
 }
 """)
+@metrics.collector.collect_metrics('gclient sync')
 def CMDsync(parser, args):
   """Checkout/update all modules."""
   parser.add_option('-f', '--force', action='store_true',
@@ -2611,6 +2620,7 @@ def CMDsync(parser, args):
 CMDupdate = CMDsync
 
 
+@metrics.collector.collect_metrics('gclient validate')
 def CMDvalidate(parser, args):
   """Validates the .gclient and DEPS syntax."""
   options, args = parser.parse_args(args)
@@ -2624,6 +2634,7 @@ def CMDvalidate(parser, args):
   return rv
 
 
+@metrics.collector.collect_metrics('gclient diff')
 def CMDdiff(parser, args):
   """Displays local diff for every dependencies."""
   parser.add_option('--deps', dest='deps_os', metavar='OS_LIST',
@@ -2639,6 +2650,7 @@ def CMDdiff(parser, args):
   return client.RunOnDeps('diff', args)
 
 
+@metrics.collector.collect_metrics('gclient revert')
 def CMDrevert(parser, args):
   """Reverts all modifications in every dependencies.
 
@@ -2671,6 +2683,7 @@ def CMDrevert(parser, args):
   return client.RunOnDeps('revert', args)
 
 
+@metrics.collector.collect_metrics('gclient runhooks')
 def CMDrunhooks(parser, args):
   """Runs hooks for files that have been modified in the local working copy."""
   parser.add_option('--deps', dest='deps_os', metavar='OS_LIST',
@@ -2690,6 +2703,7 @@ def CMDrunhooks(parser, args):
   return client.RunOnDeps('runhooks', args)
 
 
+@metrics.collector.collect_metrics('gclient revinfo')
 def CMDrevinfo(parser, args):
   """Outputs revision info mapping for the client and its dependencies.
 
@@ -2725,6 +2739,7 @@ def CMDrevinfo(parser, args):
   return 0
 
 
+@metrics.collector.collect_metrics('gclient getdep')
 def CMDgetdep(parser, args):
   """Gets revision information and variable values from a DEPS file."""
   parser.add_option('--var', action='append',
@@ -2765,6 +2780,7 @@ def CMDgetdep(parser, args):
       print(gclient_eval.GetRevision(local_scope, name))
 
 
+@metrics.collector.collect_metrics('gclient setdep')
 def CMDsetdep(parser, args):
   """Modifies dependency revisions and variable values in a DEPS file"""
   parser.add_option('--var', action='append',
@@ -2828,6 +2844,7 @@ def CMDsetdep(parser, args):
     f.write(gclient_eval.RenderDEPSFile(local_scope))
 
 
+@metrics.collector.collect_metrics('gclient verify')
 def CMDverify(parser, args):
   """Verifies the DEPS file deps are only from allowed_hosts."""
   (options, args) = parser.parse_args(args)
@@ -2852,6 +2869,7 @@ def CMDverify(parser, args):
 
 @subcommand.epilog("""For more information on what metrics are we collecting and
 why, please read metrics.README.md or visit https://bit.ly/2ufRS4p""")
+@metrics.collector.collect_metrics('gclient metrics')
 def CMDmetrics(parser, args):
   """Reports, and optionally modifies, the status of metric collection."""
   parser.add_option('--opt-in', action='store_true', dest='enable_metrics',
@@ -2910,9 +2928,21 @@ class OptionParser(optparse.OptionParser):
         '--no-nag-max', default=False, action='store_true',
         help='Ignored for backwards compatibility.')
 
-  def parse_args(self, args=None, values=None):
+  def parse_args(self, args=None, _values=None):
     """Integrates standard options processing."""
-    options, args = optparse.OptionParser.parse_args(self, args, values)
+    # Create an optparse.Values object that will store only the actual passed
+    # options, without the defaults.
+    actual_options = optparse.Values()
+    _, args = optparse.OptionParser.parse_args(self, args, actual_options)
+    # Create an optparse.Values object with the default options.
+    options = optparse.Values(self.get_default_values().__dict__)
+    # Update it with the options passed by the user.
+    options._update_careful(actual_options.__dict__)
+    # Store the options passed by the user in an _actual_options attribute.
+    # We store only the keys, and not the values, since the values can contain
+    # arbitrary information, which might be PII.
+    metrics.collector.add('arguments', actual_options.__dict__.keys())
+
     levels = [logging.ERROR, logging.WARNING, logging.INFO, logging.DEBUG]
     logging.basicConfig(
         level=levels[min(options.verbose, len(levels) - 1)],

metrics.py

@@ -108,12 +108,19 @@ class MetricsCollector(object):
     self._metrics_lock = threading.Lock()
     self._reported_metrics = {}
     self._config = _Config()
+    self._collecting_metrics = False
 
   @property
   def config(self):
     return self._config
 
+  @property
+  def collecting_metrics(self):
+    return self._collecting_metrics
+
   def add(self, name, value):
+    if not self.collecting_metrics:
+      return
     with self._metrics_lock:
       self._reported_metrics[name] = value
 
@@ -187,6 +194,7 @@ class MetricsCollector(object):
       @functools.wraps(func)
       def _inner(*args, **kwargs):
         self._collect_metrics(func, command_name, *args, **kwargs)
+      self._collecting_metrics = True
       return _inner
     return _decorator
 

tests/metrics_test.py

@@ -80,6 +80,7 @@ class MetricsCollectorTest(unittest.TestCase):
     # Assert we collected the right metrics.
     write_call = self.Popen.return_value.stdin.write.call_args
     collected_metrics = json.loads(write_call[0][0])
+    self.assertTrue(self.collector.collecting_metrics)
     self.assertEqual(collected_metrics, expected_metrics)
 
 
@@ -134,6 +135,7 @@ class MetricsCollectorTest(unittest.TestCase):
 
     fun()
 
+    self.assertFalse(self.collector.collecting_metrics)
     # We shouldn't have tried to read the config file.
     self.assertFalse(self.FileRead.called)
     # Nor tried to upload any metrics.
@@ -150,6 +152,7 @@ class MetricsCollectorTest(unittest.TestCase):
 
     fun()
 
+    self.assertFalse(self.collector.collecting_metrics)
     self.assertFalse(self.collector.config.is_googler)
     self.assertIsNone(self.collector.config.opted_in)
     self.assertEqual(self.collector.config.countdown, 0)
@@ -167,6 +170,7 @@ class MetricsCollectorTest(unittest.TestCase):
 
     fun()
 
+    self.assertFalse(self.collector.collecting_metrics)
     self.assertTrue(self.collector.config.is_googler)
     self.assertFalse(self.collector.config.opted_in)
     self.assertEqual(self.collector.config.countdown, 0)
@@ -184,6 +188,7 @@ class MetricsCollectorTest(unittest.TestCase):
 
     fun()
 
+    self.assertFalse(self.collector.collecting_metrics)
     self.assertTrue(self.collector.config.is_googler)
     self.assertFalse(self.collector.config.opted_in)
     # The countdown should've decreased after the invocation.