autoninja: Replace ninjalog_uploader_wrapper.py and reclient_metrics.py with build_telemetry.py

This CL unifies the opt-in/opt-out handling for build telemetry collections about Reclient and Ninjalog.

The user consent message will be displayed only once at the beginning of a build.

```
❯ autoninja -C out/deterministic-andorid-dbg base
*** NOTICE ***
Google-internal telemetry (including build logs, username, and hostname) is collected on corp machines to diagnose performance and fix build issues. This reminder will be shown 9 more times. See http://go/chrome-build-telemetry for details. Hide this notice or opt out by running: build_telemetry [opt-in] [opt-out]
*** END NOTICE ***

Proxy started successfully.
...
```

Bug: 345113094
Change-Id: Ie5886287c4bd20262be0ff247508ac3869441eb1
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/5669094
Reviewed-by: Yiwei Zhang <yiwzhang@google.com>
Reviewed-by: Michael Savigny <msavigny@google.com>
Commit-Queue: Junji Watanabe <jwata@google.com>
Reviewed-by: Takuto Ikuta <tikuta@chromium.org>

.gitignore

@@ -87,12 +87,6 @@ testing_support/google_appengine
 # Ignore win/mac monitoring config. It is unique for each depot_tools checkout.
 /metrics.cfg
 
-# Ignore the ninjalog upload config.
-/ninjalog.cfg
-
-# Ignore reclient metrics upload config.
-/reclient_metrics.cfg
-
 # Ignore git traces produced by git push on git-cl upload.
 /traces
 
@@ -103,4 +97,4 @@ testing_support/google_appengine
 /python2_usage.txt
 
 # Ignore the internal data used by autoninja.
-/.autoninja*
+/.autoninja*

autoninja

@@ -27,9 +27,6 @@ if [ "$retval" == "0" ] && [ "$NINJA_SUMMARIZE_BUILD" == "1" ]; then
   python3 "$(dirname -- "$0")/post_build_ninja_summary.py" "$@"
 fi
 
-# Collect ninjalog from googler.
-python3 "$(dirname -- "$0")/ninjalog_uploader_wrapper.py" --cmdline "$@"
-
 # Pass-through autoninja's error code so that if a developer types:
 # "autoninja chrome && chrome" then chrome won't run if the build fails.
 exit $retval

autoninja.bat

@@ -35,8 +35,6 @@ if "%NINJA_SUMMARIZE_BUILD%" == "1" set "NINJA_STATUS=[%%r processes, %%f/%%t @
 exit /b %ERRORLEVEL%
 :buildfailure
 
-@call %scriptdir%python-bin\python3.bat %scriptdir%ninjalog_uploader_wrapper.py --cmdline %*
-
 :: Return an error code of 1 so that if a developer types:
 :: "autoninja chrome && chrome" then chrome won't run if the build fails.
 cmd /c exit 1

autoninja.py

