metrics: Add a mechanism to notify users when we want to collect additional metrics.

When we change the version number in metrics_utils:

If the user is not a Googler, or has opted out explicitly, nothing happens
and we still don't collect metrics.

If we're collecting metrics from the user, we stop collecting metrics
and display a notice telling them what has changed.
That notice will be displayed ten times, after which we will
resume collecting metrics. A notice telling them we're collecting metrics
will still be displayed.

Bug: None
Change-Id: If1cc12b2fc06f0d6237714c4f182367b1afdf9fb
Reviewed-on: https://chromium-review.googlesource.com/c/1285395
Commit-Queue: Edward Lesmes <ehmaldonado@chromium.org>
Reviewed-by: Andrii Shyshkalov <tandrii@chromium.org>

metrics.README.md

@@ -48,6 +48,13 @@ The metrics we're collecting are:
   - Are you setting `use\_relative\_paths=True`?
   - Are you using `recursedeps`?
 
+## Why am I seeing this message *again*?
+
+We might want to collect additional metrics, and if so we will ask you for
+permission again.
+
+Opting in or out explicitly will stop the messages from being displayed.
+
 # How can I check if metrics are being collected?
 
 You can run `gclient metrics` and it will report if you have opted in, out, or

metrics.py

@@ -69,6 +69,7 @@ class _Config(object):
     # safe values otherwise.
     self._config.setdefault('countdown', DEFAULT_COUNTDOWN)
     self._config.setdefault('opt-in', None)
+    self._config.setdefault('version', metrics_utils.CURRENT_VERSION)
 
     if config != self._config:
       print(INVALID_CONFIG_WARNING, file=sys.stderr)
@@ -83,6 +84,11 @@ class _Config(object):
       print(PERMISSION_DENIED_WARNING % e, file=sys.stderr)
       self._config['opt-in'] = False
 
+  @property
+  def version(self):
+    self._ensure_initialized()
+    return self._config['version']
+
   @property
   def is_googler(self):
     self._ensure_initialized()
@@ -97,6 +103,7 @@ class _Config(object):
   def opted_in(self, value):
     self._ensure_initialized()
     self._config['opt-in'] = value
+    self._config['version'] = metrics_utils.CURRENT_VERSION
     self._write_config()
 
   @property
@@ -104,13 +111,34 @@ class _Config(object):
     self._ensure_initialized()
     return self._config['countdown']
 
+  @property
+  def should_collect_metrics(self):
+    # Don't collect the metrics unless the user is a googler, the user has opted
+    # in, or the countdown has expired.
+    if not self.is_googler:
+      return False
+    if self.opted_in is False:
+      return False
+    if self.opted_in is None and self.countdown > 0:
+      return False
+    return True
+
   def decrease_countdown(self):
     self._ensure_initialized()
     if self.countdown == 0:
       return
     self._config['countdown'] -= 1
+    if self.countdown == 0:
+      self._config['version'] = metrics_utils.CURRENT_VERSION
     self._write_config()
 
+  def reset_config(self):
+    # Only reset countdown if we're already collecting metrics.
+    if self.should_collect_metrics:
+      self._ensure_initialized()
+      self._config['countdown'] = DEFAULT_COUNTDOWN
+      self._config['opt-in'] = None
+
 
 class MetricsCollector(object):
   def __init__(self):
@@ -198,10 +226,7 @@ class MetricsCollector(object):
       # file.
       if DISABLE_METRICS_COLLECTION:
         return func
-      # Don't collect the metrics unless the user is a googler, the user has
-      # opted in, or the countdown has expired.
-      if (not self.config.is_googler or self.config.opted_in == False
-          or (self.config.opted_in is None and self.config.countdown > 0)):
+      if not self.config.should_collect_metrics:
         return func
       # Otherwise, collect the metrics.
       # Needed to preserve the __name__ and __doc__ attributes of func.
@@ -236,6 +261,13 @@ class MetricsCollector(object):
       elif not isinstance(exception[1], SystemExit):
         traceback.print_exception(*exception)
 
