Report configured Elasticsearch transport addresses as server.address - #19902
Draft
trask wants to merge 11 commits into
Draft
Report configured Elasticsearch transport addresses as server.address#19902trask wants to merge 11 commits into
trask wants to merge 11 commits into
Conversation
Pull request dashboard statusWaiting on the author · refreshed 2026-08-30 17:02 UTC Move out of draft to request review. Status above doesn't look right?
|
Contributor
There was a problem hiding this comment.
Pull request overview
Captures configured Elasticsearch transport targets for stable database semantic conventions while preserving legacy telemetry.
Changes:
- Adds frozen server-target storage and request attributes.
- Integrates target capture across Elasticsearch 5.0, 5.3, and 6.0–7.x.
- Updates span assertions and adds target-capture tests.
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
.../AbstractElasticsearchTransportClientTest.java |
Verifies stable server attributes and target-based names. |
.../ElasticTransportRequest.java |
Carries captured server address and port. |
.../ElasticsearchTransportServerTargets.java |
Stores each client’s initial target. |
.../ElasticsearchTransportServerTarget.java |
Formats single and grouped endpoints. |
.../ElasticsearchTransportAttributesGetter.java |
Exposes stable server attributes. |
.../ElasticsearchTransportServerTargetTest.java |
Tests endpoint formatting. |
.../ElasticsearchTransportServerTargetsTest.java |
Tests target immutability. |
.../ElasticsearchTransportAttributesGetterTest.java |
Adapts request construction. |
.../v6_0/Elasticsearch6TransportRequests.java |
Captures 6.x–7.x transport endpoints. |
.../v6_0/AbstractClientInstrumentation.java |
Supplies client context during instrumentation. |
.../v5_3/Elasticsearch53TransportRequests.java |
Captures 5.3 transport endpoints. |
.../v5_3/AbstractClientInstrumentation.java |
Supplies client context during instrumentation. |
.../Elasticsearch5TransportClientTest.java |
Tests multiple and reconfigured targets. |
.../v5_0/Elasticsearch5TransportRequests.java |
Captures 5.0 transport endpoints. |
.../v5_0/AbstractClientInstrumentation.java |
Supplies client context during instrumentation. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 15 out of 15 changed files in this pull request and generated no new comments.
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
instrumentation/elasticsearch/elasticsearch-transport-common-5.0/testing/src/main/java/io/opentelemetry/javaagent/instrumentation/elasticsearch/transport/common/v5_0/AbstractElasticsearchTransportClientTest.java:102
- [Style] Test code should not add
@Nullable. Remove these parameter annotations and the corresponding import, as required by the repository's Java style rules.
protected List<AttributeAssertion> clusterHealthAttributes(
@Nullable String serverAddress, @Nullable Long serverPort) {
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 15 out of 15 changed files in this pull request and generated no new comments.
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
instrumentation/elasticsearch/elasticsearch-transport-common-5.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/elasticsearch/transport/common/v5_0/ElasticsearchTransportServerTargets.java:38
- [General] An empty endpoint list is not yet a configured target, but this records
NO_TARGETpermanently. If an instrumented request runs beforeaddTransportAddress(...),isCaptured()remains true after an address is added, so all later stable spans still omitserver.*and the target-based name. Leave empty/invalid lists uncaptured (while retaining the sentinel fornull, which identifies non-transport clients) so the first subsequently observed configured target can be frozen; add a regression test for empty capture followed by a real endpoint.
if (target == null) {
SERVER_ADDRESS.set(client, NO_TARGET);
return;
trask
force-pushed
the
trask-elasticsearch-transport-targets
branch
from
August 29, 2026 00:55
71174f1 to
e1c3d64
Compare
trask
changed the title
trask
force-pushed
the
trask-elasticsearch-transport-targets
branch
from
August 29, 2026 18:29
e1c3d64 to
3916c7a
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 22 out of 22 changed files in this pull request and generated no new comments.
Suppressed comments (3)
Previously missed (3) — in code that hasn't changed since the last review.
instrumentation/elasticsearch/elasticsearch-transport-5.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/elasticsearch/transport/v5_0/Elasticsearch5TransportRequests.java:36
- [General] Concurrent address changes can leave this snapshot stale. Elasticsearch serializes each underlying list mutation, but this read/build/write runs afterward: an earlier advice can read the old list, a later advice can publish the new list, and then the earlier advice can overwrite it. Requests after both calls would report an obsolete configured target. Serialize the complete snapshot-and-publish step so advice executions cannot commit out of order.
public static void updateServerTarget(TransportClient client) {
List<ElasticsearchTransportServerTarget.Endpoint> endpoints = new ArrayList<>();
instrumentation/elasticsearch/elasticsearch-transport-5.3/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/elasticsearch/transport/v5_3/Elasticsearch53TransportRequests.java:36
- [General] Concurrent address changes can leave this snapshot stale. Elasticsearch serializes each underlying list mutation, but this read/build/write runs afterward: an earlier advice can read the old list, a later advice can publish the new list, and then the earlier advice can overwrite it. Requests after both calls would report an obsolete configured target. Serialize the complete snapshot-and-publish step so advice executions cannot commit out of order.
public static void updateServerTarget(TransportClient client) {
List<ElasticsearchTransportServerTarget.Endpoint> endpoints = new ArrayList<>();
instrumentation/elasticsearch/elasticsearch-transport-6.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/elasticsearch/transport/v6_0/Elasticsearch6TransportRequests.java:37
- [General] Concurrent address changes can leave this snapshot stale. Elasticsearch serializes each underlying list mutation, but this read/build/write runs afterward: an earlier advice can read the old list, a later advice can publish the new list, and then the earlier advice can overwrite it. Requests after both calls would report an obsolete configured target. Serialize the complete snapshot-and-publish step so advice executions cannot commit out of order.
public static void updateServerTarget(TransportClient client) {
List<ElasticsearchTransportServerTarget.Endpoint> endpoints = new ArrayList<>();
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot comment: [General] `getAddress()` resolves a configured hostname to an IP address, so a client configured for `search.example.com` will emit the IP in `server.address` and in the target-based span name instead of the configured logical target. Elasticsearch 5.x exposes the host-preserving value via `TransportAddress.getHost()`; use that here (and update the address-list expectation) so stable telemetry retains configured hostnames. Copilot comment: [General] `getAddress()` returns the resolved IP and discards a configured hostname, so `server.address` and the stable span target will not represent the configured logical endpoint. Elasticsearch 5.3's `TransportAddress.getHost()` preserves the host string without reverse lookup; pass that value into the endpoint instead. Copilot comment: [General] `getAddress()` returns the resolved IP and loses a configured hostname, causing `server.address` and the stable span target to identify the physical address rather than the configured logical endpoint. For Elasticsearch 6.x/7.x, use the wrapped socket address's host string instead. Analysis: The configured transport target must preserve the host string supplied by the client. Elasticsearch 5.x exposes that value through TransportAddress.getHost(). Elasticsearch 6.x and 7.x expose it through the wrapped InetSocketAddress.getHostString(). The address-list test must build its expected target from the same host-preserving accessors. Upsides: Stable server.address values and target-based span names now retain configured logical hostnames across all supported transport versions. The test also checks the host-preserving address-list behavior. Downsides: Existing telemetry may change from a resolved IP address to the configured hostname when applications configure transport clients with hostnames. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot comment:
[Style] Test code should not add `@Nullable`. Remove these parameter annotations and the corresponding import, as required by the repository's Java style rules.
```
protected List<AttributeAssertion> clusterHealthAttributes(
@nullable String serverAddress, @nullable Long serverPort) {
```
Analysis: This class belongs to a testing module, where the repository prohibits Nullable annotations. The helper still accepts null values from mode-dependent test expectations without these annotations.
Upsides: The test code now follows the repository rule and drops an unnecessary annotation import. Runtime test behavior is unchanged.
Downsides: The parameter declarations no longer document null acceptance through annotations.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot comment:
[General] An empty endpoint list is not yet a configured target, but this records `NO_TARGET` permanently. If an instrumented request runs before `addTransportAddress(...)`, `isCaptured()` remains true after an address is added, so all later stable spans still omit `server.*` and the target-based name. Leave empty/invalid lists uncaptured (while retaining the sentinel for `null`, which identifies non-transport clients) so the first subsequently observed configured target can be frozen; add a regression test for empty capture followed by a real endpoint.
```
if (target == null) {
SERVER_ADDRESS.set(client, NO_TARGET);
return;
```
Analysis: A non-null endpoint list identifies a transport client even when the list has no usable target yet. The capture logic must leave that client uncaptured so a later request can freeze endpoints added after the first request. A null list still identifies a non-transport client and keeps the permanent no-target sentinel.
Upsides: Stable spans can start reporting the configured target after a transport client adds its first endpoint. The regression test covers an empty capture followed by a usable endpoint.
Downsides: Requests retry target extraction while a transport client's endpoint list stays empty or invalid.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot comment: [General] `address()` and `port()` each read the `VirtualField` independently, while every request calls them consecutively. If an application adds or removes an address concurrently with a request, the request can combine the address from one target snapshot with the port from another (for example, a multi-address string with a single endpoint's port). Expose the immutable target itself and read it once when constructing `ElasticTransportRequest`, then derive both values from that same instance. Analysis: The separate accessors performed two independent VirtualField reads. Exposing the immutable target lets each transport request read one snapshot before it derives the address and port. Upsides: Each request now records a consistent address and port pair when the configured transport addresses change concurrently. Downsides: Call sites now handle a missing target before reading its address and port. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…t wrappers Copilot comment: [General] Looking up the target only on this `AbstractClient` loses it for requests issued through an Elasticsearch `FilterClient` (including clients returned by `filterWithHeader`). `FilterClient` inherits the instrumented `AbstractClient.execute` method and delegates to the underlying transport client, so the outer request is created with the wrapper as `client`; the `VirtualField` was populated only on the `TransportClient`. As a result, these stable spans omit `server.address`/`server.port` and the target suffix in the span name. Please propagate the target to wrapper clients or resolve the underlying transport client before building the request. Copilot comment: [General] Looking up the target only on this `AbstractClient` loses it for requests issued through an Elasticsearch `FilterClient` (including clients returned by `filterWithHeader`). `FilterClient` inherits the instrumented `AbstractClient.execute` method and delegates to the underlying transport client, so the outer request is created with the wrapper as `client`; the `VirtualField` was populated only on the `TransportClient`. As a result, these stable spans omit `server.address`/`server.port` and the target suffix in the span name. Please propagate the target to wrapper clients or resolve the underlying transport client before building the request. Copilot comment: [General] Looking up the target only on this `AbstractClient` loses it for requests issued through an Elasticsearch `FilterClient` (including clients returned by `filterWithHeader`). `FilterClient` inherits the instrumented `AbstractClient.execute` method and delegates to the underlying transport client, so the outer request is created with the wrapper as `client`; the `VirtualField` was populated only on the `TransportClient`. As a result, these stable spans omit `server.address`/`server.port` and the target suffix in the span name. Please propagate the target to wrapper clients or resolve the underlying transport client before building the request. Analysis: FilterClient instances execute the outer instrumented call, but the server target is stored on their delegate. Constructor advice now records each wrapper-to-delegate link. Target lookup follows that link, so long-lived wrappers observe later address changes on the transport client. Upsides: Filtered clients now keep the configured server attributes and stable span-name suffix across Elasticsearch 5.x through 7.x. Linked lookup also supports nested wrappers and later target updates. Downsides: Wrapped requests perform one extra VirtualField lookup per wrapper layer. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…tors Copilot comment: [General] This matches only the one-argument `FilterClient(Client)` constructor. Every supported Elasticsearch version also exposes the protected `FilterClient(Settings, ThreadPool, Client)` constructor for proxy subclasses; a subclass that calls it directly never records its delegate. Because the outer `AbstractClient.execute` span suppresses the delegated call, requests through such a client omit the stable server attributes and target suffix. Please instrument both constructor overloads and select their `Client` argument. Analysis: FilterClient exposes one-argument and three-argument constructors with the delegate at different positions. Separate advice for each overload records the correct Client argument. Upsides: Proxy subclasses that call the protected constructor now retain server attributes and the stable span-name suffix. The integration test exercises target updates through that constructor. Downsides: The instrumentation adds a second small constructor advice. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot comment:
[General] Concurrent address changes can leave this snapshot stale. Elasticsearch serializes each underlying list mutation, but this read/build/write runs afterward: an earlier advice can read the old list, a later advice can publish the new list, and then the earlier advice can overwrite it. Requests after both calls would report an obsolete configured target. Serialize the complete snapshot-and-publish step so advice executions cannot commit out of order.
```
public static void updateServerTarget(TransportClient client) {
List<ElasticsearchTransportServerTarget.Endpoint> endpoints = new ArrayList<>();
```
Copilot comment:
[General] Concurrent address changes can leave this snapshot stale. Elasticsearch serializes each underlying list mutation, but this read/build/write runs afterward: an earlier advice can read the old list, a later advice can publish the new list, and then the earlier advice can overwrite it. Requests after both calls would report an obsolete configured target. Serialize the complete snapshot-and-publish step so advice executions cannot commit out of order.
```
public static void updateServerTarget(TransportClient client) {
List<ElasticsearchTransportServerTarget.Endpoint> endpoints = new ArrayList<>();
```
Copilot comment:
[General] Concurrent address changes can leave this snapshot stale. Elasticsearch serializes each underlying list mutation, but this read/build/write runs afterward: an earlier advice can read the old list, a later advice can publish the new list, and then the earlier advice can overwrite it. Requests after both calls would report an obsolete configured target. Serialize the complete snapshot-and-publish step so advice executions cannot commit out of order.
```
public static void updateServerTarget(TransportClient client) {
List<ElasticsearchTransportServerTarget.Endpoint> endpoints = new ArrayList<>();
```
Analysis: Each advice previously built its snapshot after the Elasticsearch mutation lock was released. Synchronizing the full read, build, and publish sequence on the client prevents advice executions from publishing snapshots out of order.
Upsides: The stored server target cannot regress to an older address list after concurrent add or remove calls complete.
Downsides: Concurrent address updates on the same client briefly contend while the target snapshot is rebuilt.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
… 5.0.x
Review finding:
This module declares support for [5.0.0,5.3.0) in its muzzle ranges and compiles against org.elasticsearch.client:transport:5.0.0, but TransportAddress.getHost() only preserves the configured host string from Elasticsearch 5.1.1 onward. In 5.0.x, InetSocketTransportAddress.getHost() is `return getAddress();`, and getAddress() is `NetworkAddress.format(address.getAddress())`, which formats the resolved InetAddress as a numeric literal. So on Elasticsearch 5.0.0 through 5.0.2, a client configured with `new InetSocketTransportAddress(InetAddress.getByName("es.example.com"), 9300)` captures `10.0.0.1` rather than `es.example.com`, and stable server.address plus the target-based span name identify the physical address instead of the configured logical endpoint. That is exactly the behavior the PR description promises and that the 6.0 module already avoids by calling address.address().getHostString(). The module's default test library is 5.0.0, and the test cannot catch this because it builds its expected value from the same getHost() accessor. Fix: read the host through the wrapped socket address in this module too, for example `address instanceof InetSocketTransportAddress ? ((InetSocketTransportAddress) address).address().getHostString() : address.getHost()`; InetSocketTransportAddress exists throughout [5.0.0,6.0.0), so muzzle still passes over the declared range.
Analysis: InetSocketTransportAddress.getHost() delegates to getAddress() up to Elasticsearch 5.0.2, and getAddress() runs the resolved InetAddress through NetworkAddress.format, so it always yields a numeric literal. From 5.1.1 onward getHost() delegates to InetSocketAddress.getHostString() instead. Decompiling the cached jars confirms both shapes, and this module's muzzle range [5.0.0,5.3.0) covers the releases with the formatting behavior, with 5.0.0 as the default test library. Reading the host through ((InetSocketTransportAddress) address).address().getHostString() gives the same value on every release in range. InetSocketTransportAddress is the socket-backed TransportAddress throughout 5.x and its address() accessor is public in every release in range, so muzzle still passes; LocalTransportAddress is the only other implementation and falls back to getHost(). The test could not catch this because it derived its expected value from the same getHost() accessor, so it now builds the down endpoint from an InetAddress that carries an explicit host name and asserts that name appears in the captured target.
Upsides: server.address and the target-based span name identify the configured logical endpoint on every Elasticsearch release this module supports, which is what the pull request sets out to do, and matches what the 5.3 and 6.0 modules already capture. The test now proves that a configured host name survives instead of restating whatever the accessor returned.
Downsides: the instanceof check and cast add a small amount of code that the 5.3 and 6.0 modules do not need. A TransportAddress implementation outside Elasticsearch that is not an InetSocketTransportAddress keeps the previous behavior.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Review finding: The `endpoint.port >= 0` guard here, and the matching one in the multi-endpoint loop, can never be false. Endpoint is only constructed in Elasticsearch5TransportRequests, Elasticsearch53TransportRequests and Elasticsearch6TransportRequests, and all three pass TransportAddress.getPort(), which returns InetSocketAddress.getPort() and is never negative. No test supplies a negative port either, so both the `null` port branch for a single endpoint and the omitted `:port` suffix for a grouped endpoint are unreachable. Fix: drop the guards and pass the port straight through, so the single-endpoint case reads `new ElasticsearchTransportServerTarget(endpoint.host, endpoint.port)` and the group always appends `:` plus the port. Analysis: Endpoint carries an int port that only ever comes from TransportAddress.getPort(), which returns InetSocketAddress.getPort(). The JDK rejects a negative port when the socket address is constructed, so no Elasticsearch5TransportRequests, Elasticsearch53TransportRequests or Elasticsearch6TransportRequests call can produce one. The unit tests build every Endpoint with an explicit positive port too. Both guards therefore always take the same branch, so the null port for a single endpoint and the missing :port suffix for a grouped endpoint are dead. The equivalent guards in the REST instrumentation are reachable because HttpHost reports -1 for an unset port, but TransportAddress has no such value. Upsides: removes two branches that no input reaches, so the remaining code states plainly that a transport endpoint always has a port. It also removes two branches that no test can cover. Downsides: a future caller that constructs an Endpoint with a negative port would render it verbatim rather than quietly omitting it. No such caller exists, and a silent omission would be no easier to diagnose. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
trask
force-pushed
the
trask-elasticsearch-transport-targets
branch
from
August 30, 2026 15:15
8d32a2c to
db88946
Compare
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.
Stable Elasticsearch 5.0, 5.3, and 6.0 through 7.x transport spans report the client's configured addresses as the logical server. One endpoint sets
server.address=es.exampleandserver.port=9300; multiple endpoints setserver.address=es-a.example:9300,es-b.example:9301and omitserver.port.The instrumentation keeps configured hostnames instead of replacing them with sniffed or selected routing nodes. Stable span names use the configured target. Legacy semantic convention output is unchanged.
This follows the
server.addressclarification proposed in open-telemetry/semantic-conventions#4041.This PR is stacked on #19901.