GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

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

@vuanhphung vuanhphung commented Nov 11, 2026 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Collaborator

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.

All reactions
@vuanhphung Isaac
fix: log per-fetch detail at trace and one summary per result set
715c767
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]>
@vuanhphung
vuanhphung temporarily deployed to azure-prod November 11, 2026 23:35 — with GitHub Actions Inactive
@vuanhphung
vuanhphung temporarily deployed to azure-prod November 11, 2026 23:35 — with GitHub Actions Inactive
@vuanhphung
vuanhphung temporarily deployed to azure-prod November 11, 2026 23:35 — with GitHub Actions Inactive
peco-review-bot[bot]
peco-review-bot Bot approved these changes Nov 11, 2026
View reviewed changes

@peco-review-bot peco-review-bot Bot left a comment

Copy link
Copy Markdown

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.

All reactions
Comment thread lib/DBSQLOperation.ts Resolved

Uh oh!

There was an error while loading. Please reload this page.

@vuanhphung Isaac
fix: log the result set summary when the backend ends the operation
100ae53
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]>
@vuanhphung
vuanhphung temporarily deployed to azure-prod November 11, 2026 23:48 — with GitHub Actions Inactive
@vuanhphung
vuanhphung temporarily deployed to azure-prod November 11, 2026 23:48 — with GitHub Actions Inactive
@vuanhphung
vuanhphung temporarily deployed to azure-prod November 11, 2026 23:48 — with GitHub Actions Inactive
peco-review-bot[bot]
peco-review-bot Bot approved these changes Nov 11, 2026
View reviewed changes

@peco-review-bot peco-review-bot Bot left a comment

Copy link
Copy Markdown

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.

All reactions
@vuanhphung Isaac
chore: reword the result set summary and add a license header
60c3de9
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]>
@vuanhphung
vuanhphung temporarily deployed to azure-prod November 11, 2026 23:59 — with GitHub Actions Inactive
@vuanhphung
vuanhphung temporarily deployed to azure-prod November 11, 2026 23:59 — with GitHub Actions Inactive
@vuanhphung
vuanhphung temporarily deployed to azure-prod November 11, 2026 23:59 — with GitHub Actions Inactive
peco-review-bot[bot]
peco-review-bot Bot approved these changes Nov 12, 2026
View reviewed changes

@peco-review-bot peco-review-bot Bot left a comment

Copy link
Copy Markdown

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.

All reactions

This branch was previously deployed

1 inactive deployment
azure-prod — 60c3de90 Deployed Nov 11, 2026 by vuanhphung via e2e-test (24.22.1, 24_22_1) #1899
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@peco-review-bot peco-review-bot[bot] peco-review-bot[bot] approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[coverage] Conformance findings: LOGGING-005

Uh oh!

There was an error while loading. Please reload this page.

1 participant

@vuanhphung
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.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.