+    # Check if the version has changed
+    if (not DISABLE_METRICS_COLLECTION and self.config.is_googler
+        and self.config.opted_in is not False
+        and self.config.version != metrics_utils.CURRENT_VERSION):
+      metrics_utils.print_version_change(self.config.version)
+      self.config.reset_config()
+
     # Print the notice
     if (not DISABLE_METRICS_COLLECTION and self.config.is_googler
         and self.config.opted_in is None):

metrics_utils.py

@@ -12,8 +12,16 @@ import sys
 from third_party import colorama
 
 
+# Current version of metrics recording.
+# When we add new metrics, the version number will be increased, we display the
+# user what has changed, and ask the user to agree again.
+CURRENT_VERSION = 0
+
 APP_URL = 'https://cit-cli-metrics.appspot.com'
 
+EMPTY_LINE = (
+  '*                                                   *'
+)
 NOTICE_COUNTDOWN_HEADER = (
   '*****************************************************\n'
   '*  METRICS COLLECTION WILL START IN %2d EXECUTIONS   *'
@@ -22,14 +30,23 @@ NOTICE_COLLECTION_HEADER = (
   '*****************************************************\n'
   '*      METRICS COLLECTION IS TAKING PLACE           *'
 )
+NOTICE_VERSION_CHANGE_HEADER = (
+  '*****************************************************\n'
+  '*       WE ARE COLLECTING ADDITIONAL METRICS        *'
+)
 NOTICE_FOOTER = (
-  '*                                                   *\n'
   '* For more information, and for how to disable this *\n'
   '* message, please see metrics.README.md in your     *\n'
   '* depot_tools checkout.                             *\n'
   '*****************************************************\n'
 )
 
+CHANGE_NOTICE = {
+  # No changes for version 0
+  0: '',
+}
+
+
 KNOWN_PROJECT_URLS = {
   'https://chrome-internal.googlesource.com/chrome/ios_internal',
   'https://chrome-internal.googlesource.com/infra/infra_internal',
@@ -105,9 +122,21 @@ def get_repo_timestamp(path_to_repo):
 def print_notice(countdown):
   """Print a notice to let the user know the status of metrics collection."""
   colorama.init()
-  print(colorama.Fore.RED + '\033[1m', file=sys.stderr)
+  print(colorama.Fore.RED + '\033[1m', file=sys.stderr, end='')
   if countdown:
     print(NOTICE_COUNTDOWN_HEADER % countdown, file=sys.stderr)
   else:
     print(NOTICE_COLLECTION_HEADER, file=sys.stderr)
+  print(EMPTY_LINE, file=sys.stderr)
   print(NOTICE_FOOTER + colorama.Style.RESET_ALL, file=sys.stderr)
+
+
+def print_version_change(config_version):
+  """Print a notice to let the user know we are collecting more metrics."""
+  colorama.init()
+  print(colorama.Fore.RED + '\033[1m', file=sys.stderr, end='')
+  print(NOTICE_VERSION_CHANGE_HEADER, file=sys.stderr)
+  print(EMPTY_LINE, file=sys.stderr)
+  for version in range(config_version + 1, CURRENT_VERSION + 1):
+    print(CHANGE_NOTICE[version], file=sys.stderr)
+    print(EMPTY_LINE, file=sys.stderr)

tests/metrics_test.py

@@ -34,6 +34,7 @@ class MetricsCollectorTest(unittest.TestCase):
     # Keep track of the URL requests, file reads/writes and subprocess spawned.
     self.urllib2 = mock.Mock()
     self.print_notice = mock.Mock()
+    self.print_version_change = mock.Mock()
     self.Popen = mock.Mock()
     self.FileWrite = mock.Mock()
     self.FileRead = mock.Mock()
@@ -43,6 +44,9 @@ class MetricsCollectorTest(unittest.TestCase):
     mock.patch('metrics.gclient_utils.FileWrite', self.FileWrite).start()
     mock.patch('metrics.gclient_utils.FileRead', self.FileRead).start()
     mock.patch('metrics.metrics_utils.print_notice', self.print_notice).start()
+    mock.patch(
+        'metrics.metrics_utils.print_version_change',
+        self.print_version_change).start()
 
     # Patch the methods used to get the system information, so we have a known
     # environment.
@@ -90,7 +94,8 @@ class MetricsCollectorTest(unittest.TestCase):
     self.assertEqual(self.collector.config.countdown, 10)
 
     self.assert_writes_file(
-        self.config_file, {'is-googler': True, 'countdown': 10, 'opt-in': None})
+        self.config_file,
+        {'is-googler': True, 'countdown': 10, 'opt-in': None, 'version': 0})
 
   def test_writes_config_if_not_exists_non_googler(self):
     self.FileRead.side_effect = [IOError(2, "No such file or directory")]
@@ -104,7 +109,7 @@ class MetricsCollectorTest(unittest.TestCase):
 
     self.assert_writes_file(
         self.config_file,
-        {'is-googler': False, 'countdown': 10, 'opt-in': None})
+        {'is-googler': False, 'countdown': 10, 'opt-in': None, 'version': 0})
 
   def test_disables_metrics_if_cant_write_config(self):
     self.FileRead.side_effect = [IOError(2, 'No such file or directory')]
