From 6879a8a02f64037f1f3bcb78624a3402f98b9d41 Mon Sep 17 00:00:00 2001 From: Aditya Reddy <82123567+adityareddy-dev@users.noreply.github.com> Date: Mon, 9 Nov 2026 02:10:17 -0600 Subject: [PATCH 1/3] feat(node): add ClickHouse instrumentation --- .../node-integration-tests/package.json | 1 + .../tracing/clickhouse/docker-compose.yml | 20 ++ .../suites/tracing/clickhouse/instrument.mjs | 9 + .../suites/tracing/clickhouse/scenario.mjs | 61 ++++ .../suites/tracing/clickhouse/test.ts | 116 +++++++ packages/astro/src/index.server.ts | 1 + packages/aws-serverless/src/index.ts | 1 + packages/bun/src/index.ts | 1 + packages/cloudflare/src/index.ts | 1 + packages/deno/src/index.ts | 1 + packages/elysia/src/index.ts | 1 + packages/google-cloud-serverless/src/index.ts | 1 + packages/node/src/index.ts | 1 + packages/server-utils/src/index.ts | 1 + .../src/integrations/clickhouse.ts | 155 +++++++++ .../server-utils/src/integrations/index.ts | 2 + .../server-utils/src/orchestrion/channels.ts | 2 + .../config/channel-integration-definitions.ts | 1 + .../src/orchestrion/config/clickhouse.ts | 44 +++ .../src/orchestrion/config/index.ts | 2 + .../server-utils/src/utils/sql-sanitizer.ts | 296 ++++++++++++++++++ packages/server-utils/src/utils/sql.ts | 242 +------------- .../test/integrations/clickhouse.test.ts | 256 +++++++++++++++ packages/server-utils/test/utils/sql.test.ts | 36 +++ packages/solidstart/src/server/index.ts | 1 + packages/sveltekit/src/server/index.ts | 1 + pnpm-lock.yaml | 9 + 27 files changed, 1026 insertions(+), 237 deletions(-) create mode 100644 dev-packages/node-integration-tests/suites/tracing/clickhouse/docker-compose.yml create mode 100644 dev-packages/node-integration-tests/suites/tracing/clickhouse/instrument.mjs create mode 100644 dev-packages/node-integration-tests/suites/tracing/clickhouse/scenario.mjs create mode 100644 dev-packages/node-integration-tests/suites/tracing/clickhouse/test.ts create mode 100644 packages/server-utils/src/integrations/clickhouse.ts create mode 100644 packages/server-utils/src/orchestrion/config/clickhouse.ts create mode 100644 packages/server-utils/src/utils/sql-sanitizer.ts create mode 100644 packages/server-utils/test/integrations/clickhouse.test.ts diff --git a/dev-packages/node-integration-tests/package.json b/dev-packages/node-integration-tests/package.json index e801ce94c331..9ea56710d931 100644 --- a/dev-packages/node-integration-tests/package.json +++ b/dev-packages/node-integration-tests/package.json @@ -36,6 +36,7 @@ "@aws-sdk/client-sfn": "^3.1161.0", "@aws-sdk/client-sns": "^3.1161.0", "@aws-sdk/client-sqs": "^3.1161.0", + "@clickhouse/client": "1.25.0", "@earendil-works/chord": "^1.4.0", "@earendil-works/pi-ai": "^1.4.0", "@earendil-works/pi-durable": "^1.4.0", diff --git a/dev-packages/node-integration-tests/suites/tracing/clickhouse/docker-compose.yml b/dev-packages/node-integration-tests/suites/tracing/clickhouse/docker-compose.yml new file mode 100644 index 000000000000..dfc23f3c1827 --- /dev/null +++ b/dev-packages/node-integration-tests/suites/tracing/clickhouse/docker-compose.yml @@ -0,0 +1,20 @@ +services: + db: + image: clickhouse/clickhouse-server:25.8 + restart: always + ports: + - '8123:8123' + environment: + CLICKHOUSE_DB: tests + CLICKHOUSE_USER: test + CLICKHOUSE_PASSWORD: test + ulimits: + nofile: + soft: 262144 + hard: 262144 + healthcheck: + test: ['CMD-SHELL', 'clickhouse-client --user test --password test --query "SELECT 1"'] + interval: 2s + timeout: 3s + retries: 30 + start_period: 5s diff --git a/dev-packages/node-integration-tests/suites/tracing/clickhouse/instrument.mjs b/dev-packages/node-integration-tests/suites/tracing/clickhouse/instrument.mjs new file mode 100644 index 000000000000..46a27dd03b74 --- /dev/null +++ b/dev-packages/node-integration-tests/suites/tracing/clickhouse/instrument.mjs @@ -0,0 +1,9 @@ +import * as Sentry from '@sentry/node'; +import { loggingTransport } from '@sentry-internal/node-integration-tests'; + +Sentry.init({ + dsn: 'https://public@dsn.ingest.sentry.io/1337', + release: '1.0', + tracesSampleRate: 1.0, + transport: loggingTransport, +}); diff --git a/dev-packages/node-integration-tests/suites/tracing/clickhouse/scenario.mjs b/dev-packages/node-integration-tests/suites/tracing/clickhouse/scenario.mjs new file mode 100644 index 000000000000..01fc63ba13ae --- /dev/null +++ b/dev-packages/node-integration-tests/suites/tracing/clickhouse/scenario.mjs @@ -0,0 +1,61 @@ +import * as Sentry from '@sentry/node'; +import { waitForConnection } from '@sentry-internal/node-integration-tests'; +import { createClient } from '@clickhouse/client'; + +const client = createClient({ + url: 'http://localhost:8123', + username: 'test', + password: 'test', + database: 'tests', +}); + +async function run() { + // `ping` isn't instrumented, so the probe adds no spans. + await waitForConnection(async () => { + const result = await client.ping(); + if (!result.success) { + throw result.error; + } + }); + + await Sentry.startSpan({ name: 'Test Span', op: 'test' }, async () => { + try { + await client.command({ + query: 'CREATE TABLE IF NOT EXISTS events (id UInt32, name String) ENGINE = MergeTree ORDER BY id', + }); + + await client.insert({ table: 'events', values: [{ id: 1, name: 'signup' }], format: 'JSONEachRow' }); + await client.command({ query: 'INSERT INTO events FORMAT CSV\n2,inline-sensitive@example.com' }); + + const literal = await client.query({ + query: "SELECT name FROM events WHERE name = 'signup'", + format: 'JSONEachRow', + }); + await literal.json(); + + const parameterized = await client.query({ + query: 'SELECT name FROM events WHERE id = {id:UInt32}', + query_params: { id: 1 }, + format: 'JSONEachRow', + }); + await parameterized.json(); + + const { stream } = await client.exec({ query: 'SELECT count() FROM events FORMAT JSONEachRow' }); + for await (const _chunk of stream) { + // drain the response so the socket is released + } + + // Failed calls the caller handles: each should leave an errored span and no error event. + await client.query({ query: 'SELECT * FROM does_not_exist' }).catch(() => {}); + await client.command({ query: 'DROP TABLE does_not_exist' }).catch(() => {}); + await client.exec({ query: 'SELECT * FROM does_not_exist FORMAT JSONEachRow' }).catch(() => {}); + await client.insert({ table: 'does_not_exist', values: [{ id: 1 }], format: 'JSONEachRow' }).catch(() => {}); + } finally { + await client.command({ query: 'DROP TABLE IF EXISTS events' }); + } + }); + + await client.close(); +} + +run(); diff --git a/dev-packages/node-integration-tests/suites/tracing/clickhouse/test.ts b/dev-packages/node-integration-tests/suites/tracing/clickhouse/test.ts new file mode 100644 index 000000000000..d91dcf29bf39 --- /dev/null +++ b/dev-packages/node-integration-tests/suites/tracing/clickhouse/test.ts @@ -0,0 +1,116 @@ +import type { SerializedStreamedSpanContainer } from '@sentry/core'; +import { + DB_COLLECTION_NAME, + DB_NAMESPACE, + DB_OPERATION_NAME, + DB_QUERY_SUMMARY, + DB_QUERY_TEXT, + DB_SYSTEM_NAME, + DB_USER, + SENTRY_KIND, + SENTRY_OP, + SENTRY_ORIGIN, + SERVER_ADDRESS, + SERVER_PORT, +} from '@sentry/conventions/attributes'; +import { DB } from '@sentry/conventions/op'; +import { afterAll, describe, expect } from 'vitest'; +import { streamedAttribute as attr } from '../../../utils'; +import { cleanupChildProcesses, createEsmAndCjsTests, describeWithDockerCompose } from '../../../utils/runner'; + +const ORIGIN = 'auto.db.clickhouse'; + +const CONNECTION_ATTRIBUTES = { + [SENTRY_OP]: attr(DB), + [SENTRY_KIND]: attr('client'), + [SENTRY_ORIGIN]: attr(ORIGIN), + [DB_SYSTEM_NAME]: attr('clickhouse'), + [DB_NAMESPACE]: attr('tests'), + [DB_USER]: attr('test'), + [SERVER_ADDRESS]: attr('localhost'), + [SERVER_PORT]: attr(8123, 'integer'), +}; + +const CREATE_TABLE = 'CREATE TABLE IF NOT EXISTS events (id UInt32, name String) ENGINE = MergeTree ORDER BY id'; + +// One entry per instrumented call, in the order the scenario makes them. +const EXPECTED_SPANS = [ + { name: 'CREATE TABLE events', status: 'ok', operation: 'CREATE', text: CREATE_TABLE }, + { name: 'INSERT events', status: 'ok', operation: 'INSERT', text: undefined, collection: 'events' }, + { name: 'INSERT events', status: 'ok', operation: 'INSERT', text: 'INSERT INTO events' }, + // the string literal is sanitized out of the statement + { name: 'SELECT events', status: 'ok', operation: 'SELECT', text: 'SELECT name FROM events WHERE name = ?' }, + // `{name:Type}` query parameters are kept as written + { name: 'SELECT events', status: 'ok', operation: 'SELECT', text: 'SELECT name FROM events WHERE id = {id:UInt32}' }, + { name: 'SELECT events', status: 'ok', operation: 'SELECT', text: 'SELECT count() FROM events FORMAT JSONEachRow' }, + // handled failures, one per method + { name: 'SELECT does_not_exist', status: 'error', operation: 'SELECT', text: 'SELECT * FROM does_not_exist' }, + { name: 'DROP TABLE does_not_exist', status: 'error', operation: 'DROP', text: 'DROP TABLE does_not_exist' }, + { + name: 'SELECT does_not_exist', + status: 'error', + operation: 'SELECT', + text: 'SELECT * FROM does_not_exist FORMAT JSONEachRow', + }, + { + name: 'INSERT does_not_exist', + status: 'error', + operation: 'INSERT', + text: undefined, + collection: 'does_not_exist', + }, + { name: 'DROP TABLE events', status: 'ok', operation: 'DROP', text: 'DROP TABLE IF EXISTS events' }, +]; + +function assertClickhouseSpans(container: SerializedStreamedSpanContainer): void { + const segment = container.items.find(span => span.is_segment); + expect(segment?.name).toBe('Test Span'); + + const spans = container.items.filter(span => span.attributes[SENTRY_ORIGIN]?.value === ORIGIN); + for (const span of spans) { + expect(span.parent_span_id).toBe(segment?.span_id); + expect(span.attributes).toEqual(expect.objectContaining(CONNECTION_ATTRIBUTES)); + } + + expect( + spans.map(span => ({ + name: span.name, + status: span.status, + operation: span.attributes[DB_OPERATION_NAME]?.value, + text: span.attributes[DB_QUERY_TEXT]?.value, + ...(span.attributes[DB_COLLECTION_NAME] ? { collection: span.attributes[DB_COLLECTION_NAME].value } : {}), + })), + ).toEqual(EXPECTED_SPANS); + expect(spans.map(span => span.attributes[DB_QUERY_SUMMARY]?.value)).toEqual(EXPECTED_SPANS.map(span => span.name)); +} + +describeWithDockerCompose('clickhouse auto instrumentation', { workingDirectory: [__dirname] }, () => { + afterAll(() => { + cleanupChildProcesses(); + }); + + // The span container is the only envelope expected. A captured error would arrive as an `event` + // envelope ahead of it and fail the run, so this also checks that handled failures aren't captured. + describe('@clickhouse/client >=1.23', () => { + createEsmAndCjsTests(__dirname, 'scenario.mjs', 'instrument.mjs', (createTestRunner, test) => { + test('instruments `query`, `command`, `exec` and `insert`', { timeout: 90_000 }, async () => { + await createTestRunner().expect({ span: assertClickhouseSpans }).start().completed(); + }); + }); + }); + + // Before 1.23.0 the client lives in the separate `@clickhouse/client-common` package. + describe('@clickhouse/client <1.23', () => { + createEsmAndCjsTests( + __dirname, + 'scenario.mjs', + 'instrument.mjs', + (createTestRunner, test) => { + test('instruments `query`, `command`, `exec` and `insert`', { timeout: 90_000 }, async () => { + await createTestRunner().expect({ span: assertClickhouseSpans }).start().completed(); + }); + }, + { additionalDependencies: { '@clickhouse/client': '1.22.0' } }, + ); + }); +}); diff --git a/packages/astro/src/index.server.ts b/packages/astro/src/index.server.ts index f407a5723a5b..6b260b21764a 100644 --- a/packages/astro/src/index.server.ts +++ b/packages/astro/src/index.server.ts @@ -118,6 +118,7 @@ export { postgresIntegration, postgresJsIntegration, neonIntegration, + clickhouseIntegration, prismaIntegration, openTelemetryIntegration, getOtlpTracesEndpoint, diff --git a/packages/aws-serverless/src/index.ts b/packages/aws-serverless/src/index.ts index f7310775b554..694bb384654c 100644 --- a/packages/aws-serverless/src/index.ts +++ b/packages/aws-serverless/src/index.ts @@ -137,6 +137,7 @@ export { postgresIntegration, postgresJsIntegration, neonIntegration, + clickhouseIntegration, processSessionIntegration, prismaIntegration, openTelemetryIntegration, diff --git a/packages/bun/src/index.ts b/packages/bun/src/index.ts index 678df401163e..9c5bc79d1ed6 100644 --- a/packages/bun/src/index.ts +++ b/packages/bun/src/index.ts @@ -160,6 +160,7 @@ export { postgresIntegration, postgresJsIntegration, neonIntegration, + clickhouseIntegration, prismaIntegration, openTelemetryIntegration, getOtlpTracesEndpoint, diff --git a/packages/cloudflare/src/index.ts b/packages/cloudflare/src/index.ts index 60a32d7c5bbf..81a0d61ba9ad 100644 --- a/packages/cloudflare/src/index.ts +++ b/packages/cloudflare/src/index.ts @@ -191,6 +191,7 @@ export { mysql2Integration, mysqlIntegration, neonIntegration, + clickhouseIntegration, nitroIntegration, openAIIntegration, piDurableIntegration, diff --git a/packages/deno/src/index.ts b/packages/deno/src/index.ts index 4cf53f81d690..a286d05cf8ba 100644 --- a/packages/deno/src/index.ts +++ b/packages/deno/src/index.ts @@ -195,6 +195,7 @@ export { postgresIntegration, postgresJsIntegration, neonIntegration, + clickhouseIntegration, cronJobIntegration, nodeCronIntegration, nodeScheduleIntegration, diff --git a/packages/elysia/src/index.ts b/packages/elysia/src/index.ts index 107545f0cb43..c11a9c227481 100644 --- a/packages/elysia/src/index.ts +++ b/packages/elysia/src/index.ts @@ -134,6 +134,7 @@ export { postgresIntegration, postgresJsIntegration, neonIntegration, + clickhouseIntegration, prismaIntegration, processSessionIntegration, hapiIntegration, diff --git a/packages/google-cloud-serverless/src/index.ts b/packages/google-cloud-serverless/src/index.ts index 107674b83b95..b8676633639b 100644 --- a/packages/google-cloud-serverless/src/index.ts +++ b/packages/google-cloud-serverless/src/index.ts @@ -137,6 +137,7 @@ export { postgresIntegration, postgresJsIntegration, neonIntegration, + clickhouseIntegration, prismaIntegration, openTelemetryIntegration, getOtlpTracesEndpoint, diff --git a/packages/node/src/index.ts b/packages/node/src/index.ts index 0e0250f6c432..3500821e8114 100644 --- a/packages/node/src/index.ts +++ b/packages/node/src/index.ts @@ -48,6 +48,7 @@ export { postgresIntegration, postgresJsIntegration, neonIntegration, + clickhouseIntegration, redisIntegration, tediousIntegration, vercelAIIntegration, diff --git a/packages/server-utils/src/index.ts b/packages/server-utils/src/index.ts index 77c1fd37c2ea..c649d7cb9a79 100644 --- a/packages/server-utils/src/index.ts +++ b/packages/server-utils/src/index.ts @@ -80,6 +80,7 @@ export { openAIIntegration } from './integrations/openai'; export { postgresIntegration } from './integrations/pg'; export { postgresJsIntegration } from './integrations/postgresjs'; export { neonIntegration } from './integrations/neon'; +export { clickhouseIntegration } from './integrations/clickhouse'; export { tediousIntegration } from './integrations/tedious'; export { vercelAIIntegration } from './integrations/vercel-ai'; export { expressIntegration } from './integrations/express'; diff --git a/packages/server-utils/src/integrations/clickhouse.ts b/packages/server-utils/src/integrations/clickhouse.ts new file mode 100644 index 000000000000..bb3e5232caff --- /dev/null +++ b/packages/server-utils/src/integrations/clickhouse.ts @@ -0,0 +1,155 @@ +import * as diagnosticsChannel from '../utils/diagnosticsChannel'; +import { + DB_COLLECTION_NAME, + DB_NAMESPACE, + DB_OPERATION_NAME, + DB_QUERY_SUMMARY, + DB_SYSTEM_NAME, + DB_USER, + SENTRY_KIND, + SENTRY_OP, + SENTRY_ORIGIN, + SERVER_ADDRESS, + SERVER_PORT, +} from '@sentry/conventions/attributes'; +import { DB } from '@sentry/conventions/op'; +import type { IntegrationFn, SpanAttributes } from '@sentry/core'; +import { defineIntegration, startInactiveSpan } from '@sentry/core'; +import type { OrchestrionChannelContext } from '../orchestrion/types'; +import { CHANNELS } from '../orchestrion/channels'; +import { clickhouseModuleNames } from '../orchestrion/config/clickhouse'; +import { invokeOrchestrionInstrumentation } from '../orchestrion/instrumentation'; +import { bindTracingChannelToSpan } from '../tracing-channel'; +import { getLowCardinalitySqlSpanName, getSqlQueryAttributes } from '../utils/sql'; + +const INTEGRATION_NAME = 'ClickHouse' as const; +const ORIGIN = 'auto.db.clickhouse'; +const DB_SYSTEM_CLICKHOUSE = 'clickhouse'; + +type ClickHouseMethod = 'query' | 'command' | 'exec' | 'insert'; + +// The parts of `ClickHouseClient`'s `connectionParams` the span reads. `>=1.10` moved the username +// into `auth` (which can hold an access token instead); older versions keep it at the top level. +interface ClickHouseConnectionParams { + url?: URL; + database?: string; + username?: string; + auth?: { username?: string }; +} + +interface ClickHouseClient { + connectionParams?: ClickHouseConnectionParams; +} + +// `query`/`command`/`exec` take `{ query }`, `insert` takes `{ table, values }`. Every method accepts a +// per-call `auth` override. +interface ClickHouseParams { + query?: unknown; + table?: unknown; + values?: unknown; + auth?: { username?: unknown }; +} + +/** Context orchestrion attaches to the channels wrapping `ClickHouseClient`'s methods. */ +interface ClickHouseChannelContext extends OrchestrionChannelContext { + arguments: [ClickHouseParams?, ...unknown[]]; + self?: ClickHouseClient; +} + +const _clickhouseIntegration = (() => { + return { + name: INTEGRATION_NAME, + setup(client) { + invokeOrchestrionInstrumentation(client, clickhouseModuleNames, instrumentClickhouse, []); + }, + }; +}) satisfies IntegrationFn; + +function instrumentClickhouse(): void { + // `@clickhouse/client-common` (`<1.23.0`) and `@clickhouse/client` (`>=1.23.0`) publish the same + // calls under their own module names. + const channels: [string, ClickHouseMethod][] = [ + [CHANNELS.CLICKHOUSE_QUERY, 'query'], + [CHANNELS.CLICKHOUSE_COMMAND, 'command'], + [CHANNELS.CLICKHOUSE_EXEC, 'exec'], + [CHANNELS.CLICKHOUSE_INSERT, 'insert'], + [CHANNELS.CLICKHOUSE_COMMON_QUERY, 'query'], + [CHANNELS.CLICKHOUSE_COMMON_COMMAND, 'command'], + [CHANNELS.CLICKHOUSE_COMMON_EXEC, 'exec'], + [CHANNELS.CLICKHOUSE_COMMON_INSERT, 'insert'], + ]; + + for (const [channelName, method] of channels) { + // Failed calls only get an error status on the span. The error still rejects to the caller, who + // decides whether to handle it, so it isn't captured here. + bindTracingChannelToSpan(diagnosticsChannel.tracingChannel(channelName), data => { + const params = data.arguments[0]; + // `insert` with an empty array returns without sending a request. + if (method === 'insert' && Array.isArray(params?.values) && params.values.length === 0) { + return undefined; + } + + const attributes: SpanAttributes = { + [SENTRY_OP]: DB, + [SENTRY_KIND]: 'client', + [SENTRY_ORIGIN]: ORIGIN, + ...getConnectionAttributes(data.self?.connectionParams, params), + ...(method === 'insert' ? getInsertAttributes(params) : getStatementAttributes(params)), + }; + + return startInactiveSpan({ + name: getLowCardinalitySqlSpanName(attributes, DB_SYSTEM_CLICKHOUSE), + attributes, + onlyIfParent: true, + }); + }); + } +} + +function getStatementAttributes(params: ClickHouseParams | undefined): SpanAttributes { + const query = params?.query; + return { ...getSqlQueryAttributes(typeof query === 'string' ? query : undefined, 'clickhouse') }; +} + +// `insert` builds its `INSERT INTO FORMAT ` statement internally, so the span is +// described from the target table instead of a statement. +function getInsertAttributes(params: ClickHouseParams | undefined): SpanAttributes { + const table = typeof params?.table === 'string' && params.table ? params.table : undefined; + return { + [DB_OPERATION_NAME]: 'INSERT', + [DB_COLLECTION_NAME]: table, + [DB_QUERY_SUMMARY]: table ? `INSERT ${table}` : 'INSERT', + }; +} + +function getConnectionAttributes( + connectionParams: ClickHouseConnectionParams | undefined, + params: ClickHouseParams | undefined, +): SpanAttributes { + const url = connectionParams?.url; + const user = params?.auth?.username ?? connectionParams?.auth?.username ?? connectionParams?.username; + return { + [DB_SYSTEM_NAME]: DB_SYSTEM_CLICKHOUSE, + [DB_NAMESPACE]: connectionParams?.database, + [DB_USER]: typeof user === 'string' ? user : undefined, + [SERVER_ADDRESS]: url?.hostname || undefined, + [SERVER_PORT]: url ? getServerPort(url) : undefined, + }; +} + +// The client talks HTTP(S), so a URL without an explicit port uses the protocol's default. +function getServerPort(url: URL): number { + if (url.port) { + return Number(url.port); + } + return url.protocol === 'https:' ? 443 : 80; +} + +/** + * Diagnostics-channel-based `@clickhouse/client` integration. + * + * Subscribes to the channels Sentry's code transform injects into `ClickHouseClient`'s `query`, + * `command`, `exec` and `insert` methods, and emits a `db` span for each call. Requires the Sentry + * runtime hook or bundler plugin to be active. + */ +export const clickhouseIntegration = defineIntegration(_clickhouseIntegration); diff --git a/packages/server-utils/src/integrations/index.ts b/packages/server-utils/src/integrations/index.ts index e70488a63351..199f6aeea97a 100644 --- a/packages/server-utils/src/integrations/index.ts +++ b/packages/server-utils/src/integrations/index.ts @@ -28,6 +28,7 @@ import { togetherAIIntegration } from './together-ai'; import { typesafeIntegration } from './typesafe'; import { postgresJsIntegration } from './postgresjs'; import { neonIntegration } from './neon'; +import { clickhouseIntegration } from './clickhouse'; import { firebaseIntegration } from './firebase'; import { expressIntegration } from './express'; import { fastifyIntegration } from './fastify'; @@ -73,6 +74,7 @@ export function getTracingIntegrations(): Integration[] { typesafeIntegration(), postgresJsIntegration(), neonIntegration(), + clickhouseIntegration(), firebaseIntegration(), mcpServerIntegration(), ]; diff --git a/packages/server-utils/src/orchestrion/channels.ts b/packages/server-utils/src/orchestrion/channels.ts index f593fefe0ebd..aa4b4930e4e4 100644 --- a/packages/server-utils/src/orchestrion/channels.ts +++ b/packages/server-utils/src/orchestrion/channels.ts @@ -1,6 +1,7 @@ import { amqplibChannels } from './config/amqplib'; import { anthropicAiChannels } from './config/anthropic-ai'; import { awsSdkChannels } from './config/aws-sdk'; +import { clickhouseChannels } from './config/clickhouse'; import { cronChannels } from './config/cron'; import { dataloaderChannels } from './config/dataloader'; import { expressChannels } from './config/express'; @@ -61,6 +62,7 @@ export const CHANNELS = { ...amqplibChannels, ...anthropicAiChannels, ...awsSdkChannels, + ...clickhouseChannels, ...cronChannels, ...dataloaderChannels, ...expressChannels, diff --git a/packages/server-utils/src/orchestrion/config/channel-integration-definitions.ts b/packages/server-utils/src/orchestrion/config/channel-integration-definitions.ts index 46cd69d31bf9..ab8e5d240d69 100644 --- a/packages/server-utils/src/orchestrion/config/channel-integration-definitions.ts +++ b/packages/server-utils/src/orchestrion/config/channel-integration-definitions.ts @@ -19,6 +19,7 @@ export const CHANNEL_INTEGRATION_DEFINITIONS = [ { exportName: 'postgresIntegration', modules: ['pg', 'pg-pool'] }, { exportName: 'postgresJsIntegration', modules: ['postgres'] }, { exportName: 'neonIntegration', modules: ['@neondatabase/serverless'] }, + { exportName: 'clickhouseIntegration', modules: ['@clickhouse/client', '@clickhouse/client-common'] }, { exportName: 'prismaIntegration', modules: ['@prisma/orm-family-sql'] }, { exportName: 'mysqlIntegration', modules: ['mysql'] }, { exportName: 'mysql2Integration', modules: ['mysql2'] }, diff --git a/packages/server-utils/src/orchestrion/config/clickhouse.ts b/packages/server-utils/src/orchestrion/config/clickhouse.ts new file mode 100644 index 000000000000..fe0953f914f1 --- /dev/null +++ b/packages/server-utils/src/orchestrion/config/clickhouse.ts @@ -0,0 +1,44 @@ +import type { InstrumentationConfig } from '../apmTypes'; + +import { getModuleNames } from './module-names'; + +// `ClickHouseClient` sends every statement through four `async` methods: `query`, `command`, `exec` +// and `insert`. None of them calls another, so each call is traced as exactly one db span. `ping` +// and `close` carry no statement and are left alone. +// +// Two file layouts across the supported range: +// - `< 1.23.0` → the class lives in the separate `@clickhouse/client-common` package (`dist/client.js`) +// - `>= 1.23.0` → `@clickhouse/client` ships that code itself, in `dist/common/client.js` +// +// Starts at 1.1.0: before it, `command` delegated to `exec` (two spans per command) and the client +// did not keep the `connectionParams` the span attributes are read from. +// +// The Node.js `NodeClickHouseClient` subclass only forwards `query` to `super`, and its class name +// doesn't match `className`, so it doesn't add a second span. +const METHODS = ['query', 'command', 'exec', 'insert'] as const; + +const MODULES = [ + { name: '@clickhouse/client-common', versionRange: '>=1.1.0 <1.23.0', filePath: 'dist/client.js' }, + { name: '@clickhouse/client', versionRange: '>=1.23.0 <2', filePath: 'dist/common/client.js' }, +]; + +export const clickhouseConfig: InstrumentationConfig[] = MODULES.flatMap(module => + METHODS.map(methodName => ({ + channelName: methodName, + module, + functionQuery: { className: 'ClickHouseClient', methodName, kind: 'Async' as const }, + })), +); + +export const clickhouseModuleNames = getModuleNames(clickhouseConfig); + +export const clickhouseChannels = { + CLICKHOUSE_QUERY: 'orchestrion:@clickhouse/client:query', + CLICKHOUSE_COMMAND: 'orchestrion:@clickhouse/client:command', + CLICKHOUSE_EXEC: 'orchestrion:@clickhouse/client:exec', + CLICKHOUSE_INSERT: 'orchestrion:@clickhouse/client:insert', + CLICKHOUSE_COMMON_QUERY: 'orchestrion:@clickhouse/client-common:query', + CLICKHOUSE_COMMON_COMMAND: 'orchestrion:@clickhouse/client-common:command', + CLICKHOUSE_COMMON_EXEC: 'orchestrion:@clickhouse/client-common:exec', + CLICKHOUSE_COMMON_INSERT: 'orchestrion:@clickhouse/client-common:insert', +} as const; diff --git a/packages/server-utils/src/orchestrion/config/index.ts b/packages/server-utils/src/orchestrion/config/index.ts index 7636fbef80fc..837ac9230131 100644 --- a/packages/server-utils/src/orchestrion/config/index.ts +++ b/packages/server-utils/src/orchestrion/config/index.ts @@ -5,6 +5,7 @@ import { uniq } from '@sentry/core'; import { awsSdkConfig } from './aws-sdk'; import { amqplibConfig } from './amqplib'; import { anthropicAiConfig } from './anthropic-ai'; +import { clickhouseConfig } from './clickhouse'; import { cronConfig } from './cron'; import { dataloaderConfig } from './dataloader'; import { expressConfig } from './express'; @@ -67,6 +68,7 @@ export const SENTRY_INSTRUMENTATIONS: InstrumentationConfig[] = [ ...amqplibConfig, ...anthropicAiConfig, ...awsSdkConfig, + ...clickhouseConfig, ...cronConfig, ...dataloaderConfig, ...expressConfig, diff --git a/packages/server-utils/src/utils/sql-sanitizer.ts b/packages/server-utils/src/utils/sql-sanitizer.ts new file mode 100644 index 000000000000..613a04b228bd --- /dev/null +++ b/packages/server-utils/src/utils/sql-sanitizer.ts @@ -0,0 +1,296 @@ +let integerLiteralRE: RegExp | undefined; +let mssqlIntegerLiteralRE: RegExp | undefined; + +/** + * The lookbehind keeps `$n` and `?n` parameter placeholders out of the match. T-SQL has neither, + * and `$1000` there is a money literal, so `$` only guards a placeholder outside `mssql`. + * + * Lazy init: constructing a lookbehind at module scope evaluates it on import and crashes + * Safari <16.4 browser bundles that reach this file via the core barrel. + */ +function getIntegerLiteralRE(dialect: SqlDialect): RegExp { + if (dialect === 'mssql') { + if (!mssqlIntegerLiteralRE) { + mssqlIntegerLiteralRE = new RegExp('(? 0 ? truncated.substring(0, lastSpace) : truncated; } -let integerLiteralRE: RegExp | undefined; -let mssqlIntegerLiteralRE: RegExp | undefined; - -/** - * The lookbehind keeps `$n` and `?n` parameter placeholders out of the match. T-SQL has neither, - * and `$1000` there is a money literal, so `$` only guards a placeholder outside `mssql`. - * - * Lazy init: constructing a lookbehind at module scope evaluates it on import and crashes - * Safari <16.4 browser bundles that reach this file via the core barrel. - */ -function getIntegerLiteralRE(dialect: SqlDialect): RegExp { - if (dialect === 'mssql') { - if (!mssqlIntegerLiteralRE) { - mssqlIntegerLiteralRE = new RegExp('(?(); + + function getScopes(): TestStore { + return asyncStorage.getStore() || { scope: getDefaultCurrentScope(), isolationScope: getDefaultIsolationScope() }; + } + + setAsyncContextStrategy({ + withScope: callback => { + const scope = getScopes().scope.clone(); + const isolationScope = getScopes().isolationScope; + return asyncStorage.run({ scope, isolationScope }, () => callback(scope)); + }, + withSetScope: (scope, callback) => { + const isolationScope = getScopes().isolationScope; + return asyncStorage.run({ scope, isolationScope }, () => callback(scope)); + }, + withIsolationScope: callback => { + const scope = getScopes().scope; + const isolationScope = getScopes().isolationScope.clone(); + return asyncStorage.run({ scope, isolationScope }, () => callback(isolationScope)); + }, + withSetIsolationScope: (isolationScope, callback) => { + const scope = getScopes().scope; + return asyncStorage.run({ scope, isolationScope }, () => callback(isolationScope)); + }, + getCurrentScope: () => getScopes().scope, + getIsolationScope: () => getScopes().isolationScope, + getTracingChannelBinding: () => ({ + asyncLocalStorage: asyncStorage, + getStoreWithActiveSpan: span => { + const scope = getScopes().scope.clone(); + const isolationScope = getScopes().isolationScope; + _INTERNAL_setSpanForScope(scope, span); + return { scope, isolationScope }; + }, + }), + }); +} + +function makeSpan(): Span { + return { end: vi.fn(), setStatus: vi.fn(), setAttributes: vi.fn() } as unknown as Span; +} + +interface ChannelContext { + arguments: unknown[]; + self?: unknown; + result?: unknown; +} + +// `ClickHouseClient` as `>=1.10` builds it: the username sits in `auth`. +const CLIENT = { + connectionParams: { + url: new URL('http://clickhouse.internal:8123'), + database: 'analytics', + auth: { username: 'reporter', password: 'secret', type: 'Credentials' }, + }, +}; + +const CONNECTION_ATTRIBUTES = { + [SENTRY_OP]: DB, + [SENTRY_KIND]: 'client', + [SENTRY_ORIGIN]: 'auto.db.clickhouse', + [DB_SYSTEM_NAME]: 'clickhouse', + [DB_NAMESPACE]: 'analytics', + [DB_USER]: 'reporter', + [SERVER_ADDRESS]: 'clickhouse.internal', + [SERVER_PORT]: 8123, +}; + +describe('clickhouseIntegration', () => { + let startInactiveSpanSpy: MockInstance; + let captureExceptionSpy: MockInstance; + let span: Span; + + beforeAll(() => { + installTestAsyncContextStrategy(); + GLOBAL_OBJ.__SENTRY_ORCHESTRION__ = { runtime: ['@clickhouse/client', '@clickhouse/client-common'] }; + clickhouseIntegration().setup?.({ on: () => () => undefined } as unknown as Client); + }); + + afterAll(() => { + setAsyncContextStrategy(undefined); + delete GLOBAL_OBJ.__SENTRY_ORCHESTRION__; + }); + + beforeEach(() => { + span = makeSpan(); + startInactiveSpanSpy = vi.spyOn(SentryCore, 'startInactiveSpan').mockReturnValue(span); + captureExceptionSpy = vi.spyOn(SentryCore, 'captureException'); + }); + + afterEach(() => { + vi.restoreAllMocks(); + }); + + it.each([ + ['query', CHANNELS.CLICKHOUSE_QUERY], + ['command', CHANNELS.CLICKHOUSE_COMMAND], + ['exec', CHANNELS.CLICKHOUSE_EXEC], + ])('builds a db span for `%s` from the sanitized statement', async (_method, channel) => { + const ctx: ChannelContext = { + arguments: [{ query: "SELECT count() FROM events WHERE name = 'signup' AND id > 42" }], + self: CLIENT, + }; + + await tracingChannel(channel).tracePromise(async () => ({}), ctx); + + expect(startInactiveSpanSpy).toHaveBeenCalledWith({ + name: 'SELECT events', + onlyIfParent: true, + attributes: expect.objectContaining({ + ...CONNECTION_ATTRIBUTES, + [DB_QUERY_TEXT]: 'SELECT count() FROM events WHERE name = ? AND id > ?', + [DB_QUERY_SUMMARY]: 'SELECT events', + [DB_OPERATION_NAME]: 'SELECT', + }), + }); + expect(span.end).toHaveBeenCalledTimes(1); + }); + + it('builds a db span for `insert` from the target table', async () => { + const ctx: ChannelContext = { + arguments: [{ table: 'events', values: [{ id: 1, name: 'signup' }], format: 'JSONEachRow' }], + self: CLIENT, + }; + + await tracingChannel(CHANNELS.CLICKHOUSE_INSERT).tracePromise(async () => ({ executed: true }), ctx); + + expect(startInactiveSpanSpy).toHaveBeenCalledWith({ + name: 'INSERT events', + onlyIfParent: true, + attributes: expect.objectContaining({ + ...CONNECTION_ATTRIBUTES, + [DB_OPERATION_NAME]: 'INSERT', + [DB_COLLECTION_NAME]: 'events', + [DB_QUERY_SUMMARY]: 'INSERT events', + }), + }); + // The inserted values never reach the span. + expect(startInactiveSpanSpy.mock.calls[0]?.[0].attributes[DB_QUERY_TEXT]).toBeUndefined(); + expect(span.end).toHaveBeenCalledTimes(1); + }); + + it('skips `insert` with an empty array, which sends no request', async () => { + const ctx: ChannelContext = { arguments: [{ table: 'events', values: [] }], self: CLIENT }; + + await tracingChannel(CHANNELS.CLICKHOUSE_INSERT).tracePromise(async () => ({ executed: false }), ctx); + + expect(startInactiveSpanSpy).not.toHaveBeenCalled(); + }); + + it('sets an error status on a failed call without capturing the error', async () => { + const ctx: ChannelContext = { arguments: [{ query: 'SELECT * FROM does_not_exist' }], self: CLIENT }; + + await expect( + tracingChannel(CHANNELS.CLICKHOUSE_QUERY).tracePromise(async () => { + throw new Error('Table analytics.does_not_exist does not exist'); + }, ctx), + ).rejects.toThrow('Table analytics.does_not_exist does not exist'); + + expect(span.setStatus).toHaveBeenCalledWith({ + code: SPAN_STATUS_ERROR, + message: 'Table analytics.does_not_exist does not exist', + }); + expect(span.end).toHaveBeenCalledTimes(1); + expect(captureExceptionSpy).not.toHaveBeenCalled(); + }); + + it.each([ + ['query', CHANNELS.CLICKHOUSE_COMMON_QUERY], + ['command', CHANNELS.CLICKHOUSE_COMMON_COMMAND], + ['exec', CHANNELS.CLICKHOUSE_COMMON_EXEC], + ['insert', CHANNELS.CLICKHOUSE_COMMON_INSERT], + ])('builds a db span for `%s` from `@clickhouse/client-common`', async (_method, channel) => { + const ctx: ChannelContext = { arguments: [{ query: 'SELECT 1', table: 'events', values: [1] }], self: CLIENT }; + + await tracingChannel(channel).tracePromise(async () => ({}), ctx); + + expect(startInactiveSpanSpy).toHaveBeenCalledWith( + expect.objectContaining({ attributes: expect.objectContaining(CONNECTION_ATTRIBUTES) }), + ); + expect(span.end).toHaveBeenCalledTimes(1); + }); + + it('reads the username from older clients and the default port from the protocol', async () => { + const ctx: ChannelContext = { + arguments: [{ query: 'SELECT 1' }], + // `<1.10` keeps `username` at the top level of `connectionParams`. + self: { connectionParams: { url: new URL('https://ch.example.com'), database: 'default', username: 'default' } }, + }; + + await tracingChannel(CHANNELS.CLICKHOUSE_COMMON_QUERY).tracePromise(async () => ({}), ctx); + + expect(startInactiveSpanSpy).toHaveBeenCalledWith( + expect.objectContaining({ + attributes: expect.objectContaining({ + [DB_USER]: 'default', + [SERVER_ADDRESS]: 'ch.example.com', + [SERVER_PORT]: 443, + }), + }), + ); + }); + + it('prefers a per-call `auth` override for the username', async () => { + const ctx: ChannelContext = { + arguments: [{ query: 'SELECT 1', auth: { username: 'writer', password: 'secret' } }], + self: CLIENT, + }; + + await tracingChannel(CHANNELS.CLICKHOUSE_QUERY).tracePromise(async () => ({}), ctx); + + expect(startInactiveSpanSpy).toHaveBeenCalledWith( + expect.objectContaining({ attributes: expect.objectContaining({ [DB_USER]: 'writer' }) }), + ); + }); +}); diff --git a/packages/server-utils/test/utils/sql.test.ts b/packages/server-utils/test/utils/sql.test.ts index b04ce144b977..7ac5ef1b961a 100644 --- a/packages/server-utils/test/utils/sql.test.ts +++ b/packages/server-utils/test/utils/sql.test.ts @@ -769,6 +769,41 @@ describe('sanitizeSqlQuery', () => { }); }); + describe("dialect: 'clickhouse'", () => { + it.each([ + [String.raw`SELECT "a\"b", 'private@example.com' AS "c" FROM t`, String.raw`SELECT "a\"b", ? AS "c" FROM t`], + ["SELECT `a\\`b`, 'private@example.com' AS `c` FROM t", 'SELECT `a\\`b`, ? AS `c` FROM t'], + ['INSERT INTO events FORMAT CSV\n1,private@example.com', 'INSERT INTO events'], + [ + '/* comment */ INSERT INTO events (id, name) FORMAT JSONEachRow\n{"id":1,"name":"private@example.com"}', + 'INSERT INTO events (id, name)', + ], + ['INSERT INTO events FORMAT /* input */ CSV\n1,private@example.com', 'INSERT INTO events'], + ['INSERT INTO format (id) VALUES (1)', 'INSERT INTO format (id) VALUES (?)'], + ['INSERT INTO events SELECT format FROM source', 'INSERT INTO events SELECT format FROM source'], + ["INSERT INTO events (`format`) VALUES ('private@example.com')", 'INSERT INTO events (`format`) VALUES (?)'], + // `"..."` and backticks quote identifiers, as in `standard` + ['SELECT * FROM "events" WHERE `user` = \'jane@example.com\'', 'SELECT * FROM "events" WHERE `user` = ?'], + // `\` escapes the next character inside a string literal + [String.raw`SELECT * FROM t WHERE name = 'O\'Brien' AND b = 'x'`, 'SELECT * FROM t WHERE name = ? AND b = ?'], + [String.raw`SELECT * FROM t WHERE a = 'x\\' AND b = 'y'`, 'SELECT * FROM t WHERE a = ? AND b = ?'], + // `#` starts a line comment + ["SELECT * FROM t WHERE a = 'x' # trailing comment", 'SELECT * FROM t WHERE a = ?'], + // heredocs are dollar-quoted literals + ['SELECT $heredoc$secret$heredoc$ FROM t', 'SELECT ? FROM t'], + ['SELECT $1$private@example.com$1$ FROM t', 'SELECT ? FROM t'], + ['SELECT 10_000, 0b101010, 1_000.25, 0xDE_AD FROM events', 'SELECT ?, ?, ?, ? FROM events'], + ['SELECT .1_25, -10_000, 1_0e+2 FROM events', 'SELECT ?, ?, ? FROM events'], + // `{name:Type}` query parameters survive, including the digits in the type name + [ + 'SELECT * FROM t WHERE id = {id:UInt32} AND name = {name:String}', + 'SELECT * FROM t WHERE id = {id:UInt32} AND name = {name:String}', + ], + ])('sanitizes %p', (input, expected) => { + expect(sanitizeSqlQuery(input, 'clickhouse')).toBe(expected); + }); + }); + describe('unterminated literals swallow the rest of the statement', () => { it.each([ ["SELECT * FROM t WHERE a = 'jane@example.com AND b = 2", 'standard' as const], @@ -913,6 +948,7 @@ describe('toSqlDialect', () => { ['mssql', 'mssql'], ['sqlserver', 'mssql'], ['microsoft.sql_server', 'mssql'], + ['clickhouse', 'clickhouse'], ])('maps %j to %j', (system, expected) => { expect(toSqlDialect(system)).toBe(expected); }); diff --git a/packages/solidstart/src/server/index.ts b/packages/solidstart/src/server/index.ts index 39c5b31b1d38..302f60d45ce5 100644 --- a/packages/solidstart/src/server/index.ts +++ b/packages/solidstart/src/server/index.ts @@ -87,6 +87,7 @@ export { postgresIntegration, postgresJsIntegration, neonIntegration, + clickhouseIntegration, prismaIntegration, redisIntegration, requestDataIntegration, diff --git a/packages/sveltekit/src/server/index.ts b/packages/sveltekit/src/server/index.ts index 711a9b4ae3bf..79efb3f9357d 100644 --- a/packages/sveltekit/src/server/index.ts +++ b/packages/sveltekit/src/server/index.ts @@ -86,6 +86,7 @@ export { postgresIntegration, postgresJsIntegration, neonIntegration, + clickhouseIntegration, prismaIntegration, redisIntegration, requestDataIntegration, diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index cb7c360da785..997bc7c434c0 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -624,6 +624,9 @@ importers: '@aws-sdk/client-sqs': specifier: ^3.1161.0 version: 3.1161.0 + '@clickhouse/client': + specifier: 1.25.0 + version: 1.25.0 '@earendil-works/chord': specifier: ^1.4.0 version: 1.4.0 @@ -4179,6 +4182,10 @@ packages: resolution: {integrity: sha512-dlT1m5e/0yUL0kRNcQn7yGLVThkgbB0Ga/1AmfDDC/8ik6AIiSf2QLQO2zPYvefsHP0aFgxO93cVLCCfDp7kzQ==} engines: {node: '>= 20.12.0'} + '@clickhouse/client@1.25.0': + resolution: {integrity: sha512-sxoCDlA8iOEGpGrJzl9SeNvG7II6e5kSeo0JI4Mru2QhwhjydWuDEE55ryZg3vIJ0CSJ1bhn9+Xtnuvf1BZT/w==} + engines: {node: '>=20'} + '@cloudflare/kv-asset-handler@0.4.2': resolution: {integrity: sha512-SIOD2DxrRRwQ+jgzlXCqoEFiKOFqaPjhnNTGKXSRLvp1HiOvapLaFG2kEr9dYQTYe8rKrd9uvDUzmAITeNyaHQ==} engines: {node: '>=18.0.0'} @@ -23106,6 +23113,8 @@ snapshots: fast-wrap-ansi: 0.2.2 sisteransi: 1.0.5 + '@clickhouse/client@1.25.0': {} + '@cloudflare/kv-asset-handler@0.4.2': {} '@cloudflare/kv-asset-handler@0.5.0': {} From 5f41ac754c86cb7343ca38c873f6e9a214c8f474 Mon Sep 17 00:00:00 2001 From: Aditya Reddy <82123567+adityareddy-dev@users.noreply.github.com> Date: Tue, 10 Nov 2026 02:09:53 -0600 Subject: [PATCH 2/3] fix(node): Strip ClickHouse FORMAT data after INSERT SELECT The sanitizer stopped looking for the FORMAT clause once an INSERT reached SELECT, so rows read through input() stayed in db.query.text. FORMAT is now the data clause anywhere at the top level of an INSERT, unless the token before it expects a name, as in SELECT format or FROM format. --- .../server-utils/src/utils/sql-sanitizer.ts | 94 +++++++++++++++---- packages/server-utils/test/utils/sql.test.ts | 20 ++++ 2 files changed, 94 insertions(+), 20 deletions(-) diff --git a/packages/server-utils/src/utils/sql-sanitizer.ts b/packages/server-utils/src/utils/sql-sanitizer.ts index 613a04b228bd..6dbd077bca7c 100644 --- a/packages/server-utils/src/utils/sql-sanitizer.ts +++ b/packages/server-utils/src/utils/sql-sanitizer.ts @@ -59,33 +59,83 @@ const CLICKHOUSE_DOLLAR_QUOTE_RE = /\$\w*\$/y; const CLICKHOUSE_NUMBER_RE = /-?(?:0x[\dA-F_]+|0b[01_]+|(?:\d[\d_]*(?:\.[\d_]*)?|\.\d[\d_]*)(?:e[+-]?[\d_]+)?)(?![\w$])/iy; +const CLICKHOUSE_WORD_RE = /[A-Za-z_][\w$]*/y; + +// A FORMAT clause follows a complete table name or expression. After one of these tokens `format` +// is a table, column or alias name instead. +const CLICKHOUSE_OPERAND_PREFIXES = new Set([ + 'INTO', + 'TABLE', + 'SELECT', + 'DISTINCT', + 'FROM', + 'JOIN', + 'USING', + 'ON', + 'WHERE', + 'PREWHERE', + 'HAVING', + 'BY', + 'AND', + 'OR', + 'NOT', + 'AS', + 'CASE', + 'WHEN', + 'THEN', + 'ELSE', + 'IN', + 'IS', + 'LIKE', + 'ILIKE', + 'BETWEEN', + ',', + '.', + '=', + '<', + '>', + '!', + '+', + '-', + '/', + '%', + '|', + '&', +]); + interface ClickhouseInsertState { - insert: boolean; - lastKeyword?: string; + /** Whether the statement is an INSERT, decided by its first word. */ + insert?: boolean; + /** The last token: an upper-cased word, a punctuation character, or `''` for a literal or quoted identifier. */ + previous?: string; depth: number; } -function isClickhouseInsertData(sql: string, start: number, state: ClickhouseInsertState): boolean { - const char = sql[start]!; +/** + * Whether `char` starts the inline data of an INSERT. Everything after the top-level FORMAT clause is + * input data, which cannot be sanitized as SQL. That includes `INSERT ... SELECT`, where the + * `input()` table function reads that data. + */ +function isClickhouseInsertData(sql: string, start: number, char: string, state: ClickhouseInsertState): boolean { if (char === '(') state.depth++; if (char === ')') state.depth--; - if (!/[A-Za-z_]/.test(char) || isIdentifierChar(sql[start - 1])) { + if (/\s/.test(char) || (isIdentifierChar(char) && isIdentifierChar(sql[start - 1]))) { return false; } - const keyword = /^[A-Za-z_][\w$]*/.exec(sql.slice(start))![0].toUpperCase(); - if (state.lastKeyword === undefined) { - state.insert = keyword === 'INSERT'; + CLICKHOUSE_WORD_RE.lastIndex = start; + const word = CLICKHOUSE_WORD_RE.exec(sql)?.[0].toUpperCase(); + if (word) { + state.insert ??= word === 'INSERT'; } - if (state.insert && state.depth === 0 && state.lastKeyword !== 'INTO' && sql[start - 1] !== '.') { - // FORMAT starts arbitrary input data, which cannot be sanitized as SQL. - if (keyword === 'FORMAT') { - return true; - } - if (keyword === 'SELECT') { - state.insert = false; - } + if ( + state.insert && + state.depth === 0 && + word === 'FORMAT' && + !CLICKHOUSE_OPERAND_PREFIXES.has(state.previous ?? '') + ) { + return true; } - state.lastKeyword = keyword; + state.previous = word ?? char; return false; } @@ -136,7 +186,8 @@ function stripLiteralsAndComments(sql: string, dialect: SqlDialect): string { const isClickhouse = dialect === 'clickhouse'; const out: string[] = []; let i = 0; - const clickhouseInsert: ClickhouseInsertState = { insert: false, depth: 0 }; + // A literal, number or quoted identifier completes an operand, so it resets `previous`. + const clickhouseInsert: ClickhouseInsertState = { depth: 0 }; while (i < sql.length) { const char = sql[i]!; @@ -165,6 +216,7 @@ function stripLiteralsAndComments(sql: string, dialect: SqlDialect): string { const bodyEnd = sql.indexOf(tag, i + tag.length); i = bodyEnd === -1 ? sql.length : bodyEnd + tag.length; out.push('?'); + clickhouseInsert.previous = ''; continue; } } @@ -176,7 +228,7 @@ function stripLiteralsAndComments(sql: string, dialect: SqlDialect): string { // Copying would carry every literal in the rest of the statement through unlexed. out.push(runEnd === -1 ? '?' : sql.slice(i, runEnd)); i = runEnd === -1 ? sql.length : runEnd; - clickhouseInsert.lastKeyword = ''; + clickhouseInsert.previous = ''; continue; } @@ -190,6 +242,7 @@ function stripLiteralsAndComments(sql: string, dialect: SqlDialect): string { const runEnd = findQuotedRunEnd(sql, i, char, isMysql || isClickhouse || prefix === 'E'); i = runEnd === -1 ? sql.length : runEnd; out.push('?'); + clickhouseInsert.previous = ''; continue; } @@ -197,9 +250,10 @@ function stripLiteralsAndComments(sql: string, dialect: SqlDialect): string { if (number) { out.push('?'); i += number.length; + clickhouseInsert.previous = ''; continue; } - if (isClickhouse && isClickhouseInsertData(sql, i, clickhouseInsert)) { + if (isClickhouse && isClickhouseInsertData(sql, i, char, clickhouseInsert)) { break; } out.push(char); diff --git a/packages/server-utils/test/utils/sql.test.ts b/packages/server-utils/test/utils/sql.test.ts index 7ac5ef1b961a..a331fa839adf 100644 --- a/packages/server-utils/test/utils/sql.test.ts +++ b/packages/server-utils/test/utils/sql.test.ts @@ -781,6 +781,26 @@ describe('sanitizeSqlQuery', () => { ['INSERT INTO events FORMAT /* input */ CSV\n1,private@example.com', 'INSERT INTO events'], ['INSERT INTO format (id) VALUES (1)', 'INSERT INTO format (id) VALUES (?)'], ['INSERT INTO events SELECT format FROM source', 'INSERT INTO events SELECT format FROM source'], + // `input()` reads the rows that follow the `FORMAT` clause of an `INSERT ... SELECT` + [ + "INSERT INTO events SELECT * FROM input('email String') FORMAT CSV\nprivate@example.com", + 'INSERT INTO events SELECT * FROM input(?)', + ], + [ + 'INSERT INTO events SELECT lower(email) FROM input(\'email String\') WHERE email != \'\' FORMAT JSONEachRow\n{"email":"private@example.com"}', + 'INSERT INTO events SELECT lower(email) FROM input(?) WHERE email != ?', + ], + [ + "INSERT INTO events SELECT * FROM input('n UInt8') LIMIT 10 FORMAT TSV\nprivate@example.com", + 'INSERT INTO events SELECT * FROM input(?) LIMIT ?', + ], + ['INSERT INTO events SELECT id, format FROM source', 'INSERT INTO events SELECT id, format FROM source'], + ['INSERT INTO events SELECT * FROM format', 'INSERT INTO events SELECT * FROM format'], + [ + 'INSERT INTO events SELECT * FROM source WHERE format = 1 ORDER BY format', + 'INSERT INTO events SELECT * FROM source WHERE format = ? ORDER BY format', + ], + ['INSERT INTO TABLE format FORMAT CSV\nprivate@example.com', 'INSERT INTO TABLE format'], ["INSERT INTO events (`format`) VALUES ('private@example.com')", 'INSERT INTO events (`format`) VALUES (?)'], // `"..."` and backticks quote identifiers, as in `standard` ['SELECT * FROM "events" WHERE `user` = \'jane@example.com\'', 'SELECT * FROM "events" WHERE `user` = ?'], From b769d4403557aec417d21b32ecdf871936bd4e58 Mon Sep 17 00:00:00 2001 From: Aditya Reddy <82123567+adityareddy-dev@users.noreply.github.com> Date: Fri, 13 Nov 2026 04:04:01 -0600 Subject: [PATCH 3/3] fix(node): Strip nested ClickHouse comments --- .../suites/tracing/clickhouse/scenario.mjs | 6 ++++++ .../suites/tracing/clickhouse/test.ts | 2 ++ .../server-utils/src/utils/sql-sanitizer.ts | 21 +++++++++++++++++-- packages/server-utils/test/utils/sql.test.ts | 16 ++++++++++++++ 4 files changed, 43 insertions(+), 2 deletions(-) diff --git a/dev-packages/node-integration-tests/suites/tracing/clickhouse/scenario.mjs b/dev-packages/node-integration-tests/suites/tracing/clickhouse/scenario.mjs index 01fc63ba13ae..5417bfeb9fc0 100644 --- a/dev-packages/node-integration-tests/suites/tracing/clickhouse/scenario.mjs +++ b/dev-packages/node-integration-tests/suites/tracing/clickhouse/scenario.mjs @@ -33,6 +33,12 @@ async function run() { }); await literal.json(); + const nestedComment = await client.query({ + query: 'SELECT 1 /* outer /* inner */ private@example.com */', + format: 'JSONEachRow', + }); + await nestedComment.json(); + const parameterized = await client.query({ query: 'SELECT name FROM events WHERE id = {id:UInt32}', query_params: { id: 1 }, diff --git a/dev-packages/node-integration-tests/suites/tracing/clickhouse/test.ts b/dev-packages/node-integration-tests/suites/tracing/clickhouse/test.ts index d91dcf29bf39..7e221384bc93 100644 --- a/dev-packages/node-integration-tests/suites/tracing/clickhouse/test.ts +++ b/dev-packages/node-integration-tests/suites/tracing/clickhouse/test.ts @@ -40,6 +40,8 @@ const EXPECTED_SPANS = [ { name: 'INSERT events', status: 'ok', operation: 'INSERT', text: 'INSERT INTO events' }, // the string literal is sanitized out of the statement { name: 'SELECT events', status: 'ok', operation: 'SELECT', text: 'SELECT name FROM events WHERE name = ?' }, + // nested comments are removed before the query reaches the span + { name: 'SELECT', status: 'ok', operation: 'SELECT', text: 'SELECT ?' }, // `{name:Type}` query parameters are kept as written { name: 'SELECT events', status: 'ok', operation: 'SELECT', text: 'SELECT name FROM events WHERE id = {id:UInt32}' }, { name: 'SELECT events', status: 'ok', operation: 'SELECT', text: 'SELECT count() FROM events FORMAT JSONEachRow' }, diff --git a/packages/server-utils/src/utils/sql-sanitizer.ts b/packages/server-utils/src/utils/sql-sanitizer.ts index 6dbd077bca7c..e38d8d0088f1 100644 --- a/packages/server-utils/src/utils/sql-sanitizer.ts +++ b/packages/server-utils/src/utils/sql-sanitizer.ts @@ -170,6 +170,24 @@ function findQuotedRunEnd(sql: string, start: number, delimiter: string, backsla return -1; } +// An unterminated comment consumes the rest of the query. +function findBlockCommentEnd(sql: string, start: number, nested: boolean): number { + let depth = 1; + let i = start + 2; + while (i < sql.length && depth > 0) { + if (sql[i] === '*' && sql[i + 1] === '/') { + depth--; + i += 2; + } else if (nested && sql[i] === '/' && sql[i + 1] === '*') { + depth++; + i += 2; + } else { + i++; + } + } + return i; +} + /** * Replaces every string literal with `?` and drops every comment, in one pass. * @@ -200,8 +218,7 @@ function stripLiteralsAndComments(sql: string, dialect: SqlDialect): string { } if (char === '/' && next === '*') { - const commentEnd = sql.indexOf('*/', i + 2); - i = commentEnd === -1 ? sql.length : commentEnd + 2; + i = findBlockCommentEnd(sql, i, isClickhouse); continue; } diff --git a/packages/server-utils/test/utils/sql.test.ts b/packages/server-utils/test/utils/sql.test.ts index a331fa839adf..1af8376c0fdd 100644 --- a/packages/server-utils/test/utils/sql.test.ts +++ b/packages/server-utils/test/utils/sql.test.ts @@ -770,6 +770,22 @@ describe('sanitizeSqlQuery', () => { }); describe("dialect: 'clickhouse'", () => { + it.each([ + ['SELECT 1 /* outer /* inner */ private@example.com */', 'SELECT ?'], + ['SELECT /* outer /* inner */ private@example.com */ 1', 'SELECT ?'], + ['SELECT 1 /* outer /* middle /* inner */ private@example.com */ outer */ + 2', 'SELECT ? + ?'], + ["SELECT 1 /* outer ' /* inner */ private@example.com */ + 2", 'SELECT ? + ?'], + ['SELECT 1 /* outer /* inner */ private@example.com', 'SELECT ?'], + ['SELECT 1 /* outer /* inner private@example.com', 'SELECT ?'], + ['SELECT 1 /* outer /* inner */ private@example.com */ /* other /* inner */ secret */', 'SELECT ?'], + [ + 'INSERT INTO events /* outer /* inner */ private@example.com */ FORMAT CSV\n1,private@example.com', + 'INSERT INTO events', + ], + ])('removes the entire nested comment in %p', (input, expected) => { + expect(sanitizeSqlQuery(input, 'clickhouse')).toBe(expected); + }); + it.each([ [String.raw`SELECT "a\"b", 'private@example.com' AS "c" FROM t`, String.raw`SELECT "a\"b", ? AS "c" FROM t`], ["SELECT `a\\`b`, 'private@example.com' AS `c` FROM t", 'SELECT `a\\`b`, ? AS `c` FROM t'],