Skip to content

fix(api): support TimeSeries time ranges tables (schema version 8) in label lookups #3581

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

Open
bcanvural wants to merge 2 commits into
base: main
Choose a base branch
from
Open

fix(api): support TimeSeries time ranges tables (schema version 8) in label lookups #3581

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
564ed42
fix(api): support TimeSeries time ranges tables (schema version 8) in…
bcanvural Nov 3, 2026
445043d
fix(api): log unexpected failures of the time ranges check
bcanvural Nov 6, 2026
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
6 changes: 6 additions & 0 deletions .changeset/time-ranges-label-lookup.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
---
'@hyperdx/api': patch
'@hyperdx/common-utils': patch
---

fix: PromQL label lookups (dashboard variables, label autocomplete) now work on TimeSeries tables of schema version 8, the default from ClickHouse 26.10. These keep each series' time range in a separate time ranges table instead of the tags table's min_time/max_time, so match[] failed with "requires store_min_time_and_max_time = 1" and time-bounded lookups read the whole tags table.
172 changes: 161 additions & 11 deletions packages/api/src/controllers/__tests__/timeseriesEngine.int.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import {
seedTimeSeriesTagsTable,
TimeSeriesFixtureSeries,
} from '@/fixtures';
import logger from '@/utils/logger';

const CONNECTION_ID = 'timeseries-engine-int-test';
const BOUNDED_TABLE = 'test_ts_label_values';
Expand Down Expand Up @@ -319,17 +320,24 @@ describe('timeseriesEngine controller', () => {
// Time bounds are best-effort narrowing, so the request widens instead.
describe('table without min_time/max_time', () => {
it('ignores the bounds instead of failing', async () => {
expect(
await queryLabelValues({
client,
connectionId: CONNECTION_ID,
databaseName: DEFAULT_DATABASE,
tableName: UNBOUNDED_TABLE,
labelName: '__name__',
startMs: sec(RECENT_START_SEC),
endMs: sec(RECENT_START_SEC + HOUR_SEC),
}),
).toEqual(['old_metric', 'recent_metric']);
const warn = jest.spyOn(logger, 'warn');
try {
expect(
await queryLabelValues({
client,
connectionId: CONNECTION_ID,
databaseName: DEFAULT_DATABASE,
tableName: UNBOUNDED_TABLE,
labelName: '__name__',
startMs: sec(RECENT_START_SEC),
endMs: sec(RECENT_START_SEC + HOUR_SEC),
}),
).toEqual(['old_metric', 'recent_metric']);
// No time ranges table either: expected, so not logged.
expect(warn).not.toHaveBeenCalled();
} finally {
warn.mockRestore();
}
});

// A selector needs those columns to evaluate at all. Unlike the bounds it
Expand Down Expand Up @@ -475,4 +483,146 @@ describe('timeseriesEngine controller', () => {
});
});
});

