diff --git a/.changeset/fix-filter-facets-behind-proxy.md b/.changeset/fix-filter-facets-behind-proxy.md new file mode 100644 index 0000000000..3460a74ba4 --- /dev/null +++ b/.changeset/fix-filter-facets-behind-proxy.md @@ -0,0 +1,13 @@ +--- +"@hyperdx/common-utils": patch +--- + +Fix filter sidebar values disappearing behind query proxies. Batched facet-value +queries (KV rollup and map text-index lookups) previously bound one query +parameter per key; with ~100 keys this exceeded the ClickHouse web client's URL +parameter budget, silently promoting the request to a multipart/form-data body +that proxy gateways can reject — every LowCardinality-column and map-attribute +filter then vanished without an error. Keys are now inlined as SQL-escaped +literals so the query rides the POST body with a constant parameter count. Also +fixes an operator-precedence bug that applied the KV rollup time filter (and +notEmpty guard) to only the last OR branch. diff --git a/packages/api/jest.int.config.js b/packages/api/jest.int.config.js index add0482623..30584352dd 100644 --- a/packages/api/jest.int.config.js +++ b/packages/api/jest.int.config.js @@ -1,8 +1,29 @@ +const { createJsWithTsPreset } = require('ts-jest'); + const base = require('./jest.config.js'); +// http-proxy-middleware v4 and its deps ship ESM-only builds Jest's CJS +// loader can't parse. jest.setup.ts blanket-mocks the package; the +// clickhouse-proxy int tests need the real thing, so transpile it here +// (`allowJs` for the plain-JS sources, `.mjs` for httpxy's entry). +const esmDepsTransformCfg = createJsWithTsPreset({ + tsconfig: { + rootDir: './src', + allowJs: true, + }, +}); + /** @type {import("jest").Config} **/ module.exports = { ...base, + transform: { + ...esmDepsTransformCfg.transform, + '^.+\\.mjs$': esmDepsTransformCfg.transform['^.+\\.[tj]sx?$'], + }, + moduleFileExtensions: ['js', 'mjs', 'json', 'ts', 'tsx', 'node'], + transformIgnorePatterns: [ + '/node_modules/(?!(http-proxy-middleware|httpxy|is-plain-obj)/)', + ], testMatch: ['**/__tests__/**/*.int.test.ts?(x)'], // Override the unit config's ignore list: it excludes `.int.test.ts`, which // would otherwise hide every integration test from this suite. diff --git a/packages/api/jest.setup.ts b/packages/api/jest.setup.ts index 272998b094..45d8305646 100644 --- a/packages/api/jest.setup.ts +++ b/packages/api/jest.setup.ts @@ -11,7 +11,8 @@ jest.retryTimes( ); // http-proxy-middleware v4 is ESM-only and Jest's CJS module loader cannot -// load ESM packages. Auto-mock since no test exercises the proxy directly. +// load ESM packages. Auto-mock it; the clickhouse-proxy int tests opt out via +// jest.unmock() (jest.int.config.js transpiles the package for them). jest.mock('http-proxy-middleware', () => ({ createProxyMiddleware: jest.fn(() => jest.fn()), })); diff --git a/packages/api/src/routers/api/__tests__/clickhouseProxy.int.test.ts b/packages/api/src/routers/api/__tests__/clickhouseProxy.int.test.ts new file mode 100644 index 0000000000..b658edfd85 --- /dev/null +++ b/packages/api/src/routers/api/__tests__/clickhouseProxy.int.test.ts @@ -0,0 +1,153 @@ +import http from 'http'; + +import { getLoggedInAgent, getServer } from '@/fixtures'; +import Connection from '@/models/connection'; + +// Use the real proxy instead of the blanket mock in jest.setup.ts; the int +// jest config transpiles it (see jest.int.config.js). +jest.unmock('http-proxy-middleware'); + +type CapturedRequest = { + method: string | undefined; + url: string | undefined; + headers: http.IncomingHttpHeaders; + body: Buffer; +}; + +/** + * Pins /clickhouse-proxy body forwarding: text/plain re-injection and the + * raw-stream passthrough of multipart/form-data bodies, which + * `@clickhouse/client-web` emits when query params exceed its URL budget + * (regression context: ClickHouse support-escalation #8482). + * + * Known-latent cases (charset-suffixed JSON, urlencoded) are not covered; + * tracked in https://github.com/hyperdxio/hyperdx/issues/2942. + */ +describe('clickhouse-proxy body forwarding', () => { + const server = getServer(); + + let upstream: http.Server; + let upstreamUrl: string; + let captured: CapturedRequest[] = []; + + beforeAll(async () => { + await server.start(); + + upstream = http.createServer((req, res) => { + const chunks: Buffer[] = []; + req.on('data', chunk => chunks.push(chunk)); + req.on('end', () => { + captured.push({ + method: req.method, + url: req.url, + headers: req.headers, + body: Buffer.concat(chunks), + }); + res.writeHead(200, { 'Content-Type': 'application/json' }); + res.end(JSON.stringify({ data: [] })); + }); + }); + await new Promise(resolve => upstream.listen(0, resolve)); + const address = upstream.address(); + if (address == null || typeof address === 'string') { + throw new Error('Expected upstream server to listen on a TCP port'); + } + upstreamUrl = `http://127.0.0.1:${address.port}`; + }); + + beforeEach(() => { + captured = []; + }); + + afterEach(async () => { + await server.clearDBs(); + }); + + afterAll(async () => { + await new Promise((resolve, reject) => + upstream.close(err => (err ? reject(err) : resolve())), + ); + await server.stop(); + }); + + const createConnection = async (teamId: string) => { + const connection = await Connection.create({ + team: teamId, + name: 'Upstream Echo', + host: upstreamUrl, + username: 'default', + password: '', + }); + return connection._id.toString(); + }; + + it('forwards a text/plain SQL body verbatim', async () => { + const { agent, team } = await getLoggedInAgent(server); + const connectionId = await createConnection(team._id.toString()); + + const sql = 'SELECT count() FROM system.tables FORMAT JSON'; + await agent + .post('/clickhouse-proxy/?query_id=test-text') + .set('x-hyperdx-connection-id', connectionId) + .set('Content-Type', 'text/plain') + .send(sql) + .expect(200); + + expect(captured).toHaveLength(1); + expect(captured[0].body.toString()).toBe(sql); + // Connection credentials are attached server-side. + expect(captured[0].headers['x-clickhouse-user']).toBe('default'); + }); + + it('forwards a large multipart/form-data body byte-for-byte (unparsed stream passthrough)', async () => { + const { agent, team } = await getLoggedInAgent(server); + const connectionId = await createConnection(team._id.toString()); + + // Mirror the client's multipart output: a `query` part plus one + // `param_*` part per bound parameter, past the 4096-byte URL budget. + const boundary = '----clickhouse-js-test-boundary'; + const parts: string[] = [ + `--${boundary}\r\n` + + `Content-Disposition: form-data; name="query"\r\n\r\n` + + `SELECT Key FROM rollup WHERE Key IN ({p0:String}) FORMAT JSON\r\n`, + ]; + for (let i = 0; i < 120; i++) { + parts.push( + `--${boundary}\r\n` + + `Content-Disposition: form-data; name="param_HYPERDX_PARAM_${i}"\r\n\r\n` + + `some.rather.long.map.attribute.key.number.${i}\r\n`, + ); + } + parts.push(`--${boundary}--\r\n`); + const body = parts.join(''); + expect(Buffer.byteLength(body)).toBeGreaterThan(4096); + + await agent + .post('/clickhouse-proxy/?query_id=test-multipart') + .set('x-hyperdx-connection-id', connectionId) + .set('Content-Type', `multipart/form-data; boundary=${boundary}`) + .send(body) + .expect(200); + + expect(captured).toHaveLength(1); + expect(captured[0].headers['content-type']).toBe( + `multipart/form-data; boundary=${boundary}`, + ); + expect(captured[0].body.toString()).toBe(body); + expect(captured[0].headers['content-length']).toBe( + String(Buffer.byteLength(body)), + ); + }); + + it('rejects requests without a connection id header', async () => { + const { agent } = await getLoggedInAgent(server); + + await agent + .post('/clickhouse-proxy/?query_id=test-no-conn') + .set('Content-Type', 'text/plain') + .send('SELECT 1') + .expect(400); + + expect(captured).toHaveLength(0); + }); +}); diff --git a/packages/common-utils/src/__tests__/metadata.test.ts b/packages/common-utils/src/__tests__/metadata.test.ts index f238321e89..8be634c4ba 100644 --- a/packages/common-utils/src/__tests__/metadata.test.ts +++ b/packages/common-utils/src/__tests__/metadata.test.ts @@ -951,7 +951,7 @@ describe('Metadata', () => { ]), ); - jest + const doMVsAggregateSpy = jest .spyOn(metadata as any, 'doMetadataMVsAggregateColumn') .mockImplementation((...args: any[]) => { const columnName = args[1] as string; @@ -959,6 +959,9 @@ describe('Metadata', () => { columnName === 'ServiceName' || columnName === 'SeverityText', ); }); + + // Returned so tests can override the MV-aggregation answer. + return { doMVsAggregateSpy }; }; afterEach(() => { @@ -1020,8 +1023,101 @@ describe('Metadata', () => { expect(sql).toContain('BY ColumnIdentifier, Key'); expect(sql).not.toContain('mergeTreeTextIndex('); expect(Object.values(params)).toContain('otel_logs_kv_rollup_15m'); - expect(Object.values(params)).toContain('ServiceName'); - expect(Object.values(params)).toContain('SeverityText'); + // Keys are inlined as escaped literals, not bound as params — per-key + // params would push large batches into multipart bodies (see #8482). + expect(sql).toContain("Key IN ('ServiceName','SeverityText')"); + expect(Object.values(params)).not.toContain('ServiceName'); + expect(Object.values(params)).not.toContain('SeverityText'); + }); + + it('keeps the KV rollup MV query parameter count independent of the number of keys', async () => { + const { doMVsAggregateSpy } = setupDefaultLogsSchema(); + doMVsAggregateSpy.mockResolvedValue(true); + + const manyKeys = Array.from( + { length: 80 }, + (_, i) => `LogAttributes['some.rather.long.attribute.key.${i}']`, + ); + // Route map keys through the MV (not the text index) so they all land in + // one batched rollup query. + jest + .spyOn(metadata, 'getMapColumnTextIndexes') + .mockResolvedValue(new Map()); + + await metadata.getAllKeyValues({ + ...baseArgs, + keyExpressions: manyKeys, + }); + + const mvCall = (mockClickhouseClient.query as jest.Mock).mock.calls.find( + (c: any[]) => (c[0].query as string).includes('Key IN ('), + ); + expect(mvCall).toBeDefined(); + const params = mvCall![0].query_params as Record; + // A handful of fixed params — never one per key. + expect(Object.keys(params).length).toBeLessThan(10); + const sql = mvCall![0].query as string; + expect(sql).toContain("'some.rather.long.attribute.key.0'"); + expect(sql).toContain("'some.rather.long.attribute.key.79'"); + }); + + it('SQL-escapes inlined rollup keys containing quotes and backslashes', async () => { + const { doMVsAggregateSpy } = setupDefaultLogsSchema(); + doMVsAggregateSpy.mockResolvedValue(true); + jest + .spyOn(metadata, 'getMapColumnTextIndexes') + .mockResolvedValue(new Map()); + + await metadata.getAllKeyValues({ + ...baseArgs, + keyExpressions: ["LogAttributes['it's a key']"], + }); + + const mvCall = (mockClickhouseClient.query as jest.Mock).mock.calls.find( + (c: any[]) => (c[0].query as string).includes('Key IN ('), + ); + expect(mvCall).toBeDefined(); + const sql = mvCall![0].query as string; + // The embedded quote must be escaped, never spliced raw. + expect(sql).toContain("Key IN ('it\\'s a key')"); + expect(sql).not.toContain("Key IN ('it's a key')"); + }); + + it('applies the time filter to every branch of the KV rollup OR chain', async () => { + setupDefaultLogsSchema(); + + await metadata.getAllKeyValues({ + ...baseArgs, + keyExpressions: ['ServiceName', 'SeverityText'], + }); + + const mvCall = (mockClickhouseClient.query as jest.Mock).mock.calls.find( + (c: any[]) => (c[0].query as string).includes('Key IN ('), + ); + expect(mvCall).toBeDefined(); + const sql = (mvCall![0].query as string).replace(/\s+/g, ' '); + // Without parens, `a OR b AND time` binds the time filter to the + // last branch only. + expect(sql).toMatch(/WHERE \(\(ColumnIdentifier = /); + expect(sql).toMatch(/\)\) AND Timestamp >= /); + }); + + it('inlines map text index key prefixes as escaped literals, not params', async () => { + setupDefaultLogsSchema(); + + await metadata.getAllKeyValues({ + ...baseArgs, + keyExpressions: ["LogAttributes['requestId']"], + }); + + const idxCall = (mockClickhouseClient.query as jest.Mock).mock.calls.find( + (c: any[]) => (c[0].query as string).includes('startsWith(token,'), + ); + expect(idxCall).toBeDefined(); + const sql = idxCall![0].query as string; + const params = idxCall![0].query_params as Record; + expect(sql).toContain("startsWith(token, 'requestId=')"); + expect(Object.values(params)).not.toContain('requestId='); }); it('routes columns without any index or MV entry through the getKeyValues fallback (raw table scan)', async () => { @@ -1212,11 +1308,12 @@ describe('Metadata', () => { expect(mapTextIndexCalls.length).toBeGreaterThanOrEqual(2); - const allParamValues = mapTextIndexCalls.flatMap((c: any[]) => - Object.values(c[0].query_params ?? {}), - ); + // Key prefixes are inlined as escaped literals, not per-key params. + const allQueries = mapTextIndexCalls + .map((c: any[]) => c[0].query as string) + .join('\n'); for (let i = 0; i < keyCount; i++) { - expect(allParamValues).toContain(`k${i}=`); + expect(allQueries).toContain(`startsWith(token, 'k${i}=')`); } }); diff --git a/packages/common-utils/src/core/metadata.ts b/packages/common-utils/src/core/metadata.ts index c443ce58f7..d9e96e35e0 100644 --- a/packages/common-utils/src/core/metadata.ts +++ b/packages/common-utils/src/core/metadata.ts @@ -1184,9 +1184,15 @@ export class Metadata { for (const [columnName, info] of queryOptions.entries()) { const orChain = concatChSql( ' OR ', + // Inline keys as SQL-escaped literals, not bind params: ~100 + // per-key params exceed the web client's URL param budget and + // silently switch the request to a multipart body that proxies + // may reject. Keys are ingest-controlled, hence SqlString.escape. info.keys.map( k => - chSql`startsWith(token, ${{ String: `${k}${info.separator}` }})`, + chSql`startsWith(token, ${{ + UNSAFE_RAW_SQL: SqlString.escape(`${k}${info.separator}`), + }})`, ), ); const partsFilter = await this.partsOverlapFilter({ @@ -1358,17 +1364,22 @@ export class Metadata { // this should only be one mv... but we have a for loop in case const branch: ChSql[] = []; for (const [columnName, keys] of entry) { - const sql = chSql`(ColumnIdentifier = ${{ String: columnName }} AND Key IN (${concatChSql( - ',', - keys.map(key => chSql`${{ String: key }}`), - )}))`; + // Inline keys as SQL-escaped literals, not bind params: ~100 + // per-key params exceed the web client's URL param budget and + // silently switch the request to a multipart body that proxies + // may reject. Keys are ingest-controlled, hence SqlString.escape. + const sql = chSql`(ColumnIdentifier = ${{ String: columnName }} AND Key IN (${{ + UNSAFE_RAW_SQL: keys.map(key => SqlString.escape(key)).join(','), + }}))`; branch.push(sql); } + // Parenthesize the OR chain: `a OR b AND timeFilter` would bind the + // time filter (and notEmpty) to the last branch only. const sql = chSql` SELECT * FROM ( SELECT ColumnIdentifier, Key, groupUniqArray(${{ Int32: maxValuesPerKey }})(Value) as Values FROM ${tableExpr({ database: databaseName, table: mvName })} - WHERE ${concatChSql(' OR ', branch)} + WHERE (${concatChSql(' OR ', branch)}) AND ${timeFilter} AND notEmpty(Value) GROUP BY ColumnIdentifier, Key