Experimental tracing logging for `command()` by mshustov · Pull Request #520 · ClickHouse/clickhouse-js

mshustov · 2026-01-08T20:48:30Z

Summary

In this PR:

add tracing logs for the Node.js keep-alive connection code with enough context to help with debugging
making the log level available in all modules
guard log event generation with explicit guards making log-only code free when not used
massaged the code to use less of this properties for keeping context and more the params that brings us closer to thread/async safe Go-like context monad
parameterized our test even more with "all logs enabled" vs "all logs disabled" config option

Checklist

Unit and integration tests covering the common scenarios were added
A human-readable description of the changes was provided to include in CHANGELOG

Copilot

Pull request overview

This PR adds experimental tracing/logging instrumentation to the command execution flow to help diagnose stream handling and timing issues. The changes introduce detailed trace logging at key points during command execution and stream draining operations.

Key changes:

Enhanced drainStream function with optional logging parameters to trace stream drain lifecycle events
Added comprehensive logging throughout the command execution pipeline including timing metrics and stream state information
Modified parameter passing to ensure query_id is propagated consistently through the execution flow

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File	Description
`packages/client-node/src/connection/stream.ts`	Added logging to track stream drain operations including chunk counts, bytes received, and duration metrics
`packages/client-node/src/connection/node_base_connection.ts`	Instrumented command execution flow with trace logging for timing, stream states, and error conditions; adjusted query_id handling

Comments suppressed due to low confidence (1)

packages/client-node/src/connection/stream.ts:99

The event name in removeListener should be 'close' not 'onClose'. This typo means the close event listener will never be properly removed, potentially causing a memory leak in long-running applications.

    function removeListeners() {
      stream.removeListener('data', dropData)
      stream.removeListener('end', onEnd)
      stream.removeListener('error', onError)
      stream.removeListener('onClose', onClose)

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

codecov · 2026-02-17T12:08:11Z

Codecov Report

❌ Patch coverage is 73.19588% with 26 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
...client-node/src/connection/node_base_connection.ts	72.72%	8 Missing and 10 partials ⚠️
packages/client-node/src/connection/stream.ts	70.00%	6 Missing ⚠️
packages/client-common/src/client.ts	71.42%	0 Missing and 2 partials ⚠️

📢 Thoughts on this report? Let us know!

let's see if it goes anywhere far

peter-leonov-ch · 2026-02-18T08:52:40Z

I've pushed changes that:

introduce a verbosity flag (verbose) to allow us fearlessly add as much debugging code as needed without paying for it when not needed
gently introducing the Go-like context idiom to allow passing domain and log (and in future tracing) context around; this is potentially replacing/augmenting carrying client params around
for logging the actual queries added another logging option unsafeLogUnredactedQueries much like React's dangerouslySetInnerHTML or in general unsafe marker from Rust

If the verbosity flag lifts-off we should address the testing story by running tests with and without verbosity to make sure switching in production is safe. Think of this as performance vs. debug builds in other platforms.

peter-leonov-ch · 2026-02-18T13:15:37Z

After a discussion we're moving with adding a log level as an argument instead of verbosity that will do the same but keep the entity count lower.

peter-leonov-ch · 2026-02-18T16:27:25Z

Ok, log_verbose seems to work in CI. Starting to migrate to the explicit log level.

Copilot

Pull request overview

Copilot reviewed 14 out of 14 changed files in this pull request and generated 1 comment.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

packages/client-common/src/client.ts

Copilot

Pull request overview

Copilot reviewed 14 out of 14 changed files in this pull request and generated no new comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

packages/client-node/src/connection/stream.ts

peter-leonov-ch · 2026-02-19T21:21:53Z

@copilot check the code in this branch for cases where a call to logger (logWriter, log_writer, logger) can have a query directly or indirectly passed without checking for the unsafeLogUnredactedQueries parameter to be true.

Copilot · 2026-02-19T21:22:04Z

@peter-leonov-ch I've opened a new pull request, #565, to work on those changes. Once the pull request is ready, I'll request review from you.

packages/client-common/src/client.ts

mshustov · 2026-02-23T10:13:27Z

packages/client-common/src/config.ts

    },
    database: config.database ?? 'default',
    log_writer: new LogWriter(logger, 'Connection', config.log?.level),
+    log_level: config.log?.level ?? ClickHouseLogLevel.OFF,


IMO, default should be INFO

Fair point. OFF seems to be the default for the default LogWriter:

https://github.com/ClickHouse/clickhouse-js/blob/main/packages/client-common/src/logger.ts#L85

Do you want to change it and converge both to INFO?

Copilot confirms: https://github.com/ClickHouse/clickhouse-js/tasks/9ca5c8b1-2b5d-48c3-bcab-086836eee881?author=peter-leonov-ch

The current default is OFF

And also warns about suppressed warnings:

The current default log level is OFF. This means that when users create a client with no arguments, all logging is disabled - including warnings about deprecated configuration options.

Maybe we change the default to WARN instead and make sure the log levels in to code use WARN for noteworthy bits?

With WARN as default, I need to check what logs a successful ping() call generates. Looking at the other integration test, a successful ping produces a debug log, which would be suppressed at WARN level.

Looks like Copilot can draft something here for "free" 🙂

Maybe we change the default to WARN instead and make sure the log levels in to code use WARN for noteworthy bits?

do we have any logs with info level?

A quick grep shoes we do not.

packages/client-common/__tests__/unit/config.test.ts

packages/client-node/src/connection/node_base_connection.ts

