From 5056b3582bd8637903ec0dbe6e87e4b09469506c Mon Sep 17 00:00:00 2001
From: Mikhail Khokhlov <khokhlov@google.com>
Date: Mon, 14 Oct 2024 14:30:14 +0100
Subject: [PATCH] [Python API] Improve TraceProcessor error reporting

Includes the process stdout and stderr in the exception text when the
trace processor failed to start.

Change-Id: I627b00d47eb3e12e54edd7c4e09dc28dc518a219
---
 python/perfetto/trace_processor/shell.py | 19 ++++++++++++-------
 python/test/api_integrationtest.py       | 11 +++++++++++
 2 files changed, 23 insertions(+), 7 deletions(-)

diff --git a/python/perfetto/trace_processor/shell.py b/python/perfetto/trace_processor/shell.py
index 41f165549c..2211912b41 100644
--- a/python/perfetto/trace_processor/shell.py
+++ b/python/perfetto/trace_processor/shell.py
@@ -16,6 +16,7 @@
 import os
 import subprocess
 import sys
+import tempfile
 import time
 from typing import List, Optional
 from urllib import request, error
@@ -55,11 +56,13 @@ def load_shell(bin_path: str,
   if extra_flags:
     args.extend(extra_flags)
 
+  temp_stdout = tempfile.TemporaryFile()
+  temp_stderr = tempfile.TemporaryFile()
   p = subprocess.Popen(
       tp_exec + args,
       stdin=subprocess.DEVNULL,
-      stdout=subprocess.DEVNULL,
-      stderr=None if verbose else subprocess.DEVNULL)
+      stdout=temp_stdout,
+      stderr=None if verbose else temp_stderr)
 
   success = False
   for _ in range(load_timeout + 1):
@@ -72,10 +75,12 @@ def load_shell(bin_path: str,
       time.sleep(1)
 
   if not success:
-    raise PerfettoException(
-        "Trace processor failed to start. Try rerunning with "
-        "verbose=True in TraceProcessorConfig for more detailed "
-        "information and file a bug at https://goto.google.com/perfetto-bug "
-        "or https://github.com/google/perfetto/issues if necessary.")
+    p.kill()
+    temp_stdout.seek(0)
+    stdout = temp_stdout.read().decode("utf-8")
+    temp_stderr.seek(0)
+    stderr = temp_stderr.read().decode("utf-8")
+    raise PerfettoException("Trace processor failed to start.\n"
+                            f"stdout: {stdout}\nstderr: {stderr}\n")
 
   return url, p
diff --git a/python/test/api_integrationtest.py b/python/test/api_integrationtest.py
index 8e01679f27..cb6e9feb8d 100644
--- a/python/test/api_integrationtest.py
+++ b/python/test/api_integrationtest.py
@@ -134,6 +134,17 @@ def test_invalid_trace(self):
     with self.assertRaises(TraceProcessorException):
       _ = create_tp(trace=f)
 
+  def test_runtime_error(self):
+    # We emulate a situation when TP returns an error by passing the --version
+    # flag. This makes TP output version information and exit, instead of
+    # starting an http server.
+    config = TraceProcessorConfig(
+        bin_path=os.environ["SHELL_PATH"], extra_flags=["--version"])
+    with self.assertRaisesRegex(
+        TraceProcessorException,
+        expected_regex='.*Trace Processor RPC API version:.*'):
+      TraceProcessor(trace=io.BytesIO(b''), config=config)
+
   def test_trace_path(self):
     # Get path to trace_processor_shell and construct TraceProcessor
     tp = create_tp(trace=example_android_trace_path())