Failing check:

build / common / test0 (8, hotspot, indy true): java.lang.ClassNotFoundException: io.opentelemetry.javaagent.instrumentation.clickhouse.clientv2.v0_8.ClickHouseClientV2Singletons$EndpointTarget

Cause: The test used Class.forName(String), which asks the application class loader for a helper that indy mode isolates in the instrumentation class loader.

Fix: Load the nested class with the singleton's defining class loader so both standard and indy test modes can resolve it.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot comment:

[General] In CurrentServerInfo.of(), building the initial peer with DbServerTarget.builder(extracted.defaultPort()) can drop the port when the endpoint uses its default port (e.g., http/https without an explicit port). Since this peer feeds network.peer.port, it can end up unset even though the effective port is known; consider always storing an explicit peer port here (or leaving peer null when the port can’t be determined).

Analysis: CurrentServerInfo treated the effective port as a DbServerTarget default. DbServerTarget omits default ports, so network.peer.port was absent for an endpoint such as http://single.example. The code now passes the effective port as an explicit port and omits the peer when neither the endpoint nor its scheme supplies a valid port.

Upsides: network.peer.port now reports known default ports, while unknown ports do not create an incomplete peer.

Downsides: No material downside identified.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot comment:

[General] The ContextKey name "clickhouse-db-request" is very generic; using a more namespaced key string (matching the common "opentelemetry-…" pattern used elsewhere) reduces the chance of cross-instrumentation collisions when multiple libraries use ContextKey.named(...).

Analysis: ContextKey equality uses the key name. Prefixing this instrumentation-owned key with "opentelemetry-" follows the existing repository convention and distinguishes it from application or library keys.

Upsides: The renamed key is less likely to collide with another ContextKey in the same context.

Downsides: No material downside identified.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot comment:

[General] `selectedNode.getHost()` is already a bare host, but parsing it as an endpoint corrupts unbracketed IPv6 values: for example, `2001:db8::1` is interpreted as address `2001:db8::` with an embedded port of `1`. The separately obtained selected-node port remains correct, so this emits the wrong stable `network.peer.address`. Pass the host directly to `DbServerTargetBuilder`, which already validates IPv6 hosts.

Analysis: The selected node exposes the host and port separately. Endpoint parsing can treat the last IPv6 segment as a port, while DbServerTargetBuilder accepts and validates the bare IPv6 host directly.

Upsides: Stable network peer attributes preserve the full selected IPv6 address and its separately reported port.

Downsides: No material downside identified.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Review finding:

capturePeer resolves the getHost and getPort methods reflectively on every call. ClickHouseClientV2RequestInstrumentation applies RequestAdvice to HttpAPIClientHelper.executeRequest, executeMultiPartRequest, and createRequest, so capturePeer runs once per HTTP attempt, including every retry. Each run calls Class.getMethod twice, and getMethod searches the whole public method set and returns a fresh copy of the Method object every time, so the javaagent adds two method searches and two allocations to every ClickHouse request. The selected node is one of only two classes, com.clickhouse.client.ClickHouseNode or com.clickhouse.client.api.transport.Endpoint, and neither can be named directly because they do not both exist across the muzzle range [0.6.4,), so reflection itself is justified while repeating the lookup is not. Fix: resolve the two Method objects once per class and reuse them, for example by caching them in a ClassValue or a Cache keyed by the selected node class, then invoke the cached handles in capturePeer.

Analysis: capturePeer runs at the HTTP boundary, so it runs once for every request attempt and once more for every retry. Class.getMethod walks the whole public method set and returns a fresh copy of the Method, so the old code paid for two searches and two allocations on each of those attempts. A ClassValue keyed by the selected node class resolves the two accessors once per class and hands back the same PeerAccessor afterwards. ClassValue stores its value on the key class itself, so it adds no collection that has to be sized, evicted, or made weak, and the Method objects it holds already keep that same class alive. PeerAccessor.UNAVAILABLE preserves the old behaviour for a selected node that exposes neither accessor: the request keeps whatever peer it already carries instead of being cleared.

Upsides: removes two reflective method searches and two allocations from every ClickHouse v2 request and retry; a selected node class whose accessors cannot be resolved now costs one failed lookup instead of one per attempt; the version-dependent reflection is named and confined to a single small class.

Downsides: adds a nested class and a ClassValue field to a file that previously held the lookup inline.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

…r host

Review finding:

peerServerTarget screens the host through the new extractPeerHost helper before handing it to DbServerTargetBuilder.addEndpoint, but addEndpoint already runs sanitizeHost, which accepts a host only when it validates as a host name, an IPv4 literal, or an IPv6 literal. isHostName allows only ASCII letters, digits, '-', and '_' inside a segment, so every character extractPeerHost screens for -- '/', '?', '#', ',', '=', '@', and inner whitespace -- is already rejected downstream. The screening therefore changes the outcome only where it is stricter than the builder, and there it discards hosts the builder handles correctly: sanitizeHost strips the brackets from '[::1]' and returns '::1', and isIpv6Literal accepts a zone id so 'fe80::1%eth0' survives, yet extractPeerHost rejects both because it looks for '[', ']', and '%'. Leading or trailing whitespace is the same story, since sanitizeHost trims it. The result is that v1 drops network.peer.address for exactly the IPv6 forms the builder was written to handle, while the same server.getHost() value still reaches server.address unscreened through addEndpoint two methods below in this file, and the v2 side passes its host straight to addEndpoint with no screening at all. Fix: delete extractPeerHost and the hasWhitespace helper it needs, and call DbServerTarget.builder(-1).addEndpoint(host, port).build() directly, letting DbServerTargetBuilder do the validation it already owns.

Analysis: DbServerTargetBuilder.addEndpoint validates every host through sanitizeHost, which accepts only a host name, an IPv4 literal, or an IPv6 literal. That validation already rejects every character the deleted helper looked for, so the helper changed the result only where it was stricter than the builder. It was stricter for a bracketed IPv6 literal, which sanitizeHost unwraps, and for an IPv6 zone id, which isIpv6Literal accepts, so v1 dropped network.peer.address for the two IPv6 forms the builder exists to handle. ClickHouseNode.getHost() returns both forms verbatim for an IPv6 endpoint. Passing the host straight through makes v1 agree with v2, which already calls addEndpoint with no extra screening, and with the addEndpoint helper further down this same file that feeds server.address from the same ClickHouseNode.getHost() value.

Upsides: v1 reports network.peer.address for a bracketed or zone-scoped IPv6 host instead of omitting it; one validation rule governs every host in this file and in the v2 module; removes a helper that duplicated shared logic and could drift from it.

Downsides: host validation now lives only in DbServerTargetBuilder, so a future change there reaches ClickHouse peers with no local screen in between. That is the intended contract of the shared builder, which validates without resolving names, and DbServerTargetTest covers both IPv6 forms.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

…path

Review finding:

testUnsafePeerEndpointIsOmitted never exercises the peer path. It reflects into EndpointTarget.parse and asserts that one string parses to null, which is the shared configured-endpoint parser that testConfiguredEndpointsRejectUnsafeOrMalformedValues already covers with the very similar 'http://host.example%3fpassword%3dsecret:8123'. Nothing in the test reaches CurrentServerInfo.of or reads getPeer(), so it would still pass if the peer path stopped omitting unsafe endpoints entirely, and the name claims coverage the test does not provide. The file already has the right tool for this: assertCurrentPeer drives CurrentServerInfo.of and asserts the resulting peer, and the two sibling tests testInitialPeerUsesEffectivePort and testInitialPeerWithoutKnownPortIsOmitted use it. Fix: replace the body with assertCurrentPeer(new HashSet<>(asList("http://host.example%3fsecret:8123")), null, null), which asserts the peer is omitted through the code the name describes, and drop the now-unused EndpointTarget reflection from the test.

Analysis: assertCurrentPeer calls CurrentServerInfo.of and reads getPeer, so the assertion now runs the code the test name describes. CurrentServerInfo.of delegates to EndpointTarget.parse and stores a null peer when parse returns null, so the endpoint is still rejected for the same reason, and the test now fails if the peer path stops honouring that rejection. The reflection into the private EndpointTarget class goes away, and the assertion matches its two sibling tests testInitialPeerUsesEffectivePort and testInitialPeerWithoutKnownPortIsOmitted.

Upsides: the test covers the peer path its name claims, so it can catch a regression there; it no longer duplicates testConfiguredEndpointsRejectUnsafeOrMalformedValues; the peer tests in this file now share one helper and one shape.

Downsides: the assertion is one step further from EndpointTarget.parse, so a failure names CurrentServerInfo rather than the parser. The parser keeps its own direct coverage through testConfiguredEndpointsRejectUnsafeOrMalformedValues.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Review finding:

