Skip to content
Merged
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
7 changes: 7 additions & 0 deletions .changeset/mcp-promql-sources.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
---
'@hyperdx/api': minor
---

feat: Describe PromQL sources in MCP and hide them when PromQL is disabled

`clickstack_describe_source` now returns the label names and metric names of a PromQL source, from either a ClickHouse TimeSeries table or a Prometheus-compatible endpoint, instead of a column schema. `clickstack_list_sources` and `clickstack_describe_source` show which backend a PromQL source uses, and for TimeSeries tables explain how to read them with `clickstack_sql`. Prometheus endpoint sources no longer show their placeholder table. When `ENABLE_PROMQL` is off, MCP hides PromQL sources from listing, describing and deleting, and `clickstack_save_source` no longer accepts `kind: "promql"`.
7 changes: 7 additions & 0 deletions .changeset/prometheus-proxy-basic-auth.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
---
'@hyperdx/api': patch
---

fix: Authenticate to Prometheus connections whose host includes credentials

A Prometheus connection host such as `http://user:pass@prometheus:9090` made every PromQL query fail with "Failed to reach Prometheus", because `fetch` rejects URLs that carry credentials. The credentials are now sent as a Basic auth header.
206 changes: 206 additions & 0 deletions packages/api/src/controllers/promql.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,206 @@
import { z } from 'zod';

import { ClickhouseClient } from '@/clickhouse';
import { getConnectionById } from '@/controllers/connection';
import {
queryLabelNames,
queryLabelValues,
} from '@/controllers/timeseriesEngine';

type PromqlBackend = 'prometheus' | 'clickhouse';

const PROMETHEUS_FETCH_TIMEOUT_MS = 30_000;

/**
* Join a Connection host with an absolute Prometheus API path.
*
* `new URL('/api/v1/query_range', 'http://host:8481/select/0/prometheus')`
* discards `/select/0/prometheus` because an absolute path replaces the base
* pathname. VictoriaMetrics cluster (and any Prometheus-compatible server
* mounted under a prefix) needs that prefix kept. Host userinfo, query, and
* hash are left untouched.
*
* `path` must be an absolute path (every call site passes a literal starting
* with `/`) -- this is not a general-purpose URL joiner.
*
* @see https://github.com/hyperdxio/hyperdx/issues/3046
*/
export function joinPrometheusUpstreamUrl(
upstreamHost: string,
path: string,
): URL {
const url = new URL(upstreamHost);
// `new URL('prometheus:9090')` succeeds with an opaque path (`prometheus:`
// scheme). The pathname setter is a no-op there, so without this guard the
// helper would return the host unchanged, `fetch` would fail, and the proxy
// would 502 / increment query_errors for a user misconfiguration. Same check
// as clickhouseProxy.ts.
if (url.protocol !== 'http:' && url.protocol !== 'https:') {
throw new TypeError('Connection host must be http(s)');
}
// Strip ALL trailing slashes, not just one -- a host saved with a doubled
// trailing slash (e.g. `http://prom:9090//`) would otherwise leave a `//`
// in the joined path, which most servers treat as a distinct (404) path.
const basePath = url.pathname.replace(/\/+$/, '');
url.pathname = `${basePath}${path}`;
return url;
}

/**
* Removes `user:pass@` from `url` and returns it as a Basic auth header. fetch
* rejects a URL that carries credentials, so they cannot stay in the host.
*/
export function takeBasicAuthHeader(url: URL): Record<string, string> {
if (!url.username && !url.password) return {};
const credentials = `${decodeURIComponent(url.username)}:${decodeURIComponent(url.password)}`;
url.username = '';
url.password = '';
return {
Authorization: `Basic ${Buffer.from(credentials).toString('base64')}`,
};
}

const prometheusBodySchema = z.object({
status: z.string(),
data: z.unknown(),
error: z.string().optional(),
});

const labelListSchema = z.array(z.string());