@@ -14,6 +14,7 @@ does handle import statements, but it can't handle conditional setting of build
 settings.
 """
 
+import logging
 import json
 import multiprocessing
 import os
@@ -30,15 +31,18 @@ import warnings
 import google.auth
 from google.auth.transport.requests import AuthorizedSession
 
+import build_telemetry
 import gn_helper
 import ninja
+import ninjalog_uploader
 import reclient_helper
 import siso
 
 if sys.platform in ["darwin", "linux"]:
     import resource
 
-SCRIPT_DIR = os.path.dirname(os.path.realpath(__file__))
+_SCRIPT_DIR = os.path.dirname(os.path.realpath(__file__))
+_NINJALOG_UPLOADER = os.path.join(_SCRIPT_DIR, "ninjalog_uploader.py")
 
 # See [1] and [2] for the painful details of this next section, which handles
 # escaping command lines so that they can be copied and pasted into a cmd
@@ -132,7 +136,7 @@ def _is_google_corp_machine_using_external_account():
     if not _is_google_corp_machine():
         return False
 
-    with shelve.open(os.path.join(SCRIPT_DIR, ".autoninja")) as db:
+    with shelve.open(os.path.join(_SCRIPT_DIR, ".autoninja")) as db:
         last_false = db.get("last_false")
         now = time.time()
         if last_false is not None and now < last_false + 12 * 60 * 60:
@@ -182,7 +186,7 @@ def _print_cmd(cmd):
     print(*[shell_quoter(arg) for arg in cmd], file=sys.stderr)
 
 
-def main(args):
+def main(args, should_collect_logs=False):
     # if user doesn't set PYTHONPYCACHEPREFIX and PYTHONDONTWRITEBYTECODE
     # set PYTHONDONTWRITEBYTECODE=1 not to create many *.pyc in workspace
     # and keep workspace clean.
@@ -284,13 +288,15 @@ def main(args):
                 return 1
             if use_remoteexec:
                 if use_reclient:
-                    return reclient_helper.run_siso([
-                        'siso',
-                        'ninja',
-                        # Do not authenticate when using Reproxy.
-                        '-project=',
-                        '-reapi_instance=',
-                    ] + input_args[1:])
+                    return reclient_helper.run_siso(
+                        [
+                            'siso',
+                            'ninja',
+                            # Do not authenticate when using Reproxy.
+                            '-project=',
+                            '-reapi_instance=',
+                        ] + input_args[1:],
+                        should_collect_logs)
                 return siso.main(["siso", "ninja"] + input_args[1:])
             return siso.main(["siso", "ninja", "--offline"] + input_args[1:])
 
@@ -393,12 +399,31 @@ def main(args):
         _print_cmd(args)
 
     if use_reclient:
-        return reclient_helper.run_ninja(args)
+        return reclient_helper.run_ninja(args, should_collect_logs)
     return ninja.main(args)
 
 
+def _upload_ninjalog(args):
+    # Run upload script without wait.
+    creationflags = 0
+    if platform.system() == "Windows":
+        creationflags = subprocess.CREATE_NEW_PROCESS_GROUP
+    subprocess.Popen(
+        [sys.executable, _NINJALOG_UPLOADER] + args[1:],
+        stdout=subprocess.DEVNULL,
+        stderr=subprocess.DEVNULL,
+        creationflags=creationflags,
+    )
+
+
 if __name__ == "__main__":
+    # Check the log collection opt-in/opt-out status, and display notice if necessary.
+    _should_collect_logs = build_telemetry.enabled()
     try:
-        sys.exit(main(sys.argv))
+        exit_code = main(sys.argv, _should_collect_logs)
     except KeyboardInterrupt:
-        sys.exit(1)
+        exit_code = 1
+    finally:
+        if _should_collect_logs:
+            _upload_ninjalog(sys.argv)
+    sys.exit(exit_code)

ninjalog.README.md

@@ -1,61 +0,0 @@
-# Ninja build log collection
-
-[TOC]
-
-## Overview
-
-When chromium developers use autoninja for their build,
-
-e.g.
-```
-$ autoninja -C out/Release chrome
-```
-
-autoninja uploads ninja's build log for Google employees but we don't collect
-logs from external contributors.
-
-We use `cipd auth-info` to decide whether an autoninja user is a Googler or not.
-
-Before uploading logs, autoninja shows a message 10 times to warn users that we
-will collect build logs.
-
-autoninja users can also opt in or out by using the following commands:
-
-* `ninjalog_uploader_wrapper.py opt-in`
-* `ninjalog_uploader_wrapper.py opt-out`
-
-## What type of data are collected?
-
-The collected build log contains
-
-* output file of build tasks (e.g. chrome, obj/url/url/url_util.o)
-* hash of build command
-* start and end time of build tasks
-
-See [manual of ninja](https://ninja-build.org/manual.html#ref_log) for more
-details.
-
-In addition to ninja's build log, we send the following data for further
-analysis:
-
-* OS (e.g. Win, Mac or Linux)
-* number of cpu cores of building machine
-* build targets (e.g. chrome, browser_tests)
-* parallelism passed by -j flag
-* following build configs
-  * host\_os, host\_cpu
-  * target\_os, target\_cpu
-  * symbol\_level
-  * is\_debug
-  * is\_component\_build
-
- We don't collect personally identifiable information
-(e.g. username, ip address).
-
-## Why ninja log is collected? / How the collected logs are used?
-
-We (Chrome Browser Build team) collect build logs to find slow build tasks that
-harm developer's productivity. Based on collected stats, we find the
-place/build tasks where we need to focus on. Also we use collected stats to
-track chrome build performance on developer's machine. We'll use this stats to
-measure how much we can/can't improve build performance on developer's machine.

ninjalog_uploader.py

@@ -29,6 +29,8 @@ import sys
 import time
 import urllib.request
 
+import build_telemetry
+
 # These build configs affect build performance.
 ALLOWLISTED_CONFIGS = (
     "android_static_analysis",
@@ -51,26 +53,6 @@ ALLOWLISTED_CONFIGS = (
 )
 
 
-def IsGoogler():
-    """Check whether this user is Googler or not."""
-    p = subprocess.run(
-        "cipd auth-info",
-        stdout=subprocess.PIPE,
-        stderr=subprocess.PIPE,
-        text=True,
-        shell=True,
-    )
-    if p.returncode != 0:
-        return False
-    lines = p.stdout.splitlines()
-    if len(lines) == 0:
-        return False
-    l = lines[0]
-    # |l| will be like 'Logged in as <user>@google.com.' for googler using
-    # reclient.
-    return l.startswith("Logged in as ") and l.endswith("@google.com.")
-
-
 def ParseGNArgs(gn_args):
     """Parse gn_args as json and return config dictionary."""
     configs = json.loads(gn_args)
@@ -226,8 +208,10 @@ def main():
         # Disable logging.
         logging.disable(logging.CRITICAL)
 
-    if not IsGoogler():
-        return 0
+    cfg = build_telemetry.load_config()
+    if not cfg.is_googler:
+        logging.warning("Not Googler. Only Googlers can upload ninjalog.")
+        return 1
 
     ninjalog = args.ninjalog or GetNinjalog(args.cmdline)
     if not os.path.isfile(ninjalog):

ninjalog_uploader_wrapper.py

@@ -1,141 +0,0 @@
-#!/usr/bin/env python3
-# Copyright 2018 The Chromium Authors. All rights reserved.
-# Use of this source code is governed by a BSD-style license that can be
-# found in the LICENSE file.
-
-import json
-import os
-import platform
-import subprocess
-import sys
-
-import ninjalog_uploader
-import subprocess2
-import utils
-
-THIS_DIR = os.path.dirname(__file__)
-UPLOADER = os.path.join(THIS_DIR, "ninjalog_uploader.py")
-CONFIG = utils.depot_tools_config_path("ninjalog.cfg")
-VERSION = 3
-
-
-def LoadConfig():
-    if os.path.isfile(CONFIG):
-        with open(CONFIG, "r") as f:
-            try:
-                config = json.load(f)
-            except Exception:
-                # Set default value when failed to load config.
-                config = {
-                    "is-googler": ninjalog_uploader.IsGoogler(),
-                    "countdown": 10,
-                    "version": VERSION,
-                }
-
-            if config["version"] == VERSION:
-                config["countdown"] = max(0, config["countdown"] - 1)
-                return config
-
-    return {
-        "is-googler": ninjalog_uploader.IsGoogler(),
-        "countdown": 10,
-        "version": VERSION,
-    }
-
-
-def SaveConfig(config):
-    with open(CONFIG, "w") as f:
-        json.dump(config, f)
-
-
-def ShowMessage(countdown):
-    allowlisted = "\n".join(
-        ["  * %s" % config for config in ninjalog_uploader.ALLOWLISTED_CONFIGS])
-    print("""
-Your ninjalog will be uploaded to build stats server. The uploaded log will be
-used to analyze user side build performance.
-
-The following information will be uploaded with ninjalog.
-* OS (e.g. Win, Mac or Linux)
-* number of cpu cores of building machine
-* build targets (e.g. chrome, browser_tests)
-* parallelism passed by -j flag
-* following build configs
-%s
-
-Uploading ninjalog will be started after you run autoninja another %d time(s).
-
-If you don't want to upload ninjalog, please run following command.
-$ python3 %s opt-out
-
-If you want to allow upload ninjalog from next autoninja run, please run the
-following command.
-$ python3 %s opt-in
-
-If you have questions about this, please send mail to infra-dev@chromium.org
-
-You can find a more detailed explanation in
-%s
-or
-https://chromium.googlesource.com/chromium/tools/depot_tools/+/main/ninjalog.README.md
-
-""" % (
-        allowlisted,
-        countdown,
-        __file__,
-        __file__,
-        os.path.abspath(os.path.join(THIS_DIR, "ninjalog.README.md")),
-    ))
-
-
-def main():
-    config = LoadConfig()
-
-    if len(sys.argv) == 2 and sys.argv[1] == "opt-in":
-        config["opt-in"] = True
-        config["countdown"] = 0
-        SaveConfig(config)
-        print("ninjalog upload is opted in.")
-        return 0
-
-    if len(sys.argv) == 2 and sys.argv[1] == "opt-out":
-        config["opt-in"] = False
-        SaveConfig(config)
-        print("ninjalog upload is opted out.")
-        return 0
-
-    if "opt-in" in config and not config["opt-in"]:
-        # Upload is opted out.
-        return 0
-
-    if not config.get("is-googler", False):
-        # Not googler.
-        return 0
-
-    if config.get("countdown", 0) > 0:
-        # Need to show message.
-        ShowMessage(config["countdown"])
-        # Only save config if something has meaningfully changed.
-        SaveConfig(config)
-        return 0
-
-    if len(sys.argv) == 1:
-        # dry-run for debugging.
-        print("upload ninjalog dry-run")
-        return 0
-
-    # Run upload script without wait.
-    devnull = open(os.devnull, "w")
-    creationflags = 0
-    if platform.system() == "Windows":
-        creationflags = subprocess.CREATE_NEW_PROCESS_GROUP
-    subprocess2.Popen(
-        [sys.executable, UPLOADER] + sys.argv[1:],
-        stdout=devnull,
-        stderr=devnull,
-        creationflags=creationflags,
-    )
-
-
-if __name__ == "__main__":
-    sys.exit(main())

reclient_helper.py

@@ -20,7 +20,6 @@ import uuid
 
 import gclient_paths
 import ninja
-import reclient_metrics
 import siso
 
 THIS_DIR = os.path.dirname(__file__)
@@ -313,7 +312,7 @@ def reclient_setup_docs_url():
 
 
 @contextlib.contextmanager
-def build_context(argv, tool):
+def build_context(argv, tool, should_collect_logs):
     # If use_remoteexec is set, but the reclient binaries or configs don't
     # exist, display an error message and stop.  Otherwise, the build will
     # attempt to run with rewrapper wrapping actions, but will fail with
@@ -339,7 +338,7 @@ def build_context(argv, tool):
         yield 1
         return
 
-    if reclient_metrics.check_status(ninja_out):
+    if should_collect_logs:
         set_reproxy_metrics_flags(tool)
 
     if os.environ.get('RBE_instance', None):
@@ -384,10 +383,11 @@ Ensure you have completed the reproxy setup instructions:
             print('%1.3fs to stop reproxy' % elapsed)
 
 
-def run_ninja(ninja_cmd):
+def run_ninja(ninja_cmd, should_collect_logs=False):
     """Runs Ninja in build_context()."""
     # TODO: crbug.com/345113094 - rename the `tool` label to `ninja`.
-    with build_context(ninja_cmd, "ninja_reclient") as ret_code:
+    with build_context(ninja_cmd, "ninja_reclient",
+                       should_collect_logs) as ret_code:
         if ret_code:
             return ret_code
         try:
@@ -397,10 +397,10 @@ def run_ninja(ninja_cmd):
             return 1
 
 
-def run_siso(siso_cmd):
+def run_siso(siso_cmd, should_collect_logs=False):
     """Runs Siso in build_context()."""
     # TODO: crbug.com/345113094 - rename the `autosiso` label to `siso`.
-    with build_context(siso_cmd, "autosiso") as ret_code:
+    with build_context(siso_cmd, "autosiso", should_collect_logs) as ret_code:
         if ret_code:
             return ret_code
         try:

reclient_metrics

@@ -1,8 +0,0 @@
-#!/usr/bin/env bash
-
-# Copyright 2023 The Chromium Authors
-# Use of this source code is governed by a BSD-style license that can be
-# found in the LICENSE file.
-
-base_dir=$(dirname "$0")
-PYTHONDONTWRITEBYTECODE=1 exec python3 "$base_dir/reclient_metrics.py" "$@"

reclient_metrics.README.md

@@ -1,63 +0,0 @@
-# Reclient metric collection
-
-[TOC]
-
-## Overview
-
-When chromium developers enable download_remoteexec_cfg and run their build with use_remoteexec enabled,
-
-e.g.
-
-`.gclient`
-```
-solutions = [
-  {
-    "name": "src",
-    "url": "https://chromium.googlesource.com/chromium/src.git",
-    "managed": False,
-    "custom_deps": {},
-    "custom_vars": {
- ...
-      "download_remoteexec_cfg": True,
- ...
-    },
-    ...
-  },
-]
-```
-
-```
-$ gclient runhooks
-$ gn gen -C out/Release --args="use_remoteexec=true"
-$ autoninja -C out/Release chrome
-```
-
-reproxy uploads reclient's build metrics. The download_remoteexec_cfg gclient flag is only available for Google employees.
-
-Before uploading metrics, reproxy will show a message 10 times to warn users that we will collect build metrics.
-
-Users can opt in by running the following command.
-$ python3 reclient_metrics.py opt-in
-
-Users can opt out by running the following command.
-$ python3 reclient_metrics.py opt-out
-
-## What type of data are collected?
-
-We upload the contents of <ninja-out>/.reproxy_tmp/logs/rbe_metrics.txt.
-This contains
-* Flags passed to reproxy
-  * Auth related flags are filtered out by reproxy
-* Start and end time of build tasks
-* Aggregated durations and counts of events during remote build actions
-* OS (e.g. Win, Mac or Linux)
-* Number of cpu cores and the amount of RAM of the building machine
-
-
-We collect googler hostnames to help diagnose long tail issues.
-We don't collect any other personally identifiable information
-(e.g. username, ip address).
-
-## Why are reproxy metrics collected? / How are the metrics collected?
-
-We (Chrome build team/Reclient team) collect build metrics to find slow build tasks that harm developer's productivity. Based on collected stats, we find the place/build tasks where we need to focus on. Also we use collected stats to track Chrome build performance on developer's machine. We'll use these stats to measure how much we can/can't improve build performance on developer machines.

reclient_metrics.bat

@@ -1,12 +0,0 @@
-@echo off
-:: Copyright 2023 The Chromium Authors
-:: Use of this source code is governed by a BSD-style license that can be
-:: found in the LICENSE file.
-setlocal
-
-:: Ensure that "depot_tools" is somewhere in PATH so this tool can be used
-:: standalone, but allow other PATH manipulations to take priority.
-set PATH=%PATH%;%~dp0
-
-:: Defer control.
-python3 "%~dp0\reclient_metrics.py" "%*"

reclient_metrics.py

@@ -1,145 +0,0 @@
-#!/usr/bin/env python3
-# Copyright 2023 The Chromium Authors. All rights reserved.
-# Use of this source code is governed by a BSD-style license that can be
-# found in the LICENSE file.
-"""This script manages the counter for how many times developers should
-be notified before uploading reclient metrics."""
-
-import json
-import os
-import subprocess
-import sys
-
-import utils
-
-THIS_DIR = os.path.dirname(__file__)
-CONFIG = utils.depot_tools_config_path('reclient_metrics.cfg')
-VERSION = 1
-
-
-def default_config():
-    return {
-        'is-googler': is_googler(),
-        'countdown': 10,
-        'version': VERSION,
-    }
-
-
-def load_config():
-    config = None
-    try:
-        with open(CONFIG) as f:
-            raw_config = json.load(f)
-            if raw_config['version'] == VERSION:
-                raw_config['countdown'] = max(0, raw_config['countdown'] - 1)
-                config = raw_config
-    except Exception:
-        pass
-    if not config:
-        config = default_config()
-    save_config(config)
-    return config
-
-
-def save_config(config):
-    with open(CONFIG, 'w') as f:
-        json.dump(config, f)
-
-
-def show_message(config, ninja_out):
-    print("""
-Your reclient metrics will be uploaded to the chromium build metrics database. The uploaded metrics will be used to analyze user side build performance.
-
-We upload the contents of {ninja_out_abs}.
-This contains
-* Flags passed to reproxy
-  * Auth related flags are filtered out by reproxy
-* Start and end time of build tasks
-* Aggregated durations and counts of events during remote build actions
-* OS (e.g. Win, Mac or Linux)
-* Number of cpu cores and the amount of RAM of the building machine
-
-Uploading reclient metrics will be started after you run autoninja another {config_count} time(s).
-
-If you don't want to upload reclient metrics, please run following command.
-$ python3 {file_path} opt-out
-
-If you want to allow upload reclient metrics from next autoninja run, please run the
-following command.
-$ python3 {file_path} opt-in
-
-If you have questions about this, please send an email to foundry-x@google.com
-
-You can find a more detailed explanation in
-{metrics_readme_path}
-or
-https://chromium.googlesource.com/chromium/tools/depot_tools/+/main/reclient_metrics.README.md
-""".format(
-        ninja_out_abs=os.path.abspath(
-            os.path.join(ninja_out, ".reproxy_tmp", "logs", "rbe_metrics.txt")),
-        config_count=config.get("countdown", 0),
-        file_path=__file__,
-        metrics_readme_path=os.path.abspath(
-            os.path.join(THIS_DIR, "reclient_metrics.README.md")),
-    ))
-
-
-def is_googler(config=None):
-    """Check whether this user is Googler or not."""
-    if config is not None and 'is-googler' in config:
-        return config['is-googler']
-    # Use cipd auth-info to check for googler status as
-    # downloading rewrapper configs already requires cipd to be logged in
-    p = subprocess.run('cipd auth-info',
-                       stdout=subprocess.PIPE,
-                       stderr=subprocess.PIPE,
-                       text=True,
-                       shell=True)
-    if p.returncode != 0:
-        return False
-    lines = p.stdout.splitlines()
-    if len(lines) == 0:
-        return False
-    l = lines[0]
-    # |l| will be like 'Logged in as <user>@google.com.' for googlers.
-    return l.startswith('Logged in as ') and l.endswith('@google.com.')
-
-
-def check_status(ninja_out):
-    """Checks metrics collections status and shows notice to user if needed.
-
-    Returns True if metrics should be collected."""
-    config = load_config()
-    if not is_googler(config):
-        return False
-    if 'opt-in' in config:
-        return config['opt-in']
-    if config.get("countdown", 0) > 0:
-        show_message(config, ninja_out)
-        return False
-    return True
-
-
-def main(argv):
-    cfg = load_config()
-
-    if not is_googler(cfg):
-        save_config(cfg)
-        return 0
-
-    if len(argv) == 2 and argv[1] == 'opt-in':
-        cfg['opt-in'] = True
-        cfg['countdown'] = 0
-        save_config(cfg)
-        print('reclient metrics upload is opted in.')
-        return 0
-
-    if len(argv) == 2 and argv[1] == 'opt-out':
-        cfg['opt-in'] = False
-        save_config(cfg)
-        print('reclient metrics upload is opted out.')
-        return 0
-
-
-if __name__ == '__main__':
-    sys.exit(main(sys.argv))

tests/autoninja_test.py

@@ -118,7 +118,7 @@ class AutoninjaTest(trial_dir.TestCase):
         reclient_helper_calls = []
 
         @contextlib.contextmanager
-        def reclient_helper_mock(argv, tool):
+        def reclient_helper_mock(argv, tool, _should_collect_logs):
             reclient_helper_calls.append([argv, tool])
             yield 0
 
@@ -160,7 +160,7 @@ class AutoninjaTest(trial_dir.TestCase):
                                                     luci_auth_account,
                                                     expected):
         for shelve_file in glob.glob(
-                os.path.join(autoninja.SCRIPT_DIR, ".autoninja*")):
+                os.path.join(autoninja._SCRIPT_DIR, ".autoninja*")):
             # Clear cache.
             os.remove(shelve_file)
 

tests/ninjalog_uploader_test.py

@@ -16,25 +16,6 @@ import ninjalog_uploader
 
 
 class NinjalogUploaderTest(unittest.TestCase):
-    def test_IsGoogler(self):
-        with unittest.mock.patch('subprocess.run') as run_mock:
-            run_mock.return_value.returncode = 0
-            run_mock.return_value.stdout = 'Logged in as foo@google.com.\n'
-            self.assertTrue(ninjalog_uploader.IsGoogler())
-
-        with unittest.mock.patch('subprocess.run') as run_mock:
-            run_mock.return_value.returncode = 1
-            self.assertFalse(ninjalog_uploader.IsGoogler())
-
-        with unittest.mock.patch('subprocess.run') as run_mock:
-            run_mock.return_value.returncode = 0
-            run_mock.return_value.stdout = ''
-            self.assertFalse(ninjalog_uploader.IsGoogler())
-
-        with unittest.mock.patch('subprocess.run') as run_mock:
-            run_mock.return_value.returncode = 0
-            run_mock.return_value.stdout = 'Logged in as foo@example.com.\n'
-            self.assertFalse(ninjalog_uploader.IsGoogler())
 
     def test_parse_gn_args(self):
         self.assertEqual(ninjalog_uploader.ParseGNArgs(json.dumps([])), {})

tests/reclient_helper_test.py

@@ -48,7 +48,6 @@ class ReclientHelperTest(trial_dir.TestCase):
                                                         0))
     @unittest.mock.patch('subprocess.call', return_value=0)
     @unittest.mock.patch('ninja.main', return_value=0)
-    @unittest.mock.patch('reclient_metrics.check_status', return_value=False)
     def test_ninja_reclient_sets_path_env_vars(self, *_):
         reclient_bin_dir = os.path.join('src', 'buildtools', 'reclient')
         reclient_cfg = os.path.join('src', 'buildtools', 'reclient_cfgs',
@@ -102,9 +101,8 @@ class ReclientHelperTest(trial_dir.TestCase):
 
     @unittest.mock.patch('subprocess.call', return_value=0)
     @unittest.mock.patch('ninja.main', return_value=0)
-    @unittest.mock.patch('reclient_metrics.check_status', return_value=False)
-    def test_ninja_reclient_calls_reclient_binaries(self, mock_metrics_status,
-                                                    mock_ninja, mock_call):
+    def test_ninja_reclient_calls_reclient_binaries(self, mock_ninja,
+                                                    mock_call):
         reclient_bin_dir = os.path.join('src', 'buildtools', 'reclient')
         reclient_cfg = os.path.join('src', 'buildtools', 'reclient_cfgs',
                                     'reproxy.cfg')
@@ -116,7 +114,6 @@ class ReclientHelperTest(trial_dir.TestCase):
 
         self.assertEqual(0, reclient_helper.run_ninja(argv))
 
-        mock_metrics_status.assert_called_once_with("out/a")
         mock_ninja.assert_called_once_with(argv)
         mock_call.assert_has_calls([
             unittest.mock.call([
@@ -139,9 +136,7 @@ class ReclientHelperTest(trial_dir.TestCase):
                               {'AUTONINJA_BUILD_ID': "SOME_RANDOM_ID"})
     @unittest.mock.patch('subprocess.call', return_value=0)
     @unittest.mock.patch('ninja.main', return_value=0)
-    @unittest.mock.patch('reclient_metrics.check_status', return_value=True)
-    def test_ninja_reclient_collect_metrics_cache_missing(
-            self, mock_metrics_status, *_):
+    def test_ninja_reclient_collect_metrics_cache_missing(self, *_):
         reclient_bin_dir = os.path.join('src', 'buildtools', 'reclient')
         reclient_cfg = os.path.join('src', 'buildtools', 'reclient_cfgs',
                                     'reproxy.cfg')
@@ -151,7 +146,8 @@ class ReclientHelperTest(trial_dir.TestCase):
         write(reclient_cfg, '0.0')
         argv = ["ninja", "-C", "out/a", "chrome"]
 
-        self.assertEqual(0, reclient_helper.run_ninja(argv))
+        self.assertEqual(
+            0, reclient_helper.run_ninja(argv, should_collect_logs=True))
 
         self.assertIn("/SOME_RANDOM_ID", os.environ["RBE_invocation_id"])
         self.assertEqual(os.environ.get('RBE_metrics_project'),
@@ -165,8 +161,6 @@ class ReclientHelperTest(trial_dir.TestCase):
         self.assertEqual(os.environ.get('RBE_metrics_prefix'),
                          "go.chromium.org")
 
-        mock_metrics_status.assert_called_once_with("out/a")
-
     @unittest.mock.patch.dict(os.environ,
                               {'AUTONINJA_BUILD_ID': "SOME_RANDOM_ID"},
                               clear=True)
@@ -175,10 +169,7 @@ class ReclientHelperTest(trial_dir.TestCase):
                                                         0))
     @unittest.mock.patch('subprocess.call', return_value=0)
     @unittest.mock.patch('ninja.main', return_value=0)
-    @unittest.mock.patch('reclient_metrics.check_status', return_value=True)
-    def test_ninja_reclient_collect_metrics_cache_valid(self,
-                                                        mock_metrics_status,
-                                                        *_):
+    def test_ninja_reclient_collect_metrics_cache_valid(self, *_):
         reclient_bin_dir = os.path.join('src', 'buildtools', 'reclient')
         reclient_cfg = os.path.join('src', 'buildtools', 'reclient_cfgs',
                                     'reproxy.cfg')
@@ -200,7 +191,8 @@ expiry:  {
               """ % (int(time.time()) + 10 * 60))
         argv = ["ninja", "-C", "out/a", "chrome"]
 