getPeerAddress and getPeerPort read network.peer.address and network.peer.port out of a DbServerTarget, but that class documents the opposite contract. Its class javadoc says 'A target stays the same across routing, node selection, and retries, so it is derived from client configuration rather than from the endpoint that served an individual operation', and getAddress and getPort are documented as 'the value for server.address' and 'the value for server.port'. This PR now builds a DbServerTarget per HTTP attempt in ClickHouseClientV2Singletons.capturePeer and in ClickHouseClientV1Singletons.peerServerTarget, from the endpoint that served that individual operation, and reads it back as peer attributes. The shared javadoc in instrumentation-api-incubator is therefore wrong about how the type is used, which misleads the next reader of DbServerTarget. Fix: update the DbServerTarget class javadoc so it describes both uses, saying that the builder also renders a single contacted endpoint for network.peer.address and network.peer.port, and adjust the getAddress and getPort sentences accordingly.

Analysis: DbServerTarget renders a set of endpoints as one address and one port, and it validates every host the same way whoever calls it. Two callers now use that rendering for different attributes: a configured endpoint set becomes server.address and server.port, and a single contacted endpoint becomes network.peer.address and network.peer.port. The class javadoc described only the first caller and ruled out the second in so many words, so a reader of DbServerTarget could not tell that the peer values come from the same type. Naming both uses in the class javadoc, in the two getter descriptions, and in the builder javadoc leaves the behaviour alone and makes the contract match the callers. The sentence about a configured target staying the same across routing, node selection, and retries stays, because it still describes the server case and is what keeps the two uses apart.

Upsides: a reader of DbServerTarget can see both attribute pairs it feeds, so the peer use no longer looks like a misuse; the next instrumentation that needs a contacted endpoint finds the supported way to render one.

Downsides: the shared type now documents two responsibilities instead of one, which is a weaker contract than a dedicated peer type would give. A separate type would duplicate the whole host validation and rendering, so the cost outweighs the gain.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot comment:

[General] This snapshots the initial server, but ClickHouse v1 failover changes the request's shared server reference to the next node while the operation is running. The stored peer is never refreshed, so a failover span reports the first node rather than the final contacted endpoint promised by the PR description (and direct async `execute` ends before failover completes). Please update the peer from the actual attempt/final server and keep async spans open until completion as needed.

Analysis: ClickHouse seals requests before execution but shares their atomic server reference across copies. The v1 advice captured the first selected node and ended direct asynchronous spans as soon as execute returned. Refreshing the peer from that shared reference immediately before span completion records the node selected by failover. Completing stable-semconv asynchronous spans from the returned CompletableFuture ensures that refresh runs after ClickHouse finishes its failure handler.

Upsides: Failover spans report the final contacted peer, asynchronous span duration and errors cover the full operation, and a focused fake-client test verifies both the shared sealed-request reference and the completion timing.

Downsides: Stable-semconv asynchronous spans remain open until their returned future completes, and the completion callback retains the request and scope for that same period.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

… tests

Review finding:

Both new retry tests are guarded by assumeTrue(isClassPresent("com.clickhouse.client.api.transport.ClientNodeSelector")), and no released version of com.clickhouse:client-v2 contains that class. I inspected the jars in the local Gradle cache: 0.8.0 (the version this module declares with library("com.clickhouse:client-v2:0.8.0")) has no com/clickhouse/client/api/transport package at all, and 0.8.6, 0.9.0, 0.9.4, 0.9.9 and 0.10.0 (the newest release, which is what the test-latest-deps CI job resolves) contain no class whose name matches *Selector* anywhere in the jar. 0.10.0 ships only Endpoint, HttpEndpoint, and the transport.internal request and response types. The guard is therefore false in every configuration, so testRetryReportsContactedEndpoint and testExhaustedRetryReportsLastContactedEndpoint are skipped in test, in testStableSemconv, and in testLatestDeps. They are the only tests that cover this PR's headline behavior, namely refining network.peer.address and network.peer.port to the endpoint that actually served the operation across a retry, and to the last contacted endpoint when retries are exhausted. Every other assertion in the file only checks the single-endpoint case, where the peer equals the configured endpoint and would still pass with no retry handling at all. The helper putUnreachableEndpointFirst confirms the mismatch: it reads Client.class.getDeclaredField("nodeSelector"), and Client has no such field in 0.8.0 or in 0.10.0. Fix: drop the ClientNodeSelector guard and drive the endpoint order through the API that does exist, so the tests actually run. Client keeps its endpoints in a private field that is Set<String> in 0.8.0 and List<Endpoint> in 0.10.0, so the helper has to handle both shapes, or the tests have to be gated on a class that really marks the supported versions. If neither test can be made to run against a released client-v2, delete both of them rather than leaving tests that claim coverage they never provide.

