From e8a7d362cdf24e83e62c26da6a3b8f9f4f5a164d Mon Sep 17 00:00:00 2001 From: zaidjan Date: Tue, 7 Jul 2026 17:19:38 -0700 Subject: [PATCH 01/10] feat(dbm): add per-query onEvent to QueryOptions Engine-phase DBMEvents were only delivered to the instance-level onEvent set at DBM construction, so callers could not scope timings to a single queryWithTables run without correlating on metadata. Add a per-query QueryOptions.onEvent, invoked (alongside the instance callback) for that query's events only. - Single DBM: _emitEvent fans out to both the instance and the query's options.onEvent; threaded through the mount / execution / queue-duration emit sites. - Parallel/iframe: options.onEvent isn't serializable, so it stays main-side. DBMParallel registers it in the runner manager keyed by the query id (already minted per query), strips it from the EXEC_QUERY payload, and unregisters in finally. IFrameRunnerManager holds a queryId -> onEvent map and dispatches on RUNNER_ON_EVENT by the queryId echoed on the message. Falls back to the instance callback when queryId is absent (older runner bundles). - BrowserRunnerOnEventMessage gains an optional queryId. - Deprecate the instance-level onEvent (DBMConstructorOptions, IFrameRunnerManagerConstructor) in favour of the per-query callback. NOTE: the prebuilt iframe runner bundle must be rebuilt to echo the EXEC_QUERY queryId back on RUNNER_ON_EVENT for the parallel path to deliver per-query events; until then that path falls back to the instance callback (back-compat preserved). Co-Authored-By: Claude Opus 4.8 (1M context) --- .../src/dbm/__test__/dbm-parallel.spec.ts | 34 ++++++++++++++ meerkat-dbm/src/dbm/__test__/dbm.spec.ts | 33 +++++++++++++ .../src/dbm/__test__/runner-manager.spec.ts | 47 +++++++++++++++++++ .../src/dbm/dbm-parallel/dbm-parallel.ts | 14 +++++- .../src/dbm/dbm-parallel/runner-manager.ts | 37 ++++++++++++++- meerkat-dbm/src/dbm/dbm.ts | 46 +++++++++++------- meerkat-dbm/src/dbm/types.ts | 16 +++++++ .../src/window-communication/runner-types.ts | 6 +++ 8 files changed, 215 insertions(+), 18 deletions(-) diff --git a/meerkat-dbm/src/dbm/__test__/dbm-parallel.spec.ts b/meerkat-dbm/src/dbm/__test__/dbm-parallel.spec.ts index 19d4e9a7..19f1fd32 100644 --- a/meerkat-dbm/src/dbm/__test__/dbm-parallel.spec.ts +++ b/meerkat-dbm/src/dbm/__test__/dbm-parallel.spec.ts @@ -25,6 +25,8 @@ const iFrameRunnerManager = { addIFrameManager: jest.fn(), areRunnersRunning: jest.fn(), messageListener: jest.fn(), + registerQueryEventCallback: jest.fn(), + unregisterQueryEventCallback: jest.fn(), } as unknown as jest.Mocked; const runnerMock = { @@ -465,6 +467,38 @@ describe('DBMParallel', () => { ); }); + it('registers the per-query onEvent and strips it from the iframe options', async () => { + const onEvent = jest.fn(); + + runnerMock.communication.sendRequest.mockResolvedValue({ + message: { isError: false, data: [{ data: 1 }] }, + }); + + await dbmParallel.queryWithTables({ + query: 'SELECT * FROM table', + tables: [], + options: { onEvent }, + }); + + // The callback is registered main-side, keyed by the query id... + expect(iFrameRunnerManager.registerQueryEventCallback).toHaveBeenCalledWith( + expect.any(String), + onEvent + ); + // ...and unregistered once the query completes. + expect(iFrameRunnerManager.unregisterQueryEventCallback).toHaveBeenCalledWith( + expect.any(String) + ); + // ...and never serialized across the postMessage boundary. + expect(runnerMock.communication.sendRequest).toHaveBeenCalledWith( + expect.objectContaining({ + payload: expect.objectContaining({ + options: expect.objectContaining({ onEvent: undefined }), + }), + }) + ); + }); + it('should not log error when query is aborted by user', async () => { const abortController = new AbortController(); diff --git a/meerkat-dbm/src/dbm/__test__/dbm.spec.ts b/meerkat-dbm/src/dbm/__test__/dbm.spec.ts index 4156e611..80bff139 100644 --- a/meerkat-dbm/src/dbm/__test__/dbm.spec.ts +++ b/meerkat-dbm/src/dbm/__test__/dbm.spec.ts @@ -192,6 +192,39 @@ describe('DBM', () => { expect(result).toEqual(['SELECT * FROM table1']); }); + it('should invoke the per-query onEvent for this query’s engine phases', async () => { + const onEvent = jest.fn(); + + await dbm.queryWithTables({ + query: 'SELECT * FROM table1', + tables: tables, + options: { onEvent }, + }); + + const emittedNames = onEvent.mock.calls.map((call) => call[0].event_name); + expect(emittedNames).toContain('query_execution_duration'); + expect(emittedNames).toContain('mount_file_buffer_duration'); + }); + + it('should not invoke a per-query onEvent registered on a different query', async () => { + const onEventA = jest.fn(); + + await dbm.queryWithTables({ + query: 'SELECT * FROM table1', + tables: tables, + options: { onEvent: onEventA }, + }); + onEventA.mockClear(); + + // A second query without onEventA must not reach it. + await dbm.queryWithTables({ + query: 'SELECT * FROM table1', + tables: tables, + }); + + expect(onEventA).not.toHaveBeenCalled(); + }); + it('should execute multiple queries with table names', async () => { const promise1 = dbm.queryWithTables({ query: 'SELECT * FROM table1', diff --git a/meerkat-dbm/src/dbm/__test__/runner-manager.spec.ts b/meerkat-dbm/src/dbm/__test__/runner-manager.spec.ts index 0f13c2a8..f180a697 100644 --- a/meerkat-dbm/src/dbm/__test__/runner-manager.spec.ts +++ b/meerkat-dbm/src/dbm/__test__/runner-manager.spec.ts @@ -110,4 +110,51 @@ describe('IFrameRunnerManager', () => { message.message.payload.tables ); }); + + const onEventMessage = (queryId?: string) => ({ + uuid: 'mock-uuid', + message: { + type: BROWSER_RUNNER_TYPE.RUNNER_ON_EVENT, + payload: { event_name: 'query_execution_duration', duration: 5 }, + queryId, + }, + target_app: 'runner', + timestamp: Date.now(), + }); + + it('dispatches RUNNER_ON_EVENT to the per-query callback registered for its queryId', () => { + const onEvent = jest.fn(); + manager.registerQueryEventCallback('q-1', onEvent); + + manager['messageListener']('0', onEventMessage('q-1') as never); + + expect(onEvent).toHaveBeenCalledWith( + expect.objectContaining({ event_name: 'query_execution_duration', duration: 5 }) + ); + }); + + it('stops dispatching after the per-query callback is unregistered', () => { + const onEvent = jest.fn(); + manager.registerQueryEventCallback('q-2', onEvent); + manager.unregisterQueryEventCallback('q-2'); + + manager['messageListener']('0', onEventMessage('q-2') as never); + + expect(onEvent).not.toHaveBeenCalled(); + }); + + it('does not dispatch to a per-query callback registered under a different queryId', () => { + const onEvent = jest.fn(); + manager.registerQueryEventCallback('q-a', onEvent); + + manager['messageListener']('0', onEventMessage('q-b') as never); + + expect(onEvent).not.toHaveBeenCalled(); + }); + + it('does not throw when a RUNNER_ON_EVENT carries no queryId (older runner bundle)', () => { + expect(() => + manager['messageListener']('0', onEventMessage(undefined) as never) + ).not.toThrow(); + }); }); diff --git a/meerkat-dbm/src/dbm/dbm-parallel/dbm-parallel.ts b/meerkat-dbm/src/dbm/dbm-parallel/dbm-parallel.ts index 8ada82b0..a8fe5d00 100644 --- a/meerkat-dbm/src/dbm/dbm-parallel/dbm-parallel.ts +++ b/meerkat-dbm/src/dbm/dbm-parallel/dbm-parallel.ts @@ -298,6 +298,13 @@ export class DBMParallel extends TableLockManager { signal, }); + // Register the per-query onEvent so runner-emitted events for THIS query + // (echoed back with queryId on RUNNER_ON_EVENT) reach it. + this.iFrameRunnerManager.registerQueryEventCallback( + queryId, + options?.onEvent + ); + const abortPromise = new Promise((_, reject) => { this._signalListener(queryId, runners[this.counter], reject, signal); }); @@ -320,8 +327,10 @@ export class DBMParallel extends TableLockManager { tables, options: { ...options, - // Don't pass signal to iframe as it's not serializable + // Don't pass signal/onEvent to iframe as they're not + // serializable; onEvent is dispatched main-side by queryId. signal: undefined, + onEvent: undefined, }, }, } @@ -365,6 +374,9 @@ export class DBMParallel extends TableLockManager { queryInfo.signal.removeEventListener('abort', queryInfo.abortHandler); } this.activeQueries.delete(queryId); + // Remove the per-query event callback now the query is done so the map + // never leaks entries for completed/aborted queries. + this.iFrameRunnerManager.unregisterQueryEventCallback(queryId); /** * Stop the runner if there are no active queries diff --git a/meerkat-dbm/src/dbm/dbm-parallel/runner-manager.ts b/meerkat-dbm/src/dbm/dbm-parallel/runner-manager.ts index eb3600c8..ba96d969 100644 --- a/meerkat-dbm/src/dbm/dbm-parallel/runner-manager.ts +++ b/meerkat-dbm/src/dbm/dbm-parallel/runner-manager.ts @@ -16,6 +16,12 @@ export interface IFrameRunnerManagerConstructor { fetchPreQuery: (runnerId: string, tables: Table[]) => string[]; totalRunners: number; logger: DBMLogger; + /** + * @deprecated Prefer the per-query {@link QueryOptions.onEvent}. This + * instance-level callback fires for events from every query and is retained + * for back-compat; per-query events are dispatched via + * {@link IFrameRunnerManager.registerQueryEventCallback}. + */ onEvent?: (event: DBMEvent) => void; } @@ -39,6 +45,11 @@ export class IFrameRunnerManager { private runnerURL: string; private logger: DBMLogger; private onEvent?: (event: DBMEvent) => void; + // Per-query event callbacks (QueryOptions.onEvent), keyed by the query id that + // rides on EXEC_QUERY and is echoed back on RUNNER_ON_EVENT. Lets a + // runner-emitted event reach the callback scoped to just that query. + private perQueryEventCallbacks: Map void> = + new Map(); private fetchTableFileBuffers: ( tables: TableConfig[] @@ -63,6 +74,19 @@ export class IFrameRunnerManager { this.fetchPreQuery = fetchPreQuery; } + public registerQueryEventCallback( + queryId: string, + onEvent?: (event: DBMEvent) => void + ) { + if (onEvent) { + this.perQueryEventCallbacks.set(queryId, onEvent); + } + } + + public unregisterQueryEventCallback(queryId: string) { + this.perQueryEventCallbacks.delete(queryId); + } + private addIFrameManager(uuid: string) { this.iFrameReadyMap.set(uuid, false); this.iFrameManagers.set( @@ -154,11 +178,22 @@ export class IFrameRunnerManager { break; } - case BROWSER_RUNNER_TYPE.RUNNER_ON_EVENT: + case BROWSER_RUNNER_TYPE.RUNNER_ON_EVENT: { if (this.onEvent) { this.onEvent(message.message.payload); } + // Dispatch to the per-query callback for the query this event belongs + // to. `queryId` is echoed by the runner on the message (undefined for + // older runner bundles, which then reach only the instance onEvent). + const { queryId } = message.message; + const perQueryOnEvent = queryId + ? this.perQueryEventCallbacks.get(queryId) + : undefined; + if (perQueryOnEvent) { + perQueryOnEvent(message.message.payload); + } break; + } case BROWSER_RUNNER_TYPE.RUNNER_PRE_QUERY: { if (this.fetchPreQuery) { diff --git a/meerkat-dbm/src/dbm/dbm.ts b/meerkat-dbm/src/dbm/dbm.ts index 9fe3669d..6adc8f40 100644 --- a/meerkat-dbm/src/dbm/dbm.ts +++ b/meerkat-dbm/src/dbm/dbm.ts @@ -243,10 +243,15 @@ export class DBM extends TableLockManager { } } - private _emitEvent(event: DBMEvent) { + private _emitEvent(event: DBMEvent, options?: QueryOptions) { if (this.onEvent) { this.onEvent(event); } + // Also deliver to the per-query callback for the query that emitted this + // event, if one was supplied on its options. + if (options?.onEvent) { + options.onEvent(event); + } } private async _getConnection() { @@ -292,11 +297,14 @@ export class DBM extends TableLockManager { query ); - this._emitEvent({ - event_name: 'mount_file_buffer_duration', - duration: endMountTime - startMountTime, - metadata: options?.metadata, - }); + this._emitEvent( + { + event_name: 'mount_file_buffer_duration', + duration: endMountTime - startMountTime, + metadata: options?.metadata, + }, + options + ); const tablesFileData = await this.fileManager.getFilesNameForTables(tables); @@ -323,11 +331,14 @@ export class DBM extends TableLockManager { query ); - this._emitEvent({ - event_name: 'query_execution_duration', - duration: queryQueueDuration, - metadata: options?.metadata, - }); + this._emitEvent( + { + event_name: 'query_execution_duration', + duration: queryQueueDuration, + metadata: options?.metadata, + }, + options + ); return result; } @@ -388,11 +399,14 @@ export class DBM extends TableLockManager { this.currentQueryItem.query ); - this._emitEvent({ - event_name: 'query_queue_duration', - duration: startTime - this.currentQueryItem.timestamp, - metadata, - }); + this._emitEvent( + { + event_name: 'query_queue_duration', + duration: startTime - this.currentQueryItem.timestamp, + metadata: this.currentQueryItem.options?.metadata ?? metadata, + }, + this.currentQueryItem.options + ); /** * Execute the query diff --git a/meerkat-dbm/src/dbm/types.ts b/meerkat-dbm/src/dbm/types.ts index 986470ef..bcab34fd 100644 --- a/meerkat-dbm/src/dbm/types.ts +++ b/meerkat-dbm/src/dbm/types.ts @@ -28,6 +28,12 @@ export interface DBMConstructorOptions { /** * @description * A callback function that handles events emitted by the DBM. + * + * @deprecated Prefer the per-query {@link QueryOptions.onEvent}, which scopes + * events to a single `queryWithTables` run (parallel/iframe path included). + * This instance-level callback fires for events from every query on the + * instance, so callers must correlate by `metadata` to attribute them — the + * per-query callback removes that need. Retained for back-compat. */ onEvent?: (event: DBMEvent) => void; @@ -97,6 +103,16 @@ export interface QueryOptions { */ metadata?: object; + /** + * @description + * Per-query event callback. Invoked (in addition to the instance-level + * `onEvent`) for every DBMEvent emitted while executing THIS query, so a + * caller can scope engine-phase timings to a single query run without + * correlating on `metadata`. For the parallel/iframe path the callback stays + * in the calling window; the runner manager dispatches to it by the query's id. + */ + onEvent?: (event: DBMEvent) => void; + /** * @description * An AbortSignal object instance which can be used to abort the query execution. diff --git a/meerkat-dbm/src/window-communication/runner-types.ts b/meerkat-dbm/src/window-communication/runner-types.ts index 98005b76..306e4d3a 100644 --- a/meerkat-dbm/src/window-communication/runner-types.ts +++ b/meerkat-dbm/src/window-communication/runner-types.ts @@ -56,6 +56,12 @@ export interface BrowserRunnerPreQueryMessage { export interface BrowserRunnerOnEventMessage { type: typeof BROWSER_RUNNER_TYPE.RUNNER_ON_EVENT; payload: DBMEvent; + /** + * Id of the query this event belongs to (echoed from the EXEC_QUERY payload), + * so the runner manager can dispatch to the query's per-query `onEvent`. + * Optional for back-compat with older runner bundles that don't echo it. + */ + queryId?: string; } export interface BrowserRunnerCancelQueryMessage { From 66a388e7d1c6bcdfa1883f5ef1c02f3cc2ca3bfe Mon Sep 17 00:00:00 2001 From: zaidjan Date: Tue, 7 Jul 2026 17:53:58 -0700 Subject: [PATCH 02/10] refactor(dbm): clarify onEvent routing, drop dead unmount event type MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Un-deprecate the instance-level onEvent: it is the sink for events that have no single owning query (query_queue_length, load-time json_to_buffer_conversion_duration, runner-side clone_buffer_duration). The prior @deprecated tags told consumers to drop the only channel that carries those events. Reword docs to state the split explicitly: query-lifecycle events fan out to the per-query QueryOptions.onEvent; cross-query/load/runner events reach the instance sink only. Remove unmount_file_buffer_duration from the DBMEvent union — it has no emit site anywhere in the codebase. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../src/dbm/dbm-parallel/runner-manager.ts | 7 ++-- meerkat-dbm/src/dbm/types.ts | 32 +++++++++++++------ meerkat-dbm/src/logger/event-types.ts | 1 - 3 files changed, 26 insertions(+), 14 deletions(-) diff --git a/meerkat-dbm/src/dbm/dbm-parallel/runner-manager.ts b/meerkat-dbm/src/dbm/dbm-parallel/runner-manager.ts index ba96d969..f6de18ca 100644 --- a/meerkat-dbm/src/dbm/dbm-parallel/runner-manager.ts +++ b/meerkat-dbm/src/dbm/dbm-parallel/runner-manager.ts @@ -17,9 +17,10 @@ export interface IFrameRunnerManagerConstructor { totalRunners: number; logger: DBMLogger; /** - * @deprecated Prefer the per-query {@link QueryOptions.onEvent}. This - * instance-level callback fires for events from every query and is retained - * for back-compat; per-query events are dispatched via + * Instance-level callback for events that are not scoped to a single query + * (e.g. runner-side `clone_buffer_duration`, emitted inside the iframe where + * a per-query callback cannot be reached across `postMessage`). Query- + * lifecycle events are dispatched per-query via * {@link IFrameRunnerManager.registerQueryEventCallback}. */ onEvent?: (event: DBMEvent) => void; diff --git a/meerkat-dbm/src/dbm/types.ts b/meerkat-dbm/src/dbm/types.ts index bcab34fd..1074a96e 100644 --- a/meerkat-dbm/src/dbm/types.ts +++ b/meerkat-dbm/src/dbm/types.ts @@ -27,13 +27,17 @@ export interface DBMConstructorOptions { /** * @description - * A callback function that handles events emitted by the DBM. + * Instance-level callback for events that are NOT scoped to a single query. + * It receives cross-query and load-time events — `query_queue_length` (a + * queue gauge that spans queries), `json_to_buffer_conversion_duration` + * (emitted at `registerJSON`/load time, outside any `queryWithTables` run), + * and runner-side `clone_buffer_duration` (emitted inside the iframe runner, + * which a per-query callback cannot cross via `postMessage`). * - * @deprecated Prefer the per-query {@link QueryOptions.onEvent}, which scopes - * events to a single `queryWithTables` run (parallel/iframe path included). - * This instance-level callback fires for events from every query on the - * instance, so callers must correlate by `metadata` to attribute them — the - * per-query callback removes that need. Retained for back-compat. + * For events that DO belong to one query run — `mount_file_buffer_duration`, + * `query_execution_duration`, `query_queue_duration` — prefer the per-query + * {@link QueryOptions.onEvent}, which scopes them without correlating on + * `metadata`. Those query-lifecycle events fire on BOTH sinks. */ onEvent?: (event: DBMEvent) => void; @@ -106,10 +110,18 @@ export interface QueryOptions { /** * @description * Per-query event callback. Invoked (in addition to the instance-level - * `onEvent`) for every DBMEvent emitted while executing THIS query, so a - * caller can scope engine-phase timings to a single query run without - * correlating on `metadata`. For the parallel/iframe path the callback stays - * in the calling window; the runner manager dispatches to it by the query's id. + * `onEvent`) for the query-lifecycle events of THIS query — + * `mount_file_buffer_duration`, `query_execution_duration` and + * `query_queue_duration` — so a caller can scope those timings to a single + * query run without correlating on `metadata`. + * + * Cross-query and load-time events (`query_queue_length`, + * `json_to_buffer_conversion_duration`, runner-side `clone_buffer_duration`) + * are NOT delivered here — they have no single owning query and reach only the + * instance-level {@link DBMConstructorOptions.onEvent}. + * + * For the parallel/iframe path the callback stays in the calling window; the + * runner manager dispatches to it by the query's id. */ onEvent?: (event: DBMEvent) => void; diff --git a/meerkat-dbm/src/logger/event-types.ts b/meerkat-dbm/src/logger/event-types.ts index ef8e30c0..68aac881 100644 --- a/meerkat-dbm/src/logger/event-types.ts +++ b/meerkat-dbm/src/logger/event-types.ts @@ -2,7 +2,6 @@ export interface DurationEvents { event_name: | 'query_execution_duration' | 'mount_file_buffer_duration' - | 'unmount_file_buffer_duration' | 'query_queue_duration' | 'json_to_buffer_conversion_duration' | 'clone_buffer_duration'; From b3c17b5aec478bc6d93974ecb54334692618d227 Mon Sep 17 00:00:00 2001 From: zaidjan Date: Tue, 7 Jul 2026 18:04:48 -0700 Subject: [PATCH 03/10] refactor(dbm): correlate per-query events by runner id, route exclusively MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two changes to the per-query onEvent delivery: Route exclusively. _emitEvent now sends an event to the per-query callback when one is supplied and returns; otherwise it falls back to the instance sink. The two never both fire for one event, so a consumer using the per-query callback no longer sees duplicate deliveries on the instance sink. Correlate the parallel/iframe path by runner id, not an echoed query id. A runner executes one query at a time, so the runnerId that RUNNER_ON_EVENT arrives on already identifies the query. The manager keys its callbacks by runnerId and dispatches by the runnerId messageListener receives, so no queryId has to cross postMessage — dropping the runner-bundle rebuild prerequisite. Removed the now-unused queryId field from BrowserRunnerOnEventMessage. Co-Authored-By: Claude Opus 4.8 (1M context) --- meerkat-dbm/src/dbm/__test__/dbm.spec.ts | 31 +++++++++++ .../src/dbm/__test__/runner-manager.spec.ts | 53 +++++++++++++------ .../src/dbm/dbm-parallel/dbm-parallel.ts | 19 ++++--- .../src/dbm/dbm-parallel/runner-manager.ts | 35 ++++++------ meerkat-dbm/src/dbm/dbm.ts | 13 +++-- meerkat-dbm/src/dbm/types.ts | 19 ++++--- .../src/window-communication/runner-types.ts | 6 --- 7 files changed, 116 insertions(+), 60 deletions(-) diff --git a/meerkat-dbm/src/dbm/__test__/dbm.spec.ts b/meerkat-dbm/src/dbm/__test__/dbm.spec.ts index 80bff139..78b63c39 100644 --- a/meerkat-dbm/src/dbm/__test__/dbm.spec.ts +++ b/meerkat-dbm/src/dbm/__test__/dbm.spec.ts @@ -206,6 +206,37 @@ describe('DBM', () => { expect(emittedNames).toContain('mount_file_buffer_duration'); }); + it('routes exclusively: query-lifecycle events skip the instance onEvent when a per-query onEvent is supplied', async () => { + const instanceOnEvent = jest.fn(); + const perQueryOnEvent = jest.fn(); + + const dbmWithSink = new DBM({ + instanceManager: new InstanceManager(), + fileManager, + logger: log, + onEvent: instanceOnEvent, + options: { shutdownInactiveTime: 100 }, + onCreateConnection, + }); + + await dbmWithSink.queryWithTables({ + query: 'SELECT * FROM table1', + tables: tables, + options: { onEvent: perQueryOnEvent }, + }); + + const perQueryNames = perQueryOnEvent.mock.calls.map( + (call) => call[0].event_name + ); + expect(perQueryNames).toContain('query_execution_duration'); + // The same events must NOT also reach the instance sink. + const instanceNames = instanceOnEvent.mock.calls.map( + (call) => call[0].event_name + ); + expect(instanceNames).not.toContain('query_execution_duration'); + expect(instanceNames).not.toContain('mount_file_buffer_duration'); + }); + it('should not invoke a per-query onEvent registered on a different query', async () => { const onEventA = jest.fn(); diff --git a/meerkat-dbm/src/dbm/__test__/runner-manager.spec.ts b/meerkat-dbm/src/dbm/__test__/runner-manager.spec.ts index f180a697..f63e8ca5 100644 --- a/meerkat-dbm/src/dbm/__test__/runner-manager.spec.ts +++ b/meerkat-dbm/src/dbm/__test__/runner-manager.spec.ts @@ -111,50 +111,69 @@ describe('IFrameRunnerManager', () => { ); }); - const onEventMessage = (queryId?: string) => ({ + const onEventMessage = () => ({ uuid: 'mock-uuid', message: { type: BROWSER_RUNNER_TYPE.RUNNER_ON_EVENT, payload: { event_name: 'query_execution_duration', duration: 5 }, - queryId, }, target_app: 'runner', - timestamp: Date.now(), + timestamp: 0, }); - it('dispatches RUNNER_ON_EVENT to the per-query callback registered for its queryId', () => { + it('dispatches RUNNER_ON_EVENT to the per-query callback registered for the emitting runner', () => { const onEvent = jest.fn(); - manager.registerQueryEventCallback('q-1', onEvent); + manager.registerQueryEventCallback('0', onEvent); - manager['messageListener']('0', onEventMessage('q-1') as never); + manager['messageListener']('0', onEventMessage() as never); expect(onEvent).toHaveBeenCalledWith( - expect.objectContaining({ event_name: 'query_execution_duration', duration: 5 }) + expect.objectContaining({ + event_name: 'query_execution_duration', + duration: 5, + }) ); }); it('stops dispatching after the per-query callback is unregistered', () => { const onEvent = jest.fn(); - manager.registerQueryEventCallback('q-2', onEvent); - manager.unregisterQueryEventCallback('q-2'); + manager.registerQueryEventCallback('0', onEvent); + manager.unregisterQueryEventCallback('0'); - manager['messageListener']('0', onEventMessage('q-2') as never); + manager['messageListener']('0', onEventMessage() as never); expect(onEvent).not.toHaveBeenCalled(); }); - it('does not dispatch to a per-query callback registered under a different queryId', () => { + it('does not dispatch to a callback registered for a different runner', () => { const onEvent = jest.fn(); - manager.registerQueryEventCallback('q-a', onEvent); + manager.registerQueryEventCallback('0', onEvent); - manager['messageListener']('0', onEventMessage('q-b') as never); + manager['messageListener']('1', onEventMessage() as never); expect(onEvent).not.toHaveBeenCalled(); }); - it('does not throw when a RUNNER_ON_EVENT carries no queryId (older runner bundle)', () => { - expect(() => - manager['messageListener']('0', onEventMessage(undefined) as never) - ).not.toThrow(); + it('routes exclusively: the instance onEvent does not fire when a per-runner callback is registered', () => { + const instanceOnEvent = jest.fn(); + manager['onEvent'] = instanceOnEvent; + const perQueryOnEvent = jest.fn(); + manager.registerQueryEventCallback('0', perQueryOnEvent); + + manager['messageListener']('0', onEventMessage() as never); + + expect(perQueryOnEvent).toHaveBeenCalledTimes(1); + expect(instanceOnEvent).not.toHaveBeenCalled(); + }); + + it('falls back to the instance onEvent when no per-runner callback is registered', () => { + const instanceOnEvent = jest.fn(); + manager['onEvent'] = instanceOnEvent; + + manager['messageListener']('0', onEventMessage() as never); + + expect(instanceOnEvent).toHaveBeenCalledWith( + expect.objectContaining({ event_name: 'query_execution_duration' }) + ); }); }); diff --git a/meerkat-dbm/src/dbm/dbm-parallel/dbm-parallel.ts b/meerkat-dbm/src/dbm/dbm-parallel/dbm-parallel.ts index a8fe5d00..0e049396 100644 --- a/meerkat-dbm/src/dbm/dbm-parallel/dbm-parallel.ts +++ b/meerkat-dbm/src/dbm/dbm-parallel/dbm-parallel.ts @@ -298,10 +298,12 @@ export class DBMParallel extends TableLockManager { signal, }); - // Register the per-query onEvent so runner-emitted events for THIS query - // (echoed back with queryId on RUNNER_ON_EVENT) reach it. + // Register the per-query onEvent against the runner executing this query. + // The runner runs one query at a time, so RUNNER_ON_EVENT arriving on this + // runner belongs to this query and reaches its callback — no queryId needs + // to cross postMessage. this.iFrameRunnerManager.registerQueryEventCallback( - queryId, + runners[this.counter], options?.onEvent ); @@ -373,10 +375,15 @@ export class DBMParallel extends TableLockManager { if (queryInfo?.signal && queryInfo.abortHandler) { queryInfo.signal.removeEventListener('abort', queryInfo.abortHandler); } - this.activeQueries.delete(queryId); // Remove the per-query event callback now the query is done so the map - // never leaks entries for completed/aborted queries. - this.iFrameRunnerManager.unregisterQueryEventCallback(queryId); + // never leaks entries for completed/aborted queries. Keyed by the runner + // this query ran on. + if (queryInfo) { + this.iFrameRunnerManager.unregisterQueryEventCallback( + queryInfo.runnerId + ); + } + this.activeQueries.delete(queryId); /** * Stop the runner if there are no active queries diff --git a/meerkat-dbm/src/dbm/dbm-parallel/runner-manager.ts b/meerkat-dbm/src/dbm/dbm-parallel/runner-manager.ts index f6de18ca..b2b04e4d 100644 --- a/meerkat-dbm/src/dbm/dbm-parallel/runner-manager.ts +++ b/meerkat-dbm/src/dbm/dbm-parallel/runner-manager.ts @@ -46,10 +46,12 @@ export class IFrameRunnerManager { private runnerURL: string; private logger: DBMLogger; private onEvent?: (event: DBMEvent) => void; - // Per-query event callbacks (QueryOptions.onEvent), keyed by the query id that - // rides on EXEC_QUERY and is echoed back on RUNNER_ON_EVENT. Lets a - // runner-emitted event reach the callback scoped to just that query. - private perQueryEventCallbacks: Map void> = + // Per-query event callbacks (QueryOptions.onEvent), keyed by the id of the + // runner executing that query. A runner runs at most one query at a time, so + // the runnerId that RUNNER_ON_EVENT arrives on uniquely identifies the query + // whose callback should receive the event — no queryId needs to cross + // postMessage. + private perRunnerEventCallbacks: Map void> = new Map(); private fetchTableFileBuffers: ( @@ -76,16 +78,16 @@ export class IFrameRunnerManager { } public registerQueryEventCallback( - queryId: string, + runnerId: string, onEvent?: (event: DBMEvent) => void ) { if (onEvent) { - this.perQueryEventCallbacks.set(queryId, onEvent); + this.perRunnerEventCallbacks.set(runnerId, onEvent); } } - public unregisterQueryEventCallback(queryId: string) { - this.perQueryEventCallbacks.delete(queryId); + public unregisterQueryEventCallback(runnerId: string) { + this.perRunnerEventCallbacks.delete(runnerId); } private addIFrameManager(uuid: string) { @@ -180,18 +182,15 @@ export class IFrameRunnerManager { } case BROWSER_RUNNER_TYPE.RUNNER_ON_EVENT: { - if (this.onEvent) { - this.onEvent(message.message.payload); - } - // Dispatch to the per-query callback for the query this event belongs - // to. `queryId` is echoed by the runner on the message (undefined for - // older runner bundles, which then reach only the instance onEvent). - const { queryId } = message.message; - const perQueryOnEvent = queryId - ? this.perQueryEventCallbacks.get(queryId) - : undefined; + // Route exclusively by the runner the event arrived on. A runner runs + // one query at a time, so a registered callback identifies that query; + // deliver to it alone. Otherwise fall back to the instance sink. The + // two never both fire for one event. + const perQueryOnEvent = this.perRunnerEventCallbacks.get(runnerId); if (perQueryOnEvent) { perQueryOnEvent(message.message.payload); + } else if (this.onEvent) { + this.onEvent(message.message.payload); } break; } diff --git a/meerkat-dbm/src/dbm/dbm.ts b/meerkat-dbm/src/dbm/dbm.ts index 6adc8f40..0a131662 100644 --- a/meerkat-dbm/src/dbm/dbm.ts +++ b/meerkat-dbm/src/dbm/dbm.ts @@ -244,13 +244,16 @@ export class DBM extends TableLockManager { } private _emitEvent(event: DBMEvent, options?: QueryOptions) { - if (this.onEvent) { - this.onEvent(event); - } - // Also deliver to the per-query callback for the query that emitted this - // event, if one was supplied on its options. + // Route exclusively: an event that belongs to a query with a per-query + // callback goes to that callback only; everything else (cross-query gauges, + // load-time events, or queries that supplied no per-query callback) goes to + // the instance-level sink. The two sinks never both fire for one event. if (options?.onEvent) { options.onEvent(event); + return; + } + if (this.onEvent) { + this.onEvent(event); } } diff --git a/meerkat-dbm/src/dbm/types.ts b/meerkat-dbm/src/dbm/types.ts index 1074a96e..3cdea6c1 100644 --- a/meerkat-dbm/src/dbm/types.ts +++ b/meerkat-dbm/src/dbm/types.ts @@ -35,9 +35,10 @@ export interface DBMConstructorOptions { * which a per-query callback cannot cross via `postMessage`). * * For events that DO belong to one query run — `mount_file_buffer_duration`, - * `query_execution_duration`, `query_queue_duration` — prefer the per-query - * {@link QueryOptions.onEvent}, which scopes them without correlating on - * `metadata`. Those query-lifecycle events fire on BOTH sinks. + * `query_execution_duration`, `query_queue_duration` — use the per-query + * {@link QueryOptions.onEvent}. Routing is exclusive: when a query supplies + * its own callback those events go there and NOT here; when it does not, they + * fall back to this instance sink. */ onEvent?: (event: DBMEvent) => void; @@ -109,19 +110,21 @@ export interface QueryOptions { /** * @description - * Per-query event callback. Invoked (in addition to the instance-level - * `onEvent`) for the query-lifecycle events of THIS query — + * Per-query event callback for the query-lifecycle events of THIS query — * `mount_file_buffer_duration`, `query_execution_duration` and * `query_queue_duration` — so a caller can scope those timings to a single - * query run without correlating on `metadata`. + * query run without correlating on `metadata`. Routing is exclusive: when + * this callback is supplied those events go here and NOT to the instance-level + * {@link DBMConstructorOptions.onEvent}. * * Cross-query and load-time events (`query_queue_length`, * `json_to_buffer_conversion_duration`, runner-side `clone_buffer_duration`) * are NOT delivered here — they have no single owning query and reach only the - * instance-level {@link DBMConstructorOptions.onEvent}. + * instance sink. * * For the parallel/iframe path the callback stays in the calling window; the - * runner manager dispatches to it by the query's id. + * runner manager dispatches to it by the id of the runner executing the query + * (one query per runner), so no query id crosses `postMessage`. */ onEvent?: (event: DBMEvent) => void; diff --git a/meerkat-dbm/src/window-communication/runner-types.ts b/meerkat-dbm/src/window-communication/runner-types.ts index 306e4d3a..98005b76 100644 --- a/meerkat-dbm/src/window-communication/runner-types.ts +++ b/meerkat-dbm/src/window-communication/runner-types.ts @@ -56,12 +56,6 @@ export interface BrowserRunnerPreQueryMessage { export interface BrowserRunnerOnEventMessage { type: typeof BROWSER_RUNNER_TYPE.RUNNER_ON_EVENT; payload: DBMEvent; - /** - * Id of the query this event belongs to (echoed from the EXEC_QUERY payload), - * so the runner manager can dispatch to the query's per-query `onEvent`. - * Optional for back-compat with older runner bundles that don't echo it. - */ - queryId?: string; } export interface BrowserRunnerCancelQueryMessage { From d4b122fdd61496b09058e17aba79daea6a4152b3 Mon Sep 17 00:00:00 2001 From: zaidjan Date: Tue, 7 Jul 2026 18:14:04 -0700 Subject: [PATCH 04/10] refactor(dbm): route events by their own scope, not callback presence Introduce isQueryScopedEvent as the single source of truth for whether an event belongs to one query run (mount/query_execution/query_queue) or is cross-query/load-time (query_queue_length, json_to_buffer_conversion, clone_buffer). Both emit paths now key routing off it. This corrects the parallel path: the runner emits query-scoped and cross- query events on the same RUNNER_ON_EVENT channel while a per-runner callback is registered. Previously every runner event went to the per-query callback; now clone_buffer_duration correctly reaches the instance sink while the query-scoped timings reach the per-query callback. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../src/dbm/__test__/runner-manager.spec.ts | 26 +++++++++++++++++++ .../src/dbm/dbm-parallel/runner-manager.ts | 19 ++++++++------ meerkat-dbm/src/dbm/dbm.ts | 12 ++++----- meerkat-dbm/src/logger/event-types.ts | 23 ++++++++++++++++ 4 files changed, 66 insertions(+), 14 deletions(-) diff --git a/meerkat-dbm/src/dbm/__test__/runner-manager.spec.ts b/meerkat-dbm/src/dbm/__test__/runner-manager.spec.ts index f63e8ca5..41d73a29 100644 --- a/meerkat-dbm/src/dbm/__test__/runner-manager.spec.ts +++ b/meerkat-dbm/src/dbm/__test__/runner-manager.spec.ts @@ -176,4 +176,30 @@ describe('IFrameRunnerManager', () => { expect.objectContaining({ event_name: 'query_execution_duration' }) ); }); + + it('routes a cross-query event to the instance sink even while a per-runner callback is registered', () => { + const instanceOnEvent = jest.fn(); + manager['onEvent'] = instanceOnEvent; + const perQueryOnEvent = jest.fn(); + manager.registerQueryEventCallback('0', perQueryOnEvent); + + // clone_buffer_duration is runner-side buffer setup, not query-scoped. + manager['messageListener']( + '0', + { + uuid: 'mock-uuid', + message: { + type: BROWSER_RUNNER_TYPE.RUNNER_ON_EVENT, + payload: { event_name: 'clone_buffer_duration', duration: 9 }, + }, + target_app: 'runner', + timestamp: 0, + } as never + ); + + expect(instanceOnEvent).toHaveBeenCalledWith( + expect.objectContaining({ event_name: 'clone_buffer_duration' }) + ); + expect(perQueryOnEvent).not.toHaveBeenCalled(); + }); }); diff --git a/meerkat-dbm/src/dbm/dbm-parallel/runner-manager.ts b/meerkat-dbm/src/dbm/dbm-parallel/runner-manager.ts index b2b04e4d..41ea5bf7 100644 --- a/meerkat-dbm/src/dbm/dbm-parallel/runner-manager.ts +++ b/meerkat-dbm/src/dbm/dbm-parallel/runner-manager.ts @@ -1,5 +1,5 @@ import { FileBufferStore } from '../../file-manager/file-manager-type'; -import { DBMEvent, DBMLogger } from '../../logger'; +import { DBMEvent, DBMLogger, isQueryScopedEvent } from '../../logger'; import { Table } from '../../types'; import { BROWSER_RUNNER_TYPE, @@ -182,15 +182,18 @@ export class IFrameRunnerManager { } case BROWSER_RUNNER_TYPE.RUNNER_ON_EVENT: { - // Route exclusively by the runner the event arrived on. A runner runs - // one query at a time, so a registered callback identifies that query; - // deliver to it alone. Otherwise fall back to the instance sink. The - // two never both fire for one event. + // Route by the event's own scope. The runner emits both query-scoped + // events (mount/query_execution/query_queue) and cross-query ones + // (clone_buffer_duration) on this same channel. A runner runs one query + // at a time, so a callback registered for this runner belongs to its + // in-flight query; query-scoped events go there, everything else to the + // instance sink. A single event never reaches both. + const payload = message.message.payload; const perQueryOnEvent = this.perRunnerEventCallbacks.get(runnerId); - if (perQueryOnEvent) { - perQueryOnEvent(message.message.payload); + if (isQueryScopedEvent(payload) && perQueryOnEvent) { + perQueryOnEvent(payload); } else if (this.onEvent) { - this.onEvent(message.message.payload); + this.onEvent(payload); } break; } diff --git a/meerkat-dbm/src/dbm/dbm.ts b/meerkat-dbm/src/dbm/dbm.ts index 0a131662..86e71aae 100644 --- a/meerkat-dbm/src/dbm/dbm.ts +++ b/meerkat-dbm/src/dbm/dbm.ts @@ -3,7 +3,7 @@ import { Table } from 'apache-arrow/table'; import uniqBy from 'lodash/uniqBy'; import { v4 as uuidv4 } from 'uuid'; import { FileManagerType } from '../file-manager'; -import { DBMEvent, DBMLogger } from '../logger'; +import { DBMEvent, DBMLogger, isQueryScopedEvent } from '../logger'; import { InstanceManagerType } from './instance-manager'; import { TableLockManager } from './table-lock-manager'; import { @@ -244,11 +244,11 @@ export class DBM extends TableLockManager { } private _emitEvent(event: DBMEvent, options?: QueryOptions) { - // Route exclusively: an event that belongs to a query with a per-query - // callback goes to that callback only; everything else (cross-query gauges, - // load-time events, or queries that supplied no per-query callback) goes to - // the instance-level sink. The two sinks never both fire for one event. - if (options?.onEvent) { + // Route by the event's own scope, not by which callbacks exist. Query- + // scoped events go to the per-query callback (when supplied); cross-query + // and load-time events go to the instance-level sink. A single event never + // reaches both. + if (isQueryScopedEvent(event) && options?.onEvent) { options.onEvent(event); return; } diff --git a/meerkat-dbm/src/logger/event-types.ts b/meerkat-dbm/src/logger/event-types.ts index 68aac881..d961a83e 100644 --- a/meerkat-dbm/src/logger/event-types.ts +++ b/meerkat-dbm/src/logger/event-types.ts @@ -14,3 +14,26 @@ export interface QueueEvents { } export type DBMEvent = (DurationEvents | QueueEvents) & { metadata?: object }; + +/** + * Event names that belong to a single `queryWithTables` run. These are routed + * to the per-query {@link QueryOptions.onEvent} when one is supplied. + * + * Every other event (`query_queue_length` — a cross-query gauge; + * `json_to_buffer_conversion_duration` — emitted at load time outside any + * query; `clone_buffer_duration` — runner-side buffer setup) has no single + * owning query and is routed to the instance-level callback only. + */ +const QUERY_SCOPED_EVENT_NAMES: ReadonlySet = new Set([ + 'mount_file_buffer_duration', + 'query_execution_duration', + 'query_queue_duration', +]); + +/** + * Whether an event is scoped to a single query run (vs. cross-query/load-time). + * Routing decisions key off this so the scope lives with the event definition, + * not at each emit/dispatch site. + */ +export const isQueryScopedEvent = (event: DBMEvent): boolean => + QUERY_SCOPED_EVENT_NAMES.has(event.event_name); From f473aba604ed2f8e4ebf8d1b9f0b936aab749f14 Mon Sep 17 00:00:00 2001 From: zaidjan Date: Tue, 7 Jul 2026 18:21:56 -0700 Subject: [PATCH 05/10] refactor(dbm): route at emit site, drop runtime scope classifier MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Remove isQueryScopedEvent. Scope is now decided where each event is emitted, not re-derived from event_name at the sink. In-process (dbm.ts): split _emitEvent into _emitQueryEvent (per-query callback, else instance) and _emitInstanceEvent (instance only). Each call site picks the sink it belongs to — query-lifecycle timings use the former, query_queue_length uses the latter. Parallel/iframe (runner-manager.ts): the runner tags RUNNER_ON_EVENT with a `scope` decided at its emit site; messageListener dispatches on that tag with no event_name inspection. A 'query'-scoped event reaches the in-flight query's per-runner callback; anything else, or an untagged message from an older bundle, falls back to the instance sink. Note: relies on the runner bundle setting `scope` on RUNNER_ON_EVENT; until it does, parallel-path events fall back to the instance sink (back-compat preserved by the optional field). Co-Authored-By: Claude Opus 4.8 (1M context) --- .../src/dbm/__test__/runner-manager.spec.ts | 34 +++++++++++++------ .../src/dbm/dbm-parallel/runner-manager.ts | 12 +++---- meerkat-dbm/src/dbm/dbm.ts | 33 ++++++++++++------ meerkat-dbm/src/logger/event-types.ts | 23 ------------- .../src/window-communication/runner-types.ts | 7 ++++ 5 files changed, 58 insertions(+), 51 deletions(-) diff --git a/meerkat-dbm/src/dbm/__test__/runner-manager.spec.ts b/meerkat-dbm/src/dbm/__test__/runner-manager.spec.ts index 41d73a29..5c66905d 100644 --- a/meerkat-dbm/src/dbm/__test__/runner-manager.spec.ts +++ b/meerkat-dbm/src/dbm/__test__/runner-manager.spec.ts @@ -111,21 +111,22 @@ describe('IFrameRunnerManager', () => { ); }); - const onEventMessage = () => ({ + const onEventMessage = (scope?: 'query' | 'instance') => ({ uuid: 'mock-uuid', message: { type: BROWSER_RUNNER_TYPE.RUNNER_ON_EVENT, payload: { event_name: 'query_execution_duration', duration: 5 }, + scope, }, target_app: 'runner', timestamp: 0, }); - it('dispatches RUNNER_ON_EVENT to the per-query callback registered for the emitting runner', () => { + it('dispatches a query-scoped RUNNER_ON_EVENT to the callback registered for the emitting runner', () => { const onEvent = jest.fn(); manager.registerQueryEventCallback('0', onEvent); - manager['messageListener']('0', onEventMessage() as never); + manager['messageListener']('0', onEventMessage('query') as never); expect(onEvent).toHaveBeenCalledWith( expect.objectContaining({ @@ -140,7 +141,7 @@ describe('IFrameRunnerManager', () => { manager.registerQueryEventCallback('0', onEvent); manager.unregisterQueryEventCallback('0'); - manager['messageListener']('0', onEventMessage() as never); + manager['messageListener']('0', onEventMessage('query') as never); expect(onEvent).not.toHaveBeenCalled(); }); @@ -149,18 +150,18 @@ describe('IFrameRunnerManager', () => { const onEvent = jest.fn(); manager.registerQueryEventCallback('0', onEvent); - manager['messageListener']('1', onEventMessage() as never); + manager['messageListener']('1', onEventMessage('query') as never); expect(onEvent).not.toHaveBeenCalled(); }); - it('routes exclusively: the instance onEvent does not fire when a per-runner callback is registered', () => { + it('routes exclusively: the instance onEvent does not fire for a query-scoped event when a per-runner callback is registered', () => { const instanceOnEvent = jest.fn(); manager['onEvent'] = instanceOnEvent; const perQueryOnEvent = jest.fn(); manager.registerQueryEventCallback('0', perQueryOnEvent); - manager['messageListener']('0', onEventMessage() as never); + manager['messageListener']('0', onEventMessage('query') as never); expect(perQueryOnEvent).toHaveBeenCalledTimes(1); expect(instanceOnEvent).not.toHaveBeenCalled(); @@ -170,20 +171,20 @@ describe('IFrameRunnerManager', () => { const instanceOnEvent = jest.fn(); manager['onEvent'] = instanceOnEvent; - manager['messageListener']('0', onEventMessage() as never); + manager['messageListener']('0', onEventMessage('query') as never); expect(instanceOnEvent).toHaveBeenCalledWith( expect.objectContaining({ event_name: 'query_execution_duration' }) ); }); - it('routes a cross-query event to the instance sink even while a per-runner callback is registered', () => { + it('routes an instance-scoped event to the instance sink even while a per-runner callback is registered', () => { const instanceOnEvent = jest.fn(); manager['onEvent'] = instanceOnEvent; const perQueryOnEvent = jest.fn(); manager.registerQueryEventCallback('0', perQueryOnEvent); - // clone_buffer_duration is runner-side buffer setup, not query-scoped. + // The runner tags clone_buffer_duration (buffer setup) as instance scope. manager['messageListener']( '0', { @@ -191,6 +192,7 @@ describe('IFrameRunnerManager', () => { message: { type: BROWSER_RUNNER_TYPE.RUNNER_ON_EVENT, payload: { event_name: 'clone_buffer_duration', duration: 9 }, + scope: 'instance', }, target_app: 'runner', timestamp: 0, @@ -202,4 +204,16 @@ describe('IFrameRunnerManager', () => { ); expect(perQueryOnEvent).not.toHaveBeenCalled(); }); + + it('falls back to the instance sink when a RUNNER_ON_EVENT carries no scope (older runner bundle)', () => { + const instanceOnEvent = jest.fn(); + manager['onEvent'] = instanceOnEvent; + const perQueryOnEvent = jest.fn(); + manager.registerQueryEventCallback('0', perQueryOnEvent); + + manager['messageListener']('0', onEventMessage(undefined) as never); + + expect(instanceOnEvent).toHaveBeenCalledTimes(1); + expect(perQueryOnEvent).not.toHaveBeenCalled(); + }); }); diff --git a/meerkat-dbm/src/dbm/dbm-parallel/runner-manager.ts b/meerkat-dbm/src/dbm/dbm-parallel/runner-manager.ts index 41ea5bf7..58f28e28 100644 --- a/meerkat-dbm/src/dbm/dbm-parallel/runner-manager.ts +++ b/meerkat-dbm/src/dbm/dbm-parallel/runner-manager.ts @@ -1,5 +1,5 @@ import { FileBufferStore } from '../../file-manager/file-manager-type'; -import { DBMEvent, DBMLogger, isQueryScopedEvent } from '../../logger'; +import { DBMEvent, DBMLogger } from '../../logger'; import { Table } from '../../types'; import { BROWSER_RUNNER_TYPE, @@ -182,15 +182,13 @@ export class IFrameRunnerManager { } case BROWSER_RUNNER_TYPE.RUNNER_ON_EVENT: { - // Route by the event's own scope. The runner emits both query-scoped - // events (mount/query_execution/query_queue) and cross-query ones - // (clone_buffer_duration) on this same channel. A runner runs one query - // at a time, so a callback registered for this runner belongs to its - // in-flight query; query-scoped events go there, everything else to the + // The runner decides scope at the emit site and tags the message. A + // 'query' event belongs to this runner's in-flight query (one query per + // runner) and goes to that query's callback; anything else goes to the // instance sink. A single event never reaches both. const payload = message.message.payload; const perQueryOnEvent = this.perRunnerEventCallbacks.get(runnerId); - if (isQueryScopedEvent(payload) && perQueryOnEvent) { + if (message.message.scope === 'query' && perQueryOnEvent) { perQueryOnEvent(payload); } else if (this.onEvent) { this.onEvent(payload); diff --git a/meerkat-dbm/src/dbm/dbm.ts b/meerkat-dbm/src/dbm/dbm.ts index 86e71aae..c40d79a9 100644 --- a/meerkat-dbm/src/dbm/dbm.ts +++ b/meerkat-dbm/src/dbm/dbm.ts @@ -3,7 +3,7 @@ import { Table } from 'apache-arrow/table'; import uniqBy from 'lodash/uniqBy'; import { v4 as uuidv4 } from 'uuid'; import { FileManagerType } from '../file-manager'; -import { DBMEvent, DBMLogger, isQueryScopedEvent } from '../logger'; +import { DBMEvent, DBMLogger } from '../logger'; import { InstanceManagerType } from './instance-manager'; import { TableLockManager } from './table-lock-manager'; import { @@ -243,12 +243,13 @@ export class DBM extends TableLockManager { } } - private _emitEvent(event: DBMEvent, options?: QueryOptions) { - // Route by the event's own scope, not by which callbacks exist. Query- - // scoped events go to the per-query callback (when supplied); cross-query - // and load-time events go to the instance-level sink. A single event never - // reaches both. - if (isQueryScopedEvent(event) && options?.onEvent) { + /** + * Emit an event scoped to a single query run. It goes to that query's + * per-query callback when one was supplied, otherwise to the instance sink. + * Callers use this only for events that belong to one `queryWithTables` run. + */ + private _emitQueryEvent(event: DBMEvent, options?: QueryOptions) { + if (options?.onEvent) { options.onEvent(event); return; } @@ -257,6 +258,16 @@ export class DBM extends TableLockManager { } } + /** + * Emit a cross-query / load-time event. It has no single owning query, so it + * always goes to the instance-level sink. + */ + private _emitInstanceEvent(event: DBMEvent) { + if (this.onEvent) { + this.onEvent(event); + } + } + private async _getConnection() { // Wait out any in-flight teardown so we never bind a connection to an // instance that recycle/shutdown is about to terminate. Loop because a new @@ -300,7 +311,7 @@ export class DBM extends TableLockManager { query ); - this._emitEvent( + this._emitQueryEvent( { event_name: 'mount_file_buffer_duration', duration: endMountTime - startMountTime, @@ -334,7 +345,7 @@ export class DBM extends TableLockManager { query ); - this._emitEvent( + this._emitQueryEvent( { event_name: 'query_execution_duration', duration: queryQueueDuration, @@ -367,7 +378,7 @@ export class DBM extends TableLockManager { private async _startQueryExecution(metadata?: object) { this.logger.debug('Query queue length:', this.queriesQueue.length); - this._emitEvent({ + this._emitInstanceEvent({ event_name: 'query_queue_length', value: this.queriesQueue.length, metadata, @@ -402,7 +413,7 @@ export class DBM extends TableLockManager { this.currentQueryItem.query ); - this._emitEvent( + this._emitQueryEvent( { event_name: 'query_queue_duration', duration: startTime - this.currentQueryItem.timestamp, diff --git a/meerkat-dbm/src/logger/event-types.ts b/meerkat-dbm/src/logger/event-types.ts index d961a83e..68aac881 100644 --- a/meerkat-dbm/src/logger/event-types.ts +++ b/meerkat-dbm/src/logger/event-types.ts @@ -14,26 +14,3 @@ export interface QueueEvents { } export type DBMEvent = (DurationEvents | QueueEvents) & { metadata?: object }; - -/** - * Event names that belong to a single `queryWithTables` run. These are routed - * to the per-query {@link QueryOptions.onEvent} when one is supplied. - * - * Every other event (`query_queue_length` — a cross-query gauge; - * `json_to_buffer_conversion_duration` — emitted at load time outside any - * query; `clone_buffer_duration` — runner-side buffer setup) has no single - * owning query and is routed to the instance-level callback only. - */ -const QUERY_SCOPED_EVENT_NAMES: ReadonlySet = new Set([ - 'mount_file_buffer_duration', - 'query_execution_duration', - 'query_queue_duration', -]); - -/** - * Whether an event is scoped to a single query run (vs. cross-query/load-time). - * Routing decisions key off this so the scope lives with the event definition, - * not at each emit/dispatch site. - */ -export const isQueryScopedEvent = (event: DBMEvent): boolean => - QUERY_SCOPED_EVENT_NAMES.has(event.event_name); diff --git a/meerkat-dbm/src/window-communication/runner-types.ts b/meerkat-dbm/src/window-communication/runner-types.ts index 98005b76..1c1ccb2e 100644 --- a/meerkat-dbm/src/window-communication/runner-types.ts +++ b/meerkat-dbm/src/window-communication/runner-types.ts @@ -56,6 +56,13 @@ export interface BrowserRunnerPreQueryMessage { export interface BrowserRunnerOnEventMessage { type: typeof BROWSER_RUNNER_TYPE.RUNNER_ON_EVENT; payload: DBMEvent; + /** + * Routing scope decided at the emit site inside the runner. `'query'` events + * belong to the runner's in-flight query and go to that query's per-query + * callback; anything else (or omitted) goes to the instance sink. Optional so + * an older runner bundle that doesn't set it falls back to the instance sink. + */ + scope?: 'query' | 'instance'; } export interface BrowserRunnerCancelQueryMessage { From 3ba6234d6653dbdbffc6aa8a6ed9e907d9ad9843 Mon Sep 17 00:00:00 2001 From: zaidjan Date: Tue, 7 Jul 2026 18:27:58 -0700 Subject: [PATCH 06/10] chore(dbm): remove inline rationale comments from onEvent routing Drop the explanatory inline/private-method comments added while iterating on per-query event routing; keep the public-facing JSDoc on the API types. No behavior change. Co-Authored-By: Claude Opus 4.8 (1M context) --- meerkat-dbm/src/dbm/__test__/dbm-parallel.spec.ts | 3 --- meerkat-dbm/src/dbm/__test__/dbm.spec.ts | 2 -- meerkat-dbm/src/dbm/__test__/runner-manager.spec.ts | 1 - meerkat-dbm/src/dbm/dbm-parallel/dbm-parallel.ts | 11 ++--------- meerkat-dbm/src/dbm/dbm-parallel/runner-manager.ts | 9 --------- meerkat-dbm/src/dbm/dbm.ts | 9 --------- 6 files changed, 2 insertions(+), 33 deletions(-) diff --git a/meerkat-dbm/src/dbm/__test__/dbm-parallel.spec.ts b/meerkat-dbm/src/dbm/__test__/dbm-parallel.spec.ts index 19f1fd32..90229363 100644 --- a/meerkat-dbm/src/dbm/__test__/dbm-parallel.spec.ts +++ b/meerkat-dbm/src/dbm/__test__/dbm-parallel.spec.ts @@ -480,16 +480,13 @@ describe('DBMParallel', () => { options: { onEvent }, }); - // The callback is registered main-side, keyed by the query id... expect(iFrameRunnerManager.registerQueryEventCallback).toHaveBeenCalledWith( expect.any(String), onEvent ); - // ...and unregistered once the query completes. expect(iFrameRunnerManager.unregisterQueryEventCallback).toHaveBeenCalledWith( expect.any(String) ); - // ...and never serialized across the postMessage boundary. expect(runnerMock.communication.sendRequest).toHaveBeenCalledWith( expect.objectContaining({ payload: expect.objectContaining({ diff --git a/meerkat-dbm/src/dbm/__test__/dbm.spec.ts b/meerkat-dbm/src/dbm/__test__/dbm.spec.ts index 78b63c39..f80fbbe7 100644 --- a/meerkat-dbm/src/dbm/__test__/dbm.spec.ts +++ b/meerkat-dbm/src/dbm/__test__/dbm.spec.ts @@ -229,7 +229,6 @@ describe('DBM', () => { (call) => call[0].event_name ); expect(perQueryNames).toContain('query_execution_duration'); - // The same events must NOT also reach the instance sink. const instanceNames = instanceOnEvent.mock.calls.map( (call) => call[0].event_name ); @@ -247,7 +246,6 @@ describe('DBM', () => { }); onEventA.mockClear(); - // A second query without onEventA must not reach it. await dbm.queryWithTables({ query: 'SELECT * FROM table1', tables: tables, diff --git a/meerkat-dbm/src/dbm/__test__/runner-manager.spec.ts b/meerkat-dbm/src/dbm/__test__/runner-manager.spec.ts index 5c66905d..914e6866 100644 --- a/meerkat-dbm/src/dbm/__test__/runner-manager.spec.ts +++ b/meerkat-dbm/src/dbm/__test__/runner-manager.spec.ts @@ -184,7 +184,6 @@ describe('IFrameRunnerManager', () => { const perQueryOnEvent = jest.fn(); manager.registerQueryEventCallback('0', perQueryOnEvent); - // The runner tags clone_buffer_duration (buffer setup) as instance scope. manager['messageListener']( '0', { diff --git a/meerkat-dbm/src/dbm/dbm-parallel/dbm-parallel.ts b/meerkat-dbm/src/dbm/dbm-parallel/dbm-parallel.ts index 0e049396..3e62cf4c 100644 --- a/meerkat-dbm/src/dbm/dbm-parallel/dbm-parallel.ts +++ b/meerkat-dbm/src/dbm/dbm-parallel/dbm-parallel.ts @@ -298,10 +298,6 @@ export class DBMParallel extends TableLockManager { signal, }); - // Register the per-query onEvent against the runner executing this query. - // The runner runs one query at a time, so RUNNER_ON_EVENT arriving on this - // runner belongs to this query and reaches its callback — no queryId needs - // to cross postMessage. this.iFrameRunnerManager.registerQueryEventCallback( runners[this.counter], options?.onEvent @@ -329,8 +325,8 @@ export class DBMParallel extends TableLockManager { tables, options: { ...options, - // Don't pass signal/onEvent to iframe as they're not - // serializable; onEvent is dispatched main-side by queryId. + // Not serializable across postMessage; onEvent is dispatched + // main-side. signal: undefined, onEvent: undefined, }, @@ -375,9 +371,6 @@ export class DBMParallel extends TableLockManager { if (queryInfo?.signal && queryInfo.abortHandler) { queryInfo.signal.removeEventListener('abort', queryInfo.abortHandler); } - // Remove the per-query event callback now the query is done so the map - // never leaks entries for completed/aborted queries. Keyed by the runner - // this query ran on. if (queryInfo) { this.iFrameRunnerManager.unregisterQueryEventCallback( queryInfo.runnerId diff --git a/meerkat-dbm/src/dbm/dbm-parallel/runner-manager.ts b/meerkat-dbm/src/dbm/dbm-parallel/runner-manager.ts index 58f28e28..cac6cead 100644 --- a/meerkat-dbm/src/dbm/dbm-parallel/runner-manager.ts +++ b/meerkat-dbm/src/dbm/dbm-parallel/runner-manager.ts @@ -46,11 +46,6 @@ export class IFrameRunnerManager { private runnerURL: string; private logger: DBMLogger; private onEvent?: (event: DBMEvent) => void; - // Per-query event callbacks (QueryOptions.onEvent), keyed by the id of the - // runner executing that query. A runner runs at most one query at a time, so - // the runnerId that RUNNER_ON_EVENT arrives on uniquely identifies the query - // whose callback should receive the event — no queryId needs to cross - // postMessage. private perRunnerEventCallbacks: Map void> = new Map(); @@ -182,10 +177,6 @@ export class IFrameRunnerManager { } case BROWSER_RUNNER_TYPE.RUNNER_ON_EVENT: { - // The runner decides scope at the emit site and tags the message. A - // 'query' event belongs to this runner's in-flight query (one query per - // runner) and goes to that query's callback; anything else goes to the - // instance sink. A single event never reaches both. const payload = message.message.payload; const perQueryOnEvent = this.perRunnerEventCallbacks.get(runnerId); if (message.message.scope === 'query' && perQueryOnEvent) { diff --git a/meerkat-dbm/src/dbm/dbm.ts b/meerkat-dbm/src/dbm/dbm.ts index c40d79a9..66b7485a 100644 --- a/meerkat-dbm/src/dbm/dbm.ts +++ b/meerkat-dbm/src/dbm/dbm.ts @@ -243,11 +243,6 @@ export class DBM extends TableLockManager { } } - /** - * Emit an event scoped to a single query run. It goes to that query's - * per-query callback when one was supplied, otherwise to the instance sink. - * Callers use this only for events that belong to one `queryWithTables` run. - */ private _emitQueryEvent(event: DBMEvent, options?: QueryOptions) { if (options?.onEvent) { options.onEvent(event); @@ -258,10 +253,6 @@ export class DBM extends TableLockManager { } } - /** - * Emit a cross-query / load-time event. It has no single owning query, so it - * always goes to the instance-level sink. - */ private _emitInstanceEvent(event: DBMEvent) { if (this.onEvent) { this.onEvent(event); From a669bc193a4e50c116f205bbaa22e9ae6670ea02 Mon Sep 17 00:00:00 2001 From: zaidjan Date: Tue, 7 Jul 2026 19:25:46 -0700 Subject: [PATCH 07/10] fix(dbm): unregister per-query event callback on aborted query MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The abort handler clears the activeQueries entry before the finally block runs, so gating the callback teardown on `queryInfo` skipped it for aborted queries — leaking the runner's onEvent closure until the runner was reused and misrouting any trailing runner event to the aborted query's callback. Hoist the selected runner id out of the try and unregister from it directly in finally, independent of activeQueries. Bounded before (keyed by runnerId) but now cleaned up promptly on the abort path too. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../src/dbm/__test__/dbm-parallel.spec.ts | 33 +++++++++++++++++++ .../src/dbm/dbm-parallel/dbm-parallel.ts | 29 ++++++++++------ 2 files changed, 52 insertions(+), 10 deletions(-) diff --git a/meerkat-dbm/src/dbm/__test__/dbm-parallel.spec.ts b/meerkat-dbm/src/dbm/__test__/dbm-parallel.spec.ts index 90229363..95b46e04 100644 --- a/meerkat-dbm/src/dbm/__test__/dbm-parallel.spec.ts +++ b/meerkat-dbm/src/dbm/__test__/dbm-parallel.spec.ts @@ -527,5 +527,38 @@ describe('DBMParallel', () => { // Logger should not have been called since it was a user-initiated abort expect(loggerMock.error).not.toHaveBeenCalled(); }); + + it('unregisters the per-query onEvent when the query is aborted', async () => { + const abortController = new AbortController(); + const onEvent = jest.fn(); + + runnerMock.communication.sendRequest.mockImplementation( + () => + new Promise(() => { + // Never resolves + }) + ); + + const queryPromise = dbmParallel.queryWithTables({ + query: 'SELECT * FROM table', + tables: [], + options: { signal: abortController.signal, onEvent }, + }); + + await new Promise((resolve) => setTimeout(resolve, 10)); + + abortController.abort(); + + await expect(queryPromise).rejects.toThrow('Query aborted by user'); + + // The callback registered for this runner must be torn down even though + // the abort handler cleared activeQueries first. + const registeredRunnerId = ( + iFrameRunnerManager.registerQueryEventCallback as jest.Mock + ).mock.calls[0][0]; + expect(iFrameRunnerManager.unregisterQueryEventCallback).toHaveBeenCalledWith( + registeredRunnerId + ); + }); }); }); diff --git a/meerkat-dbm/src/dbm/dbm-parallel/dbm-parallel.ts b/meerkat-dbm/src/dbm/dbm-parallel/dbm-parallel.ts index 3e62cf4c..f28a184d 100644 --- a/meerkat-dbm/src/dbm/dbm-parallel/dbm-parallel.ts +++ b/meerkat-dbm/src/dbm/dbm-parallel/dbm-parallel.ts @@ -254,6 +254,10 @@ export class DBMParallel extends TableLockManager { // Deduplicate tables by name const tables = uniqBy(_tables, 'name'); + // Hoisted so the finally block can unregister the per-query event callback + // even when the query is aborted (the abort handler clears activeQueries). + let selectedRunnerId: string | undefined; + try { const start = performance.now(); /** @@ -279,9 +283,12 @@ export class DBMParallel extends TableLockManager { const runners = this.iFrameRunnerManager.getRunnerIds(); this.counter = roundRobin(this.counter, runners.length - 1); - const runner = this.iFrameRunnerManager.iFrameManagers.get( - runners[this.counter] - ); + selectedRunnerId = runners[this.counter]; + // Local const so narrowing to `string` survives into the closures below; + // the hoisted `let` is reserved for the finally block. + const runnerId = selectedRunnerId; + + const runner = this.iFrameRunnerManager.iFrameManagers.get(runnerId); /** * StartRunners only initiates the runners, it does not guarantee that the runner is ready to accept the query @@ -294,17 +301,17 @@ export class DBMParallel extends TableLockManager { } this.activeQueries.set(queryId, { - runnerId: runners[this.counter], + runnerId, signal, }); this.iFrameRunnerManager.registerQueryEventCallback( - runners[this.counter], + runnerId, options?.onEvent ); const abortPromise = new Promise((_, reject) => { - this._signalListener(queryId, runners[this.counter], reject, signal); + this._signalListener(queryId, runnerId, reject, signal); }); /** @@ -371,10 +378,12 @@ export class DBMParallel extends TableLockManager { if (queryInfo?.signal && queryInfo.abortHandler) { queryInfo.signal.removeEventListener('abort', queryInfo.abortHandler); } - if (queryInfo) { - this.iFrameRunnerManager.unregisterQueryEventCallback( - queryInfo.runnerId - ); + // Unregister independently of activeQueries: on an aborted query the abort + // handler has already removed the activeQueries entry, so gating this on + // queryInfo would leak the runner's event callback (and misroute trailing + // events). Guarded only against an early throw before a runner was picked. + if (selectedRunnerId !== undefined) { + this.iFrameRunnerManager.unregisterQueryEventCallback(selectedRunnerId); } this.activeQueries.delete(queryId); From aa748024449b8eb17d7df4f50817ab67b94ab4f5 Mon Sep 17 00:00:00 2001 From: zaidjan Date: Tue, 7 Jul 2026 19:26:30 -0700 Subject: [PATCH 08/10] chore(release): bump meerkat-dbm to 0.1.48 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Per-query onEvent feature (QueryOptions.onEvent) — backward compatible. Co-Authored-By: Claude Opus 4.8 (1M context) --- meerkat-dbm/package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/meerkat-dbm/package.json b/meerkat-dbm/package.json index 5693972c..b46efb7c 100644 --- a/meerkat-dbm/package.json +++ b/meerkat-dbm/package.json @@ -1,6 +1,6 @@ { "name": "@devrev/meerkat-dbm", - "version": "0.1.47", + "version": "0.1.48", "dependencies": { "@duckdb/duckdb-wasm": "1.32.0", "apache-arrow": "^17.0.0", From 5f1621ad69523c9b7664af4e0da6132d5cfd89a5 Mon Sep 17 00:00:00 2001 From: zaidjan Date: Tue, 7 Jul 2026 19:44:36 -0700 Subject: [PATCH 09/10] feat(core): add per-phase SQL generation instrumentation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit cubeQueryToSQL previously emitted no timing signal, so cube-to-SQL cost was invisible in end-to-end latency (it runs upstream of meerkat-dbm, which owns the execution-phase events). Add an optional onEvent callback to cubeQueryToSQL (node + browser) that emits sql_generation_duration events per phase — base_sql, ast_build, ast_deserialize_roundtrip, filter_params, projections — plus a total. The event type and a timePhase helper live in meerkat-core so the SQL builders stay decoupled from meerkat-dbm's DBMEvent; callers opt in per call and the path is a no-op when omitted. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../browser-cube-to-sql.ts | 158 +++++++++++------- meerkat-core/src/index.ts | 1 + .../sql-generation-event.spec.ts | 60 +++++++ .../instrumentation/sql-generation-event.ts | 59 +++++++ meerkat-node/src/cube-to-sql/cube-to-sql.ts | 157 ++++++++++------- 5 files changed, 322 insertions(+), 113 deletions(-) create mode 100644 meerkat-core/src/instrumentation/sql-generation-event.spec.ts create mode 100644 meerkat-core/src/instrumentation/sql-generation-event.ts diff --git a/meerkat-browser/src/browser-cube-to-sql/browser-cube-to-sql.ts b/meerkat-browser/src/browser-cube-to-sql/browser-cube-to-sql.ts index ecf7165e..0298a6a4 100644 --- a/meerkat-browser/src/browser-cube-to-sql/browser-cube-to-sql.ts +++ b/meerkat-browser/src/browser-cube-to-sql/browser-cube-to-sql.ts @@ -2,6 +2,7 @@ import { BASE_TABLE_NAME, ContextParams, Query, + SqlGenerationOnEvent, TableSchema, applyFilterParamsToBaseSQL, applyProjectionToSQLQuery, @@ -13,6 +14,7 @@ import { getCombinedTableSchema, getFilterParamsSQL, getFinalBaseSQL, + timePhase, } from '@devrev/meerkat-core'; import { AsyncDuckDBConnection } from '@duckdb/duckdb-wasm'; @@ -30,6 +32,11 @@ export interface CubeQueryToSQLParams { query: Query; tableSchemas: TableSchema[]; contextParams?: ContextParams; + /** + * Optional per-call callback for SQL-generation phase timings. Invoked once + * per phase plus once for `total`. No-op when omitted. + */ + onEvent?: SqlGenerationOnEvent; } export const cubeQueryToSQL = async ({ @@ -37,19 +44,27 @@ export const cubeQueryToSQL = async ({ query, tableSchemas, contextParams, + onEvent, }: CubeQueryToSQLParams) => { - const updatedTableSchemas: TableSchema[] = await Promise.all( - tableSchemas.map(async (schema: TableSchema) => { - const baseFilterParamsSQL = await getFinalBaseSQL({ - query, - tableSchema: schema, - getQueryOutput: (query) => getQueryOutput(query, connection), - }); - return { - ...schema, - sql: baseFilterParamsSQL, - }; - }) + const totalStart = performance.now(); + + const updatedTableSchemas = await timePhase( + 'base_sql', + () => + Promise.all( + tableSchemas.map(async (schema: TableSchema) => { + const baseFilterParamsSQL = await getFinalBaseSQL({ + query, + tableSchema: schema, + getQueryOutput: (query) => getQueryOutput(query, connection), + }); + return { + ...schema, + sql: baseFilterParamsSQL, + }; + }) + ), + onEvent ); const updatedTableSchema = await getCombinedTableSchema( @@ -57,63 +72,92 @@ export const cubeQueryToSQL = async ({ query ); - const ast = cubeToDuckdbAST(query, updatedTableSchema, { - filterType: 'PROJECTION_FILTER', - }); + const ast = await timePhase( + 'ast_build', + async () => + cubeToDuckdbAST(query, updatedTableSchema, { + filterType: 'PROJECTION_FILTER', + }), + onEvent + ); if (!ast) { throw new Error('Could not generate AST'); } - const queryTemp = astDeserializerQuery(ast); + const preBaseQuery = await timePhase( + 'ast_deserialize_roundtrip', + async () => { + const queryTemp = astDeserializerQuery(ast); + const arrowResult = await connection.query(queryTemp); + const parsedOutputQuery = arrowResult + .toArray() + .map((row) => row.toJSON()); + return deserializeQuery(parsedOutputQuery); + }, + onEvent + ); - const arrowResult = await connection.query(queryTemp); - const parsedOutputQuery = arrowResult.toArray().map((row) => row.toJSON()); + const filterParamsSQL = await timePhase( + 'filter_params', + () => + getFilterParamsSQL({ + getQueryOutput: (query) => getQueryOutput(query, connection), + query, + tableSchema: updatedTableSchema, + filterType: 'PROJECTION_FILTER', + }), + onEvent + ); - const preBaseQuery = deserializeQuery(parsedOutputQuery); - const filterParamsSQL = await getFilterParamsSQL({ - getQueryOutput: (query) => getQueryOutput(query, connection), - query, - tableSchema: updatedTableSchema, - filterType: 'PROJECTION_FILTER', - }); + const finalQuery = await timePhase( + 'projections', + async () => { + const filterParamQuery = applyFilterParamsToBaseSQL( + updatedTableSchema.sql, + filterParamsSQL + ); - const filterParamQuery = applyFilterParamsToBaseSQL( - updatedTableSchema.sql, - filterParamsSQL - ); + /** + * Replace CONTEXT_PARAMS with context params + */ + const baseQuery = detectApplyContextParamsToBaseSQL( + filterParamQuery, + contextParams || {} + ); - /** - * Replace CONTEXT_PARAMS with context params - */ - const baseQuery = detectApplyContextParamsToBaseSQL( - filterParamQuery, - contextParams || {} - ); + /** + * Replace BASE_TABLE_NAME with cube query + */ + const replaceBaseTableName = preBaseQuery.replace( + BASE_TABLE_NAME, + `(${baseQuery}) AS ${updatedTableSchema.name}` + ); - /** - * Replace BASE_TABLE_NAME with cube query - */ - const replaceBaseTableName = preBaseQuery.replace( - BASE_TABLE_NAME, - `(${baseQuery}) AS ${updatedTableSchema.name}` - ); + /** + * Add measures to the query + */ + const measures = query.measures; + const dimensions = query.dimensions || []; + const queryWithProjections = applyProjectionToSQLQuery( + dimensions, + measures, + updatedTableSchemas, + replaceBaseTableName + ); - /** - * Add measures to the query - */ - const measures = query.measures; - const dimensions = query.dimensions || []; - const queryWithProjections = applyProjectionToSQLQuery( - dimensions, - measures, - updatedTableSchemas, - replaceBaseTableName + /** + * Replace SQL expression placeholders with actual SQL + */ + return applySQLExpressions(queryWithProjections); + }, + onEvent ); - /** - * Replace SQL expression placeholders with actual SQL - */ - const finalQuery = applySQLExpressions(queryWithProjections); + onEvent?.({ + event_name: 'sql_generation_duration', + phase: 'total', + duration: performance.now() - totalStart, + }); return finalQuery; }; diff --git a/meerkat-core/src/index.ts b/meerkat-core/src/index.ts index c91e1f4e..7c9382d8 100644 --- a/meerkat-core/src/index.ts +++ b/meerkat-core/src/index.ts @@ -47,3 +47,4 @@ export type { EnsureColumnAliasBatchResult, } from './utils/ensure-sql-expression-column-alias'; export * from './utils/ensure-table-schema-alias-sql'; +export * from './instrumentation/sql-generation-event'; diff --git a/meerkat-core/src/instrumentation/sql-generation-event.spec.ts b/meerkat-core/src/instrumentation/sql-generation-event.spec.ts new file mode 100644 index 00000000..e5b056a1 --- /dev/null +++ b/meerkat-core/src/instrumentation/sql-generation-event.spec.ts @@ -0,0 +1,60 @@ +import { SqlGenerationEvent, timePhase } from './sql-generation-event'; + +describe('timePhase', () => { + it('returns the wrapped result unchanged', async () => { + const result = await timePhase('base_sql', async () => 'sql-string'); + expect(result).toBe('sql-string'); + }); + + it('does not invoke onEvent when omitted', async () => { + // Simply must not throw when no callback is supplied. + await expect( + timePhase('ast_build', async () => 42) + ).resolves.toBe(42); + }); + + it('emits a sql_generation_duration event for the phase when onEvent is supplied', async () => { + const events: SqlGenerationEvent[] = []; + + await timePhase('filter_params', async () => 'x', (e) => events.push(e)); + + expect(events).toHaveLength(1); + expect(events[0]).toEqual( + expect.objectContaining({ + event_name: 'sql_generation_duration', + phase: 'filter_params', + }) + ); + expect(typeof events[0].duration).toBe('number'); + expect(events[0].duration).toBeGreaterThanOrEqual(0); + }); + + it('forwards metadata onto the event', async () => { + const events: SqlGenerationEvent[] = []; + + await timePhase( + 'projections', + async () => null, + (e) => events.push(e), + { queryId: 'q-1' } + ); + + expect(events[0].metadata).toEqual({ queryId: 'q-1' }); + }); + + it('does not emit an event when the phase throws', async () => { + const events: SqlGenerationEvent[] = []; + + await expect( + timePhase( + 'ast_deserialize_roundtrip', + async () => { + throw new Error('boom'); + }, + (e) => events.push(e) + ) + ).rejects.toThrow('boom'); + + expect(events).toHaveLength(0); + }); +}); diff --git a/meerkat-core/src/instrumentation/sql-generation-event.ts b/meerkat-core/src/instrumentation/sql-generation-event.ts new file mode 100644 index 00000000..275ba3ea --- /dev/null +++ b/meerkat-core/src/instrumentation/sql-generation-event.ts @@ -0,0 +1,59 @@ +/** + * Instrumentation for the cube-query -> SQL generation path. This lives in + * meerkat-core (which meerkat-node/meerkat-browser already depend on) so the + * SQL builders can emit timings without taking a dependency on meerkat-dbm's + * DBMEvent. It is intentionally a separate event shape from DBMEvent: the two + * cover different phases (generation vs. execution) and different packages. + */ + +/** + * Distinct phases of `cubeQueryToSQL`. `total` wraps the whole call; the rest + * are the individually timed steps that add up to (roughly) it. + */ +export type SqlGenerationPhase = + | 'base_sql' + | 'ast_build' + | 'ast_deserialize_roundtrip' + | 'filter_params' + | 'projections' + | 'total'; + +export interface SqlGenerationEvent { + event_name: 'sql_generation_duration'; + phase: SqlGenerationPhase; + /** Duration in milliseconds. */ + duration: number; + metadata?: object; +} + +/** + * Optional per-call callback for SQL-generation timings. Supplied by the caller + * of `cubeQueryToSQL`; invoked once per phase (plus once for `total`). + */ +export type SqlGenerationOnEvent = (event: SqlGenerationEvent) => void; + +/** + * Time an async phase and, when an `onEvent` is supplied, emit its duration. + * Returns the phase's result untouched so call sites read as a thin wrapper. + * The event fires even if `fn` throws is NOT desired here — a failed phase has + * no meaningful duration to report, so timing is emitted only on success. + */ +export const timePhase = async ( + phase: SqlGenerationPhase, + fn: () => Promise, + onEvent?: SqlGenerationOnEvent, + metadata?: object +): Promise => { + if (!onEvent) { + return fn(); + } + const start = performance.now(); + const result = await fn(); + onEvent({ + event_name: 'sql_generation_duration', + phase, + duration: performance.now() - start, + metadata, + }); + return result; +}; diff --git a/meerkat-node/src/cube-to-sql/cube-to-sql.ts b/meerkat-node/src/cube-to-sql/cube-to-sql.ts index c7aa2494..14411451 100644 --- a/meerkat-node/src/cube-to-sql/cube-to-sql.ts +++ b/meerkat-node/src/cube-to-sql/cube-to-sql.ts @@ -2,6 +2,7 @@ import { BASE_TABLE_NAME, ContextParams, Query, + SqlGenerationOnEvent, TableSchema, applyFilterParamsToBaseSQL, applyProjectionToSQLQuery, @@ -13,6 +14,7 @@ import { getCombinedTableSchema, getFilterParamsSQL, getFinalBaseSQL, + timePhase, } from '@devrev/meerkat-core'; import { duckdbExec } from '../duckdb-exec'; @@ -20,85 +22,128 @@ export interface CubeQueryToSQLParams { query: Query; tableSchemas: TableSchema[]; contextParams?: ContextParams; + /** + * Optional per-call callback for SQL-generation phase timings. Invoked once + * per phase plus once for `total`. No-op when omitted. + */ + onEvent?: SqlGenerationOnEvent; } export const cubeQueryToSQL = async ({ query, tableSchemas, contextParams, + onEvent, }: CubeQueryToSQLParams) => { - const updatedTableSchemas: TableSchema[] = await Promise.all( - tableSchemas.map(async (schema: TableSchema) => { - const baseFilterParamsSQL = await getFinalBaseSQL({ - query, - tableSchema: schema, - getQueryOutput: duckdbExec, - }); - return { - ...schema, - sql: baseFilterParamsSQL, - }; - }) + const totalStart = performance.now(); + + const updatedTableSchemas = await timePhase( + 'base_sql', + () => + Promise.all( + tableSchemas.map(async (schema: TableSchema) => { + const baseFilterParamsSQL = await getFinalBaseSQL({ + query, + tableSchema: schema, + getQueryOutput: duckdbExec, + }); + return { + ...schema, + sql: baseFilterParamsSQL, + }; + }) + ), + onEvent ); const updatedTableSchema = getCombinedTableSchema(updatedTableSchemas, query); - const ast = cubeToDuckdbAST(query, updatedTableSchema, { - filterType: 'PROJECTION_FILTER', - }); + const ast = await timePhase( + 'ast_build', + async () => + cubeToDuckdbAST(query, updatedTableSchema, { + filterType: 'PROJECTION_FILTER', + }), + onEvent + ); if (!ast) { throw new Error('Could not generate AST'); } - const queryTemp = astDeserializerQuery(ast); + const preBaseQuery = await timePhase( + 'ast_deserialize_roundtrip', + async () => { + const queryTemp = astDeserializerQuery(ast); + const queryOutput = (await duckdbExec(queryTemp)) as Record< + string, + string + >[]; + return deserializeQuery(queryOutput); + }, + onEvent + ); - const queryOutput = (await duckdbExec(queryTemp)) as Record[]; - const preBaseQuery = deserializeQuery(queryOutput); + const filterParamsSQL = await timePhase( + 'filter_params', + () => + getFilterParamsSQL({ + query, + tableSchema: updatedTableSchema, + filterType: 'PROJECTION_FILTER', + getQueryOutput: duckdbExec, + }), + onEvent + ); - const filterParamsSQL = await getFilterParamsSQL({ - query, - tableSchema: updatedTableSchema, - filterType: 'PROJECTION_FILTER', - getQueryOutput: duckdbExec, - }); + const finalQuery = await timePhase( + 'projections', + async () => { + const filterParamQuery = applyFilterParamsToBaseSQL( + updatedTableSchema.sql, + filterParamsSQL + ); - const filterParamQuery = applyFilterParamsToBaseSQL( - updatedTableSchema.sql, - filterParamsSQL - ); + /** + * Replace CONTEXT_PARAMS with context params + */ + const baseQuery = detectApplyContextParamsToBaseSQL( + filterParamQuery, + contextParams || {} + ); - /** - * Replace CONTEXT_PARAMS with context params - */ - const baseQuery = detectApplyContextParamsToBaseSQL( - filterParamQuery, - contextParams || {} - ); + /** + * Replace BASE_TABLE_NAME with cube query + */ + const replaceBaseTableName = preBaseQuery.replace( + BASE_TABLE_NAME, + `(${baseQuery}) AS ${updatedTableSchema.name}` + ); - /** - * Replace BASE_TABLE_NAME with cube query - */ - const replaceBaseTableName = preBaseQuery.replace( - BASE_TABLE_NAME, - `(${baseQuery}) AS ${updatedTableSchema.name}` - ); + /** + * Add measures to the query + */ + const measures = query.measures; + const dimensions = query.dimensions || []; + const queryWithProjections = applyProjectionToSQLQuery( + dimensions, + measures, + updatedTableSchemas, + replaceBaseTableName + ); - /** - * Add measures to the query - */ - const measures = query.measures; - const dimensions = query.dimensions || []; - const queryWithProjections = applyProjectionToSQLQuery( - dimensions, - measures, - updatedTableSchemas, - replaceBaseTableName + /** + * Replace SQL expression placeholders with actual SQL + */ + return applySQLExpressions(queryWithProjections); + }, + onEvent ); - /** - * Replace SQL expression placeholders with actual SQL - */ - const finalQuery = applySQLExpressions(queryWithProjections); + onEvent?.({ + event_name: 'sql_generation_duration', + phase: 'total', + duration: performance.now() - totalStart, + }); return finalQuery; }; From c39e7ee7e191f7174298149b01a68e4b35ba576f Mon Sep 17 00:00:00 2001 From: zaidjan Date: Tue, 7 Jul 2026 21:15:18 -0700 Subject: [PATCH 10/10] chore(release): bump meerkat-core/node/browser to 0.0.134 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit SQL generation instrumentation (sql_generation_duration events) — backward compatible optional onEvent on cubeQueryToSQL. Co-Authored-By: Claude Opus 4.8 (1M context) --- meerkat-browser/package.json | 2 +- meerkat-core/package.json | 2 +- meerkat-node/package.json | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/meerkat-browser/package.json b/meerkat-browser/package.json index 28d2537f..ddf89312 100644 --- a/meerkat-browser/package.json +++ b/meerkat-browser/package.json @@ -1,6 +1,6 @@ { "name": "@devrev/meerkat-browser", - "version": "0.0.133", + "version": "0.0.134", "dependencies": { "tslib": "^2.3.0", "@devrev/meerkat-core": "*", diff --git a/meerkat-core/package.json b/meerkat-core/package.json index 46797006..f74bf625 100644 --- a/meerkat-core/package.json +++ b/meerkat-core/package.json @@ -1,6 +1,6 @@ { "name": "@devrev/meerkat-core", - "version": "0.0.133", + "version": "0.0.134", "dependencies": { "tslib": "^2.3.0" }, diff --git a/meerkat-node/package.json b/meerkat-node/package.json index 630b4e94..9cea0ac3 100644 --- a/meerkat-node/package.json +++ b/meerkat-node/package.json @@ -1,6 +1,6 @@ { "name": "@devrev/meerkat-node", - "version": "0.0.133", + "version": "0.0.134", "dependencies": { "@devrev/meerkat-core": "*", "@swc/helpers": "~0.5.0",