metrics: Don't print notice if reporting build metrics.

Change-Id: Ib7e23cd1b57c6d0497f0e0ff1fe31b38f11bfc00
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/2876082
Reviewed-by: Anthony Polito <apolito@google.com>
Commit-Queue: Edward Lesmes <ehmaldonado@chromium.org>

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:

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()
 

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,
-    },
-  }
 
 
 

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

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

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

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 = [