// Schema version 8 (the default from ClickHouse 26.10) moved min_time and
// max_time out of the tags table into a separate time ranges table. Older
// servers don't know the `version` setting, so these run only where the
// table can be created.
describe('table of schema version 8 (time ranges table)', () => {
const V8_TABLE = 'test_ts_label_values_v8';
const V8_NO_RANGES_TABLE = 'test_ts_label_values_v8_no_ranges';
let supported = true;

const v8LabelValues = (
args: Partial<Parameters<typeof queryLabelValues>[0]> & {
labelName: string;
},
) => labelValues({ tableName: V8_TABLE, ...args });

beforeAll(async () => {
try {
await seedTimeSeriesTagsTable({
table: V8_TABLE,
series: SERIES,
withSamples: true,
timeRanges: true,
});
await seedTimeSeriesTagsTable({
table: V8_NO_RANGES_TABLE,
series: SERIES,
storeTimeBounds: false,
timeRanges: true,
});
} catch (e) {
if (!/Unknown setting 'version'/.test(String(e))) throw e;
supported = false;
}
});

afterAll(async () => {
for (const table of [V8_TABLE, V8_NO_RANGES_TABLE]) {
await dropTimeSeriesTable({ table });
}
});

it('scopes label values to the window through the time ranges table', async () => {
if (!supported) return;
expect(
await v8LabelValues({
labelName: 'job',
startMs: sec(RECENT_START_SEC),
endMs: sec(RECENT_START_SEC + HOUR_SEC),
}),
).toEqual(['api', 'web']);
});

it('applies lone bounds against the time ranges', async () => {
if (!supported) return;
expect(
await v8LabelValues({
labelName: '__name__',
endMs: sec(OLD_START_SEC + HOUR_SEC),
}),
).toEqual(['old_metric']);
expect(
await v8LabelValues({
labelName: '__name__',
startMs: sec(RECENT_START_SEC),
}),
).toEqual(['recent_metric']);
});

it('returns nothing for a window no series covers', async () => {
if (!supported) return;
expect(
await v8LabelValues({
labelName: '__name__',
startMs: sec(OLD_START_SEC + 2 * HOUR_SEC),
endMs: sec(OLD_START_SEC + 3 * HOUR_SEC),
}),
).toEqual([]);
});

it('evaluates match[] and narrows it by the window', async () => {
if (!supported) return;
expect(
await v8LabelValues({
labelName: 'job',
match: ['recent_metric{job="api"}', 'old_metric'],
}),
).toEqual(['api', 'batch']);
expect(
await v8LabelValues({
labelName: 'job',
match: ['{job=~".+"}'],
startMs: sec(RECENT_START_SEC),
endMs: sec(RECENT_START_SEC + HOUR_SEC),
}),
).toEqual(['api', 'web']);
});

it('scopes label names to the window and the selector', async () => {
if (!supported) return;
const names = (
args: Partial<Parameters<typeof queryLabelNames>[0]> = {},
) =>
queryLabelNames({
client,
connectionId: CONNECTION_ID,
databaseName: DEFAULT_DATABASE,
tableName: V8_TABLE,
...args,
});
expect(
await names({
startMs: sec(RECENT_START_SEC),
endMs: sec(RECENT_START_SEC + HOUR_SEC),
}),
).toEqual(['__name__', 'job']);
expect(await names({ match: ['old_metric'] })).toEqual([
'__name__',
'job',
'region',
]);
});

it('rejects match[] without a time ranges table, and ignores the bounds', async () => {
if (!supported) return;
await expect(
labelValues({
tableName: V8_NO_RANGES_TABLE,
labelName: 'job',
match: ['recent_metric'],
}),
).rejects.toThrow(/store_time_ranges = 1/);
expect(
await labelValues({
tableName: V8_NO_RANGES_TABLE,
labelName: '__name__',
startMs: sec(RECENT_START_SEC),
endMs: sec(RECENT_START_SEC + HOUR_SEC),
}),
).toEqual(['old_metric', 'recent_metric']);
});
});
});
99 changes: 70 additions & 29 deletions packages/api/src/controllers/timeseriesEngine.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,11 +32,23 @@ export type TimeSeriesTagsQueryArgs = {
signal?: AbortSignal;
};

// 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 =>

@github-actions github-actions Bot Nov 6, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🔵 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

@bcanvural bcanvural Nov 6, 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.

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.

e instanceof Error &&
/Unknown table function timeSeriesTimeRanges|has no TimeRanges target table/.test(
e.message,
);

