[client-v2, jdbc-v2] Remove sensitive and repetitive logging by LjungeldTheHelper · Pull Request #3300 · ClickHouse/clickhouse-java

LjungeldTheHelper · 2026-11-05T01:29:07Z

Summary

Part of #3295, limited to client-v2 and jdbc-v2. The logs of the client and the driver no longer carry
application data at any level (TRACE included) and no longer repeat per retry, request stream, statement, or
metadata object. The other modules (clickhouse-data, including the NonBlockingPipedOutputStream example from the
issue, and the V1 client/HTTP/JDBC, R2DBC, and so on) are not changed here and are left for a follow-up.

All 73 logging calls in client-v2/src/main and jdbc-v2/src/main (including the JavaCC grammar) were classified.
The changes:

Sensitive data removed

jdbc-v2 StatementImpl: both TRACE logs of the complete SQL (the source comment already said they expose
passwords).
jdbc-v2 JavaCC parser: the DEBUG log of the SQL and of the parser exception, which quotes the statement. The
generic WARN and the fallback stay; the WARN no longer says "Enable DEBUG logging for details".
jdbc-v2 ANTLR facade: the syntax-error message (it quotes tokens) and the table name are no longer logged; line
and position stay.
client-v2 WorkloadIdentitySupport: the long User-Agent WARN reports the length, not the value (client name
and metadata).
client-v2 ProcessParser: the server value and the exception are no longer logged.
client-v2 HttpAPIClientHelper: query IDs and the authority are no longer logged; a failure to read the server
error is logged by exception type (the message can echo the body).
jdbc-v2 prepared-statement metadata fallback: logs the exception type instead of the exception, whose server
message echoes the SQL with its literals.
jdbc-v2 getTables DEBUG of its arguments, cancel() query IDs, Query ID per statement.

Duplicates and repetition removed

Retries: no WARN per attempt and no WARN before the final exception. An operation that succeeds after retries
logs one WARN: Query succeeded after 2 retries; last retried failure: java.net.ConnectException. A failed
operation logs nothing extra; its exception reaches the caller. Metrics and spans still record every attempt.
ClientNodeSelector: Endpoint <host> quarantined only when an alive endpoint becomes quarantined, not for each
failure that extends the quarantine; All endpoints are non-responsive once per outage instead of once per
request.
Transport DEBUG log-and-rethrow in executeRequest (it logged authority and exception message), the
BinaryStreamReader log-and-throw, the PreparedStatementImpl and DatabaseMetaDataImpl ERROR-and-throw, and
the ResultSetImpl.close() logs of exceptions that it throws.
Per-request-stream DEBUG in the LZ4 stream constructors; per-endpoint DEBUG at build.
ShowStatementsMetaData: one DEBUG with the count of skipped tables/databases per call instead of one per
object; per-column type-parse DEBUG in metadata rows removed.

Severity

Swallowed close/cleanup failures: DEBUG with the exception type (ClientUtils.quietClose logs the closeable's
type, not its toString()).
ERROR only where the failure cannot reach the caller (driver (de)registration, connection abort on network
timeout). Metrics registration, executor shutdown, local host name, and async-config read failures are WARN.
POJO "no getter/setter for column" is DEBUG (a read-only or write-only POJO is normal).

ClientUtils.rootCauseClassName(Throwable) (internal package) names a failure without its message.

Compatibility

