Repository navigation
Conversation
…me window describe_source listed a PromQL source's names for the last 24 hours. On a ClickHouse TimeSeries table the window costs a pass over every series id (the time ranges GROUP BY on the SQL path, a join on the HTTP API), which runs out of memory on large tables. Read metric names from the tags table alone, and a metric's label names with the new metricName argument.
🦋 Changeset detectedLatest commit: be348cb 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 |
|
@bcanvural is attempting to deploy a commit to the HyperDX Team on Vercel. A member of the Team first needs to authorize it. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| 'client' | 'databaseName' | 'tableName' | 'limit' | 'signal' | ||
| > & { metricName?: string }): Promise<string[]> { | ||
| const metric = chSql`${{ Identifier: 'metric_name' }}`; | ||
| if (metricName == null) |
There was a problem hiding this comment.
🔵 minor — queryTimeSeriesNames copies two queries that queryLabelValues and queryLabelNames in this file already run
Without metricName, this runs the same SELECT DISTINCT metric_name … ORDER BY val LIMIT n as queryLabelValues({ labelName: '__name__' }) with no startMs/endMs/match, because getSeriesFilterConditions returns [] when none of them is given (line 164). The arrayJoin(arrayConcat(['__name__'], mapKeys(tags))) expression is also copied word for word from queryLabelNames (line 319). Call queryLabelValues without a window to get the metric names. For the per-metric label names, let queryLabelNames take extra conditions, or pull the expression into a shared constant, so the label-name SQL has one source of truth.
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.
Fixed in be348cb: the catalog calls queryLabelValues({ labelName: '__name__' }) without a window, and queryLabelNames takes an optional metricName. The new function is gone.
| from: { databaseName: string; tableName: string }; | ||
| promqlBackend?: PromqlSourceBackend; | ||
| }; | ||
| startMs: number; |
There was a problem hiding this comment.
🔵 minor — Callers must still pass startMs/endMs to getPromqlLabelCatalog, but ClickHouse sources now ignore them
After this change only the isPrometheusEndpoint branch reads the window. Make startMs/endMs optional, or have the Prometheus caller compute them, so the signature no longer implies that ClickHouse sources are filtered by time. describePromqlSource.ts still computes LABEL_LOOKBACK_MS and passes it for both backends.
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: the doc comment on getPromqlLabelCatalog says only a Prometheus endpoint answers for the window, and its only caller makes one call for both backends.
PR Review
2 finding(s): 🔴 0 critical · 🟠 0 major · 🔵 2 minor 2 posted as inline comment(s) on the changed lines. Severity is the reviewer's own estimate and is used for ordering, not filtering. No finding blocks merge automatically; a maintainer decides. |
Deep ReviewScope: PromQL catalog listing in The core change is sound. The SQL path is correctly parameterized ( 🟡 P2 — recommended
🔵 P3 nitpicks (6)
Reviewers (12): correctness, testing, maintainability, project-standards, kieran-typescript, performance, api-contract, adversarial, security, agent-native, previous-comments, learnings-researcher. 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 |
Without a window, queryLabelValues('__name__') already reads only the tags
table, so call it, and give queryLabelNames an optional metricName instead of
a second copy of its query. Say when no series has the metricName.
|
be348cb reuses the existing label lookups (thread on timeseriesEngine.ts), adds a note when no series has the Not changing:
|
Summary
Fixes #3648 (parts 1 and 2).
clickstack_describe_sourceon a PromQL source listed label and metric names for the last 24 hours. On a ClickHouse TimeSeries table, that window costs a pass over every series id, whatever the window's length: the time rangesGROUP BY idon the SQL path, and a join with every series on ClickHouse's Prometheus HTTP API. On a table with about 217M series every call failed at an 8 GiB memory limit, so agents had no way to find metric names.This PR:
SELECT DISTINCT metric_name FROM timeSeriesTags(...)), with no time window, for every ClickHouse PromQL source, whichever backend evaluates its queries. The result notes that the names include series with no recent samples;metricNameargument: on a ClickHouse table it lists that metric's label names withmetric_name = …, which uses the tags table's sort key. Without it, a ClickHouse source returns metric names only, with a note on how to get label names;/labels), and passesmetricNameto it asmatch[]={__name__="…"}.The label routes (
/labels,/label/:name/values) are unchanged, including the HTTP API path from #3628.ClickHouse #127095 (open) rewrites the handler's label endpoints to read the tags table directly. Once it ships, the catalog could go back to the HTTP API, still without a window: with
start/endand nomatch[], it joins the time ranges table as well.Tests
promqlSources.int.test.ts: metric names come from the whole table (a series two days old is listed);metricNamelists one metric's label names; a Prometheus endpoint getsmetricNameasmatch[]; anautosource on a server whose handler serves label lookups still reads names with SQL. The three tests that checked catalog lookups through the handler are replaced by that one.promqlSources,timeseriesEngine,prometheus,promqlQueryTool): 208/208 on ClickHouse 26.10.3.7.describe_sourcecan't list metric or label names on a large TimeSeries table: both catalog paths read every series id in the table #3648 with 2M series: adescribe_sourcecall takes 11–23 ms (was 656 ms on the SQL path and 3.2 s through the handler). The metric-name query uses 4.3 MiB (was 316 MiB and 811 MiB).tscreports one error incheckAlerts/template.ts, which is already onmain.How to reproduce
Starting point:
yarn devwith ClickHouse 26.10, and a PromQL source on a TimeSeries table with many series (or the repro test in #3648).On
main:clickstack_describe_sourceon the source: on a large table it fails with a memory limit error (partial: true, no names).On this branch:
metricName, it returns that metric's label names.References
describe_sourcecan't list metric or label names on a large TimeSeries table: both catalog paths read every series id in the table #3648match[]pinsmetric_name), ClickHouse #127095