/**
* Indicates whether the tags inner table associated with the given TimeSeries table
* includes the optional `min_time` and `max_time` columns.
* Where the given TimeSeries table keeps each series' time range, if anywhere:
* - `tags`: the `min_time`/`max_time` columns of the tags inner table (schema
* version 7 or earlier, with `store_min_time_and_max_time` on);
* - `timeRanges`: the time ranges inner table (version 8 and later, with
* `store_time_ranges` on, the default from ClickHouse 26.10).
*/
async function timeSeriesTagsTableHasTimeBounds({
async function timeSeriesTimeBounds({
metadata,
connectionId,
database,
Expand All @@ -46,24 +58,31 @@ async function timeSeriesTagsTableHasTimeBounds({
connectionId: string;
database: string;
table: string;
}): Promise<boolean> {
try {
const columns = await metadata.getTimeSeriesTableColumns({
connectionId,
databaseName: database,
tableName: table,
innerTableType: 'Tags',
});

const columnNames = new Set(columns.map(column => column.name));
return columnNames.has('min_time') && columnNames.has('max_time');
} catch (e) {
logger.warn(
{ err: e, database, table },
'Failed to check if TimeSeries tags table has time bounds columns',
);
return false;
}): Promise<'tags' | 'timeRanges' | undefined> {
for (const [innerTableType, bounds] of [

@github-actions github-actions Bot Nov 6, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🔵 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

@bcanvural bcanvural Nov 6, 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.

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.

['Tags', 'tags'],
['TimeRanges', 'timeRanges'],
] as const) {
try {
const columns = await metadata.getTimeSeriesTableColumns({
connectionId,
databaseName: database,
tableName: table,
innerTableType,
});
const columnNames = new Set(columns.map(column => column.name));
if (columnNames.has('min_time') && columnNames.has('max_time'))
return bounds;
} catch (e) {

@github-actions github-actions Bot Nov 6, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟠 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

@bcanvural bcanvural Nov 6, 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.

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.

@github-actions github-actions Bot Nov 6, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🔵 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

@bcanvural bcanvural Nov 6, 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.

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.

if (innerTableType === 'TimeRanges' && isNoTimeRangesTableError(e))
continue;
logger.warn(
{ err: e, database, table, innerTableType },
'Failed to check if TimeSeries table has time bounds',
);
}
}
return undefined;
}

// `timeSeriesSelector()` requires both bounds, so we define the minimum and maximum possible times
Expand Down Expand Up @@ -122,10 +141,11 @@ async function getSeriesFilterConditions({
const selectors = match?.length ? match : undefined;
if (startMs == null && endMs == null && selectors == null) return [];

// Both filters lean on min_time/max_time, which the tags inner table carries
// only when the table was created with store_min_time_and_max_time on.
// Both filters lean on each series' time range: min_time/max_time in the
// tags inner table up to schema version 7, a separate time ranges inner
// table from version 8.
const metadata = new Metadata(client, new MetadataCache());
const tableHasTimeBounds = await timeSeriesTagsTableHasTimeBounds({
const timeBounds = await timeSeriesTimeBounds({
metadata,
connectionId,
database: databaseName,
Expand All @@ -135,12 +155,12 @@ async function getSeriesFilterConditions({
const conditions: ChSql[] = [];

if (selectors != null) {
// timeSeriesSelector() prunes on min_time/max_time unconditionally, so
// without those columns it cannot run. Unlike the bounds it also cannot
// degrade to "no filter" — that would answer a different question.
if (!tableHasTimeBounds) {
// timeSeriesSelector() prunes on the series' time range, so without one it
// cannot run. Unlike the bounds it also cannot degrade to "no filter" —
// that would answer a different question.
if (timeBounds == null) {
throw new Error(
'match[] requires a TimeSeries table created with store_min_time_and_max_time = 1',
'match[] requires a TimeSeries table that stores time ranges (store_time_ranges = 1, or store_min_time_and_max_time = 1 before version 8)',
);
}
conditions.push(
Expand All @@ -154,7 +174,7 @@ async function getSeriesFilterConditions({
);
}

if (tableHasTimeBounds) {
if (timeBounds === 'tags') {
if (endMs != null)
conditions.push(
chSql`(min_time IS NULL OR min_time <= fromUnixTimestamp64Milli(${{ Int64: endMs }}))`,
Expand All @@ -163,6 +183,27 @@ async function getSeriesFilterConditions({
conditions.push(
chSql`(max_time IS NULL OR max_time >= fromUnixTimestamp64Milli(${{ Int64: startMs }}))`,
);
} else if (timeBounds === 'timeRanges' && selectors == null) {
// The selector above already keeps only series with samples in the
// window. Without one, the time ranges table answers the window; its rows
// are partial aggregates until merged, hence the GROUP BY.
const having: ChSql[] = [];
if (endMs != null)
having.push(
chSql`min(min_time) <= fromUnixTimestamp64Milli(${{ Int64: endMs }})`,
);
if (startMs != null)
having.push(
chSql`max(max_time) >= fromUnixTimestamp64Milli(${{ Int64: startMs }})`,
);
conditions.push(
chSql`id IN (
SELECT id
FROM timeSeriesTimeRanges(${{ Identifier: databaseName }}, ${{ Identifier: tableName }})
GROUP BY id
HAVING ${concatChSql(' AND ', ...having)}
)`,
);
}

return conditions;
Expand Down
Loading
Loading