diff --git a/git_cl.py b/git_cl.py index 5ad4947903..55ff1e09b7 100755 --- a/git_cl.py +++ b/git_cl.py @@ -5336,7 +5336,7 @@ class OptionParser(optparse.OptionParser): global settings settings = Settings() - if not metrics.DISABLE_METRICS_COLLECTION: + if metrics.collector.config.should_collect_metrics: # GetViewVCUrl ultimately calls logging method. project_url = settings.GetViewVCUrl().strip('/+') if project_url in metrics_utils.KNOWN_PROJECT_URLS: diff --git a/metrics.py b/metrics.py index df366a4013..902a228611 100644 --- a/metrics.py +++ b/metrics.py @@ -30,7 +30,6 @@ DEPOT_TOOLS = os.path.dirname(os.path.abspath(__file__)) CONFIG_FILE = os.path.join(DEPOT_TOOLS, 'metrics.cfg') UPLOAD_SCRIPT = os.path.join(DEPOT_TOOLS, 'upload_metrics.py') -DISABLE_METRICS_COLLECTION = os.environ.get('DEPOT_TOOLS_METRICS') == '0' DEFAULT_COUNTDOWN = 10 INVALID_CONFIG_WARNING = ( @@ -52,6 +51,28 @@ class _Config(object): if self._initialized: return + # Metrics collection is disabled, so don't collect any metrics. + if not metrics_utils.COLLECT_METRICS: + self._config = { + 'is-googler': False, + 'countdown': 0, + 'opt-in': False, + 'version': metrics_utils.CURRENT_VERSION, + } + self._initialized = True + return + + # We are running on a bot. Ignore config and collect metrics. + if metrics_utils.REPORT_BUILD: + self._config = { + 'is-googler': True, + 'countdown': 0, + 'opt-in': True, + 'version': metrics_utils.CURRENT_VERSION, + } + self._initialized = True + return + try: config = json.loads(gclient_utils.FileRead(CONFIG_FILE)) except (IOError, ValueError): @@ -117,9 +138,6 @@ class _Config(object): @property def should_collect_metrics(self): - # DEPOT_TOOLS_REPORT_BUILD is set on bots to report metrics. - if os.getenv(metrics_utils.DEPOT_TOOLS_REPORT_BUILD): - return True # Don't report metrics if user is not a Googler. if not self.is_googler: return False @@ -247,13 +265,8 @@ class MetricsCollector(object): environment and the function performance. """ def _decorator(func): - # Do this first so we don't have to read, and possibly create a config - # file. - if DISABLE_METRICS_COLLECTION: - return func if not self.config.should_collect_metrics: return func - # Otherwise, collect the metrics. # Needed to preserve the __name__ and __doc__ attributes of func. @functools.wraps(func) def _inner(*args, **kwargs): @@ -287,15 +300,14 @@ class MetricsCollector(object): traceback.print_exception(*exception) # Check if the version has changed - if (not DISABLE_METRICS_COLLECTION and self.config.is_googler + if (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): + if self.config.is_googler and self.config.opted_in is None: metrics_utils.print_notice(self.config.countdown) self.config.decrease_countdown() diff --git a/metrics_utils.py b/metrics_utils.py index 5792118f66..5a7c77e80e 100644 --- a/metrics_utils.py +++ b/metrics_utils.py @@ -24,7 +24,8 @@ CURRENT_VERSION = 2 APP_URL = 'https://cit-cli-metrics.appspot.com' -DEPOT_TOOLS_REPORT_BUILD = 'DEPOT_TOOLS_REPORT_BUILD' +REPORT_BUILD = os.getenv('DEPOT_TOOLS_REPORT_BUILD') +COLLECT_METRICS = os.getenv('DEPOT_TOOLS_COLLECT_METRICS') != '0' def get_notice_countdown_header(countdown): @@ -192,18 +193,18 @@ def get_git_version(): def get_bot_metrics(): - build = os.getenv(DEPOT_TOOLS_REPORT_BUILD) - if not build or build.count('/') != 3: + try: + project, bucket, builder, build = REPORT_BUILD.split('/') + return { + 'build_id': int(build), + 'builder': { + 'project': project, + 'bucket': bucket, + 'builder': builder, + }, + } + except (AttributeError, ValueError): return None - project, bucket, builder, build = build.split('/') - return { - 'build_id': int(build), - 'builder': { - 'project': project, - 'bucket': bucket, - 'builder': builder, - }, - } diff --git a/tests/gclient_eval_unittest.py b/tests/gclient_eval_unittest.py index 0d90232f83..84c3a75d90 100755 --- a/tests/gclient_eval_unittest.py +++ b/tests/gclient_eval_unittest.py @@ -14,9 +14,9 @@ sys.path.insert(0, os.path.dirname(os.path.dirname(os.path.abspath(__file__)))) from third_party import schema -import metrics +import metrics_utils # We have to disable monitoring before importing gclient. -metrics.DISABLE_METRICS_COLLECTION = True +metrics_utils.COLLECT_METRICS = False import gclient import gclient_eval diff --git a/tests/gclient_test.py b/tests/gclient_test.py index fcc02ce7e6..a4ab9fcb66 100755 --- a/tests/gclient_test.py +++ b/tests/gclient_test.py @@ -24,9 +24,9 @@ else: sys.path.insert(0, os.path.dirname(os.path.dirname(os.path.abspath(__file__)))) -import metrics +import metrics_utils # We have to disable monitoring before importing gclient. -metrics.DISABLE_METRICS_COLLECTION = True +metrics_utils.COLLECT_METRICS = False import gclient import gclient_eval diff --git a/tests/git_cl_test.py b/tests/git_cl_test.py index 9ab5bb3197..d743095c79 100755 --- a/tests/git_cl_test.py +++ b/tests/git_cl_test.py @@ -31,8 +31,9 @@ else: sys.path.insert(0, os.path.dirname(os.path.dirname(os.path.abspath(__file__)))) import metrics +import metrics_utils # We have to disable monitoring before importing git_cl. -metrics.DISABLE_METRICS_COLLECTION = True +metrics_utils.COLLECT_METRICS = False import clang_format import contextlib diff --git a/tests/metrics_test.py b/tests/metrics_test.py index 05915d8e21..af4e58f539 100644 --- a/tests/metrics_test.py +++ b/tests/metrics_test.py @@ -181,7 +181,28 @@ class MetricsCollectorTest(unittest.TestCase): fun() self.assert_collects_metrics() - @mock.patch('metrics.DISABLE_METRICS_COLLECTION', True) + @mock.patch('metrics_utils.REPORT_BUILD', 'p/b/b/1') + def test_collects_metrics_report_build_set(self): + """Tests that metrics are collected when REPORT_BUILD is set.""" + @self.collector.collect_metrics('fun') + def fun(): + pass + + fun() + self.assert_collects_metrics({ + 'bot_metrics': { + 'build_id': 1, + 'builder': { + 'project': 'p', + 'bucket': 'b', + 'builder': 'b', + } + } + }) + # We shouldn't have tried to read the config file. + self.assertFalse(self.FileRead.called) + + @mock.patch('metrics_utils.COLLECT_METRICS', False) def test_metrics_collection_disabled(self): """Tests that metrics collection can be disabled via a global variable.""" @self.collector.collect_metrics('fun') @@ -341,7 +362,7 @@ class MetricsCollectorTest(unittest.TestCase): self.assertEqual(cm.exception.code, 0) self.assertFalse(self.print_notice.called) - @mock.patch('metrics.DISABLE_METRICS_COLLECTION', True) + @mock.patch('metrics_utils.COLLECT_METRICS', False) def test_doesnt_print_notice_disable_metrics_collection(self): with self.assertRaises(SystemExit) as cm: with self.collector.print_notice_and_exit(): @@ -351,6 +372,16 @@ class MetricsCollectorTest(unittest.TestCase): # We shouldn't have tried to read the config file. self.assertFalse(self.FileRead.called) + @mock.patch('metrics_utils.REPORT_BUILD', 'p/b/b/1') + def test_doesnt_print_notice_report_build(self): + with self.assertRaises(SystemExit) as cm: + with self.collector.print_notice_and_exit(): + pass + self.assertEqual(cm.exception.code, 0) + self.assertFalse(self.print_notice.called) + # We shouldn't have tried to read the config file. + self.assertFalse(self.FileRead.called) + def test_print_notice_handles_exceptions(self): """Tests that exception are caught and we exit with an appropriate code.""" self.FileRead.side_effect = [