@@ -132,7 +137,7 @@ class MetricsCollectorTest(unittest.TestCase):
   def test_collects_system_information(self):
     """Tests that we collect information about the runtime environment."""
     self.FileRead.side_effect = [
-        '{"is-googler": true, "countdown": 0, "opt-in": null}'
+        '{"is-googler": true, "countdown": 0, "opt-in": null, "version": 0}'
     ]
     @self.collector.collect_metrics('fun')
     def fun():
@@ -144,7 +149,7 @@ class MetricsCollectorTest(unittest.TestCase):
   def test_collects_added_metrics(self):
     """Tests that we can collect custom metrics."""
     self.FileRead.side_effect = [
-        '{"is-googler": true, "countdown": 0, "opt-in": null}'
+        '{"is-googler": true, "countdown": 0, "opt-in": null, "version": 0}'
     ]
     @self.collector.collect_metrics('fun')
     def fun():
@@ -156,7 +161,7 @@ class MetricsCollectorTest(unittest.TestCase):
   def test_collects_metrics_when_opted_in(self):
     """Tests that metrics are collected when the user opts-in."""
     self.FileRead.side_effect = [
-        '{"is-googler": true, "countdown": 1234, "opt-in": true}'
+        '{"is-googler": true, "countdown": 1234, "opt-in": true, "version": 0}'
     ]
     @self.collector.collect_metrics('fun')
     def fun():
@@ -183,7 +188,7 @@ class MetricsCollectorTest(unittest.TestCase):
   def test_metrics_collection_disabled_not_googler(self):
     """Tests that metrics collection is disabled for non googlers."""
     self.FileRead.side_effect = [
-        '{"is-googler": false, "countdown": 0, "opt-in": null}'
+        '{"is-googler": false, "countdown": 0, "opt-in": null, "version": 0}'
     ]
     @self.collector.collect_metrics('fun')
     def fun():
@@ -201,7 +206,7 @@ class MetricsCollectorTest(unittest.TestCase):
   def test_metrics_collection_disabled_opted_out(self):
     """Tests that metrics collection is disabled if the user opts out."""
     self.FileRead.side_effect = [
-        '{"is-googler": true, "countdown": 0, "opt-in": false}'
+        '{"is-googler": true, "countdown": 0, "opt-in": false, "version": 0}'
     ]
     @self.collector.collect_metrics('fun')
     def fun():
@@ -219,7 +224,7 @@ class MetricsCollectorTest(unittest.TestCase):
   def test_metrics_collection_disabled_non_zero_countdown(self):
     """Tests that metrics collection is disabled until the countdown expires."""
     self.FileRead.side_effect = [
-        '{"is-googler": true, "countdown": 1, "opt-in": null}'
+        '{"is-googler": true, "countdown": 1, "opt-in": null, "version": 0}'
     ]
     @self.collector.collect_metrics('fun')
     def fun():
@@ -237,7 +242,7 @@ class MetricsCollectorTest(unittest.TestCase):
   def test_handles_exceptions(self):
     """Tests that exception are caught and we exit with an appropriate code."""
     self.FileRead.side_effect = [
-        '{"is-googler": true, "countdown": 0, "opt-in": true}'
+        '{"is-googler": true, "countdown": 0, "opt-in": true, "version": 0}'
     ]
     @self.collector.collect_metrics('fun')
     def fun():