/**
* GETs a Prometheus API path from an external Prometheus-compatible endpoint
* and returns the response's `data`, checked against `dataSchema`. Errors name
* only the origin and path: the host may carry credentials in its userinfo or
* query string.
*/
async function fetchPrometheusData<T extends z.ZodTypeAny>(
host: string,
path: string,
params: Record<string, string>,
dataSchema: T,
signal?: AbortSignal,
): Promise<z.infer<T>> {
const url = joinPrometheusUpstreamUrl(host, path);
for (const [key, value] of Object.entries(params)) {
url.searchParams.set(key, value);
}
Comment thread
karl-power marked this conversation as resolved.
Resolved
const redactedTarget = `${url.origin}${url.pathname}`;
const timeout = AbortSignal.timeout(PROMETHEUS_FETCH_TIMEOUT_MS);

let resp: Response;
try {
resp = await fetch(url, {
headers: takeBasicAuthHeader(url),
signal: signal ? AbortSignal.any([signal, timeout]) : timeout,
});
Comment thread
greptile-apps[bot] marked this conversation as resolved.
Resolved
} catch (err) {
throw new Error(
err instanceof Error && err.name === 'TimeoutError'
? `Prometheus request to ${redactedTarget} timed out`
: `Failed to reach Prometheus at ${redactedTarget}`,
);
}

const body = prometheusBodySchema.safeParse(
await resp.json().catch(() => undefined),
);
if (!resp.ok || !body.success || body.data.status !== 'success') {
throw new Error(
(body.success && body.data.error) ||
`Prometheus at ${redactedTarget} returned HTTP ${resp.status}`,
);
}
const data = dataSchema.safeParse(body.data.data);
if (!data.success) {
throw new Error(
`Prometheus at ${redactedTarget} returned an unexpected response`,
);
}
return data.data;
}

