Skip to content

fix(asap-tools): probe the ClickHouse adapter's readiness endpoint #905

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
milindsrivastava1997 merged 1 commit into from
Oct 15, 2026
Merged

fix(asap-tools): probe the ClickHouse adapter's readiness endpoint #905

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions asap-tools/experiments/constants.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,8 @@
PROMETHEUS_REMOTE_WRITE_RECEIVER_FLAG = "--web.enable-remote-write-receiver"
HTTP_READINESS_CURL_TIMEOUT_SECONDS = 5
QUERY_ENGINE_RUNTIME_INFO_ENDPOINT = "/api/v1/status/runtimeinfo"
# The ClickHouse adapter's runtime-info endpoint; it serves no Prometheus API.
QUERY_ENGINE_CLICKHOUSE_RUNTIME_INFO_ENDPOINT = "/clickhouse/ping"
# Query engine spill files live in this subdirectory of its output dir, which
# the container mounts, so host and container paths agree. Results syncs skip it.
QUERY_ENGINE_SPILL_SUBDIR = "spill"
Expand Down
13 changes: 12 additions & 1 deletion asap-tools/experiments/experiment_utils/services/query_engine.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,14 @@ def get_http_port(self) -> int:
return 8088


def runtime_info_endpoint(backend_type: str) -> str:

@milindsrivastava1997 milindsrivastava1997 Oct 15, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

runtime_info_endpoint() maps only "clickhouse"; every other backend falls through to the Prometheus path. elastic_querydsl / elastic_sql use ElasticHttpAdapter, which only routes runtime info at /_cluster/health (elastic_http.rs:232), so the probe 404s and wait_until_ready still times out after 60 s — the same failure this PR fixes for ClickHouse.

"""The readiness endpoint of the engine's query adapter for `backend_type`:

@milindsrivastava1997 milindsrivastava1997 Oct 15, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Altitude: this duplicates get_runtime_info_path() from the Rust adapters in Python, and it has already drifted (Elastic missing). servers/http.rs registers /metrics on the same router for every adapter — probing that (or having the engine report its path) would remove the mapping entirely and not break when adapters are added/renamed.

the ClickHouse adapter serves no Prometheus API."""
if backend_type == "clickhouse":
return constants.QUERY_ENGINE_CLICKHOUSE_RUNTIME_INFO_ENDPOINT
return constants.QUERY_ENGINE_RUNTIME_INFO_ENDPOINT


class QueryEngineRustService(BaseQueryEngineService):
"""Service for managing the Rust query engine process."""

Expand All @@ -100,6 +108,8 @@ def __init__(
self.remote_spill_dir: Optional[str] = None
self.local_spill_report: Optional[str] = None
self.http_port: Optional[int] = None
# The started backend's adapter's; set by start().

@milindsrivastava1997 milindsrivastava1997 Oct 15, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: the attribute self.runtime_info_endpoint shadows the module-level function runtime_info_endpoint() (line 285 reads like a self-reference), and the comment is missing a noun ("The started backend's adapter's; set by start()."). Consider renaming e.g. readiness_path.

self.runtime_info_endpoint = constants.QUERY_ENGINE_RUNTIME_INFO_ENDPOINT

# ------------------------------------------------------------------
# Config-file helpers (used by both bare-metal and containerized paths)
Expand Down Expand Up @@ -272,6 +282,7 @@ def start(
remote_write_port: Port the precompute engine listens on for Prometheus remote
write; should match streaming.remote_write.base_port (default 8080)
"""
self.runtime_info_endpoint = runtime_info_endpoint(backend_config["type"])

@milindsrivastava1997 milindsrivastava1997 Oct 15, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor: start() now indexes backend_config["type"] as its first statement, so a backend dict without "type" raises a bare KeyError before the config-generation error path. (resolve_backend_config already indexes it, so only direct callers of start() are affected.)

if spill:
self.remote_spill_dir = os.path.join(
experiment_output_dir, "query_engine_output", SPILL_SUBDIR
Expand Down Expand Up @@ -652,7 +663,7 @@ def _is_runtime_info_ready(self) -> bool:
cmd = (
f"curl -fsS --max-time {constants.HTTP_READINESS_CURL_TIMEOUT_SECONDS} "
f"http://127.0.0.1:{self.get_http_port()}"

@milindsrivastava1997 milindsrivastava1997 Oct 15, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The probe uses get_http_port() (hard-coded 8088) rather than the http_port passed to start(); self.http_port is only set when spill=True. If mode_server_urls[sketchdb] (e.g. experiment_run_clickhouse.py:746) uses a non-8088 port, readiness never succeeds regardless of the endpoint fix.

f"{constants.QUERY_ENGINE_RUNTIME_INFO_ENDPOINT} >/dev/null"
f"{self.runtime_info_endpoint} >/dev/null"
)
result = self.provider.execute_command(
node_idx=self.node_offset,
Expand Down
23 changes: 22 additions & 1 deletion asap-tools/experiments/tests/test_victoriametrics_readiness.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,10 @@
from constants import BASELINE_EXPERIMENT_NAME, SKETCHDB_EXPERIMENT_NAME
from experiment_utils.services.base import DockerServiceBase
from experiment_utils.services.docker_victoriametrics import get_remote_write_urls
from experiment_utils.services.query_engine import QueryEngineRustService
from experiment_utils.services.query_engine import (
QueryEngineRustService,
runtime_info_endpoint,
)


class RemoteWriteConfigurationTest(unittest.TestCase):
Expand Down Expand Up @@ -84,6 +87,24 @@ def test_query_engine_requires_runtime_info_endpoint(self):
provider.execute_command.call_args_list[1].kwargs["cmd"],
)

def test_clickhouse_backed_query_engine_probes_its_adapters_endpoint(self):

@milindsrivastava1997 milindsrivastava1997 Oct 15, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: a ClickHouse query-engine readiness test lives in test_victoriametrics_readiness.py, and the Prometheus mapping check is appended to the end of the ClickHouse test — if the ClickHouse asserts fail, the Prometheus check never runs. Consider a separate test (and file).

# It serves /clickhouse/ping and 404s on the Prometheus runtime-info
# path, so probing that one never saw it ready.
provider = Mock()
provider.execute_command.return_value = subprocess.CompletedProcess(
args="curl", returncode=0, stdout="", stderr=""
)
service = QueryEngineRustService(provider, use_container=False, node_offset=0)
service.runtime_info_endpoint = runtime_info_endpoint("clickhouse")

@milindsrivastava1997 milindsrivastava1997 Oct 15, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This sets service.runtime_info_endpoint by hand, so the actual fix — start() picking the endpoint from backend_config["type"] — is untested. Deleting the new line in start() leaves this test passing. Driving start() with a mocked provider + clickhouse backend_config and asserting on the probe command would cover it.


self.assertTrue(service._is_runtime_info_ready())
self.assertIn(
"/clickhouse/ping", provider.execute_command.call_args.kwargs["cmd"]
)
self.assertEqual(
runtime_info_endpoint("prometheus"), "/api/v1/status/runtimeinfo"
)


if __name__ == "__main__":
unittest.main()
Loading