Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
5 changes: 5 additions & 0 deletions .changeset/promql-catalog-tags-table.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@hyperdx/api': patch
---

fix(mcp): `clickstack_describe_source` on a PromQL source over a ClickHouse TimeSeries table reads metric names from the tags table without a time window, so it no longer runs out of memory on large tables. Label names are listed per metric with the new `metricName` argument.
82 changes: 33 additions & 49 deletions packages/api/src/controllers/promql.ts
Original file line number Diff line number Diff line change
Expand Up @@ -256,33 +256,39 @@ async function fetchPrometheusData<T extends z.ZodTypeAny>(

export type PromqlLabelCatalog = {
backend: PromqlBackend;
labelNames: string[];
/** Without `metricName`, only a Prometheus endpoint lists label names. */
labelNames?: string[];
metricNames: string[];
};

/**
* Lists the label names and metric names a PromQL source holds in
* [startMs, endMs], at most `limit` of each. Returns `limit + 1` entries when
* there are more, so callers can tell the list was cut. A ClickHouse source
* reads them through the Prometheus HTTP API where resolveClickhouseLabelBackend
* allows it, and with SQL otherwise.
* Lists the metric names a PromQL source holds, and label names: of
* `metricName` when given, else of every series on a Prometheus endpoint. At
* most `limit` of each; returns `limit + 1` entries when there are more, so
* callers can tell the list was cut.
*
* A Prometheus endpoint answers for [startMs, endMs]. A ClickHouse TimeSeries
* table is read with SQL on its tags table, without the window, whatever
* backend evaluates its queries: the window costs a pass over every series
* id, on the HTTP API too, which large tables can't afford (#3648).
*/
export async function getPromqlLabelCatalog({
teamId,
source,
startMs,
endMs,
metricName,
limit,
signal,
}: {
teamId: string;
source: {
connection: { toString(): string };
from: { databaseName: string; tableName: string };
promqlBackend?: PromqlSourceBackend;
};
startMs: number;

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 — 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

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: the doc comment on getPromqlLabelCatalog says only a Prometheus endpoint answers for the window, and its only caller makes one call for both backends.

endMs: number;
metricName?: string;
limit: number;
signal?: AbortSignal;
}): Promise<PromqlLabelCatalog> {
Expand All @@ -295,53 +301,32 @@ export async function getPromqlLabelCatalog({
throw new Error('Connection not found');
}

const params = {
start: String(startMs / 1000),
end: String(endMs / 1000),
limit: String(limit + 1),
};
const fetchCatalog = (fetchLabels: (path: string) => Promise<string[]>) =>
Promise.all([
fetchLabels('/labels'),
fetchLabels('/label/__name__/values'),
]);

if (connection.isPrometheusEndpoint) {
const [labelNames, metricNames] = await fetchCatalog(path =>
const params = {
start: String(startMs / 1000),
end: String(endMs / 1000),
limit: String(limit + 1),
};
const fetchLabels = (path: string, extra: Record<string, string> = {}) =>
fetchPrometheusData(
connection.host,
`/api/v1${path}`,
params,
{ ...params, ...extra },
labelListSchema,
signal,
);
const [labelNames, metricNames] = await Promise.all([
fetchLabels(
'/labels',
metricName != null
? { 'match[]': `{__name__=${JSON.stringify(metricName)}}` }
: {},
),
);
fetchLabels('/label/__name__/values'),
]);
return { backend: 'prometheus', labelNames, metricNames };
}

const { databaseName, tableName } = source.from;
const labelBackend = await resolveClickhouseLabelBackend({
connection,
databaseName,
tableName,
promqlBackend: source.promqlBackend,
});
if (labelBackend === 'clickhouse-http') {
const [labelNames, metricNames] = await fetchCatalog(path => {
const { upstream, headers } = clickhousePrometheusRequest(connection);
return fetchPrometheusData(
upstream,
`${CLICKHOUSE_PROMETHEUS_API_PREFIX}${path}`,
{ ...params, database: databaseName, table: tableName },
labelListSchema,
signal,
headers,
CLICKHOUSE_HTTP_API_UNAVAILABLE_MESSAGE,
);
});
return { backend: 'clickhouse', labelNames, metricNames };
}

const args = {
client: new ClickhouseClient({
host: connection.host,
Expand All @@ -350,15 +335,14 @@ export async function getPromqlLabelCatalog({
requestTimeout: PROMETHEUS_FETCH_TIMEOUT_MS,
}),
connectionId: connection.id,
databaseName,
tableName,
startMs,
endMs,
databaseName: source.from.databaseName,
tableName: source.from.tableName,
limit: limit + 1,
signal,
};
// No startMs/endMs: without them neither lookup reads the time ranges.
const [labelNames, metricNames] = await Promise.all([
queryLabelNames(args),
metricName != null ? queryLabelNames({ ...args, metricName }) : undefined,
queryLabelValues({ ...args, labelName: '__name__' }),
]);
return { backend: 'clickhouse', labelNames, metricNames };
Expand Down
10 changes: 9 additions & 1 deletion packages/api/src/controllers/timeseriesEngine.ts
Original file line number Diff line number Diff line change
Expand Up @@ -312,12 +312,20 @@ export async function queryLabelValues({
*/
export async function queryLabelNames({
limit,
metricName,
...args
}: TimeSeriesTagsQueryArgs): Promise<string[]> {
}: TimeSeriesTagsQueryArgs & {
/** Only this metric's series; the tags table is sorted by metric name. */
metricName?: string;
}): Promise<string[]> {
// The engine keeps `__name__` in `metric_name`, not `tags`, so it is folded
// back in explicitly.
const value = chSql`arrayJoin(arrayConcat([${{ String: '__name__' }}], mapKeys(${{ Identifier: 'tags' }})))`;
const conditions = await getSeriesFilterConditions(args);
if (metricName != null)
conditions.push(
chSql`${{ Identifier: 'metric_name' }} = ${{ String: metricName }}`,
);

return queryDistinctTagsValues({ ...args, value, conditions, limit });
}
Expand Down
148 changes: 78 additions & 70 deletions packages/api/src/mcp/__tests__/promqlSources.int.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@ import { Source } from '@/models/source';
import { callTool, createTestClient, getFirstText } from './mcpTestUtils';

const mockFetch = jest.mocked(global.fetch);
const { realFetch } = globalThis as unknown as { realFetch: typeof fetch };

const TABLE = 'mcp_promql_sources';

Expand Down Expand Up @@ -80,6 +79,13 @@ describe('MCP PromQL sources', () => {
startSec: nowSec - 600,
endSec: nowSec - 60,
},
// Two days old: outside the old 24-hour catalog window.
{
metricName: 'stale_metric',
tags: { job: 'batch', region: 'eu' },
startSec: nowSec - 3 * 86400,
endSec: nowSec - 2 * 86400,
},
],
withSamples: true,
});
Expand Down Expand Up @@ -167,7 +173,7 @@ describe('MCP PromQL sources', () => {
expect(text).toContain('up');
});

it('describes a TimeSeries table by its label names and metric names', async () => {
it('describes a TimeSeries table by its metric names, with no time window', async () => {
const result = await callTool(client, 'clickstack_describe_source', {
sourceId: promqlSource._id.toString(),
});
Expand All @@ -180,14 +186,62 @@ describe('MCP PromQL sources', () => {
backend: 'clickhouse',
table: `${DEFAULT_DATABASE}.${TABLE}`,
});
expect(output.labelNames).toEqual(['__name__', 'instance', 'job']);
expect(output.metricNames).toEqual(['http_requests_total', 'up']);
expect(output.labelNames).toBeUndefined();
expect(output.metricNames).toEqual([
'http_requests_total',
'stale_metric',
'up',
]);
expect(output.note).toContain('metricName');
expect(output.columns).toBeUndefined();
expect(output.nextSteps.sql).toContain(
`timeSeriesData(${DEFAULT_DATABASE}.${TABLE})`,
);
});

it("lists one metric's label names with metricName", async () => {
const result = await callTool(client, 'clickstack_describe_source', {
sourceId: promqlSource._id.toString(),
metricName: 'stale_metric',
});

const output = JSON.parse(getFirstText(result));
expect(output.labelNames).toEqual(['__name__', 'job', 'region']);
expect(output.metricNames).toContain('stale_metric');
});

it('says so when no series has the metricName', async () => {
const result = await callTool(client, 'clickstack_describe_source', {
sourceId: promqlSource._id.toString(),
metricName: 'no_such_metric',
});

const output = JSON.parse(getFirstText(result));
expect(output.labelNames).toEqual([]);
expect(output.note).toContain('"no_such_metric"');
});

it('passes metricName to a Prometheus endpoint as a match[] selector', async () => {
const source = await createPrometheusSource();
mockFetch.mockImplementation(async () => prometheusResponse(['up']));

await callTool(client, 'clickstack_describe_source', {
sourceId: source._id.toString(),
metricName: 'http.server.duration',
});

const urls = mockFetch.mock.calls.map(
([input]) => new URL(String(input)),
);
const byPath = (path: string) => urls.find(u => u.pathname === path);
expect(byPath('/api/v1/labels')?.searchParams.get('match[]')).toBe(
'{__name__="http.server.duration"}',
);
expect(
byPath('/api/v1/label/__name__/values')?.searchParams.has('match[]'),
).toBe(false);
});

it('describes a Prometheus endpoint source through its label API', async () => {
const source = await createPrometheusSource();
mockFetch.mockImplementation(async input =>
Expand Down Expand Up @@ -225,6 +279,19 @@ describe('MCP PromQL sources', () => {
}
});

it('adds no metric-name note when an endpoint has no labels in the window', async () => {
const source = await createPrometheusSource();
mockFetch.mockImplementation(async () => prometheusResponse([]));

const result = await callTool(client, 'clickstack_describe_source', {
sourceId: source._id.toString(),
});

const output = JSON.parse(getFirstText(result));
expect(output.labelNames).toEqual([]);
expect(output.note).toBeUndefined();
});

it('still returns the source config when the label lookup fails', async () => {
const source = await createPrometheusSource();
mockFetch.mockRejectedValue(new Error('fetch failed'));
Expand Down Expand Up @@ -304,83 +371,24 @@ describe('MCP PromQL sources', () => {
versionSpy = undefined;
});

it("sends the label lookups with the source's table and the connection's credentials", async () => {
// The handler's label lookups join every series as well (#3648).
it('reads names with SQL even where the handler serves label lookups', async () => {
stubServerVersion([26, 10, 0, 0]);
mockFetch.mockImplementation(async input => {
const { pathname } = new URL(String(input));
if (pathname.endsWith('/format_query')) {
return prometheusResponse('up');
}
return prometheusResponse(
pathname.endsWith('/label/__name__/values')
? ['up']
: ['__name__', 'job'],
);
});
mockFetch.mockImplementation(async () => prometheusResponse('up'));

const output = await describeAutoSource();

expect(output.source).toMatchObject({
backend: 'clickhouse',
table: `${DEFAULT_DATABASE}.${TABLE}`,
});
expect(output.labelNames).toEqual(['__name__', 'job']);
expect(output.metricNames).toEqual(['up']);
const urls = labelLookupUrls();
expect(urls.map(u => u.pathname).sort()).toEqual([
`${CLICKHOUSE_PROMETHEUS_API_PREFIX}/label/__name__/values`,
`${CLICKHOUSE_PROMETHEUS_API_PREFIX}/labels`,
expect(output.metricNames).toEqual([
'http_requests_total',
'stale_metric',
'up',
]);
for (const url of urls) {
expect(url.searchParams.getAll('database')).toEqual([
DEFAULT_DATABASE,
]);
expect(url.searchParams.getAll('table')).toEqual([TABLE]);
expect(url.searchParams.get('start')).toBeTruthy();
expect(url.searchParams.get('end')).toBeTruthy();
expect(url.searchParams.get('limit')).toBe('201');
}
for (const [input, init] of mockFetch.mock.calls) {
if (new URL(String(input)).pathname.endsWith('/format_query')) {
continue;
}
expect(init?.headers).toMatchObject({
'X-ClickHouse-User': config.CLICKHOUSE_USER,
'X-ClickHouse-Key': config.CLICKHOUSE_PASSWORD,
'X-ClickHouse-Query-Id': expect.stringMatching(/^hdx-mcp-/),
});
}
});

// 26.8's handler answers label lookups with a 400 ("not implemented").
it('reads labels with SQL on ClickHouse 26.8, whose handler serves only queries', async () => {
stubServerVersion([26, 8, 36, 3]);
mockFetch.mockImplementation(async () => prometheusResponse('up'));

const output = await describeAutoSource();

expect(output.labelNames).toEqual(['__name__', 'instance', 'job']);
expect(output.metricNames).toEqual(['http_requests_total', 'up']);
expect(labelLookupUrls()).toEqual([]);
});

// Needs ClickHouse 26.9+, whose handler serves label lookups.
it('lists labels from the CI ClickHouse handler', async () => {
mockFetch.mockImplementation(realFetch);

const output = await describeAutoSource();

expect(output.labelNames).toEqual(['__name__', 'instance', 'job']);
expect(output.metricNames).toEqual(['http_requests_total', 'up']);
expect(
labelLookupUrls()
.map(u => u.pathname)
.sort(),
).toEqual([
`${CLICKHOUSE_PROMETHEUS_API_PREFIX}/label/__name__/values`,
`${CLICKHOUSE_PROMETHEUS_API_PREFIX}/labels`,
]);
});
});

it('keeps promqlBackend through a describe -> rename -> save round trip', async () => {
Expand Down
Loading