diff --git a/metrics.README.md b/metrics.README.md index 12358802e..04674d659 100644 --- a/metrics.README.md +++ b/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 diff --git a/metrics.py b/metrics.py index 6b556e23c..c2c88e0d8 100644 --- a/metrics.py +++ b/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): diff --git a/metrics_utils.py b/metrics_utils.py index 936c692a1..9bdae9e68 100644 --- a/metrics_utils.py +++ b/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) diff --git a/tests/metrics_test.py b/tests/metrics_test.py index ab440473f..a97c2673f 100644 --- a/tests/metrics_test.py +++ b/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()