@@ -252,7 +257,7 @@ class MetricsCollectorTest(unittest.TestCase):
   def test_handles_system_exit(self):
     """Tests that the sys.exit code is respected and metrics are collected."""
     self.FileRead.side_effect = [
-        '{"is-googler": true, "countdown": 0, "opt-in": true}'
+        '{"is-googler": true, "countdown": 0, "opt-in": true, "version": 0}'
     ]
     @self.collector.collect_metrics('fun')
     def fun():
@@ -268,7 +273,7 @@ class MetricsCollectorTest(unittest.TestCase):
   def test_handles_system_exit_non_zero(self):
     """Tests that the sys.exit code is respected and metrics are collected."""
     self.FileRead.side_effect = [
-        '{"is-googler": true, "countdown": 0, "opt-in": true}'
+        '{"is-googler": true, "countdown": 0, "opt-in": true, "version": 0}'
     ]
     @self.collector.collect_metrics('fun')
     def fun():
@@ -284,7 +289,7 @@ class MetricsCollectorTest(unittest.TestCase):
   def test_prints_notice_non_zero_countdown(self):
     """Tests that a notice is printed while the countdown is non-zero."""
     self.FileRead.side_effect = [
-        '{"is-googler": true, "countdown": 1234, "opt-in": null}'
+        '{"is-googler": true, "countdown": 1234, "opt-in": null, "version": 0}'
     ]
     with self.assertRaises(SystemExit) as cm:
       with self.collector.print_notice_and_exit():
@@ -295,7 +300,7 @@ class MetricsCollectorTest(unittest.TestCase):
   def test_prints_notice_zero_countdown(self):
     """Tests that a notice is printed when the countdown reaches 0."""
     self.FileRead.side_effect = [
-        '{"is-googler": true, "countdown": 0, "opt-in": null}'
+        '{"is-googler": true, "countdown": 0, "opt-in": null, "version": 0}'
     ]
     with self.assertRaises(SystemExit) as cm:
       with self.collector.print_notice_and_exit():
@@ -306,7 +311,7 @@ class MetricsCollectorTest(unittest.TestCase):
   def test_doesnt_print_notice_opted_in(self):
     """Tests that a notice is not printed when the user opts-in."""
     self.FileRead.side_effect = [
-        '{"is-googler": true, "countdown": 0, "opt-in": true}'
+        '{"is-googler": true, "countdown": 0, "opt-in": true, "version": 0}'
     ]
     with self.assertRaises(SystemExit) as cm:
       with self.collector.print_notice_and_exit():
@@ -317,7 +322,7 @@ class MetricsCollectorTest(unittest.TestCase):
   def test_doesnt_print_notice_opted_out(self):
     """Tests that a notice is not printed when the user opts-out."""
     self.FileRead.side_effect = [
-        '{"is-googler": true, "countdown": 0, "opt-in": false}'
+        '{"is-googler": true, "countdown": 0, "opt-in": false, "version": 0}'
     ]
     with self.assertRaises(SystemExit) as cm:
       with self.collector.print_notice_and_exit():
@@ -338,7 +343,7 @@ class MetricsCollectorTest(unittest.TestCase):
   def test_print_notice_handles_exceptions(self):
     """Tests that exception are caught and we exit with an appropriate code."""
     self.FileRead.side_effect = [
-        '{"is-googler": true, "countdown": 0, "opt-in": null}'
+        '{"is-googler": true, "countdown": 0, "opt-in": null, "version": 0}'
     ]
     # print_notice should catch the exception, print it and invoke sys.exit()
     with self.assertRaises(SystemExit) as cm:
@@ -350,7 +355,7 @@ class MetricsCollectorTest(unittest.TestCase):
   def test_print_notice_handles_system_exit(self):
     """Tests that the sys.exit code is respected and a notice is displayed."""
     self.FileRead.side_effect = [
-        '{"is-googler": true, "countdown": 0, "opt-in": null}'
+        '{"is-googler": true, "countdown": 0, "opt-in": null, "version": 0}'
     ]
     # print_notice should catch the exception, print it and invoke sys.exit()
     with self.assertRaises(SystemExit) as cm:
