fix(admin): use default HTTP port for by-host connections; don't crash tracing on empty trace

snuba/admin/clickhouse/common.py

@@ -8,6 +8,7 @@
 from snuba import settings
 from snuba.clickhouse.native import ClickhousePool
 from snuba.clusters.cluster import (
+    DEFAULT_CLICKHOUSE_HTTP_PORT,
     ClickhouseClientSettings,
     ClickhouseCluster,
     ClickhouseNode,
@@ -106,9 +107,28 @@ def _build_validated_pool(
     # Go through the shared connection cache so the driver (native vs
     # clickhouse-connect/HTTP) is selected by the runtime config, behind the
     # abstract ClickhousePool type, just like the cluster's own connections.
+    #
+    # Pick the HTTP port for the clickhouse-connect (HTTP) driver. (The native
+    # driver ignores http_port and talks to clickhouse_port directly, so it is
+    # unaffected either way.)
+    #
+    # cluster.get_http_port() is the port of the cluster's configured query
+    # endpoint, which may be a load balancer / proxy on a non-default port. It
+    # is correct *only* when we are connecting to that endpoint — i.e. the query
+    # node, the same host the normal read path reaches on
+    # cluster.get_http_port() (this is what get_ro_query_node_connection, and
+    # thus the tracing/querylog/cardinality tools, rely on). For any other host
+    # — a specific individual node selected by host in the admin tools — that
+    # port does not apply: an individual node serves HTTP on the well-known
+    # default port, so use that instead.
+    query_node = cluster.get_query_node()
+    is_query_node = (
+        clickhouse_host == query_node.host_name and clickhouse_port == query_node.native_port
+    )
+    http_port = cluster.get_http_port() if is_query_node else DEFAULT_CLICKHOUSE_HTTP_PORT
     return connection_cache.get_node_connection(
         client_settings,
-        ClickhouseNode(clickhouse_host, clickhouse_port, http_port=cluster.get_http_port()),
+        ClickhouseNode(clickhouse_host, clickhouse_port, http_port=http_port),
         username,
         password,
         database,

snuba/admin/clickhouse/trace_log_parsing.py

@@ -219,6 +219,13 @@ def summarize_trace_output(raw_trace_logs: str) -> TracingSummary:
     parsed = format_log_to_dict(raw_trace_logs)
 
     summary = TracingSummary({})
+    if not parsed:
+        # No parseable trace lines. This is the normal case for the
+        # clickhouse-connect (HTTP) driver, which does not surface the server's
+        # send_logs_level output, so trace_output is always empty. Return an
+        # empty summary instead of indexing parsed[0], which used to raise
+        # "list index out of range" and 500 the tracing tool.
+        return summary
     query_node = parsed[0]["node_name"]
     summary.query_summaries[query_node] = QuerySummary(query_node, True, parsed[0]["query_id"])
     for line in parsed:

tests/admin/test_system_queries.py

@@ -341,6 +341,131 @@ def test_clusterless_rejects_unvalidated_host(
         )
 
 
+@pytest.mark.parametrize(
+    "helper_name",
+    [
+        "get_ro_node_connection",
+        "get_sudo_node_connection",
+        "get_clusterless_node_connection",
+        "get_ro_clusterless_node_connection",
+    ],
+)
+def test_by_host_connection_uses_default_http_port(helper_name: str) -> None:
+    """
+    These helpers connect to a *specific individual* node by hostname (not the
+    cluster's query endpoint — see test_query_node_connection_uses_cluster_http_port).
+    The cluster's configured http_port belongs to the query endpoint (which may
+    sit behind a proxy/load balancer), not to an individual node, so a by-host
+    HTTP connection must target the node's own ClickHouse HTTP listener — the
+    well-known default port — rather than cluster.get_http_port().
+
+    Regression: the clickhouse-connect (HTTP) driver path passed
+    cluster.get_http_port(), which would send admin by-host traffic to the
+    wrong port. The native driver is unaffected (it uses the native port), but
+    the node we build must carry the default HTTP port for the HTTP driver.
+    """
+    from snuba.admin.clickhouse import common
+    from snuba.clusters.cluster import (
+        DEFAULT_CLICKHOUSE_HTTP_PORT,
+        ClickhouseCluster,
+        connection_cache,
+    )
+
+    helper = getattr(common, helper_name)
+
+    # A port that is deliberately not the well-known default, so the assertions
+    # below distinguish "used the default" from "used the cluster's port" even
+    # if the test cluster happens to be configured with the default port.
+    sentinel_cluster_http_port = 65432
+
+    # Snapshot and restore the module-level connection cache around the call.
+    # The cache key is built from str(StorageKey), which falls back to its repr
+    # ("StorageKey.ERRORS-..."), so matching on a literal prefix is brittle;
+    # snapshot/restore is independent of the key format. Clearing it first also
+    # guarantees the helper builds a fresh node instead of returning a cached
+    # entry, so connection_cache.get_node_connection is actually invoked.
+    saved_connections = dict(common.NODE_CONNECTIONS)
+    common.NODE_CONNECTIONS.clear()
+    try:
+        with (
+            patch.object(common, "_validate_node"),  # treat the host as valid
+            patch.object(
+                ClickhouseCluster,
+                "get_http_port",
+                return_value=sentinel_cluster_http_port,
+            ),
+            patch.object(connection_cache, "get_node_connection") as mock_pool,
+        ):
+            helper(
+                "specific-node.example.com",
+                9000,
+                "errors",
+                ClickhouseClientSettings.QUERY,
+            )
+
+        assert mock_pool.called, "expected a pool to be acquired for a valid host"
+        node = mock_pool.call_args.args[1]
+        assert node.http_port == DEFAULT_CLICKHOUSE_HTTP_PORT
+        assert node.http_port != sentinel_cluster_http_port, (
+            "by-host connections must not use the cluster's configured http_port"
+        )
+    finally:
+        # Restore the cache exactly, so this test never leaks a mocked
+        # connection into others (regardless of the cache key format).
+        common.NODE_CONNECTIONS.clear()
+        common.NODE_CONNECTIONS.update(saved_connections)
+
+
+def test_query_node_connection_uses_cluster_http_port() -> None:
+    """
+    Counterpart to test_by_host_connection_uses_default_http_port: the
+    query-node helper (get_ro_query_node_connection — used by the tracing,
+    querylog and cardinality tools) connects to the cluster's *configured query
+    endpoint*, the same host the normal read path reaches on
+    cluster.get_http_port(). That endpoint may be a load balancer on a
+    non-default HTTP port, so this path must keep using cluster.get_http_port()
+    rather than the by-host default — otherwise the HTTP driver would send those
+    tools to the wrong port.
+    """
+    from snuba.admin.clickhouse import common
+    from snuba.clusters.cluster import ClickhouseCluster, connection_cache
+
+    # A port that is deliberately not the well-known default, so the assertion
+    # below proves the query-node path uses the cluster's configured port and
+    # not the by-host default.
+    sentinel_cluster_http_port = 65432
+
+    # get_ro_query_node_connection caches in CLUSTER_CONNECTIONS, and the
+    # underlying get_ro_node_connection caches in NODE_CONNECTIONS; snapshot and
+    # restore both so the call is exercised and nothing leaks.
+    saved_node = dict(common.NODE_CONNECTIONS)
+    saved_cluster = dict(common.CLUSTER_CONNECTIONS)
+    common.NODE_CONNECTIONS.clear()
+    common.CLUSTER_CONNECTIONS.clear()
+    try:
+        with (
+            patch.object(common, "_validate_node"),  # treat the host as valid
+            patch.object(
+                ClickhouseCluster,
+                "get_http_port",
+                return_value=sentinel_cluster_http_port,
+            ),
+            patch.object(connection_cache, "get_node_connection") as mock_pool,
+        ):
+            common.get_ro_query_node_connection("errors", ClickhouseClientSettings.QUERY)
+
+        assert mock_pool.called, "expected a pool to be acquired for the query node"
+        node = mock_pool.call_args.args[1]
+        assert node.http_port == sentinel_cluster_http_port, (
+            "the query-node connection must use the cluster's configured http_port"
+        )
+    finally:
+        common.NODE_CONNECTIONS.clear()
+        common.NODE_CONNECTIONS.update(saved_node)
+        common.CLUSTER_CONNECTIONS.clear()
+        common.CLUSTER_CONNECTIONS.update(saved_cluster)
+
+
 @pytest.mark.parametrize(
     "sql_query, sudo_mode",
     [

tests/admin/test_trace_log_format.py

@@ -10,6 +10,20 @@
 from tests.fixtures import get_raw_join_query_trace
 
 
+def test_tracing_summary_empty_trace_output() -> None:
+    """
+    The clickhouse-connect (HTTP) driver does not surface the server's
+    send_logs_level output, so trace_output is always empty on that path.
+    Summarizing it must return an empty summary, not raise — it used to
+    IndexError on parsed[0] ("list index out of range"), which 500'd the
+    tracing tool when the HTTP driver was enabled.
+    """
+    assert summarize_trace_output("").query_summaries == {}
+    # Whitespace / lines that don't parse into the expected structure also
+    # yield no summaries rather than raising.
+    assert summarize_trace_output("   \n\n").query_summaries == {}
+
+
 def test_tracing_summary() -> None:
     output = summarize_trace_output(get_raw_join_query_trace())
     assert set(output.query_summaries.keys()) == {