Analysis: com.clickhouse.client.api.transport.ClientNodeSelector is in no released
com.clickhouse:client-v2 jar. 0.8.0, the version this module declares, has no
com.clickhouse.client.api.transport package at all, and 0.10.0, the newest release and the
one the test-latest-deps job resolves, ships only Endpoint, HttpEndpoint, and the
transport.internal types. The guard was therefore always false, and both tests were skipped
in test, in testStableSemconv, and under latest deps. Released versions also never rotate
endpoints between attempts, so an end to end retry test cannot be written against them, and
the helper that reordered Client.endpoints and rebuilt Client.nodeSelector reached for a
field shape and a type that no release has. The replacement drives
ClickHouseClientV2Singletons.capturePeer directly with two contacted endpoints in turn and
asserts the request peer follows each one, which is the behavior the retry tests described:
the peer names the endpoint that served the attempt, and the last contacted endpoint wins.
It reads the methods by name because the instrumentation classes are loaded by the agent,
so the test class loader cannot name their parameter types.

Upsides: the peer refinement added by ClickHouseClientV2RequestInstrumentation now has
coverage that actually executes, in both semconv modes and on every supported library
version. The test file no longer claims retry coverage it never provided, and it no longer
carries a helper that mutates private Client fields that do not exist.

Downsides: the new test exercises capturePeer directly rather than through a real failover,
so it does not prove that the advice matches the right HttpAPIClientHelper methods. No
released client-v2 version rotates endpoints between attempts, so that end to end path
cannot be covered today.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot comment:

Binding `@Advice.Return` as `CompletableFuture<?>` is brittle: if the instrumented method’s return type changes or differs across supported versions/overloads, ByteBuddy can fail to apply the advice (or fail at runtime) due to type mismatch. Safer pattern is to bind `@Advice.Return(typing = Assigner.Typing.DYNAMIC) Object result` and then `instanceof CompletableFuture` check before calling `endOnCompletion`.

Analysis: The matcher identifies the query overload by its arguments but does not constrain its return type. A dynamic Object binding lets the advice remain applicable if a supported ClickHouse version changes that return type. The exit advice now defers completion only when the returned value is a CompletableFuture.

Upsides: ByteBuddy can apply the advice across return-type changes, while current CompletableFuture behavior stays unchanged.

Downsides: A future ClickHouse return type that is not a CompletableFuture will end the span when the method returns rather than when asynchronous work completes.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot comment:

`ClickHouseDbRequest` is an `@AutoValue` but now contains additional mutable state (`volatile peer`) that is not part of AutoValue’s generated `equals/hashCode/toString`. This can be surprising and error-prone if the object is ever used as a key or logged/compared in tests. Prefer modeling `peer` as part of the AutoValue state (e.g., add it as a constructor property / abstract getter) or introduce a separate mutable holder specifically for per-operation peer updates (with a clear contract). If keeping this approach, add a brief comment documenting that `peer` is intentionally mutable and excluded from value semantics.

Copilot comment:

`ClickHouseDbRequest` is an `@AutoValue` but now contains additional mutable state (`volatile peer`) that is not part of AutoValue’s generated `equals/hashCode/toString`. This can be surprising and error-prone if the object is ever used as a key or logged/compared in tests. Prefer modeling `peer` as part of the AutoValue state (e.g., add it as a constructor property / abstract getter) or introduce a separate mutable holder specifically for per-operation peer updates (with a clear contract). If keeping this approach, add a brief comment documenting that `peer` is intentionally mutable and excluded from value semantics.

Copilot comment:

`ClickHouseDbRequest` is an `@AutoValue` but now contains additional mutable state (`volatile peer`) that is not part of AutoValue’s generated `equals/hashCode/toString`. This can be surprising and error-prone if the object is ever used as a key or logged/compared in tests. Prefer modeling `peer` as part of the AutoValue state (e.g., add it as a constructor property / abstract getter) or introduce a separate mutable holder specifically for per-operation peer updates (with a clear contract). If keeping this approach, add a brief comment documenting that `peer` is intentionally mutable and excluded from value semantics.

Analysis: The selected peer is per-operation state that can change after request creation when the client retries. Making it an immutable AutoValue property would prevent later attempts from updating the span attributes. The request is used as an instrumenter request rather than as a map key. A focused comment now states that the mutable peer does not participate in AutoValue value semantics.

Upsides: Readers can see the peer's lifecycle and understand why equality, hash code, and string output do not include it without adding a separate holder for one mutable field.

Downsides: Code that treats ClickHouseDbRequest as a complete immutable value still will not observe peer changes through AutoValue's generated value methods.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Keep the shared v1 advice return statically typed as Object because it covers both synchronous and asynchronous methods. Restrict the v2 matcher to CompletableFuture methods and bind its known return type directly.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>