siso: introduce flag check for collector by checking its help page.

Bug: b/473530378, b/455433899
Change-Id: I09f53cd181cc3cd516e2fa035616efb06a6a6964
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/7365204
Reviewed-by: Junji Watanabe <jwata@google.com>
Commit-Queue: Alex Ovsienko <ovsienko@google.com>

siso.py

@@ -45,13 +45,12 @@ def parse_args(args: list[str]) -> tuple[str, str]:
     return subcmd, out_dir
 
 
-# Trivial check if siso contains subcommand.
-# Subcommand completes successfully if subcommand is present, returning 0,
-# and 2 if it's not present.
-def _is_subcommand_present(siso_path: str, subc: str) -> bool:
-    return subprocess.call([siso_path, "help", subc],
-                           stdout=subprocess.DEVNULL,
-                           stderr=subprocess.DEVNULL) == 0
+# Trivial check if siso contains subcommand and returns its help page
+# or nothing if subcommand is not present.
+def _subcommand_help(siso_path: str, subc: str) -> str:
+    return subprocess.run([siso_path, "help", subc],
+                          capture_output=True,
+                          text=True).stdout
 
 
 # Fetch PID platform independently of possibly running collector
@@ -255,8 +254,9 @@ def apply_telemetry_flags(args: list[str], env: dict[str, str],
         "enable_cloud_monitoring", "enable_cloud_profiler",
         "enable_cloud_trace", "enable_cloud_logging"
     ]
-    if _is_subcommand_present(siso_path, "collector"):
-        telemetry_flags.append("enable_collector")
+    if "collector_address" in _subcommand_help(siso_path, "collector"):
+        if "collector_address" in _subcommand_help(siso_path, "ninja"):
+            telemetry_flags.append("enable_collector")
     # Despite go.dev/issue/68312 being fixed, the issue is still reproducible
     # for googlers. Due to this, the flag is still applied while the
     # issue is being investigated.

tests/siso_test.py

@@ -75,17 +75,16 @@ def test_apply_sisorc(siso_test_fixture: Any) -> None:
 
 
 def test_is_subcommand_present(mocker: Any) -> None:
-    mock_call = mocker.patch("siso.subprocess.call")
 
-    def side_effect(cmd, *_, **__):
-        if cmd[2] in ["collector", "ninja"]:
-            return 0
-        return 2
+    def side_effect(_: str, subcmd):
+        if subcmd in ["collector", "ninja"]:
+            return "help page"
+        return ""
 
-    mock_call.side_effect = side_effect
-    assert siso._is_subcommand_present("siso_path", "collector")
-    assert siso._is_subcommand_present("siso_path", "ninja")
-    assert not siso._is_subcommand_present("siso_path", "unknown")
+    mocker.patch("siso._subcommand_help", side_effect=side_effect)
+    assert siso._subcommand_help("siso_path", "collector")
+    assert siso._subcommand_help("siso_path", "ninja")
+    assert not siso._subcommand_help("siso_path", "unknown")
 
 
 @pytest.mark.parametrize("args, want", [
@@ -105,7 +104,14 @@ def test_apply_metrics_labels(args: List[str], want: List[str]) -> None:
 
 @pytest.fixture
 def siso_subcmd_present(mocker: Any) -> Generator[None, None, None]:
-    mocker.patch("siso._is_subcommand_present", return_value=True)
+    mocker.patch("siso._subcommand_help",
+                 return_value="""
+Starts the OTEL collector daemon.
+  -collector_address string
+    	address to listen on for collector. Can be path for unix socket unix:///path/to/socket or host:port. (default "127.0.0.1:4317")
+  -project string
+    	cloud project ID. can be set by $SISO_PROJECT
+                 """)
     yield
 
 
@@ -183,12 +189,12 @@ def test_apply_telemetry_flags_sets_expected_env_var(siso_subcmd_present: Any,
 
 def test_apply_telemetry_flags_collector_not_present(mocker: Any) -> None:
 
-    def mock_subcommand(_: str, subcmd: str) -> bool:
+    def mock_subcommand(_: str, subcmd: str) -> str:
         if subcmd == "collector":
-            return False
-        return True
+            return ""
+        return "help page"
 
-    mocker.patch("siso._is_subcommand_present", side_effect=mock_subcommand)
+    mocker.patch("siso._subcommand_help", side_effect=mock_subcommand)
     args = ["ninja", "-C", "out/Default", "--metrics_project", "some_project"]
     env = {}
     want = [
@@ -200,6 +206,43 @@ def test_apply_telemetry_flags_collector_not_present(mocker: Any) -> None:
     assert got == want
 
 
+@pytest.mark.parametrize("collector_help, ninja_help, collector_present", [
+    pytest.param("some help", "some help", False, id="all_missing"),
+    pytest.param(
+        "some help", "--collector_address", False, id="ninja_flags_present"),
+    pytest.param(
+        "--collector_address", "some help", False,
+        id="collector_flags_present"),
+    pytest.param("--collector_address",
+                 "--collector_address",
+                 True,
+                 id="all_flags_present"),
+])
+def test_apply_telemetry_flags_help_pages(mocker: Any, collector_help: str,
+                                          ninja_help: str,
+                                          collector_present: bool) -> None:
+
+    def mock_subcommand(_: str, subcmd: str) -> str:
+        if subcmd == "collector":
+            return collector_help
+        if subcmd == "ninja":
+            return ninja_help
+        return "some help"
+
+    mocker.patch("siso._subcommand_help", side_effect=mock_subcommand)
+    args = ["ninja", "-C", "out/Default", "--metrics_project", "some_project"]
+    env = {}
+    want = [
+        "ninja", "-C", "out/Default", "--metrics_project", "some_project",
+        "--enable_cloud_monitoring", "--enable_cloud_profiler",
+        "--enable_cloud_trace", "--enable_cloud_logging"
+    ]
+    if collector_present:
+        want.append("--enable_collector")
+    got = siso.apply_telemetry_flags(args, env, "siso_path")
+    assert got == want
+
+
 @pytest.mark.parametrize("args, env, want", [
     pytest.param(
         ["--metrics_project", "proj1"], {}, "proj1", id="metrics_project_arg"),