Capture the Elasticsearch REST logical server and network peer addresses - #19901
Draft
trask wants to merge 14 commits into
Draft
Capture the Elasticsearch REST logical server and network peer addresses#19901trask wants to merge 14 commits into
trask wants to merge 14 commits into
Conversation
Pull request dashboard statusWaiting on the author · refreshed 2026-09-01 01:33 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
Adds configured Elasticsearch REST targets and actual network-peer telemetry while preserving legacy span naming and server attributes.
Changes:
- Captures immutable configured targets for Elasticsearch 5.x–7.x clients.
- Propagates actual socket peers from Apache HttpAsyncClient.
- Updates stable span names, attributes, documentation, and tests.
Reviewed changes
Copilot reviewed 28 out of 28 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
.../ElasticsearchServerTargetTest.java |
Tests target normalization and sanitization. |
.../ElasticsearchDbAttributesGetterTest.java |
Tests peer attribute extraction. |
.../ElasticsearchSpanNameExtractor.java |
Uses stable database span naming. |
.../ElasticsearchServerTarget.java |
Models sanitized configured targets. |
.../ElasticsearchRestRequest.java |
Carries target and peer state. |
.../ElasticsearchDbAttributesGetter.java |
Exposes server and network-peer attributes. |
.../ElasticsearchClientAttributeExtractor.java |
Preserves configured stable targets. |
.../ElasticsearchServerTargets.java |
Stores targets in a virtual field. |
elasticsearch-rest-7.0/library/.../ElasticsearchRest7Test.java |
Tests library target behavior. |
.../RestClientWrapper.java |
Adds builder wrapping and target capture. |
.../ElasticsearchRest7Telemetry.java |
Exposes the builder overload. |
elasticsearch-rest-7.0/library/README.md |
Documents builder-based wrapping. |
elasticsearch-rest-7.0/javaagent/.../ElasticsearchRest7Test.java |
Tests 7.x agent attributes and targets. |
elasticsearch-rest-7.0/javaagent/.../RestClientInstrumentation.java |
Propagates target and peer state. |
elasticsearch-rest-7.0/javaagent/.../RestClientConstructorInstrumentation.java |
Captures initial 7.x nodes. |
.../ElasticsearchRest7InstrumentationModule.java |
Registers constructor instrumentation. |
elasticsearch-rest-6.4/javaagent/.../ElasticsearchRest6Test.java |
Tests 6.x agent behavior. |
elasticsearch-rest-6.4/javaagent/.../RestClientInstrumentation.java |
Propagates 6.x target and peer state. |
elasticsearch-rest-6.4/javaagent/.../RestClientConstructorInstrumentation.java |
Captures initial 6.x nodes. |
.../ElasticsearchRest6InstrumentationModule.java |
Registers constructor instrumentation. |
elasticsearch-rest-5.0/javaagent/.../ElasticsearchRest5Test.java |
Tests 5.x agent behavior. |
elasticsearch-rest-5.0/javaagent/.../RestClientInstrumentation.java |
Propagates 5.x target and peer state. |
elasticsearch-rest-5.0/javaagent/.../RestClientConstructorInstrumentation.java |
Captures initial 5.x hosts. |
.../ElasticsearchRest5InstrumentationModule.java |
Registers constructor instrumentation. |
.../ElasticsearchClientTest.java |
Verifies API-client target and peer telemetry. |
.../ApacheHttpAsyncClientInstrumentation.java |
Captures the connected socket peer. |
.../SearchPeerStateTest.java |
Tests peer-state propagation and replacement. |
.../SearchPeerState.java |
Implements nested peer capture state. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
This was referenced Aug 28, 2026
trask
changed the title
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Review finding:
This new public class lives in an `.internal` package, so the repository's own ErrorProne check `OtelInternalJavadoc` requires it to carry the standard disclaimer javadoc. Instead of adding that javadoc, the class silences the check with `@SuppressWarnings("OtelInternalJavadoc")`. Every other class in this same package (`ElasticsearchRestRequest`, `ElasticsearchDbAttributesGetter`, `ElasticsearchClientAttributeExtractor`, `ElasticsearchRestInstrumenterFactory`) carries the disclaimer, and the only other uses of this suppression in the repository are compile stubs and a test appender, not real internal API classes. Fix: delete the suppression and add the standard javadoc above the class, `/** This class is internal and is hence not for public use. Its APIs are unstable and can change at any time. */`.
Analysis: OtelInternalJavadoc matches every public class whose package name contains an `internal` segment and reports it unless the class javadoc carries one of two exact disclaimers. ElasticsearchServerTarget is public and sits in io.opentelemetry.instrumentation.elasticsearch.rest.common.v5_0.internal, so the check applies. Suppressing it removes the report but leaves the class undocumented, which is the opposite of what the check exists to achieve. The other three suppressions in the repository are on a Liberty compile stub, a Liberty dispatcher stub, and a test-only Logback appender, none of which is an internal API class that callers read. Replacing the suppression with the V1 disclaimer satisfies the check for the reason it was written and matches every sibling class in the package.
Upsides: The class now states that it is internal and unstable, the same way the rest of the package does, so a reader who reaches it from the javaagent module or from the elasticsearch-rest-7.0 library learns not to depend on it. The suppression no longer hides future javadoc regressions on this class.
Downsides: No material downside identified.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…happens Review finding: This comment says "a request is served by the running server after a retry", but no retry happens in this test. The client is built as `RestClient.builder(httpHost, deadHost)` with the live host first, and Elasticsearch's `RestClient.selectNodes` rotates the living nodes by `lastNodeIndex.getAndIncrement()`, which is 0 for a freshly built client, so the first attempt goes to the first configured node and succeeds. The dead host is never contacted. The same wrong comment sits in the elasticsearch-rest-7.0 library test, which also lists the live host first; the elasticsearch-rest-6.4 and elasticsearch-rest-7.0 javaagent tests list the dead host first, so the retry claim is correct only there. Fix: in the two live-host-first tests, replace the retry claim with what is actually true of the port, for example "nothing listens on this port, so it is never the host that answers a request". Analysis: RestClient initializes lastNodeIndex to 0, keeps the builder's argument order in its node list, starts with an empty blacklist, and selects with the no-op NodeSelector.ANY, so the first request from a fresh client rotates by 0 and goes to the first configured node. In elasticsearch-api-client-7.16 ElasticsearchClientTest and in the elasticsearch-rest-7.0 library ElasticsearchRest7Test the live host is configured first, so that first attempt succeeds and the dead host is never contacted. The comment there tells a reader the opposite, and a reader who believes it would look for retry behavior the test does not exercise. The new wording states what the dead port is actually for in these two tests, which is to be a second entry in the configured target while never answering. The elasticsearch-rest-6.4 and elasticsearch-rest-7.0 javaagent tests configure the dead host first, so a retry really does happen there and their comment stays as it is. Upsides: The two tests now explain their dead port correctly, so a reader does not conclude that they cover retry handling. The remaining retry comments are left only where a retry actually occurs, which makes that distinction meaningful. Downsides: The two files no longer say anything about ordering, so a reader who wants to see the retry path has to look at the elasticsearch-rest-6.4 or elasticsearch-rest-7.0 javaagent test instead. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Review finding: This comment says the dead port "is only ever a configured target", but `deadHost()` has two callers in this file and that is false for one of them. `configuredNodeListIsTheWholeTarget` passes it to `RestClient.builder(httpHost, deadHost)`, so there it really is part of the configured target. `theTargetDoesNotFollowLaterNodeChanges` builds the client with `RestClient.builder(httpHost)` and only then calls `setNodes(asList(new Node(httpHost), new Node(deadHost())))`, and that test asserts `assertConfiguredTarget(null)` precisely because a node added after construction is not a configured target. A reader of that second test is told the opposite of what the test proves. The rest of the comment is correct for both callers, because `lastNodeIndex` is still 0 when each client makes its first request, so the live host answers. Fix: drop the inaccurate clause and keep only what holds for both callers, for example `// nothing listens on this port, so it never answers a request`. Analysis: The `deadHost()` helper serves two tests with different intent. In `configuredNodeListIsTheWholeTarget` the dead host is passed to the builder, so it is part of the configured target and the assertion expects it in the host list. In `theTargetDoesNotFollowLaterNodeChanges` the client is built with the live host alone and the dead host arrives later through `setNodes`, and the assertion expects no configured target at all, which is the whole point of that test. A comment that calls the dead port a configured target therefore contradicts the second test. What holds for both callers is only that nothing listens on the port, so the port never answers a request. The sibling comment in the elasticsearch-api-client-7.16 test has a single caller that does put the dead host in the configured target, so it stays as it is. Upsides: The helper now describes what is true wherever it is used, so a reader of `theTargetDoesNotFollowLaterNodeChanges` is no longer told that the dead host is a configured target while the test asserts the opposite. Downsides: The comment no longer mentions the configured target at all, so a reader of `configuredNodeListIsTheWholeTarget` learns that role from the test body rather than from the helper. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Add an explicit configured-host wrapper overload and keep stable server attributes absent when target provenance is unavailable. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
trask
force-pushed
the
trask-elasticsearch-rest-targets
branch
from
August 30, 2026 15:15
fa8f77f to
51011a0
Compare
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot comment: [General] This enables peer capture for every Elasticsearch 5 request, including the default legacy-semconv mode where both peer getters discard the captured value. That makes the Apache async instrumentation allocate/wrap its response consumer (and potentially create a context and `InetSocketAddress`) on the hot path for telemetry that cannot be emitted. Please only put `SearchPeerState` into the context when `emitStableDatabaseSemconv()` is true. Copilot comment: [General] This enables peer capture for every Elasticsearch 6 request, including the default legacy-semconv mode where both peer getters discard the captured value. That makes the Apache async instrumentation allocate/wrap its response consumer (and potentially create a context and `InetSocketAddress`) on the hot path for telemetry that cannot be emitted. Please only put `SearchPeerState` into the context when `emitStableDatabaseSemconv()` is true. Copilot comment: [General] This enables peer capture for every Elasticsearch 7+ request, including the default legacy-semconv mode where both peer getters discard the captured value. That makes the Apache async instrumentation allocate/wrap its response consumer (and potentially create a context and `InetSocketAddress`) on the hot path for telemetry that cannot be emitted. Please only put `SearchPeerState` into the context when `emitStableDatabaseSemconv()` is true. Analysis: SearchPeerState activates extra peer capture in the Apache asynchronous client. The Elasticsearch peer getters only use the captured address under stable database semantic conventions. Each REST instrumentation now stores the state only when stable database semantic conventions are enabled. Upsides: Legacy mode avoids the context entry and Apache peer capture work. Stable and duplicate modes keep the network peer attributes. Downsides: Each started Elasticsearch span adds one semantic convention mode check. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Failing check: build / common / test0 and test1: org.opentest4j.AssertionFailedError: Expected span to have name <localhost:32855,localhost:32854> but was <localhost:32854,localhost:32855> Cause: The changed tests kept the RestClient constructor order, but the new server target renderer sorts endpoints before it emits the span name and server.address. Fix: Expect the live host before the higher-port dead host in the Elasticsearch REST 6 and 7 tests so the assertions match the deterministic target order. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 31 out of 31 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/apache-httpasyncclient-4.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/apachehttpasyncclient/v4_1/ApacheHttpAsyncClientInstrumentation.java:248
- [General] This open-state guard drops the peer for transport failures that close or begin closing an established connection before
failed()runs. Apache'sHttpInetConnectionaddress accessors do not requireisOpen(), and the following null/port checks already omit connections without a resolved endpoint. Removing this guard lets failed Elasticsearch spans retain the last resolved socket peer as intended.
if (!inetConnection.isOpen()) {
return;
}
Copilot comment:
[General] This open-state guard drops the peer for transport failures that close or begin closing an established connection before `failed()` runs. Apache's `HttpInetConnection` address accessors do not require `isOpen()`, and the following null/port checks already omit connections without a resolved endpoint. Removing this guard lets failed Elasticsearch spans retain the last resolved socket peer as intended.
```
if (!inetConnection.isOpen()) {
return;
}
```
Analysis: Apache's NHttpConnectionBase reads the remote address from its IOSession without requiring the session to be open. Removing the open-state guard lets failure callbacks capture that endpoint, while the existing null address and negative port checks still reject unavailable endpoints.
Upsides: Elasticsearch failure spans retain the resolved network peer when Apache closes or starts closing the connection before the callback runs.
Downsides: The capture path queries the remote endpoint on closed connections, but Apache's accessors support that state and the existing validity checks discard missing endpoints.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Failing check: build / common / test0 (11, hotspot, indy false): java.io.IOException: listener timeout after waiting for [30000] ms Cause: The new configured-node-list test placed the unreachable node first, so the request waited for the client timeout instead of reaching Elasticsearch. Fix: Put the live node first. The test still captures the complete configured node list while the request completes without a retry timeout. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
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 REST telemetry separates the configured target from the runtime socket peer. A single configured host reports
server.address=es.exampleand, for a non-default port,server.port=9200.Configured host lists preserve duplicates and use deterministic lexical ordering. Hosts sharing one non-default port report it separately, for example
server.address=es-a.example,es-b.examplewithserver.port=9200. Mixed ports remain attached to each endpoint, for exampleserver.address=es-a.example:9200,es-b.example:9201, with noserver.port. HTTP port 80 and HTTPS port 443 are omitted.The logical target comes from the hosts supplied when the client is built and does not follow sniffing or later node updates. Stable span names use the same target. With the javaagent,
network.peer.addressandnetwork.peer.portreport the last resolved socket endpoint observed for the request, so retries can report a different peer without changing the logical target; unresolved peers are omitted.For manual instrumentation,
wrap(RestClientBuilder)andwrap(RestClient)cannot read the original configured hosts through the Elasticsearch API, so they omit stableserver.addressandserver.portrather than deriving them from mutable routing nodes. When the original hosts are known,wrap(restClient, configuredHosts)captures them explicitly and keeps them independent of later node updates.This follows the configured database target clarification in open-telemetry/semantic-conventions#4058.