-        self.assertEqual(0, reclient_helper.run_ninja(argv))
+        self.assertEqual(
+            0, reclient_helper.run_ninja(argv, should_collect_logs=True))
 
         self.assertIn("/SOME_RANDOM_ID", os.environ["RBE_invocation_id"])
         self.assertEqual(os.environ.get('RBE_metrics_project'),
@@ -214,16 +206,12 @@ expiry:  {
         self.assertEqual(os.environ.get('RBE_metrics_prefix'),
                          "go.chromium.org")
 
-        mock_metrics_status.assert_called_once_with("out/a")
-
     @unittest.mock.patch.dict(os.environ,
                               {'AUTONINJA_BUILD_ID': "SOME_RANDOM_ID"},
                               clear=True)
     @unittest.mock.patch('subprocess.call', return_value=0)
     @unittest.mock.patch('ninja.main', return_value=0)
-    @unittest.mock.patch('reclient_metrics.check_status', return_value=True)
-    def test_ninja_reclient_collect_metrics_cache_expired(
-            self, mock_metrics_status, *_):
+    def test_ninja_reclient_collect_metrics_cache_expired(self, *_):
         reclient_bin_dir = os.path.join('src', 'buildtools', 'reclient')
         reclient_cfg = os.path.join('src', 'buildtools', 'reclient_cfgs',
                                     'reproxy.cfg')
@@ -245,7 +233,8 @@ expiry:  {
               """ % (int(time.time())))
         argv = ["ninja", "-C", "out/a", "chrome"]
 
-        self.assertEqual(0, reclient_helper.run_ninja(argv))
+        self.assertEqual(
+            0, reclient_helper.run_ninja(argv, should_collect_logs=True))
 
         self.assertIn("/SOME_RANDOM_ID", os.environ["RBE_invocation_id"])
         self.assertEqual(os.environ.get('RBE_metrics_project'),
@@ -259,14 +248,11 @@ expiry:  {
         self.assertEqual(os.environ.get('RBE_metrics_prefix'),
                          "go.chromium.org")
 
-        mock_metrics_status.assert_called_once_with("out/a")
 
     @unittest.mock.patch.dict(os.environ, {})
     @unittest.mock.patch('subprocess.call', return_value=0)
     @unittest.mock.patch('ninja.main', return_value=0)
-    @unittest.mock.patch('reclient_metrics.check_status', return_value=False)
-    def test_ninja_reclient_do_not_collect_metrics(self, mock_metrics_status,
-                                                   *_):
+    def test_ninja_reclient_do_not_collect_metrics(self, *_):
         reclient_bin_dir = os.path.join('src', 'buildtools', 'reclient')
         reclient_cfg = os.path.join('src', 'buildtools', 'reclient_cfgs',
                                     'reproxy.cfg')
@@ -283,11 +269,8 @@ expiry:  {
         self.assertEqual(os.environ.get('RBE_metrics_labels'), None)
         self.assertEqual(os.environ.get('RBE_metrics_prefix'), None)
 
-        mock_metrics_status.assert_called_once_with("out/a")
-
     @unittest.mock.patch('subprocess.call', return_value=0)
     @unittest.mock.patch('ninja.main', return_value=0)
-    @unittest.mock.patch('reclient_metrics.check_status', return_value=True)
     @unittest.mock.patch('reclient_helper.datetime_now')
     def test_ninja_reclient_clears_log_dir(self, mock_now, *_):
         reclient_bin_dir = os.path.join('src', 'buildtools', 'reclient')

tests/reclient_metrics_test.py

@@ -1,260 +0,0 @@
-#!/usr/bin/env python3
-# Copyright (c) 2023 The Chromium Authors. All rights reserved.
-# Use of this source code is governed by a BSD-style license that can be
-# found in the LICENSE file.
-
-import io
-import os
-import os.path
-import sys
-import tempfile
-import unittest
-import unittest.mock
-
-ROOT_DIR = os.path.dirname(os.path.dirname(os.path.abspath(__file__)))
-sys.path.insert(0, ROOT_DIR)
-
-import reclient_metrics
-
-
-class ReclientMetricsTest(unittest.TestCase):
-    def test_is_googler(self):
-        with unittest.mock.patch('subprocess.run') as run_mock:
-            run_mock.return_value.returncode = 0
-            run_mock.return_value.stdout = 'Logged in as abc@google.com.'
-            self.assertTrue(reclient_metrics.is_googler())
-
-        with unittest.mock.patch('subprocess.run') as run_mock:
-            run_mock.return_value.returncode = 1
-            self.assertFalse(reclient_metrics.is_googler())
-
-        with unittest.mock.patch('subprocess.run') as run_mock:
-            run_mock.return_value.returncode = 0
-            run_mock.return_value.stdout = ''
-            self.assertFalse(reclient_metrics.is_googler())
-
-        with unittest.mock.patch('subprocess.run') as run_mock:
-            run_mock.return_value.returncode = 0
-            run_mock.return_value.stdout = 'Logged in as foo@example.com.'
-            self.assertFalse(reclient_metrics.is_googler())
-
-        with unittest.mock.patch('subprocess.run') as run_mock:
-            self.assertTrue(reclient_metrics.is_googler({
-                'is-googler': True,
-            }))
-            self.assertFalse(
-                reclient_metrics.is_googler({
-                    'is-googler': False,
-                }))
-            run_mock.assert_not_called()
-
-    def test_load_and_save_config(self):
-        with tempfile.TemporaryDirectory() as tmpdir:
-            reclient_metrics.CONFIG = os.path.join(tmpdir,
-                                                   'reclient_metrics.cfg')
-            with unittest.mock.patch('subprocess.run') as run_mock:
-                run_mock.return_value.returncode = 0
-                run_mock.return_value.stdout = 'Logged in as abc@google.com.'
-                cfg1 = reclient_metrics.load_config()
-                self.assertDictEqual(
-                    cfg1, {
-                        'is-googler': True,
-                        'countdown': 10,
-                        'version': reclient_metrics.VERSION,
-                    })
-                reclient_metrics.save_config(cfg1)
-                cfg2 = reclient_metrics.load_config()
-                self.assertDictEqual(
-                    cfg2, {
-                        'is-googler': True,
-                        'countdown': 9,
-                        'version': reclient_metrics.VERSION,
-                    })
-                run_mock.assert_called_once()
-
-    def test_check_status(self):
-        with tempfile.TemporaryDirectory() as tmpdir:
-            reclient_metrics.CONFIG = os.path.join(tmpdir,
-                                                   'reclient_metrics.cfg')
-            with unittest.mock.patch('subprocess.run') as run_mock:
-                run_mock.return_value.returncode = 0
-                run_mock.return_value.stdout = 'Logged in as abc@google.com.'
-                with unittest.mock.patch('sys.stdout',
-                                         new=io.StringIO()) as stdout_mock:
-                    self.assertFalse(reclient_metrics.check_status("outdir"))
-                    self.assertIn("Your reclient metrics will",
-                                  stdout_mock.getvalue())
-                    self.assertIn(
-                        os.path.join("outdir", ".reproxy_tmp", "logs",
-                                     "rbe_metrics.txt"), stdout_mock.getvalue())
-                run_mock.assert_called_once()
-
-        with tempfile.TemporaryDirectory() as tmpdir:
-            reclient_metrics.CONFIG = os.path.join(tmpdir,
-                                                   'reclient_metrics.cfg')
-            with unittest.mock.patch('subprocess.run') as run_mock:
-                run_mock.return_value.returncode = 0
-                run_mock.return_value.stdout = 'Logged in as abc@google.com.'
-                for i in range(10):
-                    with unittest.mock.patch('sys.stdout',
-                                             new=io.StringIO()) as stdout_mock:
-                        self.assertFalse(
-                            reclient_metrics.check_status("outdir"))
-                        self.assertIn("Your reclient metrics will",
-                                      stdout_mock.getvalue())
-                        self.assertIn(
-                            os.path.join("outdir", ".reproxy_tmp", "logs",
-                                         "rbe_metrics.txt"),
-                            stdout_mock.getvalue())
-                        self.assertIn(
-                            "you run autoninja another %d time(s)" % (10 - i),
-                            stdout_mock.getvalue())
-                with unittest.mock.patch('sys.stdout',
-                                         new=io.StringIO()) as stdout_mock:
-                    self.assertTrue(reclient_metrics.check_status("outdir"))
-                    self.assertNotIn("Your reclient metrics will",
-                                     stdout_mock.getvalue())
-                    self.assertNotIn(
-                        os.path.join("outdir", ".reproxy_tmp", "logs",
-                                     "rbe_metrics.txt"), stdout_mock.getvalue())
-                run_mock.assert_called_once()
-
-        with tempfile.TemporaryDirectory() as tmpdir:
-            reclient_metrics.CONFIG = os.path.join(tmpdir,
-                                                   'reclient_metrics.cfg')
-            with unittest.mock.patch('subprocess.run') as run_mock:
-                run_mock.return_value.returncode = 0
-                run_mock.return_value.stdout = 'Logged in as abc@example.com.'
-                with unittest.mock.patch('sys.stdout',
-                                         new=io.StringIO()) as stdout_mock:
-                    self.assertFalse(reclient_metrics.check_status("outdir"))
-                    self.assertNotIn("Your reclient metrics will",
-                                     stdout_mock.getvalue())
-                    self.assertNotIn(
-                        os.path.join("outdir", ".reproxy_tmp", "logs",
-                                     "rbe_metrics.txt"), stdout_mock.getvalue())
-                run_mock.assert_called_once()
-
-        with tempfile.TemporaryDirectory() as tmpdir:
-            reclient_metrics.CONFIG = os.path.join(tmpdir,
-                                                   'reclient_metrics.cfg')
-            with unittest.mock.patch('subprocess.run') as run_mock:
-                run_mock.return_value.returncode = 1
-                run_mock.return_value.stdout = ''
-                with unittest.mock.patch('sys.stdout',
-                                         new=io.StringIO()) as stdout_mock:
-                    self.assertFalse(reclient_metrics.check_status("outdir"))
-                    self.assertNotIn("Your reclient metrics will",
-                                     stdout_mock.getvalue())
-                    self.assertNotIn(
-                        os.path.join("outdir", ".reproxy_tmp", "logs",
-                                     "rbe_metrics.txt"), stdout_mock.getvalue())
-                run_mock.assert_called_once()
-
-        with tempfile.TemporaryDirectory() as tmpdir:
-            reclient_metrics.CONFIG = os.path.join(tmpdir,
-                                                   'reclient_metrics.cfg')
-            with unittest.mock.patch('subprocess.run') as run_mock:
-                run_mock.return_value.returncode = 0
-                run_mock.return_value.stdout = 'Logged in as abc@google.com.'
-                reclient_metrics.main(["reclient_metrics.py", "opt-in"])
-                with unittest.mock.patch('sys.stdout',
-                                         new=io.StringIO()) as stdout_mock:
-                    self.assertTrue(reclient_metrics.check_status("outdir"))
-                    self.assertNotIn("Your reclient metrics will",
-                                     stdout_mock.getvalue())
-                    self.assertNotIn(
-                        os.path.join("outdir", ".reproxy_tmp", "logs",
-                                     "rbe_metrics.txt"), stdout_mock.getvalue())
-                run_mock.assert_called_once()
-
-        with tempfile.TemporaryDirectory() as tmpdir:
-            reclient_metrics.CONFIG = os.path.join(tmpdir,
-                                                   'reclient_metrics.cfg')
-            with unittest.mock.patch('subprocess.run') as run_mock:
-                run_mock.return_value.returncode = 0
-                run_mock.return_value.stdout = 'Logged in as abc@google.com.'
-                for i in range(3):
-                    with unittest.mock.patch('sys.stdout',
-                                             new=io.StringIO()) as stdout_mock:
-                        self.assertFalse(
-                            reclient_metrics.check_status("outdir"))
-                        self.assertIn("Your reclient metrics will",
-                                      stdout_mock.getvalue())
-                        self.assertIn(
-                            os.path.join("outdir", ".reproxy_tmp", "logs",
-                                         "rbe_metrics.txt"),
-                            stdout_mock.getvalue())
-                        self.assertIn(
-                            "you run autoninja another %d time(s)" % (10 - i),
-                            stdout_mock.getvalue())
-                reclient_metrics.main(["reclient_metrics.py", "opt-in"])
-                with unittest.mock.patch('sys.stdout',
-                                         new=io.StringIO()) as stdout_mock:
-                    self.assertTrue(reclient_metrics.check_status("outdir"))
-                    self.assertNotIn("Your reclient metrics will",
-                                     stdout_mock.getvalue())
-                    self.assertNotIn(
-                        os.path.join("outdir", ".reproxy_tmp", "logs",
-                                     "rbe_metrics.txt"), stdout_mock.getvalue())
-                run_mock.assert_called_once()
-
-        with tempfile.TemporaryDirectory() as tmpdir:
-            reclient_metrics.CONFIG = os.path.join(tmpdir,
-                                                   'reclient_metrics.cfg')
-            with unittest.mock.patch('subprocess.run') as run_mock:
-                run_mock.return_value.returncode = 0
-                run_mock.return_value.stdout = 'Logged in as abc@example.com.'
-                with unittest.mock.patch('sys.stdout',
-                                         new=io.StringIO()) as stdout_mock:
-                    self.assertFalse(reclient_metrics.check_status("outdir"))
-                    self.assertNotIn("Your reclient metrics will",
-                                     stdout_mock.getvalue())
-                    self.assertNotIn(
-                        os.path.join("outdir", ".reproxy_tmp", "logs",
-                                     "rbe_metrics.txt"), stdout_mock.getvalue())
-                reclient_metrics.main(["reclient_metrics.py", "opt-in"])
-                with unittest.mock.patch('sys.stdout',
-                                         new=io.StringIO()) as stdout_mock:
-                    self.assertFalse(reclient_metrics.check_status("outdir"))
-                    self.assertNotIn("Your reclient metrics will",
-                                     stdout_mock.getvalue())
-                    self.assertNotIn(
-                        os.path.join("outdir", ".reproxy_tmp", "logs",
-                                     "rbe_metrics.txt"), stdout_mock.getvalue())
-                run_mock.assert_called_once()
-
-        with tempfile.TemporaryDirectory() as tmpdir:
-            reclient_metrics.CONFIG = os.path.join(tmpdir,
-                                                   'reclient_metrics.cfg')
-            with unittest.mock.patch('subprocess.run') as run_mock:
-                run_mock.return_value.returncode = 0
-                run_mock.return_value.stdout = 'Logged in as abc@google.com.'
-                for i in range(3):
-                    with unittest.mock.patch('sys.stdout',
-                                             new=io.StringIO()) as stdout_mock:
-                        self.assertFalse(
-                            reclient_metrics.check_status("outdir"))
-                        self.assertIn("Your reclient metrics will",
-                                      stdout_mock.getvalue())
-                        self.assertIn(
-                            os.path.join("outdir", ".reproxy_tmp", "logs",
-                                         "rbe_metrics.txt"),
-                            stdout_mock.getvalue())
-                        self.assertIn(
-                            "you run autoninja another %d time(s)" % (10 - i),
-                            stdout_mock.getvalue())
-                reclient_metrics.main(["reclient_metrics.py", "opt-out"])
-                with unittest.mock.patch('sys.stdout',
-                                         new=io.StringIO()) as stdout_mock:
-                    self.assertFalse(reclient_metrics.check_status("outdir"))
-                    self.assertNotIn("Your reclient metrics will",
-                                     stdout_mock.getvalue())
-                    self.assertNotIn(
-                        os.path.join("outdir", ".reproxy_tmp", "logs",
-                                     "rbe_metrics.txt"), stdout_mock.getvalue())
-                run_mock.assert_called_once()
-
-
-if __name__ == '__main__':
-    unittest.main()