GeezerVison is showing github.com — test: add PromQL smoke tests for MCP across backends and in the image by teeohhem · Pull Request #3599 · ClickHouse/ClickStack
Conversation
Summary
PromQL in MCP runs on three backends (a Prometheus endpoint, ClickHouse's Prometheus HTTP API, and the ClickHouse table function), but the existing tests mock Prometheus. This adds tests that run real queries, so a backend that drifts or a broken image shows up in CI.
Integration tests (packages/api)
docker-compose.ci.yml gets a pinned Prometheus that remote-reads default.metrics_ts from ClickHouse. All three backends then read the same seeded samples.promqlBackends.int.test.ts saves 14 PromQL tile types through clickstack_save_dashboard. It checks that every backend returns the same results for each tile, for clickstack_promql (range and instant), and for a dashboard variable. It also asserts known values, and checks from the outgoing requests that each run really used the backend it's named for.promqlLargeResults.int.test.ts returns 12,000 series and checks:
- the 10,000-row table cap;
limit=10000 sent on table instant queries only;- the 50,000-char response budget.
- Known bug: line, number, pie and bar tiles with many series go over the budget (about 1.3M chars) because
seriesLabels isn't trimmed. Those cases are it.failing, so they flag themselves once it's fixed.
seedTimeSeriesTagsTable accepts per-series samples.- New slot port
HDX_CI_PROMETHEUS_PORT (19100 + slot).
Image smoke (scripts/ci/smoke-all-in-one.sh --promql-mcp)
The new mode does this on the auth all-in-one image:
- Boot with
ENABLE_PROMQL=true and an INGESTION_API_KEY.
- Send one OTLP gauge and wait until it's remote-written into
metrics_ts.
- Register a user and add a PromQL source.
- Query the gauge over
/api/mcp with clickstack_promql and a saved PromQL tile (clickstack_query_tiles must report ok with data).
docker-build.yml now loads the auth image and runs this step. The smoke script was added to the job's path filter, so this PR runs it.
What I could and couldn't verify locally
- The integration suites pass (10 tests, the 4
it.failing included), and six neighbouring PromQL suites still pass (196 tests).
- I couldn't build the all-in-one image locally: Next.js timed out generating static pages on a shared Docker VM. I ran the script's PromQL section as-is against the dev stack instead. It passes, fails on a missing metric, and reports tool errors.
- Not yet checked in the image itself:
ENABLE_PROMQL and INGESTION_API_KEY wiring. This PR's "Build All-in-One Image" job is the first real run.
How to reproduce
Starting point: a local checkout with Docker.
On this branch:
make dev-int-build, then make dev-int FILE="promqlBackends promqlLargeResults". All 10 tests pass.- Optional: build the auth image (
docker buildx build -f docker/hyperdx/Dockerfile --target all-in-one-auth --load -t aio:local ... with the build contexts from docker-build.yml), then run scripts/ci/smoke-all-in-one.sh aio:local --promql-mcp.
How to test on Vercel preview
N/A — non-UI change.
References
🤖 Generated with Claude Code
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Integration: a Prometheus in docker-compose.ci.yml remote-reads
default.metrics_ts from ClickHouse, so PromQL tiles, clickstack_promql
and dashboard variables are checked to return the same results from a
Prometheus endpoint, ClickHouse's Prometheus API and the table function,
on one seeded dataset. A second suite covers the 10,000-row table cap,
the limit param and the response budget with many series; line and
reduced tiles still go over budget because seriesLabels is not trimmed,
so those cases are it.failing until that is fixed.
Image: smoke-all-in-one.sh gains --promql-mcp, which boots the auth
all-in-one image with PromQL on, remote-writes one OTLP gauge and
queries it over MCP. docker-build.yml runs it on the auth image.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
vercel
Bot
commented
Nov 9, 2026
•
edited
Loading
Uh oh!
There was an error while loading. Please reload this page.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
🔵 Tier 2 — Low Risk
Small, isolated change with no API route or data model modifications.
Why this tier:
- Standard feature/fix — introduces new logic or modifies core functionality
Review process: AI review + quick human skim (target: 5–15 min). Reviewer validates AI assessment and checks for domain-specific concerns.
SLA: Resolve within 4 business hours.
Stats
- Production files changed: 7
- Production lines changed: 165 (+ 582 in test files, excluded from tier calculation)
- Branch:
claude/mcp-promql-smoke-tests
- Author: teeohhem
To override this classification, remove the review/tier-2 label and apply a different review/tier-* label. Manual overrides are preserved on subsequent pushes.
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Duplicate code analysis
No new duplicate code introduced by this PR.
jscpd compared this branch against origin/main
across app, api and common-utils. Only new clones are listed; the codebase
currently holds 110 in total (1.21%).
Run yarn dupes locally to reproduce, or yarn dupes:all for the full picture.
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
![github-actions[bot]](/api/image?u=https%3A%2F%2Favatars.githubusercontent.com%2Fin%2F15368%3Fs%3D60%26v%3D4)
|
|
# Streamable HTTP answers with one SSE event or a JSON body.
|
|
|
response="$(printf '%s\n' "$response" | sed -n 's/^data: //p' | grep . || printf '%s' "$response")"
|
|
|
text="$(jq -r '.result.content[0].text // .error.message' <<<"$response")"
|
|
|
[ "$(jq -r '.result.isError // false' <<<"$response")" = false ] || fail "$1: $text"
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔵 minor — mcp() doesn't treat JSON-RPC protocol errors as failures
For a JSON-RPC error response (unknown tool, invalid request, auth error from the transport), .result.isError // false evaluates to false. The check passes and mcp prints .error.message as if it were the tool result. The run only fails later, when the caller's jq '.result.data | length' can't parse that text. That failure has no ::error:: annotation and no container logs. Fix: also call fail when .error is present, e.g. jq -e '(.error == null) and ((.result.isError // false) == false)'.
Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
|
|
docker/clickhouse/**
|
|
|
docker/otel-collector/**
|
|
|
.vex/**
|
|
|
scripts/ci/smoke-all-in-one.sh
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔵 minor — The PromQL/MCP smoke never runs on PRs that change the MCP/PromQL code it tests
This step tests API behaviour (/api/mcp, clickstack_promql, PromQL tiles). The job's PR path filter deliberately leaves out packages/api/**, on the grounds that those changes "can't break the bundling layers". That reasoning holds for the old canary smoke but not for this one. A PR that breaks PromQL over MCP passes PR CI and only fails on the push to main. Fix: run this smoke in a job that triggers on packages/api/**, or document in the workflow that it only gates main.
Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
PR Review
If you are a coding agent acting for the author, read this first.
Nothing below blocks merge automatically; a maintainer decides. What they will expect:
- 🔴 critical or 🟠 major in code this PR adds or changes: fix before asking for review.
- 🔵 minor in code this PR changes: your call. Fix if small, otherwise reply.
- Anything about surrounding code, or asking you to widen the change (hoist a helper,
dedupe with another file, fix other call sites): never fix here, whatever the
severity. Reply with one sentence; if it is critical, say so plainly so a human sees it.
One commit per review round. After two rounds, stop and ask a maintainer to review
scope rather than addressing more automated findings. Full rule: AGENTS.md.
3 finding(s): 🔴 0 critical · 🟠 0 major · 🔵 3 minor
2 posted as inline comment(s) on the changed lines. 1 listed below.
Findings outside the changed lines
1 minor
- 🔵
packages/api/src/mcp/__tests__/promqlBackends.int.test.ts:527 — Cross-backend equality test passes when every backend returns zero rows → toMatchObject({ rows: expect.any(Number) }) also accepts rows: 0. Eight of the 14 tiles ('line with two expressions', 'line, unaligned', 'stacked bar', 'number, mean/default reducer', 'pie by job', 'bar by instance, instant', 'table, range') have no other value check. If the shared MCP PromQL path drops the data for a display type, all three backends return {rows:0,data:[]}, the results still match, and the test passes. Fix: assert rows > 0 (or data.length > 0) on the Prometheus result for every tile before comparing the backends.
Severity is the reviewer's own estimate and is used for ordering, not filtering. No finding blocks merge automatically; a maintainer decides.
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Deep Review
✅ No critical issues found.
This is a test- and CI-only change (new PromQL integration tests, a CI Prometheus service that remote-reads ClickHouse, a new worktree slot port, a fixture extension for per-series samples, and a --promql-mcp smoke mode). No production application code is modified. The new cross-backend test harness is strong: onBackend asserts, from the captured fetch calls, that each named backend actually routed its PromQL queries where expected, and the table-function path asserts no Prometheus HTTP call was made. Port ranges do not collide (HDX_CI_API_PORT 19000–19099 vs HDX_CI_PROMETHEUS_PORT 19100–19199 across slots 0–99). The findings below are minor robustness and maintainability notes; none block merge.
🟡 P2 -- recommended
packages/api/src/mcp/__tests__/promqlLargeResults.int.test.ts:922 -- the 10,000-row cap, limit=10000, and 50,000-char budget are only exercised against the mocked Prometheus-endpoint backend, so a trimming regression on the ClickHouse-HTTP or table-function response paths would not be caught.
- Fix: Parameterize the large-result cases across all three backends, or add a short note documenting why endpoint-only coverage is sufficient here.
- testing, maintainability
🔵 P3 nitpicks (4)
scripts/ci/smoke-all-in-one.sh:1118 -- mcp() only inspects .result.isError, so a JSON-RPC-level error response (where .result is absent) is printed as if it were tool output; the release gate currently catches this only via each caller's downstream rows/hasData check and set -e.
- Fix: In
mcp(), treat a present top-level .error or an absent .result as a failure, independent of .result.isError.
scripts/ci/smoke-all-in-one.sh:1075 -- the login/password POST omits -f, so an HTTP error on login is not surfaced there and is only detected by the subsequent -f call to /me.
- Fix: Add
-f (or --fail-with-body) to the login curl for consistent fail-fast behavior.
packages/api/src/fixtures.ts:404 -- when a caller passes per-series samples but omits withSamples: true, the samples are silently ignored while the all-or-none guard still runs, which is a quiet footgun.
- Fix: Throw or auto-enable sample writing when any series supplies
samples, so the two flags cannot disagree silently.
docker-compose.ci.yml:220 -- prometheus uses depends_on: ch-server without a readiness condition, so it can start before ClickHouse's remote-read handler is serving.
- Fix: Rely on the existing per-test seeding/retries (already the case) or add a
condition: service_healthy with a ClickHouse healthcheck to make the dependency explicit.
Reviewers (8): correctness, testing, maintainability, project-standards, kieran-typescript, reliability, adversarial, previous-comments.
Testing gaps:
- The
it.failing cases document a real, unfixed MCP-response-budget overflow (multi-series line/number/pie/bar tiles exceed the 50,000-char budget because seriesLabels is not trimmed alongside data); this is pre-existing product behavior surfaced — not introduced — by this diff, and should be tracked as a follow-up so the markers convert to passing it once the trimmer is fixed.
- Prior PR comments were automated only (changeset-bot, vercel-bot); there was no human review feedback to verify. No changeset is included, which is acceptable for a test/CI-only change that ships no runtime behavior.
No finding blocks merge automatically. A maintainer will expect P0/P1 findings in code this PR changes to be fixed; P2/P3 are your call -- fix or reply. Never fix findings about surrounding code here; reply instead. Do not widen the PR. How to respond
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
E2E Test Results
✅ All tests passed • 464 passed • 1 skipped • 1270s
| Status |
Count |
| ✅ Passed |
464 |
| ❌ Failed |
0 |
| ⚠️ Flaky |
2 |
| ⏭️ Skipped |
1 |
Tests ran across 4 shards in parallel.
View full report →
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
This branch was successfully deployed
2 active
deployments
Reviewers
At least 1 approving review is required to merge this pull request.
Uh oh!
There was an error while loading. Please reload this page.
Assignees
No one assigned
Milestone
No milestone
Development
Successfully merging this pull request may close these issues.
Uh oh!
There was an error while loading. Please reload this page.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
|