feat(client-v2): add span recorder SPI for operation and request tracing - #2988
Open
polyglotAI-bot wants to merge 3 commits into
Open
feat(client-v2): add span recorder SPI for operation and request tracing#2988polyglotAI-bot wants to merge 3 commits into
polyglotAI-bot wants to merge 3 commits into
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}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 3 potential issues.
❌ 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 822109d. Configure here.
| } finally { | ||
| // unregister transport request once we are done | ||
| unregisterTransportReq(queryId); | ||
| operationSpan.end(); |
There was a problem hiding this comment.
Query retries skip transport unregister
Medium Severity
In queryImpl, the retry loop no longer calls unregisterTransportReq after each attempt, unlike the POJO and stream insert paths. A failed attempt can leave the prior TransportRequest in ongoingRequests until the next registerTransportReq or the outer finally, which changes cancelTransportRequest and retry-time cancellation behavior for queries with retries.
Reviewed by Cursor Bugbot for commit 822109d. Configure here.
Collaborator
Author
There was a problem hiding this comment.
This PR does not change the unregister semantics of the query path. On main, queryImpl already calls unregisterTransportReq(queryId) exactly once, in the finally of the block that wraps the whole retry loop - not per attempt (see Client.java on main, the finally { // unregister transport request once we are done ... } after the for loop). This PR only added catch/operationSpan.end() next to that same finally, so ongoingRequests behaves exactly as before for queries with retries.
The asymmetry you noticed between the query path and the insert paths is real but pre-existing, and @chernser reads it the other way round in #discussion_r3692392021: the per-attempt unregisterTransportReq in the POJO insert path is the bug, and the single outer unregister (what the query path already does) is the intended shape. I will align the insert paths with that in the follow-up round rather than adding per-attempt unregister calls to queries.
| try { | ||
| CompletableFuture<QueryResponse> future = query("SELECT 1 FORMAT TabSeparated"); | ||
| CompletableFuture<QueryResponse> future = queryImpl("SELECT 1 FORMAT TabSeparated", null, null, | ||
| SpanSupport.OPERATION_PING, null); |
There was a problem hiding this comment.
Timeout leaves operation span open
Low Severity
With async_operations, callers that use CompletableFuture.get(timeout, …)—for example ping(long) and getTableSchemaImpl—can return on TimeoutException while the operation still runs on the executor. The operation span is ended only in the supplier’s finally, so traces can show a long-lived or later-successful span after the caller already timed out.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 822109d. Configure here.
Collaborator
Author
There was a problem hiding this comment.
This is intended rather than a leak: the span measures the client operation, and after future.get(timeout, ...) returns the operation genuinely is still running - it will finish and end its span in the suppliers finally. Ending the span early when the caller stops waiting would report a duration that is not the operations, and would also mean writing to a span the still-running operation keeps using (it would then be ended twice). The callers own timeout is visible in the callers span, which the operation span is a child of.
The span lifetime is documented on Span ("An operation span covers sending the request and receiving the response head; it is ended when the operation hands its response to the caller"), and every span is ended exactly once - asserted by getEndCount() == 1 in the unit tests. No code change made.
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
| if (httpClientHelper.shouldRetry(e, requestSettings.getAllSettings()) && requestIsNotCancelled(queryId)) { | ||
| if (i < maxAttempts) { | ||
| selectedEndpoint = logRetryAndSelectNextNode("Insert", i, maxAttempts, requestSettings.getQueryId(), selectedEndpoint, e); | ||
| SpanSupport.recordSuccess(operationSpan, metrics); |
Contributor
There was a problem hiding this comment.
operations should be done via span recorder. no need extra class
spanRecorder.recordSuccess()
spanRecorder.recordError()
Collaborator
Author
There was a problem hiding this comment.
I want to make sure I build the shape you mean here, because this and #discussion_r3692365724 point at slightly different ones and I have just done the latter (moved SpanSupport into com.clickhouse.client.api.observability as an instance base class).
My reading of the two together: the recording operations belong on the recorder, and the "base class in the observability package" is the recorder base class - i.e. fold SpanSupport into DefaultSpanRecorder:
public interface SpanRecorder {
Span startSpan(String spanName, QuerySettings settings);
Span startSpan(String spanName, InsertSettings settings);
Span startRequestSpan(String spanName, Span operationSpan);
void recordSuccess(Span span, OperationMetrics metrics);
void recordError(Span span, Throwable t);
// + recordHttpStatus(Span, int) / recordEndpoint(Span, String, int) for request spans
}
// base class: computes all the attribute values, so every recorder reports the same keys/values
public class DefaultSpanRecorder implements SpanRecorder { ... }and Client/HttpAPIClientHelper then call spanRecorder.startSpan(...), spanRecorder.recordSuccess(span, metrics), spanRecorder.recordError(span, e) with no helper class in between. An implementation extends DefaultSpanRecorder, gets the standard attributes for free, and overrides only span creation (that is what the OTel recorder in PR 2 needs).
Is that what you want? The one thing I want to confirm before moving it, since it changes the SPI surface: the record* methods then become part of the public interface an implementation can override, so a recorder can deviate from the standard attribute values - whereas the current split guarantees every recorder reports the same ones. If you prefer that guarantee, the alternative is to keep the value computation in SpanSupport and have DefaultSpanRecorder delegate to it. Happy to do either - just say which, and I will do it together with the two items below (try/unregisterTransportReq and the constructor overload) in one push.
chernser
reviewed
Jul 31, 2026
|
|
||
| final int maxRetries = ClientConfigProperties.RETRY_ON_FAILURE.getOrDefault(requestSettings.getAllSettings()); | ||
| final int maxAttempts = Math.max(maxRetries, endpoints.size() - 1); | ||
| final Span operationSpan = SpanSupport.startInsertSpan(spanRecorder, requestSettings, tableName, |
Collaborator
Author
There was a problem hiding this comment.
Same as #discussion_r3692397262 - will switch this call site to the recorder once you confirm the SPI shape there (spanRecorder.recordSuccess(...) / recordError(...) vs. keeping the value computation in a separate class that the default recorder delegates to).
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: |
|
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?