@@ -362,7 +367,7 @@ class MetricsCollectorTest(unittest.TestCase):
   def test_print_notice_handles_system_exit_non_zero(self):
     """Tests that the sys.exit code is respected and a notice is displayed."""
     self.FileRead.side_effect = [
-        '{"is-googler": true, "countdown": 0, "opt-in": null}'
+        '{"is-googler": true, "countdown": 0, "opt-in": null, "version": 0}'
     ]
     # When an exception is raised, we should catch it, update exit-code,
     # collect metrics, and re-raise it.
@@ -375,7 +380,7 @@ class MetricsCollectorTest(unittest.TestCase):
   def test_counts_down(self):
     """Tests that the countdown works correctly."""
     self.FileRead.side_effect = [
-        '{"is-googler": true, "countdown": 10, "opt-in": null}'
+        '{"is-googler": true, "countdown": 10, "opt-in": null, "version": 0}'
     ]
 
     # We define multiple functions to ensure it has no impact on countdown.
@@ -403,13 +408,14 @@ class MetricsCollectorTest(unittest.TestCase):
     self.print_notice.assert_called_once_with(10)
 
     self.assert_writes_file(
-        self.config_file, {'is-googler': True, 'countdown': 9, 'opt-in': None})
+        self.config_file,
+        {'is-googler': True, 'countdown': 9, 'opt-in': None, 'version': 0})
 
   def test_nested_functions(self):
     """Tests that a function can call another function for which metrics are
     collected."""
     self.FileRead.side_effect = [
-        '{"is-googler": true, "countdown": 0, "opt-in": true}'
+        '{"is-googler": true, "countdown": 0, "opt-in": true, "version": 0}'
     ]
 
     @self.collector.collect_metrics('barn')
@@ -426,6 +432,148 @@ class MetricsCollectorTest(unittest.TestCase):
     # Assert that we collected metrics for fun, but not for barn.
     self.assert_collects_metrics({'fun-metric': 1001})
 
