diff --git a/services/intake/.clickhouse-version b/services/intake/.clickhouse-version new file mode 100644 index 0000000000..59c8fb1977 --- /dev/null +++ b/services/intake/.clickhouse-version @@ -0,0 +1 @@ +26.3 diff --git a/services/intake/README.md b/services/intake/README.md index fc02964f24..0306eba666 100644 --- a/services/intake/README.md +++ b/services/intake/README.md @@ -54,6 +54,9 @@ environment instead of package-scoped `uv run --package ...` commands. Prerequisite: Docker must be installed and running locally. +Intake is tested and profiled on ClickHouse 26.3 LTS. Other ClickHouse versions +may not be supported. + Start a local ClickHouse container for span and trace storage: ```bash diff --git a/services/intake/scripts/spans/run_clickhouse.sh b/services/intake/scripts/spans/run_clickhouse.sh index 17829ca9a3..bced9e21cf 100755 --- a/services/intake/scripts/spans/run_clickhouse.sh +++ b/services/intake/scripts/spans/run_clickhouse.sh @@ -5,11 +5,12 @@ set -euo pipefail container_name="nmp-intake-clickhouse" -image="${CLICKHOUSE_IMAGE:-clickhouse/clickhouse-server:24.3}" clickhouse_user="${CLICKHOUSE_USER:-default}" clickhouse_password="${CLICKHOUSE_PASSWORD:-}" script_dir="$(cd -- "$(dirname -- "${BASH_SOURCE[0]}")" && pwd)" repo_root="$(cd -- "${script_dir}/../../../.." && pwd)" +clickhouse_version="$(tr -d '[:space:]' < "${script_dir}/../../.clickhouse-version")" +image="${CLICKHOUSE_IMAGE:-clickhouse/clickhouse-server:${clickhouse_version}}" data_dir="${CLICKHOUSE_DATA_DIR:-${repo_root}/tmp/intake-clickhouse}" ensure_host_dirs() { @@ -21,13 +22,25 @@ ensure_tmp_dir() { docker exec "${container_name}" sh -c "mkdir -p /var/lib/clickhouse/tmp && chown clickhouse:clickhouse /var/lib/clickhouse/tmp" >/dev/null } +ensure_expected_image() { + local actual_image + actual_image="$(docker inspect --format "{{.Config.Image}}" "${container_name}")" + if [[ "${actual_image}" != "${image}" ]]; then + echo "${container_name} uses ${actual_image}, but Intake requires ${image}." >&2 + echo "Remove the existing container after preserving any data you need, then rerun this script." >&2 + exit 1 + fi +} + if docker ps --filter "name=^/${container_name}$" --filter "status=running" --format "{{.Names}}" | grep -qx "${container_name}"; then + ensure_expected_image ensure_tmp_dir echo "${container_name} is already running" exit 0 fi if docker ps -a --filter "name=^/${container_name}$" --format "{{.Names}}" | grep -qx "${container_name}"; then + ensure_expected_image ensure_host_dirs docker start "${container_name}" >/dev/null ensure_tmp_dir diff --git a/services/intake/src/nmp/intake/spans/evaluation_session_repository.py b/services/intake/src/nmp/intake/spans/evaluation_session_repository.py index b809c82589..839072b031 100644 --- a/services/intake/src/nmp/intake/spans/evaluation_session_repository.py +++ b/services/intake/src/nmp/intake/spans/evaluation_session_repository.py @@ -171,7 +171,7 @@ async def list_sessions( if needs_pre_metrics: # Two-query path for cost/tokens sorts. # - # ClickHouse 24.3 inlines CTEs (does not materialise them), so a single query that + # ClickHouse inlines regular CTEs (does not materialise them), so a single query that # references `page_sessions` from multiple downstream CTEs (current_page_spans, # session_metrics, session_scores, final SELECT) would re-execute the expensive # all-session span aggregation once per reference. Splitting into two queries @@ -340,7 +340,7 @@ def _metric_sort_page_ids_sql( ORDER BY + LIMIT/OFFSET to return the ordered (workspace, session_id) pairs for the requested page. Only IDs are returned — row hydration is a separate query. - Why separate: ClickHouse 24.3 inlines CTEs rather than materialising them, so a + Why separate: ClickHouse inlines regular CTEs rather than materialising them, so a single query that references `page_sessions` from multiple CTEs would re-execute the expensive all-session span aggregation once per reference. Returning IDs here and hydrating in _hydrate_by_ids_sql ensures the aggregation runs exactly once. diff --git a/services/intake/tests/integration/spans/conftest.py b/services/intake/tests/integration/spans/conftest.py index 65b1001783..88946c6061 100644 --- a/services/intake/tests/integration/spans/conftest.py +++ b/services/intake/tests/integration/spans/conftest.py @@ -9,6 +9,7 @@ from collections.abc import Callable from datetime import datetime, timezone from importlib.util import find_spec +from pathlib import Path from typing import Any from uuid import uuid4 @@ -23,6 +24,9 @@ ) from nmp.testing import create_test_client +_CLICKHOUSE_VERSION_FILE = Path(__file__).resolve().parents[3] / ".clickhouse-version" +CLICKHOUSE_VERSION = _CLICKHOUSE_VERSION_FILE.read_text(encoding="utf-8").strip() + def _run(coro: Any) -> Any: return asyncio.run(coro) @@ -55,7 +59,7 @@ def clickhouse_container(): from testcontainers.clickhouse import ClickHouseContainer with ClickHouseContainer( - "clickhouse/clickhouse-server:24.3", + f"clickhouse/clickhouse-server:{CLICKHOUSE_VERSION}", username="test", password="test", dbname="default", @@ -63,6 +67,11 @@ def clickhouse_container(): yield container +@pytest.fixture(scope="session") +def clickhouse_version() -> str: + return CLICKHOUSE_VERSION + + @pytest.fixture(scope="session") def clickhouse_settings(clickhouse_container) -> ClickHouseSettings: return ClickHouseSettings( diff --git a/services/intake/tests/integration/spans/test_clickhouse_bootstrap.py b/services/intake/tests/integration/spans/test_clickhouse_bootstrap.py index db26401b5b..9d6149b60e 100644 --- a/services/intake/tests/integration/spans/test_clickhouse_bootstrap.py +++ b/services/intake/tests/integration/spans/test_clickhouse_bootstrap.py @@ -11,6 +11,17 @@ from nmp.intake.spans.clickhouse_client import ClickHouseSpanClient, bootstrap_schema +def test_clickhouse_server_matches_supported_lts( + clickhouse_client: ClickHouseSpanClient, + clickhouse_version: str, + run_async, +) -> None: + result = run_async(clickhouse_client.query("SELECT version()")) + + assert len(result.result_rows) == 1 + assert str(result.result_rows[0][0]).startswith(f"{clickhouse_version}.") + + def test_clickhouse_bootstrap_is_idempotent(clickhouse_client: ClickHouseSpanClient, run_async): run_async(bootstrap_schema(clickhouse_client)) run_async(bootstrap_schema(clickhouse_client))