Repository navigation
feat: upgrade clickhouse versions and use promql remote endpoints - #3142
Conversation
🦋 Changeset detectedLatest commit: 21cfdec The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
github-actions
Bot
added
the
review/tier-4
🔴 Tier 4 — CriticalTouches authentication, tenancy data models, the public API or shipped database config — or substantially changes the query rendering engine, background tasks, the OTel pipeline, image build, or release CI. Why this tier:
Additional context: touches background tasks or the delivery pipeline lightly (12 lines, under the 30-line bar for Tier 4) Review process: Deep review from a domain expert. Synchronous walkthrough may be required. Stats
|
f0b98ef to
9741791
Compare
E2E Test Results✅ All tests passed • 359 passed • 1 skipped • 1560s
Tests ran across 4 shards in parallel. |
Deep ReviewScope: ClickHouse 26.8 upgrade + PromQL routed through ClickHouse's Prometheus HTTP API with a version/handler-aware fallback to the Most previously-flagged blockers are already fixed in the current head: the 🔴 P0/P1 -- must fix
Consequence worth calling out: because 🟡 P2 -- recommended
🔵 P3 nitpicks (5)
Reviewers (9): correctness, security, performance, reliability, api-contract, testing, maintainability, project-standards, previous-comments. Testing gaps:
Note: the shell/git environment was unavailable during this run (bwrap failure), so findings were derived from reading the head revision of the changed files rather than a computed diff; line citations reflect current file state. |
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
PR Review5 finding(s): 🔴 0 critical · 🟠 3 major · 🔵 2 minor 4 posted as inline comment(s) on the changed lines. 1 listed below. Findings outside the changed lines1 minor
Severity is the reviewer's own estimate and is used for ordering, not filtering. |
| }, | ||
| ]; | ||
| expect(formatMatrixResponse(rows as any)).toEqual([ | ||
| { |
There was a problem hiding this comment.
The new formatter test passes rows as any, which violates the repository directive to avoid any and prevents TypeScript from checking that the fixture matches the formatter input. The vector formatter test repeats this at line 44. This repository requirement must be satisfied before merging.
Context Used: CLAUDE.md (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
pulpdrew
left a comment
pulpdrew
left a comment
There was a problem hiding this comment.
LGTM. The HTTP endpoint seems to work correctly locally with my existing source.
Just to confirm, is there anything we need to / can do for the labels and values endpoints?
| * Whether a ClickHouse connection answers PromQL over HTTP. Two conditions, | ||
| * cheapest first: the server is new enough (26.6+), and the prometheus_api_v1 | ||
| * handler is actually configured — a self-managed server needs an | ||
| * `<http_handlers>` rule for that, which a stock install lacks. The handler | ||
| * answers every request, error or not, with Prometheus JSON (`{status, ...}`); | ||
| * a server without it answers a plain-text 404. `format_query` is used as the | ||
| * probe because it is the one endpoint that needs no table. | ||
| * | ||
| * Probed on every request, like the version check, so a handler enabled or | ||
| * removed on the server is reflected immediately. |
There was a problem hiding this comment.
I think it would be great in the future if we could set something on the source config that indicates the HTTP API should be used, instead of having to make an extra probe request on every query. Makes sense that this could be required for backwards compatibility though.
There was a problem hiding this comment.
Yeah I was thinking that too, it could be a setting that someone could alter if they desire. I didn't want to tackle that yet though. Will create a ticket for it
I'm not sure they are fully implemented yet, but we should do that. I'll ticket it |
| return this.cache.getOrFetch<number>( | ||
| `${connectionId}.${databaseName}.${tableName}.getTimeSeriesTableVersion`, | ||
| async () => { | ||
| const sql = chSql`SELECT toUInt32OrZero(extract(engine_full, 'version = (\\d+)')) AS version FROM system.tables WHERE database = ${{ String: databaseName }} AND name = ${{ String: tableName }}`; |
There was a problem hiding this comment.
🟠 major — getTimeSeriesTableVersion regex is under-escaped, so it always returns 0 and the samples branch is unreachable
The template literal 'version = (\\d+)' yields the SQL text 'version = (\d+)', and ClickHouse string literals convert any unrecognised escape \c to c (see the repo's own escapeSqlString/escapeRegexValue usage in packages/common-utils/src/variables.ts:42 and the doubled-backslash expectations in packages/common-utils/src/__tests__/variables.test.ts:907). The regex ClickHouse actually compiles is version = (d+), which never matches version = 3, so extract returns '', toUInt32OrZero returns 0, and timeSeriesSamplesColumn (packages/api/src/controllers/timeseriesEngine.ts:314) always answers time_series. On the exact case this was written for — a stock ClickHouse 26.8 with no <http_handlers> rule, so the probe fails and the fallback runs, holding a version = 3 TimeSeries table — SELECT time_series fails with Unknown identifier and every PromQL chart 400s. Write 'version = (\\\\d+)' (SQL text \\d), or drop the DDL regex entirely and pick the column from metadata.getColumns({databaseName, tableName}) the way timeSeriesTagsTableHasTimeBounds already picks min_time/max_time from a column list. Either way add a test: getTimeSeriesTableVersion currently has none, which is why this passed CI (CI's tables are created without an explicit version, so time_series is right by accident). Also confirm engine_full even carries version = N for a table created by docker/otel-collector/schema/seed/00008_otel_metrics_timeseries.sql, which sets no version.
| client: ClickhouseClient, | ||
| connection: { id: string; host: string; username: string; password?: string }, | ||
| ): Promise<boolean> { | ||
| if ( |
There was a problem hiding this comment.
🟠 major — Capability probe runs uncached on every /query and /query_range, and the version gate is subsumed by the handler probe
clickhouseServesPrometheusHttpApi is called per request and caches nothing: connectionSupportsPrometheusHttpApi builds new Metadata(client, new MetadataCache()) (packages/api/src/controllers/timeseriesEngine.ts:410), so SELECT version() is re-issued every time, then a second fetch to /format_query follows. A dashboard of N PromQL tiles now costs 3N round trips instead of N, and for any connection without the handler (ClickHouse Cloud, or any server whose config.xml the user does not control) both extra trips are pure waste before the fallback runs. The version gate also cannot change the outcome: a server predating prometheus_api_v1 has no such handler, so the probe already returns false — drop connectionSupportsPrometheusHttpApi/supportsPrometheusHttpApi and keep only the probe, memoized per connection id (the shared getMetadata cache, or a small TTL map) rather than re-run per request. Related: the fallback is silent — getServerVersion swallows its error and the probe swallows every exception including a >5s PROBE_TIMEOUT_MS, and backend is 'clickhouse' for both routes, so a healthy 26.8 deployment quietly demoted to the legacy path produces no log line and no metric to distinguish it.
| @@ -0,0 +1,7 @@ | |||
| --- | |||
| '@hyperdx/api': minor | |||
There was a problem hiding this comment.
🟠 major — Changeset omits @hyperdx/common-utils, which this PR changes and the api package now imports new symbols from
The PR adds supportsPrometheusHttpApi (packages/common-utils/src/core/clickhouseVersion.ts:179) and Metadata.getTimeSeriesTableVersion (packages/common-utils/src/core/metadata.ts:618), both imported by packages/api/src/controllers/timeseriesEngine.ts, yet the changeset lists only '@hyperdx/api': minor. packages/api/package.json:24 depends on "@hyperdx/common-utils": "^0.28.1", so without a common-utils entry the published api release resolves a common-utils that lacks these exports. Every other changeset that touches two packages lists both (e.g. .changeset/quote-row-where-identifiers.md, .changeset/stream-metric-names-from-index.md) — add '@hyperdx/common-utils': minor. While editing it, the body says "ClickHouse 26.8+" but the code gates on 26.6 (PROMETHEUS_HTTP_API_MIN), and it does not mention that a self-managed server needs its own <http_handlers> rule or it silently keeps using the table functions.
| password: connection.password, | ||
| requestTimeout: PROMETHEUS_CH_TIMEOUT_MS, | ||
| }); | ||
| const client = newClickhouseClient(connection); |
There was a problem hiding this comment.
🔵 minor — /labels and /label/:name/values stay on the inner-table path the changeset calls non-forward-compatible
The changeset's stated reason for moving off the table functions is that ClickHouse only holds the Prometheus HTTP API forward-compatible while TimeSeries is in preview — and this PR proves it, having to drop all_tags from seedTimeSeriesTagsTable (packages/api/src/fixtures.ts:338) and from packages/common-utils/src/__tests__/metadata.int.test.ts because the 26.5→26.8 bump changed the tags inner table. handleLabelLookup still reads timeSeriesTags()/timeSeriesSelector() columns directly, so the two endpoints most coupled to that schema are the ones left behind, and they will break on the next engine change. The plumbing is already here (proxyToPrometheus takes headers, and database/table are allowlisted), so route these through ${CLICKHOUSE_PROMETHEUS_API_PREFIX}/labels and /label/{name}/values when the probe succeeds, keeping the table-function query as the same fallback.
The apk break that killed the last release came in from outside the repo. The Dockerfile pins floating upstream tags, and Docker Hub re-pointed clickhouse-server:26.8-alpine from ClickHouse 26.8.6 to 26.8.7 around 15:00 UTC, taking apk-tools from 3.0.6 to 3.0.8 with it. Nothing we had would catch that: 21:22 (D-1) PR #3142 Docker Build -> passes, base @9f10a193 (3.0.6) 00:36 nightly -> passes, base @9f10a193 (3.0.6) ~15:01 upstream re-tags 26.8-alpine -> @2bece7c6 (3.0.8) 16:17 merge to main -> no image build 17:48 merge to main -> no image build 19:31 Release -> fails Both gates were green because both ran before the re-tag, and the two merges that followed it built no images: Docker Build only ran on pull requests, and its path filter would have skipped those diffs anyway. The next thing to touch the new base was the release itself. So run Docker Build on pushes to main too, and ignore the path filter there. The filter is right for pull requests, where the question is "did this diff break the image" and staying fast matters; it is the wrong instrument on main, where the question is "does the image still build today". Both all-in-one targets and the end-to-end smoke test run as they already do on pull requests, so a drifted base now shows up as a red build on the next merge rather than in a release hours later. The twelve copies of the changed-files condition collapse into one `gate` step per job, since the condition now has two variables rather than one. Pull-request and workflow_dispatch behaviour is unchanged. Concurrency is keyed on the event as well as the ref: `github.ref` is the same for every push to main, and the existing cancel-in-progress group would have each merge cancel the one before it and leave that commit unverified. Pull requests keep cancel-on-new-push. Also set `pull: true` on the nightly's three image builds and on the all-in-one PR build, so the floating tags are re-resolved against the registry instead of whatever the runner happens to hold. Fresh runners already re-resolve, so this is hygiene rather than the fix — it keeps holding if these ever move to a persistent builder.
## Summary Fixes the otel smoke tests, which fail because #3142 added a prometheus related ClickHouse config property that causes the (deliberately older) ClickHouse compat version used in the otel smoke tests to fail on startup. This is fixed by removing the prometheus setting from the compat container's config. To prevent this in the future, the conditions for running the smoke tests have been widened to include changes to the ClickHouse config. Related: Smoke tests are failing on #3100. ### Screenshots or video No behavior changes. ### How to test on Vercel preview No behavior changes. ### References - Linear Issue: Closes HDX-5434 - Related PRs:
Summary
Upgrades the version of ClickHouse used for our published all-in-one images as well as local development. Alongside that, some prometheus changes were required to make promql work with the newer version. This also introduces using the prometheus api endpoints (semi-stable) rather than
prometheusQueryRange(unstable API across versions while the table is experimental).The prometheus route will default to trying through the prometheus api. If it does not pass the health check, it will ball back to using
prometheusQueryorprometheusQueryRange. The fallback will also do some checks to see the table version of the TimeSeries table and if so, use different labels when fetching the series (thetime_seriescolumn was changed tosamplesfor TimeSeries tables with a version > 2)References