feat(client-v2): add span recorder SPI for operation and request tracing - #2988
Merged
Conversation
Adds a backend-agnostic observability SPI so an application can observe every client operation and every transport request as spans: - new public package com.clickhouse.client.api.observability with SpanRecorder, Span (both interfaces with no-op defaults and NOOP constants) and the SpanAttribute enum holding the attribute keys; - Client.Builder.setSpanRecorder(SpanRecorder) as the entry point; when unset (or null) nothing is recorded and no span work is done; - one operation span per query, command, insert, ping and getTableSchema, started on the calling thread so it joins the caller's ambient trace, and one child request span per transport attempt including retries. Attribute values are computed by the client (internal SpanSupport), so every recorder reports the same information. An OpenTelemetry implementation of the SPI follows in a separate module. Implements: #2974
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
chernser
reviewed
Jul 31, 2026
chernser
reviewed
Jul 31, 2026
chernser
reviewed
Jul 31, 2026
Client V2 CoverageCoverage Report
Class Coverage
|
JDBC V2 CoverageCoverage Report
Class Coverage
|
JDBC V1 CoverageCoverage Report
Class Coverage
|
Client V1 CoverageCoverage Report
Class Coverage
|
chernser
reviewed
Jul 31, 2026
chernser
reviewed
Jul 31, 2026
chernser
reviewed
Jul 31, 2026
chernser
reviewed
Jul 31, 2026
Applies the review requests on the observability SPI: - Span and SpanRecorder are plain interfaces without default methods; the new public DefaultSpanRecorder class implements them, holds the span that records nothing (DefaultSpanRecorder.NOOP_SPAN) and is the base class an implementation extends to record only what it cares about; - SpanSupport moved from com.clickhouse.client.api.internal to the public com.clickhouse.client.api.observability package and turned into an instance base class with overridable methods, so another implementation can reuse or extend the client's attribute values; - the single-endpoint special case is gone - an operation span reports the first configured endpoint, while every attempt keeps reporting its own endpoint on the request span. A recorder that returns no span can no longer break an operation, and a recorder that records nothing keeps the fast path. Tests cover a partial implementation, a recorder returning null, and using and extending SpanSupport from another package.
TriageCategory: Summary What this impacts
Concerns
Required reviewer action
|
…ration Addresses two review comments on #2988. The insert paths unregistered their transport request in a finally attached to every attempt of the retry loop, so between two attempts the query id resolved to nothing at all and cancelTransportRequest() had nothing to look up. Both insert paths now unregister once in a finally around the whole retry loop - the shape the query path already had. Covered by two tests: the registration is observed to survive the gap between two attempts through the public DataStreamWriter.onRetry() hook, and it is asserted to be gone once the operation ended for query, stream insert and POJO insert, both when the operation recovers on a retry and when it exhausts its retries. The HttpAPIClientHelper constructor overload added by this PR is removed, so the helper keeps a single constructor. Tests build it through a new HttpAPIClientHelperFactory instead, so a future constructor change is applied in one place.
Collaborator
Author
|
@chernser pushed
Verification: Still waiting on your call for one thing before I touch it: r3692397262 / r3692401134 - "operations should be done via span recorder, no need extra class: |
…servability-span-spi
Collaborator
Author
|
Merged Verification after the merge: @chernser the PR is still waiting on one design answer before it can be finished — review threads r3692397262 / r3692401134: whether the attribute-value computation stays in the reusable |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.
There are 3 total unresolved issues (including 2 from previous reviews).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit ee72d9c. Configure here.
The request span reported http.response.status_code only when the request returned or when the failure carried a ServerException, so the status was dropped for the responses the transport maps onto an exception that does not carry it: 407 (ClientMisconfigurationException), 502 and 503 (ConnectException) and an unknown status (ClientException). A trace of a gateway outage therefore had no status at all. The status is now recorded in the single place where a request is executed, directly after the response head arrives, so every response reports it.
chernser
reviewed
Aug 18, 2026
chernser
reviewed
Aug 18, 2026
chernser
reviewed
Aug 18, 2026
…servability-span-spi # Conflicts: # client-v2/src/main/java/com/clickhouse/client/api/Client.java # client-v2/src/test/java/com/clickhouse/client/api/transport/TransportBaseTests.java
Collaborator
Author
|
Rebased on Conflicts and how they were resolved
Verification (worktree on this branch, fresh ClickHouse server):
@chernser one open question still blocks the rest of this PR (threads r3692397262 / r3692401134): should the attribute-value computation stay in the reusable |
chernser
requested changes
Aug 18, 2026
chernser
left a comment
chernser
left a comment
Contributor
There was a problem hiding this comment.
- sync with main
- see comments and change the code.
- add instructions instead of heavy refactoring - span will start within existing methods. no need to wrap trying to deduplicate code. always check if deduplication causes wrapping exception handling or adds complexity.
…ion methods Per @chernser's review on #2988: - Revert the private queryImpl(sql, params, settings, operationName, collectionName) overload. ping() and getTableSchemaImpl() call query(...) again, unchanged from main. Every operation the client implements on top of a query is reported as a query; a recorder that wants to describe it differently derives that from the settings it is given. Drops the OPERATION_PING / OPERATION_GET_TABLE_SCHEMA constants, which no longer have a producer, and the two extra startQuerySpan parameters. - Revert the runOperation(...) wrapper. The three core methods call runAsyncOperation again, so no exception handling is added around them. - SpanSupport requires a non-null recorder, and Builder.setSpanRecorder rejects null: the default is already DefaultSpanRecorder.NOOP, so a null recorder is a configuration error, not a way to disable recording. Tests: SpanRecorderTest asserts ping and getTableSchema now report a plain query span with the statement they run; new cases pin the rejection of a null recorder on both the SPI class and the builder. docs/features.md and the CHANGELOG entry follow the code.
chernser
reviewed
Aug 18, 2026
chernser
requested changes
Aug 18, 2026
chernser
left a comment
chernser
left a comment
Contributor
There was a problem hiding this comment.
turn around SpanSupport and SpanRecorder: span recorder is things called first because it can be completely custom. SpanSupport can be used by implementation to fill common fields from known structures but its logic should be opt-in.
The client held a SpanSupport that wrapped the registered recorder: the client called the support, which computed the span name and every attribute and then asked the recorder only to start a span. An implementation could therefore not change any of those values. Turn the layering around, as requested in review: - The client now holds the SpanRecorder and calls it first, with everything it knows about the operation - the settings object, the statement, the target table, the batch size, the endpoint, the metrics of the completed operation, the failure and the HTTP status. - SpanSupport no longer wraps a recorder and holds no state. It derives the standard span names and attribute values from those same structures, and a recorder implementation calls it when it wants them. Using it is opt-in and every method may be overridden; DefaultSpanRecorder#getSpanSupport() is the entry point for a subclass. - SpanRecorder and Span stay plain interfaces without default methods; DefaultSpanRecorder keeps providing the no-op behaviour, and it now also answers the outcome methods. A recorder is never handed a span it did not create, and a request that is executed without a span reports nothing to the recorder. Tests assert the new layering: a recorder that ignores SpanSupport gets only the values it sets itself (its support accessor fails the test when the client reaches for it), a recorder that opts in gets the standard ones, and the values SpanSupport derives are pinned directly.
…servability-span-spi
Collaborator
Author
|
Pushed 72e6167 — the SPI layering is inverted as requested (details in the thread on Two residual points I did not change, because both follow from earlier revert requests. Say the word and each is a few lines:
|
chernser
approved these changes
Aug 18, 2026
|
6 tasks
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
2 participants
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.
Description
Implements #2974 — PR 1 of 2 (SPI +
client-v2instrumentation, no OpenTelemetry dependency), per the plan agreed in this comment and @chernser's approval ("This is good spec and lets implement in two PRs as proposed"). PR 2 will add the OpenTelemetry recorder as a separate Maven module.Adds a backend-agnostic observability SPI so an application can observe every client operation and every transport request as spans. Nothing is recorded and no span-related work is done unless a recorder is registered.
All three spec corrections from the review are applied:
class SpanAttributes→enum SpanAttribute;startSpanis overloaded onQuerySettings/InsertSettings— the recorder gets the resolved settings object instead of a long argument list and takes what it needs from it;SpanRecorderandSpanare interfaces whose methods all have no-op defaults, plusSpanRecorder.NOOP/Span.NOOP, so there are no null checks and a partial implementation is safe.Design
New public package
com.clickhouse.client.api.observability:Entry point:
Client.Builder.setSpanRecorder(SpanRecorder)(unset/null⇒ record nothing).query/execute/insert/ping/getTableSchema, started on the calling thread so it joins the caller's ambient trace even whenasync_operationsruns the operation on the client's executor; each transport attempt — including every retry — starts a child request span viastartRequestSpan(name, operationSpan). No OpenTelemetry type appears in any signature.setAttribute(String, Object)) rather than aSpanAttributeoverload, so an implementation cannot silently drop values (e.g.db.query.parameter.<key>) by overriding only one overload.SpanSupport, with the keys defined bySpanAttribute— so every recorder reports identical information; a recorder only maps them to its backend.<operation> <namespace>[.<table>]for an operation (e.g.query default,insert default.events) andPOSTfor a request span (all client requests are HTTP POSTs, so the name is known at creation and noupdateNamehook is needed).db.system.name,db.namespace,clickhouse.query_id,db.query.text(query/command only),db.query.parameter.<name>,db.operation.name(insert/ping/getTableSchema),db.collection.name(insert/getTableSchema),db.operation.batch.size(POJO insert),server.address/server.port(on the operation span when the client has a single endpoint, and per attempt on each request span),db.response.returned_rowson success,error.type+db.response.status_codeon failure, plushttp.request.method/http.response.status_codeon request spans.HttpAPIClientHelper.executeRequest(TransportRequest)rather than replacing it, so that method remains the single place a request is executed (and existing overrides of it still bind).Compatibility: purely additive — new package, one new builder method, one new
executeRequest(TransportRequest, Span)overload and one newHttpAPIClientHelperconstructor (the existing 1-arg/4-arg forms are kept and still used). No existing signature, default or behaviour changed; the default (no recorder) path is unchanged. Java 8 compatible. No new runtime dependency.Changes
client-v2/.../api/observability/{SpanRecorder,Span,SpanAttribute}.java— the SPI (new).client-v2/.../api/internal/SpanSupport.java— starts spans and computes all attribute values; every method short-circuits onSpanRecorder.NOOP/Span.NOOP(new).client-v2/.../api/Client.java—Builder.setSpanRecorder(...); operation spans on the query/command path (queryImpl, whichpingandgetTableSchemanow call with their own operation name/target so one span describes the operation the application actually called), the POJO insert path and the stream/writer insert path; spans started on the calling thread and ended exactly once, also on failure.client-v2/.../api/internal/HttpAPIClientHelper.java— per-attempt request span around request execution, with HTTP status and failure recording.CHANGELOG.md,docs/features.md.Size note (
AGENTS.mdscope discipline): ~750 LOC of production code (517 new + 241 modified) in one module, which is the first of the two slices agreed on the issue.Test
New tests only; no existing test edited or weakened.
SpanRecorderUnitTest(11 cases, WireMock/dead endpoint): statement text +db.query.parameter.*+ query id + endpoint + HTTP status on a successful query; POJO insert reportsdb.operation.batch.size, stream insert does not; one request span per attempt via@DataProvider(0/1/3 retries ⇒ 1/2/4 request spans), each a child of the one operation span, each ended exactly once, all carryingerror.type; per-attemptserver.address/server.porton failover (and no endpoint on the operation span when several are configured); insert failure records the error on the operation and every request span;useAsyncRequests(true)starts the operation span on the caller's thread while the request runs on the executor;setSpanRecorder(null)records nothing; the SPI default methods returnNOOP.SpanRecorderTest(6 integration cases, real server):db.response.returned_rowsand the server-assigned query id; a server error recordserror.type=…ServerException,db.response.status_code=60and the HTTP status on both the operation and the request span;ping,getTableSchema,getTableSchemaFromQueryand a command each produce the expected span name/attributes (including a contrast assertion that a named operation reports nodb.query.text).mvn -pl client-v2 test→ 540 passed;mvn -pl client-v2 -DskipUTs=true -Dit.test=SpanRecorderTest,QueryTests,InsertTests,CommandTests,MetadataTests verify→ 147 passed;mvn -pl jdbc-v2,packages/clickhouse-jdbc-all -am -DskipTests install→ OK.Docs / surface
CHANGELOG.md: entry under0.11.0-rc1→ New Features, tagged**[client-v2]**, with the issue link.docs/features.md: newclient-v2bullet describing the SPI, the span shape, every attribute, the span lifetime and the zero-overhead default.0.11.0-rc1). No backport needed.docs/changes_checklist.mdwalk-throughBuilder.setSpanRecorder,executeRequest(TransportRequest, Span), newHttpAPIClientHelperctor): names/params follow the module's existing builder and helper patterns (setSSLContext,registerClientMetrics); nullability is explicit (nullrecorder ⇒ record nothing); behaviour-focused tests added;docs/features.mdupdated as the checklist requires for a user-visibleclient-v2feature. Interface additions carry defaults, so implementors stay source- and binary-compatible.SpanAttribute): brand-new enum, keys unique and not used for parsing, serialization or persisted values.catchblocks that rethrow the original exception, and retry classification (shouldRetry) is untouched.NOOPreference comparisons and null/port > 0checks before recording an attribute; the retry loops' control flow is unchanged (verified by the retry-count tests and the untouched failover/insert suites).Pre-PR validation gate
docs/features.md+CHANGELOG.mdupdatedAGENTS.md,docs/ai-review.mdanddocs/changes_checklist.mdFollow-ups
clickhouse-jdbc-allwiring, in-memory-exporter tests).jdbc-v2surfacing:jdbc-v2builds itsClientfrom string properties, so injecting a recorder instance needs its own small decision — aspanRecorderdriver property naming a class to instantiate, or a setter onDataSourceImpl. Happy to add whichever you prefer in a follow-up; @chernser, which would you like?