[android] Stop capturing full-suite logcats.

This also switches from running tests through the butler to
running tests alongside the butler s.t. we can independently
control process lifetimes.

Bug: 721889
Change-Id: I176b7340d9b3f097ad660b66ea64ffb4816287a8
Reviewed-on: https://chromium-review.googlesource.com/667808
Commit-Queue: John Budorick <jbudorick@chromium.org>
Reviewed-by: Benjamin Pastene <bpastene@chromium.org>
Cr-Commit-Position: refs/heads/master@{#502757}

build/android/pylib/gtest/gtest_test_instance.py

@@ -287,6 +287,7 @@ class GtestTestInstance(test_instance.TestInstance):
     self._extract_test_list_from_filter = args.extract_test_list_from_filter
     self._filter_tests_lock = threading.Lock()
     self._shard_timeout = args.shard_timeout
+    self._should_save_logcat = bool(args.json_results_file)
     self._store_tombstones = args.store_tombstones
     self._total_external_shards = args.test_launcher_total_shards
     self._suite = args.suite_name[0]
@@ -437,6 +438,12 @@ class GtestTestInstance(test_instance.TestInstance):
   def shard_timeout(self):
     return self._shard_timeout
 
+  # TODO(jbudorick): Remove this once mikecase lands
+  # https://codereview.chromium.org/2933993002/
+  @property
+  def should_save_logcat(self):
+    return self._should_save_logcat
+
   @property
   def store_tombstones(self):
     return self._store_tombstones

build/android/pylib/local/device/local_device_environment.py

@@ -2,6 +2,7 @@
 # Use of this source code is governed by a BSD-style license that can be
 # found in the LICENSE file.
 
+import contextlib
 import datetime
 import functools
 import logging
@@ -9,6 +10,7 @@ import os
 import shutil
 import tempfile
 import threading
+import time
 
 import devil_chromium
 from devil import base_error
@@ -21,9 +23,20 @@ from devil.android.sdk import adb_wrapper
 from devil.utils import file_utils
 from devil.utils import parallelizer
 from pylib import constants
+from pylib.android import logdog_logcat_monitor
 from pylib.base import environment
 from pylib.utils import instrumentation_tracing
 from py_trace_event import trace_event
+from py_utils import contextlib_ext
+
+
+LOGCAT_FILTERS = [
+  '*:e',
+  'chromium:v',
+  'cr_*:v',
+  'DEBUG:I',
+  'StrictMode:D',
+]
 
 
 def _DeviceCachePath(device):
@@ -75,6 +88,39 @@ def handle_shard_failures_with(on_failure):
   return decorator
 
 
+# TODO(jbudorick): Reconcile this with the output manager logic in
+# https://codereview.chromium.org/2933993002/ once that lands.
+@contextlib.contextmanager
+def OptionalPerTestLogcat(
+    device, test_name, condition, additional_filter_specs=None,
+    deobfuscate_func=None):
+  """Conditionally capture logcat and stream it to logdog.
+
+  Args:
+    device: (DeviceUtils) the device from which logcat should be captured.
+    test_name: (str) the test name to use in the stream name.
+    condition: (bool) whether or not to capture the logcat.
+    additional_filter_specs: (list) additional logcat filters.
+    deobfuscate_func: (callable) an optional unary function that
+      deobfuscates logcat lines. The callable should take an iterable
+      of logcat lines and return a list of deobfuscated logcat lines.
+  Yields:
+    A LogdogLogcatMonitor instance whether condition is true or not,
+    though it may not be active.
+  """
+  stream_name = 'logcat_%s_%s_%s' % (
+      test_name,
+      time.strftime('%Y%m%dT%H%M%S-UTC', time.gmtime()),
+      device.serial)
+  filter_specs = LOGCAT_FILTERS + (additional_filter_specs or [])
+  logmon = logdog_logcat_monitor.LogdogLogcatMonitor(
+      device.adb, stream_name, filter_specs=filter_specs,
+      deobfuscate_func=deobfuscate_func)
+
+  with contextlib_ext.Optional(logmon, condition):
+    yield logmon
+
+
 class LocalDeviceEnvironment(environment.Environment):
 
   def __init__(self, args, _error_func):

build/android/pylib/local/device/local_device_gtest_run.py

@@ -413,12 +413,15 @@ class LocalDeviceGtestRun(local_device_test_run.LocalDeviceTestRun):
       for f in flags:
         logging.info('  %s', f)
 
-      with contextlib_ext.Optional(
-          trace_event.trace(str(test)),
-          self._env.trace_output):
-        output = self._delegate.Run(
-            test, device, flags=' '.join(flags),
-            timeout=timeout, retries=0)
+      with local_device_environment.OptionalPerTestLogcat(
+          device, hash(tuple(test)),
+          self._test_instance.should_save_logcat) as logmon:
+        with contextlib_ext.Optional(
+            trace_event.trace(str(test)),
+            self._env.trace_output):
+          output = self._delegate.Run(
+              test, device, flags=' '.join(flags),
+              timeout=timeout, retries=0)
 
       if self._test_instance.enable_xml_result_parsing:
         try:
@@ -432,6 +435,8 @@ class LocalDeviceGtestRun(local_device_test_run.LocalDeviceTestRun):
               str(e))
           gtest_xml = None
 
