diff --git a/.changeset/gate-clickhouse-query-debug-log.md b/.changeset/gate-clickhouse-query-debug-log.md new file mode 100644 index 0000000000..50d2183654 --- /dev/null +++ b/.changeset/gate-clickhouse-query-debug-log.md @@ -0,0 +1,21 @@ +--- +"@hyperdx/common-utils": patch +"@hyperdx/api": patch +"@hyperdx/app": patch +--- + +fix: route per-query SQL debug logging through an injectable logger (#2416) + +`BaseClickhouseClient.logDebugQuery` dumped raw SQL to the console on every +ClickHouse query, unconditionally and outside the pino logger, flooding API +logs (and, via the browser SDK's consoleCapture, telemetry) with query spam. + +Query logging is now silent by default and routed through an optional +per-client `customLogger` on `ClickhouseClientOptions` (the `Logger` interface +from `@clickhouse/client-common`). The app enables it in dev builds and in +local mode, where queries hit ClickHouse directly and the devtools console is +the only place to see them; anywhere else, pass a `customLogger` at the client +you're debugging. + +`@hyperdx/api` is bumped without source changes: it bundles common-utils, so +its released images stop emitting the per-query dump. diff --git a/packages/app/src/clickhouse.ts b/packages/app/src/clickhouse.ts index 6d5def56d8..2301943660 100644 --- a/packages/app/src/clickhouse.ts +++ b/packages/app/src/clickhouse.ts @@ -9,12 +9,13 @@ import { chSql, ClickhouseClientOptions, ColumnMeta, + DefaultLogger, ResponseJSON, } from '@hyperdx/common-utils/dist/clickhouse'; import { ClickhouseClient } from '@hyperdx/common-utils/dist/clickhouse/browser'; import { useQuery, UseQueryOptions } from '@tanstack/react-query'; -import { IS_LOCAL_MODE } from '@/config'; +import { IS_DEV, IS_LOCAL_MODE } from '@/config'; import { getLocalConnections } from '@/connection'; import api from './api'; @@ -22,9 +23,15 @@ import { DEFAULT_QUERY_TIMEOUT } from './defaults'; const PROXY_CLICKHOUSE_HOST = '/api/clickhouse-proxy'; +const queryLogger = IS_DEV || IS_LOCAL_MODE ? new DefaultLogger() : undefined; + export const getClickhouseClient = ( options: ClickhouseClientOptions = {}, ): ClickhouseClient => { + options = { + customLogger: queryLogger, + ...options, + }; if (IS_LOCAL_MODE) { const localConnections = getLocalConnections(); if (localConnections.length === 0) { diff --git a/packages/cli/src/api/client.ts b/packages/cli/src/api/client.ts index 93c7023dbb..d519d40a33 100644 --- a/packages/cli/src/api/client.ts +++ b/packages/cli/src/api/client.ts @@ -351,9 +351,6 @@ export class ProxyClickhouseClient extends BaseClickhouseClient { }); } - // Silence the "Sending Query: ..." debug output from BaseClickhouseClient - protected override logDebugQuery(): void {} - // This subclass always builds a node client, so narrow the base class's // platform-agnostic client type to the node-specific one. protected getClient(): NodeClickHouseClient { diff --git a/packages/common-utils/src/clickhouse/__tests__/index.test.ts b/packages/common-utils/src/clickhouse/__tests__/index.test.ts index a8bab44109..eace5cf9c2 100644 --- a/packages/common-utils/src/clickhouse/__tests__/index.test.ts +++ b/packages/common-utils/src/clickhouse/__tests__/index.test.ts @@ -5,6 +5,7 @@ import { isMissingColumnError, JSDataType, } from '@/clickhouse'; +import { ClickhouseClient } from '@/clickhouse/node'; describe('isMissingColumnError', () => { it.each([ @@ -124,3 +125,57 @@ describe('convertCHDataTypeToJSType', () => { expect(convertCHDataTypeToJSType('Nullable(Bool)')).toBe(JSDataType.Bool); }); }); + +describe('BaseClickhouseClient.logQuery', () => { + const logQuery = ( + client: ClickhouseClient, + query: string, + params?: Record, + ) => (client as any).logQuery(query, params); + const makeLogger = () => ({ + trace: jest.fn(), + debug: jest.fn(), + info: jest.fn(), + warn: jest.fn(), + error: jest.fn(), + }); + + afterEach(() => { + jest.restoreAllMocks(); + }); + + it('stays silent when no customLogger is configured', () => { + const debugSpy = jest.spyOn(console, 'debug').mockImplementation(() => {}); + const client = new ClickhouseClient({ host: 'http://localhost' }); + logQuery(client, 'SELECT 1 FROM system.one'); + expect(debugSpy).not.toHaveBeenCalled(); + }); + + it('logs through the customLogger passed to the client', () => { + const customLogger = makeLogger(); + const client = new ClickhouseClient({ + host: 'http://localhost', + customLogger, + }); + logQuery(client, 'SELECT 1 FROM system.one'); + expect(customLogger.trace).toHaveBeenCalledWith({ + module: 'clickhouse', + message: 'Sending query', + args: { sql: 'SELECT 1 FROM system.one' }, + }); + }); + + it('interpolates query_params into the logged SQL', () => { + const customLogger = makeLogger(); + const client = new ClickhouseClient({ + host: 'http://localhost', + customLogger, + }); + logQuery(client, 'SELECT {id:Int32}', { id: 5 }); + expect(customLogger.trace).toHaveBeenCalledWith({ + module: 'clickhouse', + message: 'Sending query', + args: { sql: 'SELECT 5' }, + }); + }); +}); diff --git a/packages/common-utils/src/clickhouse/browser.ts b/packages/common-utils/src/clickhouse/browser.ts index 169b994db6..83cd880c16 100644 --- a/packages/common-utils/src/clickhouse/browser.ts +++ b/packages/common-utils/src/clickhouse/browser.ts @@ -129,7 +129,7 @@ export class ClickhouseClient extends BaseClickhouseClient { this.client = this.buildClient(); } - this.logDebugQuery(query, query_params); + this.logQuery(query, query_params); let clickhouseSettings: ClickHouseSettings | undefined; // If this is the settings query, we must not process the clickhouse settings, or else we will infinitely recurse diff --git a/packages/common-utils/src/clickhouse/index.ts b/packages/common-utils/src/clickhouse/index.ts index 7e24cd5793..1e422d06cf 100644 --- a/packages/common-utils/src/clickhouse/index.ts +++ b/packages/common-utils/src/clickhouse/index.ts @@ -3,6 +3,7 @@ import type { BaseResultSet, ClickHouseSettings, DataFormat, + Logger, ResponseHeaders, ResponseJSON, Row, @@ -33,9 +34,11 @@ export type { BaseResultSet, ClickHouseSettings, DataFormat, + Logger, ResponseJSON, Row, }; +export { DefaultLogger } from '@clickhouse/client-common'; export enum JSDataType { Array = 'array', @@ -631,6 +634,8 @@ export type ClickhouseClientOptions = { application?: string; /** Defines how long the client will wait for a response from the ClickHouse server before aborting the request, in milliseconds */ requestTimeout?: number; + /** Logger for per-query SQL debug output. When omitted, query logging is silent. */ + customLogger?: Logger; }; export abstract class BaseClickhouseClient { @@ -647,6 +652,7 @@ export abstract class BaseClickhouseClient { */ protected maxRowReadOnly: boolean; protected requestTimeout: number = 3600000; + protected readonly customLogger?: Logger; constructor({ host, @@ -655,6 +661,7 @@ export abstract class BaseClickhouseClient { queryTimeout, application, requestTimeout, + customLogger, }: ClickhouseClientOptions) { this.host = host!; this.username = username; @@ -662,6 +669,7 @@ export abstract class BaseClickhouseClient { this.queryTimeout = queryTimeout; this.maxRowReadOnly = false; this.application = application; + this.customLogger = customLogger; if (requestTimeout != null && requestTimeout >= 0) { this.requestTimeout = requestTimeout; } @@ -680,10 +688,12 @@ export abstract class BaseClickhouseClient { await this.client?.close(); } - protected logDebugQuery( + protected logQuery( query: string, query_params: Record = {}, ): void { + if (!this.customLogger) return; + let debugSql = ''; try { debugSql = parameterizedQueryToSql({ sql: query, params: query_params }); @@ -691,11 +701,11 @@ export abstract class BaseClickhouseClient { debugSql = query; } - console.debug('--------------------------------------------------------'); - - console.debug('Sending Query:', debugSql); - - console.debug('--------------------------------------------------------'); + this.customLogger.trace({ + module: 'clickhouse', + message: 'Sending query', + args: { sql: debugSql }, + }); } protected async processClickhouseSettings({ diff --git a/packages/common-utils/src/clickhouse/node.ts b/packages/common-utils/src/clickhouse/node.ts index 37c510525c..4ab917e65f 100644 --- a/packages/common-utils/src/clickhouse/node.ts +++ b/packages/common-utils/src/clickhouse/node.ts @@ -47,7 +47,7 @@ export class ClickhouseClient extends BaseClickhouseClient { queryId, shouldSkipApplySettings, }: QueryInputs): Promise> { - this.logDebugQuery(query, query_params); + this.logQuery(query, query_params); let clickhouseSettings: ClickHouseSettings | undefined; // If this is the settings query, we must not process the clickhouse settings, or else we will infinitely recurse