feat(dbm): add per-query onEvent to QueryOptions#295
Open
zaidjan-devrev wants to merge 10 commits into
Open
Conversation
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) <noreply@anthropic.com>
zaidjan-devrev
requested review from
itsTalwar,
ujaval403 and
vpbs2
as code owners
|
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) <noreply@anthropic.com>
…vely 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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
|
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) <noreply@anthropic.com>
|
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) <noreply@anthropic.com>
Per-query onEvent feature (QueryOptions.onEvent) — backward compatible. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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) <noreply@anthropic.com>
SQL generation instrumentation (sql_generation_duration events) — backward compatible optional onEvent on cubeQueryToSQL. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
Engine-phase
DBMEvents (query_execution_duration,query_queue_duration,mount_file_buffer_duration,unmount_file_buffer_duration,json_to_buffer_conversion_duration,clone_buffer_duration) are only delivered to the instance-levelonEventset at DBM construction. There is no way to scope these events to a singlequeryWithTablesrun — consumers work around it by stuffing a correlation id intooptions.metadata, subscribing to the shared instanceonEvent, and demultiplexing by that id.This adds a first-class per-query callback so the engine hands each query's events straight to the caller that issued it.
API
QueryOptionsgains:Invoked in addition to the instance-level
onEvent(unchanged), for every event emitted while executing that query.Implementation
dbm.ts):_emitEvent(event, options)fans out to both the instance sink andoptions.onEvent; threaded through the mount / execution / queue-duration emit sites. Thequery_queue_lengthgauge is cross-query and stays on the instance sink only.dbm-parallel.ts,runner-manager.ts):options.onEventis a function → can't crosspostMessage(the payload already stripssignalfor the same reason). The callback stays main-side;DBMParallelregisters it in the runner manager keyed by the query id it already mints, stripsonEventfrom theEXEC_QUERYpayload, and unregisters infinally.IFrameRunnerManagerholds aqueryId -> onEventmap and dispatches onRUNNER_ON_EVENTby thequeryIdechoed on the message.BrowserRunnerOnEventMessagegains an optionalqueryId.onEvent(DBMConstructorOptions,IFrameRunnerManagerConstructor) in favour of the per-query callback.The prebuilt iframe runner bundle must be rebuilt to echo the
EXEC_QUERYqueryIdback onRUNNER_ON_EVENTfor the parallel path to deliver per-query events. Until a runner bundle that does this ships, the parallel/iframe path falls back to the instance-levelonEvent(back-compat preserved by the optionalqueryId+ fallback dispatch). Ship/bump the runner bundle version before consumers rely on per-query events on the parallel path.Back-compat
onEventunchanged — existing consumers unaffected.queryIdonRUNNER_ON_EVENTis optional; a new manager against an old runner gets no per-query dispatch (instance sink still fires).metadatabehaviour.Tests
dbm.spec.ts: per-queryonEventreceives this query's phases; not invoked for a different query.dbm-parallel.spec.ts: callback registered by queryId, unregistered after, and stripped from the iframe options.runner-manager.spec.ts:RUNNER_ON_EVENTdispatches by queryId; isolation across queryIds; unregister stops dispatch; no-queryId message doesn't throw.🤖 Generated with Claude Code