+      logcat_url = logmon.GetLogcatURL()
+
     for s in self._servers[str(device)]:
       s.Reset()
     if self._test_instance.app_files:
@@ -451,14 +456,14 @@ class LocalDeviceGtestRun(local_device_test_run.LocalDeviceTestRun):
       results = gtest_test_instance.ParseGTestOutput(
           output, self._test_instance.symbolizer, device.product_cpu_abi)
 
-    # Check whether there are any crashed testcases.
-    self._crashes.update(r.GetName() for r in results
-                         if r.GetType() == base_test_result.ResultType.CRASH)
+    tombstones_url = None
+    for r in results:
+      if self._test_instance.should_save_logcat:
+        r.SetLink('logcat', logcat_url)
 
-    if self._test_instance.store_tombstones:
-      tombstones_url = None
-      for result in results:
-        if result.GetType() == base_test_result.ResultType.CRASH:
+      if r.GetType() == base_test_result.ResultType.CRASH:
+        self._crashes.add(r.GetName())
+        if self._test_instance.store_tombstones:
           if not tombstones_url:
             resolved_tombstones = tombstones.ResolveTombstones(
                 device,
@@ -470,7 +475,7 @@ class LocalDeviceGtestRun(local_device_test_run.LocalDeviceTestRun):
                 device.serial)
             tombstones_url = logdog_helper.text(
                 stream_name, '\n'.join(resolved_tombstones))
-          result.SetLink('tombstones', tombstones_url)
+          r.SetLink('tombstones', tombstones_url)
 
     tests_stripped_disabled_prefix = set()
     for t in test:

build/android/pylib/local/device/local_device_instrumentation_test_run.py

@@ -22,7 +22,6 @@ from devil.android.tools import system_app
 from devil.utils import reraiser_thread
 from incremental_install import installer
 from pylib import valgrind_tools
-from pylib.android import logdog_logcat_monitor
 from pylib.base import base_test_result
 from pylib.constants import host_paths
 from pylib.instrumentation import instrumentation_test_instance