Waiting on: * #520 ## Summary * adding tests to cover for the tricky connection related situations * augment logs with context required for debugging ## Checklist Delete items not relevant to your PR: - [x] Unit and integration tests covering the common scenarios were added - [x] A human-readable description of the changes was provided to include in CHANGELOG --------- Co-authored-by: Copilot <[email protected]>

mshustov requested a review from Copilot January 8, 2026 20:48

Copilot AI reviewed Jan 8, 2026

View reviewed changes

DO NOT MERGE. experimental tracing logging for command

cbb468b

peter-leonov-ch force-pushed the tracing-logging-command branch from 25ae82e to cbb468b Compare February 17, 2026 12:05

peter-leonov-ch added 9 commits February 17, 2026 21:03

add verbosity guard

8b3daf6

typo

0c8c2b8

introduce Go context-like context

ae06c96

let's see if it goes anywhere far

only log this with verbosity on

e013012

stricter context

2eb1844

allow logging queries under an "unsafe" flag

9cd5941

pass query_id as context down and use in conditional logs

ff28d7b

clean up a bit

c3ccbae

relax logger requirement and fix the tests

6a9fa11

peter-leonov-ch marked this pull request as ready for review February 18, 2026 09:55

peter-leonov-ch self-requested a review as a code owner February 18, 2026 09:55

peter-leonov-ch changed the title ~~DO NOT MERGE. experimental tracing logging for command~~ Experimental tracing logging for command Feb 18, 2026

peter-leonov-ch added 4 commits February 18, 2026 14:19

run tests in two log modes

dde649d

document logs and context

0aa9c91

pass log_verbose from the client to the connection

e085bc4

fix unit tests

f8b44c0

peter-leonov-ch force-pushed the tracing-logging-command branch from 921afc3 to f8b44c0 Compare February 18, 2026 16:12

peter-leonov-ch added 5 commits February 18, 2026 18:15

migrate to log_level

09d7d12

parse and pass the value from CI

540cd60

fix tests

319bd64

whoopsy

7d23a11

fix the dropping coverage

fe91d5d

peter-leonov-ch added 4 commits February 19, 2026 19:08

guard the query with the new option

5c3565b

guard more

43ae3e6

expose the now finished unsafeLogUnredactedQueries

cdc22d0

test this code branch too

3e03831

peter-leonov-ch requested a review from Copilot February 19, 2026 18:41

Copilot started reviewing on behalf of peter-leonov-ch February 19, 2026 18:42 View session

Copilot AI reviewed Feb 19, 2026

View reviewed changes

packages/client-common/src/client.ts Outdated Show resolved Hide resolved

peter-leonov-ch added 2 commits February 19, 2026 19:51

just use the connection params at this time

5bec14c

guard one more query

6998e66

peter-leonov-ch requested a review from Copilot February 19, 2026 19:29

Copilot started reviewing on behalf of peter-leonov-ch February 19, 2026 19:29 View session

Copilot AI reviewed Feb 19, 2026

View reviewed changes

peter-leonov-ch reviewed Feb 19, 2026

View reviewed changes

packages/client-node/src/connection/stream.ts Show resolved Hide resolved

peter-leonov-ch previously approved these changes Feb 19, 2026

View reviewed changes

Copilot AI mentioned this pull request Feb 19, 2026

Guard query logging with unsafeLogUnredactedQueries flag #565

Merged

2 tasks

mshustov commented Feb 23, 2026

View reviewed changes

packages/client-common/src/client.ts Outdated Show resolved Hide resolved

mshustov commented Feb 23, 2026

View reviewed changes

packages/client-common/src/client.ts Show resolved Hide resolved

mshustov commented Feb 23, 2026

View reviewed changes

packages/client-common/__tests__/unit/config.test.ts Outdated Show resolved Hide resolved

mshustov commented Feb 23, 2026

View reviewed changes

packages/client-node/src/connection/node_base_connection.ts Show resolved Hide resolved

use constants in tests

c216986

peter-leonov-ch dismissed their stale review via c216986 February 23, 2026 10:51

changed to a TODO

80f3608

peter-leonov-ch approved these changes Feb 23, 2026

View reviewed changes

peter-leonov-ch mentioned this pull request Feb 23, 2026

Provide more context in logs for connection and request handling #567

Merged

2 tasks

peter-leonov-ch merged commit 81d0e22 into main Feb 23, 2026
92 of 94 checks passed

peter-leonov-ch deleted the tracing-logging-command branch February 23, 2026 17:25

Conversation

mshustov commented Jan 8, 2026 • edited by peter-leonov-ch Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Checklist

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

codecov bot commented Feb 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

peter-leonov-ch commented Feb 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

peter-leonov-ch commented Feb 18, 2026

Uh oh!

peter-leonov-ch commented Feb 18, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

peter-leonov-ch commented Feb 19, 2026

Uh oh!

Copilot AI commented Feb 19, 2026

Uh oh!

Uh oh!

Uh oh!

mshustov Feb 23, 2026

Choose a reason for hiding this comment

Uh oh!

peter-leonov-ch Feb 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

peter-leonov-ch Feb 23, 2026

Choose a reason for hiding this comment

Uh oh!

mshustov Feb 23, 2026

Choose a reason for hiding this comment

Uh oh!

peter-leonov-ch Feb 23, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

mshustov commented Jan 8, 2026 •

edited by peter-leonov-ch

Loading

codecov bot commented Feb 17, 2026 •

edited

Loading

peter-leonov-ch commented Feb 18, 2026 •

edited

Loading

peter-leonov-ch Feb 23, 2026 •

edited

Loading