[SEA-NodeJS] Unify kernel + driver logging into one DBSQLLogger sink#417
Open
msrathore-db wants to merge 2 commits into
Open
[SEA-NodeJS] Unify kernel + driver logging into one DBSQLLogger sink#417msrathore-db wants to merge 2 commits into
msrathore-db wants to merge 2 commits into
Conversation
The Rust kernel emits its diagnostics via `tracing`; in a Node process nothing
subscribed to them, so kernel logs were silently dropped — the `DBSQLLogger`
only ever saw driver-side lines. This wires the kernel into the SAME logger (and
file) the driver uses, so logs from all three layers — driver, napi shim,
kernel — land in one place.
How:
- `SeaBackend.connect()` calls `installKernelLogBridge(binding, logger, level)`
(new `lib/sea/SeaLogging.ts`), which registers a JS callback with the binding's
`initKernelLogging`. The callback maps each kernel `LogRecord`
(`{ level, target, message }`) onto the driver's `LogLevel` and re-emits it
through `IClientContext.getLogger()`, tagged `[kernel <target>] …`.
- Kernel verbosity follows the driver logger's level: `IDBSQLLogger` gains an
optional `getLevel()` (implemented by `DBSQLLogger`); the bridge passes it to
the kernel so events the sink would drop never cross the bridge. Defaults to
`info` for loggers that don't expose a level.
- Process-global, last-writer-wins + graceful no-op on an older binding without
the bridge export (logging is advisory) — see SeaLogging docs.
Requires kernel napi `initKernelLogging`/`setKernelLogLevel`/`LogRecord`
(databricks-sql-kernel#126); `KERNEL_REV` bumped + binding rebuilt so the
generated `native/sea/index.d.ts` carries them.
Verified: tsc clean, eslint + prettier clean, SEA unit suite 256 passing (incl.
new SeaLogging unit tests), and a live end-to-end run confirms a single
DBSQLLogger file holds both driver lines and 29 `[kernel databricks::sql::kernel]`
lines for one `SELECT 1` (committed as tests/e2e/sea/logging-e2e.test.ts).
Co-authored-by: Isaac
Signed-off-by: Madhavendra Rathore <madhavendra.rathore@databricks.com>
Follow-up to the unified-logging wiring: a runtime `logger.setLevel(...)` now
retargets the kernel-side bridge too, not just the driver's own transports —
keeping kernel verbosity in lock-step with the driver's at runtime, not only at
connect.
- `IDBSQLLogger` gains an optional `onLevelChange(listener) => unsubscribe`;
`DBSQLLogger` implements it (fires subscribers from `setLevel`, swallowing a
throwing listener so it can't break level setting).
- `installKernelLogBridge` now returns an unsubscribe handle and, when both the
logger can notify (`onLevelChange`) and the binding can retarget
(`setKernelLogLevel`), subscribes to forward level changes to the kernel.
- `SeaBackend` stores the handle and drops it on `close()` (no stale listener;
the process-global kernel sink follows the existing last-writer-wins model).
Backend-agnostic: `DBSQLLogger` never references the SEA binding — the SEA layer
subscribes via the interface. Loggers without `onLevelChange` keep the
connect-time level; older bindings without `setKernelLogLevel` simply don't
subscribe.
Verified: tsc / eslint / prettier clean; unit suite 1183 passing incl. new
`DBSQLLogger` level-subscription tests + `SeaLogging` runtime-retarget tests
(retarget, unsubscribe, no-notify logger, no-retarget binding). Live: with the
logger started at `warn`, a healthy SELECT logged 0 kernel lines; after
`logger.setLevel('debug')` mid-session the next SELECT logged 12
`[kernel databricks::sql::kernel]` lines — proving the kernel was retargeted.
Co-authored-by: Isaac
Signed-off-by: Madhavendra Rathore <madhavendra.rathore@databricks.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.
What
Routes the Rust kernel's logs into the same
DBSQLLogger(and file) the Node driver already logs through, so logs from all three layers — driver, napi shim, kernel — land in one place.Today the kernel emits via
tracing, but in a Node process nothing subscribes to those events, so they're silently dropped — the user'sDBSQLLoggeronly ever sees driver-side lines. (The Python connector already unifies viapyo3_log; this is the Node equivalent.)How
lib/sea/SeaLogging.ts:installKernelLogBridge(binding, logger, level)registers a JS callback with the binding'sinitKernelLogging. Each kernelLogRecord({ level, target, message }) is level-mapped onto the driver'sLogLeveland re-emitted through the client'sIDBSQLLogger, tagged[kernel <target>] …so origin is clear in a shared file.SeaBackend.connect()installs the bridge usingIClientContext.getLogger().IDBSQLLoggergains an optionalgetLevel()(implemented byDBSQLLogger); the bridge passes it to the kernel so events the sink would discard never cross the bridge. Loggers without a level default the bridge toinfo.So a user who does
new DBSQLClient({ logger: new DBSQLLogger({ level: 'debug', filepath: '…' }) })and connects withuseSEA: truenow gets kernel logs in that same file, at that same level.Dependency
Requires the kernel napi exports
initKernelLogging/setKernelLogLevel/LogRecordfrom databricks/databricks-sql-kernel#126.KERNEL_REVis pinned to that branch and the binding rebuilt so the generatednative/sea/index.d.tscarries them. Merge order: kernel #126 first, then bumpKERNEL_REVhere to the merged SHA.Verification
SeaLoggingunit tests: level mapping, batch routing, err-drop, older-binding no-op).DBSQLLoggerfile held both driver lines and 29[kernel databricks::sql::kernel]lines (full SEA lifecycle — create session,ExecuteStatement … (attempt 1/6), polling, result manifest, close) for a singleSELECT 1, level-mapped. Committed astests/e2e/sea/logging-e2e.test.ts.This pull request and its description were written by Isaac.