diff --git a/gclient.py b/gclient.py index 9e58342f74..e14ee41a83 100755 --- a/gclient.py +++ b/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)], diff --git a/metrics.py b/metrics.py index ccac340a1c..714ebadac4 100644 --- a/metrics.py +++ b/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 diff --git a/tests/metrics_test.py b/tests/metrics_test.py index 26cd0e0b96..a4955162e5 100644 --- a/tests/metrics_test.py +++ b/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.