feat(client-v2-otel): add OpenTelemetry span recorder module - #3065
Open
polyglotAI-bot wants to merge 2 commits into
Open
feat(client-v2-otel): add OpenTelemetry span recorder module#3065polyglotAI-bot wants to merge 2 commits into
polyglotAI-bot wants to merge 2 commits into
Conversation
Adds the optional module client-v2-otel with OpenTelemetrySpanRecorder, an implementation of the client-v2 observability SPI that reports operation and transport-request spans to OpenTelemetry. The recorder derives every span name and attribute through SpanSupport, so it reports the standard values, and maps them onto OpenTelemetry: CLIENT spans, an operation span under the current context, a request span per attempt under its operation span, typed attributes, and ERROR status plus an exception event on failure. Implements: #2974
Client V2 CoverageCoverage Report
Class Coverage
|
JDBC V2 CoverageCoverage Report
Class Coverage
|
JDBC V1 CoverageCoverage Report
Class Coverage
|
Client V1 CoverageCoverage Report
Class Coverage
|
Contributor
There was a problem hiding this comment.
Pull request overview
Adds an optional OpenTelemetry implementation of the client-v2 span recorder SPI for issue #2974.
Changes:
- Adds
OpenTelemetrySpanRecorderwith typed attributes, nesting, failures, and idempotent completion. - Adds unit and integration coverage.
- Registers and documents the new Maven module.
Compatibility is additive; existing client-v2 dependencies remain unchanged. The request URL contract and lazy-global test coverage remain unresolved. Author-reported tests were not independently rerun.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
pom.xml |
Registers the module and OpenTelemetry version. |
client-v2-otel/pom.xml |
Defines dependencies and Java 8 compilation. |
OpenTelemetrySpanRecorder.java |
Implements the OpenTelemetry recorder. |
OpenTelemetrySpanRecorderUnitTest.java |
Tests recorder behavior and edge cases. |
OpenTelemetrySpanRecorderTest.java |
Adds live-server tracing tests. |
docs/features.md |
Documents features and compatibility traits. |
CHANGELOG.md |
Announces the new module. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| public Span startRequestSpan(Span operationSpan, String host, int port) { | ||
| SpanSupport support = getSpanSupport(); | ||
| OpenTelemetrySpan span = startSpan(support.requestSpanName(), parentContextOf(operationSpan)); | ||
| support.fillRequestAttributes(span, host, port); |
Collaborator
Author
There was a problem hiding this comment.
Confirmed as a real gap, but it cannot be closed inside this module: SpanRecorder.startRequestSpan(Span, String host, int port) (merged in #2988) is never given the URI, and there is no url.full key in SpanAttribute. Reconstructing it here from host+port is not possible — the path and the query string are unknown to the recorder.
@chernser — this is a decision on the SPI you just approved, so I do not want to change it unilaterally. Options:
- Extend the SPI now, before 0.10.0 ships. Add
SpanAttribute.URL_FULL("url.full"), widenstartRequestSpanto also take the request URI, fill it inSpanSupport.fillRequestAttributes, and passHttpPost#getUri()fromHttpAPIClientHelper. The SPI is unreleased (0.10.0-rc1-SNAPSHOT), so widening the existing method is still free; adding an overload instead is not, becauseSpanRecorderis a plain interface with no default methods, so every implementor would have to implement both. This is a client-v2 change on top of an otel-module PR — I would send it as its own small PR rather than bundle it here. - Record only the sanitized URL (scheme + host + port + path, query string dropped). The ClickHouse HTTP query string carries settings, query parameters and, in some setups, credentials; semconv itself requires
url.fullto be sanitized. Same SPI change as (1), just a narrower value. - Amend the requirement. Keep
http.request.method+server.address+server.portas the request-span attributes and state indocs/features.mdwhyurl.fullis omitted, for the reason above.
My recommendation is (2): it satisfies the issue's request-span contract, and it avoids exporting a query string that can contain sensitive values. Tell me which you prefer and I will follow up — (1)/(2) as a separate client-v2 PR with the integration assertion, or (3) as a docs line here.
- INSTRUMENTATION_SCOPE_NAME is documented as the default scope name, and the javadoc now states that forTracer(Tracer) reports the scope of the given tracer instead. - Add a test that creates the no-argument recorder before the global SDK is installed and asserts that a span started afterwards reaches that SDK. It fails if the global instance is read in the constructor.
TriageCategory: Summary What this impacts
Concerns
Required reviewer action
|
|
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
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 2 of 2, on top of the SPI merged in #2988.
Adds the optional module
com.clickhouse:client-v2-otelwithOpenTelemetrySpanRecorder, a consumer of the merged SPI. It is registered like any other recorder:No change to
client-v2code: the module only implementsSpanRecorderand opts in toSpanSupportfor the standard names and attributes, so it reports the same information as any other recorder.Design
OpenTelemetrySpanRecorder extends DefaultSpanRecorder(packagecom.clickhouse.client.api.observability.otel). Everystart...method takes the name fromSpanSupport(querySpanName/insertSpanName/requestSpanName) and the attributes fromfill*Attributes; everyrecord...method delegates to the matchingSpanSupportmethod. Nothing is recomputed here.new OpenTelemetrySpanRecorder(openTelemetry),OpenTelemetrySpanRecorder.forTracer(tracer)(application-chosen instrumentation scope), ornew OpenTelemetrySpanRecorder()forGlobalOpenTelemetry. The global instance is read when a span is started, not in the constructor, so a client may be built before the application installs its SDK (reading it too early would pin the no-op instance and make a laterGlobalOpenTelemetry.set(...)throw). Default scope name:com.clickhouse.client.Context.current(), so it joins the application's ambient trace; each request span — one per attempt, including retries — is started under its operation span's context. A request span whose operation span was not created by this recorder falls back to the current context instead of failing. Both kinds areSpanKind.CLIENT.String→ string,Boolean→ boolean,Double/Float→ double, any otherNumber→ long, anything else →String.valueOf. Anullkey or value records nothing.setErrorsets statusERRORand recordserror.type;recordFailure/recordRequestFailureadditionally record the throwable as an OpenTelemetry exception event, so the message and stack trace are not lost (the SPI hands the recorder the throwable;error.typealone drops everything but the class name).end()is idempotent (AtomicBoolean), matching the SPI contract; the recorder holds no per-operation state and is thread-safe.Dependency placement.
opentelemetry-apiis a normal dependency of this module only;client-v2is untouched and still needs no OpenTelemetry on the classpath, which is the issue's "no new runtime dependency" constraint. The module is not added topackages/clickhouse-jdbc-all— that would shade OpenTelemetry into the uber-jar for every JDBC user, andjdbc-v2has no way to configure a recorder yet (see Follow-ups). Say the word if you want it in the package anyway; it is a two-line change.Compatibility: purely additive — a new module and one new public class. No existing signature or behaviour changed. Java 8 (
release 8).Changes
client-v2-otel/pom.xml— new module: depends onclient-v2+opentelemetry-api; test scope addsopentelemetry-sdk,opentelemetry-sdk-testing, TestNG and theclickhouse-clienttest-jar for the integration harness.client-v2-otel/.../observability/otel/OpenTelemetrySpanRecorder.java— the recorder and itsSpanimplementation.pom.xml— new<module>client-v2-otel</module>and theopentelemetry.versionproperty (1.51.0).CHANGELOG.md,docs/features.md— newclient-v2-otelsection, including the compatibility-sensitive traits (span kind/nesting, attribute typing, failure mapping, idempotent end, no span made current).Test
New tests only; no existing test edited or weakened.
OpenTelemetrySpanRecorderUnitTest(25 cases, in-memory exporter): query span name/kind/scope and every standard attribute; insert span withdb.collection.name+db.operation.batch.size, and a contrast case that a stream insert (BATCH_SIZE_UNKNOWN) records no batch size and an insert records nodb.query.text; request span is a child of the operation span withhttp.request.method/http.response.status_code/ per-attemptserver.address; two attempts under one operation span (failed attempt isERROR, retry isUNSET, operation staysUNSET); operation span joins an ambient trace; foreign operation span → current context (with and without an ambient span); client failure →ERROR+error.typeand no server error code;ServerException→error.type,db.response.status_code=60,http.response.status_code=404on the request span and the operation span; success records the query id anddb.response.returned_rows, and records nothing when metrics arenull; failure recorded as an exception event with type and message; idempotentend(); attribute typing via@DataProvider(8 rows: String / Boolean / int / long / short / double / float / other object);nullkey or value ignored;forTracerreports under the given scope name and version;nullOpenTelemetry/Tracerrejected.OpenTelemetrySpanRecorderTest(3 integration cases, real server): a successful query exports the operation span withdb.response.returned_rows=3, the server-assigned query id and a childPOSTspan with HTTP 200; a failing query exportsERROR+error.type=…ServerException+db.response.status_code=60on the operation span and HTTP 404 on the request span; a POJO insert exportsinsert <db>.<table>withdb.operation.batch.size=1and its child request span.mvn -pl client-v2-otel -DskipITs=true test→ 25 passed;mvn -pl client-v2-otel -DskipUTs=true -Dit.test=OpenTelemetrySpanRecorderTest verify→ 3 passed;mvn -pl client-v2 -DskipITs=true test→ 556 passed (unchanged);mvn -Dj8 -DskipTests install(full reactor, incl.jdbc-v2andpackages/clickhouse-jdbc-all) → BUILD SUCCESS.Docs / surface
CHANGELOG.md: entry under0.11.0-rc1→ New Features, tagged**[client-v2-otel]**, with the issue link. The feat(client-v2): add span recorder SPI for operation and request tracing #2988 entry's closing sentence now points at this module instead of announcing it as upcoming.docs/features.md: new## client-v2-otelsection with a feature list and compatibility-sensitive traits.0.11.0-rc1). No backport needed.docs/changes_checklist.mdwalk-throughOpenTelemetrySpanRecorderfollows the module's naming and the SPI's documented extension pattern (extendDefaultSpanRecorder, override what you record); nullability is explicit (nullOpenTelemetry/Tracerrejected withIllegalArgumentException,nullattribute key/value ignored);forTracer(...)is a static factory rather than a second constructor sonew OpenTelemetrySpanRecorder(null)cannot be an ambiguous call; behaviour-focused tests added;docs/features.mdupdated.opentelemetry-api), scoped to the new module only, version pinned by a parent property next to the other version properties.client-v2andclickhouse-jdbc-allgain nothing transitively.nullchecks and theinstanceoffallback for a foreign operation span; both are covered by tests.CI note (no workflow file touched, per
AGENTS.md)The whole-reactor
compilejob builds the new module and runs its unit tests, so they gate this PR. Two things need a workflow change, which I did not make:build.yml/test_head.ymlenumerate projects explicitly (project: ["clickhouse-http-client", "client-v2", …]), so the module's integration test does not run in CI untilclient-v2-otelis added to those matrices.release.ymlenumerates the jars attached to a release, so the new artifact must be added there before it is published.Tell me which you want and I will add it in a follow-up (or apply it here if you prefer a CI change in this PR).
Pre-PR validation gate
Client)docs/features.md+CHANGELOG.mdupdatedAGENTS.md,docs/ai-review.mdanddocs/changes_checklist.mdFollow-ups
jdbc-v2surfacing — still the open question from #2988:jdbc-v2builds itsClientfrom string properties, so injecting a recorder needs its own small decision (aspan_recorderdriver property naming a class to instantiate, or a setter onDataSourceImpl). @chernser which would you like? I kept it out of this PR so the recorder itself can land independently.