Repository navigation
Conversation
… label lookups ClickHouse 26.10 creates TimeSeries tables of schema version 8 by default, which keep each series' time range in a separate time ranges inner table instead of min_time/max_time in the tags table. The label lookups only looked at the tags table, so match[] was rejected and time bounds were dropped, reading the whole tags table. Read the time ranges table when the tags table has no time bounds. The test fixture pins version 7 for the tags layout where the server knows the setting, and the new tests run where version 8 tables can be created.
|
@bcanvural is attempting to deploy a commit to the HyperDX Team on Vercel. A member of the Team first needs to authorize it. |
🦋 Changeset detectedLatest commit: 445043d The changes in this PR will be included in the next version bump. This PR includes changesets to release 4 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 |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| const columnNames = new Set(columns.map(column => column.name)); | ||
| if (columnNames.has('min_time') && columnNames.has('max_time')) | ||
| return bounds; | ||
| } catch (e) { |
There was a problem hiding this comment.
🟠 major — TimeRanges probe silently swallows every error, so any failure reads as 'table has no time ranges'
The catch treats every DESCRIBE timeSeriesTimeRanges(...) failure as the expected 'no time ranges target' case and does not log it. That includes timeouts, permission errors and network errors. On a version 8 table, a transient failure therefore has two effects. match[] is rejected with the misleading 'requires store_time_ranges = 1' error. Start/end are silently dropped, so the lookup scans the whole tags table, which the PR description says hits the 20 GiB read limit on large tables. Only swallow the specific 'no time ranges table' error code and log or rethrow anything else. Alternatively, choose the inner table with the existing Metadata.getTimeSeriesTableVersion (packages/common-utils/src/core/metadata.ts:714, already used by timeSeriesSamplesColumn in this file) rather than probing until something throws.
Nothing blocks merge automatically, but a maintainer will expect this fixed if it is a real defect in code this PR changes. If it is about surrounding code, reply and say so instead of patching. Do not widen the PR. How to respond
There was a problem hiding this comment.
Fixed in 445043d. Only the errors a table without a time ranges table gives are expected now: servers before 26.10 have no timeSeriesTimeRanges(), newer ones answer "has no TimeRanges target table". Anything else is logged, as main already does for the tags check, and still falls back to no bounds the way that check always has. The existing no-bounds test, which hits the expected error on CI's 26.8, now also asserts that nothing is logged.
I didn't switch to getTimeSeriesTableVersion: it adds a system.tables query to every lookup on version 7 tables, and a version 8 table can still have time ranges off, so the check would stay.
| ); | ||
| return false; | ||
| }): Promise<'tags' | 'timeRanges' | undefined> { | ||
| for (const [innerTableType, bounds] of [ |
There was a problem hiding this comment.
🔵 minor — Each version 8 lookup, and each version 7 lookup with time bounds off, now runs two uncached DESCRIBE queries, and the second can fail on every request
getSeriesFilterConditions builds a new MetadataCache on every call, so the cache never holds anything. A version 8 table now costs two round trips before the real query. A table without bounds also costs a server-side exception on every lookup. Decide which inner table to describe from the table version (getTimeSeriesTableVersion, which reads system.tables once), or describe only the inner table that version implies.
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
There was a problem hiding this comment.
Not changing. A version 7 table with bounds still runs one DESCRIBE, as on main. A version 8 table adds one more, which reads only table metadata, next to a lookup that reads millions of rows. The per-call MetadataCache is main's.
PR Review
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 lines1 minor
Severity is the reviewer's own estimate and is used for ordering, not filtering. No finding blocks merge automatically; a maintainer decides. |
Deep Review✅ No critical issues found. The change is well-contained and defensively written. SQL is parameterized throughout ( 🟡 P2 — recommended
🔵 P3 nitpicks (2)
Reviewers: correctness, testing, maintainability, project-standards, kieran-typescript, performance, api-contract, previous-comments, adversarial. Testing gaps:
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 |
|
445043d logs unexpected failures of the time ranges check (thread on line 67). Not changing:
|
| const columnNames = new Set(columns.map(column => column.name)); | ||
| if (columnNames.has('min_time') && columnNames.has('max_time')) | ||
| return bounds; | ||
| } catch (e) { |
There was a problem hiding this comment.
🔵 minor — When the Tags probe fails, the loop still probes TimeRanges and logs a second warning
If the Tags DESCRIBE throws (table dropped, missing grants, network error), the loop logs a warning and then runs a second DESCRIBE against timeSeriesTimeRanges. That second query fails for the same reason and logs a second warning. On a v7 table with time bounds, a temporary failure on the Tags probe is followed by the 'has no TimeRanges target table' error, which is silently skipped. The user then gets the misleading 'match[] requires … store_time_ranges = 1' error. Return undefined after an unexpected failure on the Tags probe instead of moving on to TimeRanges.
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
There was a problem hiding this comment.
Not changing. After a failed tags check, main already falls back to no bounds and rejects match[]. This adds the time ranges check after it, which warns again only if it also fails for another reason.
| // What a table without a time ranges table answers (version 7 or earlier, or | ||
| // store_time_ranges off): servers before 26.10 have no timeSeriesTimeRanges(), | ||
| // newer ones reject the table. Any other error is unexpected. | ||
| const isNoTimeRangesTableError = (e: unknown): boolean => |
There was a problem hiding this comment.
🔵 minor — Table layout is detected by matching ClickHouse error text when a version lookup already exists
Metadata.getTimeSeriesTableVersion (packages/common-utils/src/core/metadata.ts:714) already reads the table's schema version from system.tables, and this file already uses it in timeSeriesSamplesColumn. Use it to choose which inner table to DESCRIBE: TimeRanges for version >= 8, Tags otherwise. Then the regex on 'Unknown table function timeSeriesTimeRanges|has no TimeRanges target table' is not needed, and it would break silently if ClickHouse rewords either message.
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
There was a problem hiding this comment.
Answered in the thread on line 67: a version 8 table can have time ranges off, so knowing the version doesn't remove the check, and reading it adds a query to every version 7 lookup. If ClickHouse rewords either message, the check logs a warning and falls back as before, and CI's no-bounds test fails on the 26.8 wording.
|
On the latest review, not changing:
|
Summary
Fixes #3580.
ClickHouse 26.10 creates TimeSeries tables of schema version 8 by default. These keep each series' time range in a separate time ranges inner table instead of
min_time/max_timein the tags table. The PromQL label lookups (/labels,/label/:name/values, used by dashboard variables and label autocomplete) looked only at the tags table, so on these tables:match[]was rejected ("requires … store_min_time_and_max_time = 1");This PR:
match[]ontimeSeriesSelector()(which prunes on the time ranges itself), and filters start/end throughid IN (SELECT id FROM timeSeriesTimeRanges(db, table) GROUP BY id HAVING min(min_time) <= end AND max(max_time) >= start). TheGROUP BYis needed because the table holds partial aggregates until its parts merge;store_time_ranges = 0), and names both settings in it;TimeRangestogetTimeSeriesTableColumns' inner table types.Version 7 tables behave as before.
Tests
timeseriesEngine.int.test.ts: a new block runs the window, lone-bound,match[]and label-name cases on a version 8 table, and the reject/ignore cases on a version 8 table without time ranges. These tests return early on servers that don't know theversionsetting (CI's 26.8.36.3).seedTimeSeriesTagsTablegets atimeRangesoption (a version 8 table with a time ranges table), and now pinsversion = 7for the tags layout where the server knows the setting. Without that, every existing TimeSeries suite fails on 26.10, because the default table is version 8 and the fixture insertsmin_time/max_timeinto the tags table.timeseriesEngine,prometheusrouter andqueryAttributionint suites, 160/160 on ClickHouse 26.10.3.7 and on 26.8.36.3. On 26.10 the 6 new tests fail without the fix.Cost (real data, ClickHouse 26.10, 1-hour window, label values for
k8s_namespace_name)match[]viatimeSeriesSelector()On a larger table (about 13 M series, tags table 25 GiB uncompressed), the same lookup with
match[]=upand a 1-hour window reads 1.5 GB in 1.6 s; without a selector it reads the whole tags table and fails on a 20 GiB per-query read limit, with or without the window.The window costs a scan of the time ranges table. An alternative is to route label lookups through ClickHouse's Prometheus HTTP API (as
query/query_rangealready do on 26.6+); its/label/<name>/valueshandlesmatch[]and the window on version 8 tables. Happy to switch to that if you prefer.Not changed:
packages/common-utils/src/__tests__/metadata.int.test.tscreates a table withstore_min_time_and_max_time = 0, which a 26.10 server rejects for a version 8 table. It still passes on CI's 26.8; I left it alone.How to reproduce
Starting point:
yarn dev(the lookups run in the API) with ClickHouse 26.10.On
main:SET allow_experimental_time_series_table = 1; CREATE TABLE default.ts ENGINE = TimeSeries;(version 8), and write a few series to it (e.g. Prometheus remote write).default.ts, and a dashboard variable with amatchselector: "match[] requires a TimeSeries table created with store_min_time_and_max_time = 1". Or callGET /api/prometheus/label/job/values?sourceId=…&match[]=up.On this branch:
How to test on Vercel preview
N/A — API change; the preview runs in local mode without the API.
References