+  @mock.patch('metrics.metrics_utils.CURRENT_VERSION', 5)
+  def test_version_change_from_hasnt_decided(self):
+    # The user has not decided yet, and the countdown hasn't reached 0, so we're
+    # not collecting metrics.
+    self.FileRead.side_effect = [
+        '{"is-googler": true, "countdown": 9, "opt-in": null, "version": 0}'
+    ]
+    with self.assertRaises(SystemExit) as cm:
+      with self.collector.print_notice_and_exit():
+        self.collector.add('foo-metric', 1)
+
+    self.assertEqual(cm.exception.code, 0)
+    # We display the notice informing the user of the changes.
+    self.print_version_change.assert_called_once_with(0)
+    # But the countdown is not reset.
+    self.assert_writes_file(
+        self.config_file,
+        {'is-googler': True, 'countdown': 8, 'opt-in': None, 'version': 0})
+    # And no metrics are uploaded.
+    self.assertFalse(self.Popen.called)
+
+  @mock.patch('metrics.metrics_utils.CURRENT_VERSION', 5)
+  def test_version_change_from_opted_in_by_default(self):
+    # The user has not decided yet, but the countdown has reached 0, and we're
+    # collecting metrics.
+    self.FileRead.side_effect = [
+        '{"is-googler": true, "countdown": 0, "opt-in": null, "version": 0}'
+    ]
+    with self.assertRaises(SystemExit) as cm:
+      with self.collector.print_notice_and_exit():
+        self.collector.add('foo-metric', 1)
+
+    self.assertEqual(cm.exception.code, 0)
+    # We display the notice informing the user of the changes.
+    self.print_version_change.assert_called_once_with(0)
+    # We reset the countdown.
+    self.assert_writes_file(
+        self.config_file,
+        {'is-googler': True, 'countdown': 9, 'opt-in': None, 'version': 0})
+    # No metrics are uploaded.
+    self.assertFalse(self.Popen.called)
+
+  @mock.patch('metrics.metrics_utils.CURRENT_VERSION', 5)
+  def test_version_change_from_opted_in(self):
+    # The user has opted in, and we're collecting metrics.
+    self.FileRead.side_effect = [
+        '{"is-googler": true, "countdown": 0, "opt-in": true, "version": 0}'
+    ]
+    with self.assertRaises(SystemExit) as cm:
+      with self.collector.print_notice_and_exit():
+        self.collector.add('foo-metric', 1)
+
+    self.assertEqual(cm.exception.code, 0)
+    # We display the notice informing the user of the changes.
+    self.print_version_change.assert_called_once_with(0)
+    # We reset the countdown.
+    self.assert_writes_file(
+        self.config_file,
+        {'is-googler': True, 'countdown': 9, 'opt-in': None, 'version': 0})
+    # No metrics are uploaded.
+    self.assertFalse(self.Popen.called)
+
+  @mock.patch('metrics.metrics_utils.CURRENT_VERSION', 5)
+  def test_version_change_from_opted_out(self):
+    # The user has opted out and we're not collecting metrics.
+    self.FileRead.side_effect = [
+        '{"is-googler": true, "countdown": 0, "opt-in": false, "version": 0}'
+    ]
+    with self.assertRaises(SystemExit) as cm:
+      with self.collector.print_notice_and_exit():
+        self.collector.add('foo-metric', 1)
+
+    self.assertEqual(cm.exception.code, 0)
+    # We don't display any notice.
+    self.assertFalse(self.print_version_change.called)
+    self.assertFalse(self.print_notice.called)
+    # We don't upload any metrics.
+    self.assertFalse(self.Popen.called)
+    # We don't modify the config.
+    self.assertFalse(self.FileWrite.called)
+
+  @mock.patch('metrics.metrics_utils.CURRENT_VERSION', 5)
+  def test_version_change_non_googler(self):
+    # The user is not a googler and we're not collecting metrics.
+    self.FileRead.side_effect = [
+        '{"is-googler": false, "countdown": 10, "opt-in": null, "version": 0}'
+    ]
+    with self.assertRaises(SystemExit) as cm:
+      with self.collector.print_notice_and_exit():
+        self.collector.add('foo-metric', 1)
+
+    self.assertEqual(cm.exception.code, 0)
+    # We don't display any notice.
+    self.assertFalse(self.print_version_change.called)
+    self.assertFalse(self.print_notice.called)
+    # We don't upload any metrics.
+    self.assertFalse(self.Popen.called)
+    # We don't modify the config.
+    self.assertFalse(self.FileWrite.called)
+
+  @mock.patch('metrics.metrics_utils.CURRENT_VERSION', 5)
+  def test_opting_in_updates_version(self):
+    # The user is seeing the notice telling him of the version changes.
+    self.FileRead.side_effect = [
+        '{"is-googler": true, "countdown": 8, "opt-in": null, "version": 0}'
+    ]
+
+    self.collector.config.opted_in = True
+
+    # We don't display any notice.
+    self.assertFalse(self.print_version_change.called)
+    self.assertFalse(self.print_notice.called)
+    # We don't upload any metrics.
+    self.assertFalse(self.Popen.called)
+    # We update the version and opt-in the user.
+    self.assert_writes_file(
+        self.config_file,
+        {'is-googler': True, 'countdown': 8, 'opt-in': True, 'version': 5})
+
+  @mock.patch('metrics.metrics_utils.CURRENT_VERSION', 5)
+  def test_opting_in_by_default_updates_version(self):
+    # The user will be opted in by default on the next execution.
+    self.FileRead.side_effect = [
+        '{"is-googler": true, "countdown": 1, "opt-in": null, "version": 0}'
+    ]
+
+    with self.assertRaises(SystemExit) as cm:
+      with self.collector.print_notice_and_exit():
+        self.collector.add('foo-metric', 1)
+
+    self.assertEqual(cm.exception.code, 0)
+    # We display the notices.
+    self.print_notice.assert_called_once_with(1)
+    self.print_version_change.assert_called_once_with(0)
+    # We don't upload any metrics.
+    self.assertFalse(self.Popen.called)
+    # We update the version and set the countdown to 0. In subsequent runs,
+    # we'll start collecting metrics.
+    self.assert_writes_file(
+        self.config_file,
+        {'is-googler': True, 'countdown': 0, 'opt-in': None, 'version': 5})
+
 
 if __name__ == '__main__':
   unittest.main()