export type PromqlLabelCatalog = {
backend: PromqlBackend;
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.
*/
export async function getPromqlLabelCatalog({
teamId,
Comment thread
karl-power marked this conversation as resolved.
Resolved
source,
startMs,
endMs,
limit,
signal,
}: {
teamId: string;
source: {
connection: { toString(): string };
from: { databaseName: string; tableName: string };
};
startMs: number;
endMs: number;
limit: number;
signal?: AbortSignal;
}): Promise<PromqlLabelCatalog> {
const connection = await getConnectionById(
teamId,
source.connection.toString(),
true,
);
if (!connection) {
throw new Error('Connection not found');
}

if (connection.isPrometheusEndpoint) {
const params = {
start: String(startMs / 1000),
end: String(endMs / 1000),
limit: String(limit + 1),
};
const [labelNames, metricNames] = await Promise.all([
fetchPrometheusData(
connection.host,
'/api/v1/labels',
params,
labelListSchema,
signal,
),
fetchPrometheusData(
connection.host,
'/api/v1/label/__name__/values',
params,
labelListSchema,
signal,
),
]);
Comment on lines +162 to +182

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.

I expect we might have to tune this on another iteration. For some set with hundreds of thousands of series, the values could be tens to hundreds of megabytes.

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.

Good call, yes. I'll add a PR to cap this somehow at the end of the stack

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.

Addressed here:
#3541

return { backend: 'prometheus', labelNames, metricNames };
}

const args = {
client: new ClickhouseClient({
host: connection.host,
username: connection.username,
password: connection.password,
requestTimeout: PROMETHEUS_FETCH_TIMEOUT_MS,
}),
connectionId: connection.id,
databaseName: source.from.databaseName,
tableName: source.from.tableName,
startMs,
endMs,
limit: limit + 1,
signal,
};
const [labelNames, metricNames] = await Promise.all([
queryLabelNames(args),
queryLabelValues({ ...args, labelName: '__name__' }),
]);
Comment thread
greptile-apps[bot] marked this conversation as resolved.
Resolved
return { backend: 'clickhouse', labelNames, metricNames };
}
5 changes: 5 additions & 0 deletions packages/api/src/controllers/timeseriesEngine.ts
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,8 @@ export type TimeSeriesTagsQueryArgs = {
endMs?: number;
limit?: number;
match?: string[];
/** Cancels the DISTINCT query; the tags-table column lookup is not cancellable. */
signal?: AbortSignal;
};

/**
Expand Down Expand Up @@ -177,13 +179,15 @@ async function queryDistinctTagsValues({
value,
conditions,
limit,
signal,
}: {
client: ClickhouseClient;
databaseName: string;
tableName: string;
value: ChSql;
conditions: ChSql[];
limit?: number;
signal?: AbortSignal;
}): Promise<string[]> {
const where = conditions.length
? chSql`WHERE ${concatChSql(' AND ', ...conditions)}`
Expand All @@ -203,6 +207,7 @@ async function queryDistinctTagsValues({
query: query.sql,
query_params: query.params,
format: 'JSON',
abort_signal: signal,
clickhouse_settings: {
allow_experimental_time_series_table: 1,
max_execution_time: PROMETHEUS_MAX_EXECUTION_SEC,
Expand Down
49 changes: 48 additions & 1 deletion packages/api/src/mcp/__tests__/describeSourceTimeout.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,9 @@ jest.mock('@/clickhouse', () => ({ ClickhouseClient: jest.fn() }));
jest.mock('@hyperdx/common-utils/dist/core/metadata', () => ({
getMetadata: () => mockMetadata,
}));
jest.mock('@/controllers/promql', () => ({
getPromqlLabelCatalog: jest.fn(),
}));
jest.mock('@/mcp/tools/sources/metricNames', () => ({
sampleMetricNamesWithLookback: jest.fn(),
}));
Expand All @@ -23,6 +26,7 @@ jest.mock('@/utils/trimToolResponse', () => ({
import { SourceKind } from '@hyperdx/common-utils/dist/types';

import { getConnectionById } from '@/controllers/connection';
import { getPromqlLabelCatalog } from '@/controllers/promql';
import { getSource } from '@/controllers/sources';
import {
DESCRIBE_BACKSTOP_MS,
Expand Down Expand Up @@ -58,6 +62,13 @@ const metricSource = {
metricTables: { gauge: 'otel_metrics_gauge', sum: 'otel_metrics_sum' },
};

const promqlSource = {
...traceSource,
kind: SourceKind.Promql,
timestampValueExpression: 'timestamp',
from: { databaseName: 'default', tableName: 'metrics_ts' },
};

const never = () => new Promise<never>(() => {});

const rejectOnAbort = (signal: AbortSignal) =>
Expand All @@ -70,7 +81,7 @@ function getHandler() {
null;
registerDescribeSource({
server: {},
context: { teamId: 'team-1', userId: 'user-1' },
context: { teamId: 'team-1', userId: 'user-1', promqlEnabled: true },
registerTool: (_name: string, _config: unknown, h: typeof handler) => {
handler = h;
},
Expand Down Expand Up @@ -353,4 +364,40 @@ describe('clickstack_describe_source deadline', () => {
expect(source.skippedStages).toEqual(['metricNames']);
});
});

describe('PromQL sources', () => {
beforeEach(() => {
jest
.mocked(getSource)
// @ts-expect-error mock source only has the fields describe_source reads
.mockResolvedValue(promqlSource);
});

it('aborts the label lookup at the deadline', async () => {
jest
.mocked(getPromqlLabelCatalog)
.mockImplementation(({ signal }) => rejectOnAbort(signal!));

const result = await runWithClock(DESCRIBE_TIMEOUT_MS);

expect(result.isError).toBeFalsy();
const output = parse(result);
expect(output.partial).toBe(true);
expect(output.source.config).toMatchObject({ kind: SourceKind.Promql });
});

it('returns the source config at the backstop when the lookup hangs', async () => {
jest.mocked(getPromqlLabelCatalog).mockImplementation(never);

const result = await runWithClock(
DESCRIBE_TIMEOUT_MS + DESCRIBE_BACKSTOP_MS,
);

expect(result.isError).toBeFalsy();
const output = parse(result);
expect(output.partial).toBe(true);
expect(output.error).toContain('timed out');
expect(output.source.config).toMatchObject({ kind: SourceKind.Promql });
});
});
});
Loading
Loading