@@ -440,16 +439,11 @@ class LocalDeviceInstrumentationTestRun(
     time_ms = lambda: int(time.time() * 1e3)
     start_ms = time_ms()
 
-    stream_name = 'logcat_%s_%s_%s' % (
-        test_name.replace('#', '.'),
-        time.strftime('%Y%m%dT%H%M%S-UTC', time.gmtime()),
-        device.serial)
-    logmon = logdog_logcat_monitor.LogdogLogcatMonitor(
-        device.adb, stream_name, filter_specs=LOGCAT_FILTERS,
-        deobfuscate_func=self._test_instance.MaybeDeobfuscateLines)
-
-    with contextlib_ext.Optional(
-        logmon, self._test_instance.should_save_logcat):
+    with local_device_environment.OptionalPerTestLogcat(
+        device, test_name.replace('#', '.'),
+        self._test_instance.should_save_logcat,
+        additional_filter_specs=['%s:I' % _TAG],
+        deobfuscate_func=self._test_instance.MaybeDeobfuscateLines) as logmon:
       with _LogTestEndpoints(device, test_name):
         with contextlib_ext.Optional(
             trace_event.trace(test_name),
@@ -872,6 +866,7 @@ class LocalDeviceInstrumentationTestRun(
 
     return timeout
 
+
 def _IsRenderTest(test):
   """Determines if a test or list of tests has a RenderTest amongst them."""
   if not isinstance(test, list):

build/android/test_runner.py

@@ -76,8 +76,9 @@ def AddTestLauncherOptions(parser):
       '--test-launcher-summary-output',
       '--json-results-file',
       dest='json_results_file', type=os.path.realpath,
-      help='If set, will dump results in JSON form '
-           'to specified file.')
+      help='If set, will dump results in JSON form to the specified file. '
+           'Note that this will also trigger saving per-test logcats to '
+           'logdog.')
   parser.add_argument(
       '--test-launcher-shard-index',
       type=int, default=os.environ.get('GTEST_SHARD_INDEX', 0),

build/android/test_wrapper/logdog_wrapper.py

@@ -6,6 +6,7 @@
 """Wrapper for adding logdog streaming support to swarming tasks."""
 
 import argparse
+import contextlib
 import logging
 import os
 import signal
@@ -36,58 +37,82 @@ def CommandParser():
                       help='The logdog bin cmd.')
   parser.add_argument('--target-devices-file', required=False,
                       help='The target devices file.')
-  parser.add_argument('--logcat-output-file',
-                      help='The logcat output file.')
   return parser
 
+
 def CreateStopTestsMethod(proc):
   def StopTests(signum, _frame):
     logging.error('Forwarding signal %s to test process', str(signum))
     proc.send_signal(signum)
   return StopTests
 
+
+@contextlib.contextmanager
+def NoLeakingProcesses(popen):
+  try:
+    yield popen
+  finally:
+    try:
+      if popen.poll() is None:
+        popen.kill()
+    except OSError:
+      logging.warning('Failed to kill %s. Process may be leaked.',
+                      str(popen.pid))
+
+
 def main():
   parser = CommandParser()
   args, extra_cmd_args = parser.parse_known_args(sys.argv[1:])
 
   logging.basicConfig(level=logging.INFO)
-  with tempfile_ext.NamedTemporaryDirectory() as logcat_output_dir:
-    test_cmd = [
-        os.path.join('bin', 'run_%s' % args.target),
-        '--logcat-output-file',
-        (args.logcat_output_file if args.logcat_output_file
-            else os.path.join(logcat_output_dir, 'logcats')),
-        '--target-devices-file', args.target_devices_file,
-        '-v']
-
-    with tempfile_ext.NamedTemporaryDirectory(
-        prefix='tmp_android_logdog_wrapper') as temp_directory:
-      if not os.path.exists(args.logdog_bin_cmd):
-        logging.error(
-            'Logdog binary %s unavailable. Unable to create logdog client',
-            args.logdog_bin_cmd)
-      else:
-        test_cmd += ['--upload-logcats-file']
-        streamserver_uri = 'unix:%s' % os.path.join(temp_directory,
-                                                    'butler.sock')
-        prefix = os.path.join('android', 'swarming', 'logcats',
-                              os.environ.get('SWARMING_TASK_ID'))
-
-        # Call test_cmdline through logdog butler subcommand.
-        test_cmd = [
-            args.logdog_bin_cmd, '-project', PROJECT,
-            '-output', OUTPUT,
-            '-prefix', prefix,
-            '--service-account-json', SERVICE_ACCOUNT_JSON,
-            '-coordinator-host', COORDINATOR_HOST,
-            'run', '-streamserver-uri', streamserver_uri, '--'] + test_cmd
-
-      test_cmd += extra_cmd_args
-      test_proc = subprocess.Popen(test_cmd)
-      with signal_handler.SignalHandler(signal.SIGTERM,
-                                        CreateStopTestsMethod(test_proc)):
-        result = test_proc.wait()
-    return result
+  test_cmd = [
+      os.path.join('bin', 'run_%s' % args.target),
+      '--target-devices-file', args.target_devices_file,
+      '-v']
+  test_env = dict(os.environ)
+  logdog_cmd = []
+
+  with tempfile_ext.NamedTemporaryDirectory(
+      prefix='tmp_android_logdog_wrapper') as temp_directory:
+    if not os.path.exists(args.logdog_bin_cmd):
+      logging.error(
+          'Logdog binary %s unavailable. Unable to create logdog client',
+          args.logdog_bin_cmd)
+    else:
+      streamserver_uri = 'unix:%s' % os.path.join(temp_directory,
+                                                  'butler.sock')
+      prefix = os.path.join('android', 'swarming', 'logcats',
+                            os.environ.get('SWARMING_TASK_ID'))
+
+      logdog_cmd = [
+          args.logdog_bin_cmd,
+          '-project', PROJECT,
+          '-output', OUTPUT,
+          '-prefix', prefix,
+          '--service-account-json', SERVICE_ACCOUNT_JSON,
+          '-coordinator-host', COORDINATOR_HOST,
+          'serve',
+          '-streamserver-uri', streamserver_uri]
+      test_env.update({
+          'LOGDOG_STREAM_PROJECT': PROJECT,
+          'LOGDOG_STREAM_PREFIX': prefix,
+          'LOGDOG_STREAM_SERVER_PATH': streamserver_uri,
+          'LOGDOG_COORDINATOR_HOST': COORDINATOR_HOST,
+      })
+
+    test_cmd += extra_cmd_args
+
+    with NoLeakingProcesses(subprocess.Popen(logdog_cmd)) as logdog_proc:
+      with NoLeakingProcesses(
+          subprocess.Popen(test_cmd, env=test_env)) as test_proc:
+        with signal_handler.SignalHandler(signal.SIGTERM,
+                                          CreateStopTestsMethod(test_proc)):
+          result = test_proc.wait()
+          logdog_proc.terminate()
+          logdog_proc.wait()
+
+  return result
+
 
 if __name__ == '__main__':
   sys.exit(main())

tools/mb/mb.py

@@ -1118,7 +1118,6 @@ class MetaBuildWrapper(object):
           '--target', target,
           '--target-devices-file', '${SWARMING_BOT_FILE}',
           '--logdog-bin-cmd', '../../bin/logdog_butler',
-          '--logcat-output-file', '${ISOLATED_OUTDIR}/logcats',
           '--store-tombstones']
     elif is_fuchsia and test_type != 'script':
       cmdline = [os.path.join('bin', 'run_%s' % target)]