GeezerVison is showing github.com — fix: log per-fetch detail at trace and one summary per result set by vuanhphung · Pull Request #599 · databricks/databricks-sql-nodejs
Conversation
vuanhphung
commented
Nov 11, 2026
•
edited
Loading
Uh oh!
There was an error while loading. Please reload this page.
Summary
Fixes #498 (LOGGING-005). At LogLevel.debug, both backends logged one Fetched chunk of size: N … record per fetchChunk call, which DBSQLOperation shares across backends. They logged nothing when a result set was torn down, and LogLevel had no trace member to move the per-fetch record to.
This adds LogLevel.trace and moves the per-fetch record to it. At debug, each result set now logs one summary when it is closed or cancelled, including when the server ends the statement, for example Result set of operation with id: … closed (rows: 2, fetches: 2). A result set abandoned after a few rows still gets one. The counts come from two counters that fetchChunk increments.
winston's default npm levels have no trace, and a transport set to an unknown level drops every record. DBSQLLogger now uses the npm levels plus trace (between debug and silly), so logger.logger.silly() and the other level methods still work. Custom IDBSQLLogger implementations receive trace records only while their getLevel() returns LogLevel.trace. Loggers without getLevel() never receive them. Kernel trace records are now forwarded as LogLevel.trace under the same rule.
Testing
- Unit tests cover the debug, trace and partial-read phases of the spec, cancel and repeated close, operations without a result set, loggers without
getLevel(), DBSQLLogger at trace, and the kernel bridge mapping. npm test (1543 passing), lint, prettier, type-check and build pass.
- Live warehouse, both backends: on
origin/main, debug_full logs 5 per-fetch debug records and 0 summaries. On this branch it logs 0 per-fetch records and 1 summary, debug_partial (2 of 100 rows) logs 1 summary, and trace_full logs 5 per-fetch trace records plus 1 summary. Cancel then close logs one summary. The existing batched-fetch, iterator, CloudFetch and kernel logging e2e specs pass.
- Perf (medians, fresh processes, alternating order): with the default logger level, 200k one-row fetches take 237 ms on main and 119 ms here, since the per-fetch winston call is gone. A 20M-row drain in 10k-row chunks takes 535.6 ms vs 535.7 ms. Live drains (5 runs each) stay within noise: Thrift 2M rows 5367 vs 5450 ms, kernel 2M rows 4667 vs 4615 ms.
This pull request and its description were written by Isaac.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Add LogLevel.trace and move the per-fetch "Fetched chunk of size" record to it, so debug volume no longer grows with the number of fetches. Each result set now logs one debug summary of the rows and fetches it read when it is closed or cancelled.
DBSQLLogger configures winston's npm levels plus trace, since a transport at an unknown level drops every record. Trace records are only sent to loggers whose getLevel() returns LogLevel.trace, so custom loggers never see a level they don't know.
Fixes #498
Signed-off-by: Vu Anh Phung <[email protected]>
Co-authored-by: Isaac <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Verdict: 1 Low
Looks good. I found one low-severity gap: no summary is logged when the backend flips the operation's closed/cancelled flags before the user calls close(). The trace level, the winston level map, the gating on getLevel() and the kernel bridge filtering are all correct, and the tests cover them.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
When waitUntilReady reports the operation cancelled or closed, the facade flags are set there and a later close() returns early, so no summary was logged. Log it at that point too, at most once per result set.
Signed-off-by: Vu Anh Phung <[email protected]>
Co-authored-by: Isaac <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Verdict: 2 Nit
Looks good, with no blocking issues. I checked LogLevel.trace end to end and found no problems: the winston level map, getLevel() gating in fetchChunk and the kernel bridge, and the once-only summary on close, cancel and backend-reported terminal states. Tests cover each path. Two nits only: the new test file has no license header, and the summary says "1 fetches".
Other findings
- ⚪ Nit — The new
tests/unit/DBSQLOperationLogging.test.ts has no Apache license header, while its neighbours (DBSQLLogger.test.ts, kernel/logging.test.ts) start with the Databricks copyright block.
- ⚪ Nit — The summary reads
in 1 fetches when only one fetch was made. This is cosmetic, but the tests assert the exact string, so changing it later means updating them.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Report counts as "(rows: N, fetches: M)" so the summary reads correctly for any count, and add the license header to the new test file.
Signed-off-by: Vu Anh Phung <[email protected]>
Co-authored-by: Isaac <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✅ No issues identified by the review bot.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
This branch was previously deployed
1 inactive
deployment
Uh oh!
There was an error while loading. Please reload this page.
Assignees
No one assigned
Milestone
No milestone
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.
|