No public API, configuration key or default, protocol, serialization, retry, timeout, parsing, result, or JDBC
behavior changes. Exceptions and their messages are unchanged. The only observable difference is fewer and safer log
messages; code that parses the removed messages (for example the per-attempt ... Retrying. WARN) will no longer
see them. The no-HTTP-wire-logging behavior and its tests (#3252) are preserved and extended.

Tests

HttpAPIClientHelperTest.testNoWireOrSensitiveLoggingAtTrace: every client-v2 logger at TRACE, wire logger at
DEBUG, pooled/unpooled, HTTP/HTTPS, Basic/password header/Bearer, failover from a dead endpoint, success and a
server error that echoes the SQL. No password, Basic/Bearer token, header value, SQL, body, error body, URL
setting value, client name, client metadata, or query ID in the log; the server error still reaches the caller.
ClientFailoverUnitTest.testRetryLoggedOncePerOperation: recovered → exactly one summary naming the failure type;
failed → none; one quarantine WARN; no query ID or endpoint address.
HttpAPIClientHelperTest: the User-Agent WARN has the length, not the value; the unknown-status WARN has
neither the query ID nor the authority.
StatementLoggingTest (WireMock, no server user created): Statement with a password literal and
PreparedStatement with a bound value, JavaCC/ANTLR4/ANTLR4_PARAMS_PARSER, success and server error, driver and
client loggers at TRACE: neither the SQL nor the value is logged.
ClickHouseSqlParserLoggingTest: at TRACE, no SQL token or parser detail; the fallback and the WARN stay.
Updated integration tests: DatabaseMetaDataTest.testMetadataCallsLogNoPatterns (was asserting the getTables
arguments were logged), JDBCErrorHandlingTests.testPreparedStatementMetadataFallbackLogsNoSql.
ClientUtilsTest: quietClose at DEBUG with the type; rootCauseClassName data provider.

The new tests fail against main (checked by reverting the main sources: 5/5 in jdbc-v2, 10 in client-v2).

Run locally (JDK 17, JDK 8 toolchain):

mvn -pl client-v2 -am test: client-v2 830 tests, 0 failures
mvn -pl jdbc-v2 test (after -am install -DskipTests): 1979 tests, 0 failures. mvn -pl jdbc-v2 -am test fails
in the reactor with module-info.java: module not found on main as well, so dependencies were installed first.
Integration (Testcontainers ClickHouse): DatabaseMetaDataTest#testMetadataCallsLogNoPatterns+testGetColumns*,
JDBCErrorHandlingTests: 13 tests, 0 failures
client-v2 main sources compile with -Dj8 (release 8).

Checklist (`docs/changes_checklist.md`, Logging)

Level matches the situation: ERROR only where the caller cannot see the failure, WARN for degraded states,
DEBUG for bounded diagnostics. ✔
No log in a row, value, retry, polling, or per-stream path. ✔
Exceptions are attached only where their content is known to be safe; otherwise the type is logged. ✔ (A
deliberate exception to "attach the throwable": the message is the leak.)
No secrets, credentials, tokens, SQL values, or payloads. ✔
No expensive string building when disabled (string concatenation in log calls replaced with placeholders). ✔
Retry paths: one summary per operation; logs, backoff, and classification reviewed together. ✔
No duplication of errors already propagated with context. ✔

Checklist

Part of [logging] Review Logging #3295 (client-v2 and jdbc-v2 only; the issue stays open for the other modules)
Unit and integration tests covering the common scenarios were added
A human-readable description of the changes was provided to include in CHANGELOG (history/latest/3295.md)
docs/features.md updated; user documentation in the follow-up docs PR

🤖 Generated with Claude Code

Note

Low Risk
Runtime behavior and thrown exceptions are unchanged; the main impact is observability—monitoring or tooling that parsed removed log formats will need updating.

Overview
client-v2 and jdbc-v2 logging is tightened so logs do not carry SQL, credentials, query IDs, full exception messages, endpoint addresses (except host on quarantine), User-Agent values, or other application data—even at TRACE.

Retry and failover behavior in logs: Per-attempt WARN messages with query IDs and endpoints are removed. Successful operations after retries emit a single summary (logRecoveredRetries) with retry count and root exception type via new ClientUtils.rootCauseClassName. Endpoint quarantine and “all endpoints down” warnings are deduplicated in ClientNodeSelector.

JDBC: Statement SQL TRACE, parser SQL/DEBUG detail, metadata argument logging, and similar paths are removed or reduced to generic or type-only messages. Cleanup and close failures move to DEBUG with exception types; several former ERROR logs on paths that already throw are dropped.

Docs/tests: docs/features.md, history/latest/3295.md, and broad WireMock/TRACE logging tests document and enforce the policy. Public API, retries, metrics, and spans are unchanged aside from log output.

^{Reviewed by Cursor Bugbot for commit d13beae. Bugbot is set up for automated code reviews on this repo. Configure here.}

…ckHouse#3295) Logs of client-v2 and jdbc-v2 no longer carry SQL, bound values, credentials, query IDs, client metadata, endpoint addresses or exception messages at any level, and no longer repeat per retry, request stream, statement or metadata object. - client-v2: one WARN per operation that recovers after retries instead of one per attempt plus one before the throw; endpoint quarantine and the all-endpoints-down fallback logged once; transport log-and-rethrow removed; User-Agent warning reports the length only; per-stream compressor DEBUG removed; swallowed failures logged by type. - jdbc-v2: SQL TRACE and JavaCC DEBUG of the SQL and parser error removed; per-statement query ID, metadata argument and per-object logs removed or aggregated; log-and-throw duplicates removed. - Tests: canaries at TRACE for both modules, retry log volume. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

chernser · 2026-11-05T02:38:08Z

                result.put(key, value);
            } catch (NumberFormatException e) {
-                LOG.debug("Skipping summary field '" + key + "' with non-numeric value '" + valueStr + "'", e);
+                LOG.debug("Skipping summary field '{}' with a non-numeric value", key);


move to trace level. This minor problem.

CLAassistant · 2026-11-05T13:38:35Z

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

chernser · 2026-11-06T19:19:54Z

        return str == null || str.trim().isEmpty();
    }

    public static void quietClose(Closeable closeable, Logger log) {


make this final to indicate that closeable never change

chernser · 2026-11-06T19:22:50Z

            }
        } catch (Exception e) {
-            LOG.error("Failed to get local host name or address", e);
+            LOG.warn("Failed to get local host name or address: {}", ClientUtils.rootCauseClassName(e));


do not do like this. We need to have stacktrace for exception like before. Just exception class name is useless. keep log warn but also preserve exception stack in log - here is should be very short

chernser · 2026-11-06T19:24:30Z

                throw e;
            } catch (Exception e) {
-                LOG.warn("Failed to read error message (queryId = " + queryId + ")", e);
+                LOG.warn("Failed to read the server error message (HTTP status {}): {}", httpCode,


again - we should have stacktrace here. exception here might not have any sensitive information .

chernser · 2026-11-06T19:25:58Z

                    throw e;
                }
-                logIncompleteBodyOnClose(getQueryId(), e);
+                logIncompleteBodyOnClose(e);


why did you removed queryId ? this should present - may be we need to extend logic to grab it from different places, but it should present.
When queryId is missing - we should place something like unknown

chernser · 2026-11-06T19:26:10Z

        public InputStream createDataInputStream() {
            try {
-                InputStream stream = new ResponseBodyInputStream(delegate.getEntity().getContent(), getQueryId());
+                InputStream stream = new ResponseBodyInputStream(delegate.getEntity().getContent());


queryId should be present here

chernser · 2026-11-06T19:26:46Z

-        private final String queryId;
-
-        ResponseBodyInputStream(InputStream in, String queryId) {
+        ResponseBodyInputStream(InputStream in) {


queryId should be present - do not try to drop it - it is not sensitive information

chernser · 2026-11-06T19:27:07Z

                super.close();
            } catch (ConnectionClosedException e) {
-                logIncompleteBodyOnClose(queryId, e);
+                logIncompleteBodyOnClose(e);


queryId should be present in log - this is whole feature

chernser · 2026-11-06T19:28:15Z

-        } catch (ConnectException | NoRouteToHostException e) {
-            LOG.debug("Failed to connect to '{}': {}", req.getAuthority(), e.getMessage());
-            throw e;
        } catch (Exception e) {


return removed catch branches - they are for purpose.

chernser · 2026-11-06T19:28:38Z

        }
        final Header exceptionCodeHeader = httpResponse.getFirstHeader(ClickHouseHttpProto.HEADER_EXCEPTION_CODE);
-        LOG.warn("Server returned error response: status={}, queryId='{}', authority='{}', {}={}",
+        LOG.warn("Server returned error response: status={}, {}={}",


queryId and authority should be present

chernser · 2026-11-06T19:34:43Z

            if (stmt.isInsert()) {
-                LOG.debug("Assuming a function in the values list of an insert into {} that could not be parsed without errors",
-                        stmt.getTable());
+                LOG.debug("Assuming a function in the values list of an insert that could not be parsed without errors");


Table should be present. log should be trace

chernser · 2026-11-06T19:35:17Z

                ClickHouseDataType dt = c.getDataType();
                type = JdbcUtils.convertToSqlType(dt);
            } catch (Exception e) {
-                log.debug("Failed to convert column data type to SQL type: " + typeName, e);


make it trace - do not remove.

chernser · 2026-11-06T19:36:40Z

-                    // Don't log the raw SQL (may carry sensitive literals); the cause is enough to diagnose.
+                    // Only the type of the failure: the SQL and the server message (which can echo it) may carry
+                    // sensitive literals.
                    LOG.warn("Failed to resolve result-set metadata for prepared statement; "


make it debug or even trace. But preserve exception stack trace

chernser · 2026-11-06T19:37:07Z

                try {
                    reader.close();
                } catch (Exception re) {
-                    log.debug("Error closing reader", re);


leave it as trace

actually, leave it - it is not frequent error but if happens we need to know enough context to fix.

chernser · 2026-11-06T19:38:19Z

                currentResultSet.close();
            } catch (Exception e) {
-                LOG.error("Failed to close previous result set", e);
+                LOG.debug("Failed to close previous result set: {}", ClientUtils.rootCauseClassName(e));


preserve exception stacktrace.

chernser · 2026-11-06T19:39:13Z

                    writtenRows = response.getWrittenRows();
                } catch (Exception e) {
-                    LOG.debug("Failed to read written-rows count from response; defaulting to 0", e);
+                    LOG.debug("Failed to read written-rows count from response; defaulting to 0: {}",


just ignore exception - no need to get root cause here

chernser · 2026-11-06T19:39:29Z

                    reader.close();
                } catch (Exception closeEx) {
-                    LOG.warn("Failed to close reader when schema is null", closeEx);
+                    LOG.debug("Failed to close reader when schema is null: {}",


here we need to know stacktrace

chernser · 2026-11-06T19:39:44Z

                    response.close();
                } catch (Exception ex) {
-                    LOG.warn("Failed to close response after exception", ex);
+                    LOG.debug("Failed to close response after exception: {}", ClientUtils.rootCauseClassName(ex));


here we need to know stacktrace, too

chernser · 2026-11-06T22:21:46Z

                    resultSet.close();
                } catch (Exception e) {
-                    LOG.error("Failed to close result set", e);
+                    LOG.debug("Failed to close result set: {}", ClientUtils.rootCauseClassName(e));


need exception stack

chernser · 2026-11-06T22:22:09Z

                        ClientConfigProperties.ASYNC_OPERATIONS.getDefaultValue()));
            } catch (Exception e) {
-                LOG.error("Failed to read client configuration " + ClientConfigProperties.ASYNC_OPERATIONS.getKey(), e);
+                LOG.warn("Failed to read client configuration {}", ClientConfigProperties.ASYNC_OPERATIONS.getKey(), e);


exception stack ?

chernser · 2026-11-06T22:22:57Z

-                log.debug("Failed to parse SQL: '" + sql + "'", e);
+                // Neither the SQL nor the parser error is logged at any level: the error quotes the statement,
+                // which can carry credentials or other sensitive literals.
+                log.warn("Failed to parse SQL. If you believe the SQL is valid, please feel free to open an issue on GitHub with this warning and the statement, with any sensitive values removed.");


remove this log at all.

chernser · 2026-11-06T22:25:34Z

+     * Logs one message per metadata call, with the count only: the names of the objects are not logged, and a call
+     * over many objects does not log one message per object.
+     */
+    private static void logSkipped(int skipped, String objects) {


no need to log this.

chernser

correct comments

LjungeldTheHelper requested review from chernser and mzitnik as code owners November 5, 2026 01:29

LjungeldTheHelper mentioned this pull request Nov 5, 2026

[docs] What client-v2 and jdbc-v2 log, and a safe logging configuration #3301

Open

2 tasks done

chernser reviewed Nov 5, 2026

View reviewed changes

chernser reviewed Nov 6, 2026

View reviewed changes

chernser requested changes Nov 6, 2026

View reviewed changes

Conversation

LjungeldTheHelper commented Nov 5, 2026 • edited by cursor Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Compatibility

Tests

Checklist (docs/changes_checklist.md, Logging)

Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CLAassistant commented Nov 5, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chernser left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

LjungeldTheHelper commented Nov 5, 2026 •

edited by cursor Bot

Loading

Checklist (`docs/changes_checklist.md`, Logging)