Repository navigation
Capture configured OpenSearch REST server targets - #19903
Merged
trask merged 30 commits intoSep 9, 2026
Merged
Conversation
Pull request dashboard statusMerged · refreshed 2026-09-10 16:34 UTC Status above doesn't look right?
|
Contributor
There was a problem hiding this comment.
Pull request overview
Adds stable OpenSearch server targets while retaining the actual connected endpoint as network peer telemetry.
Changes:
- Captures initial node configuration for OpenSearch 1.x–3.x.
- Propagates peer socket details through Apache HttpClient instrumentation.
- Extends trace and metric assertions for target and peer attributes.
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
AbstractOpenSearchRestTest.java |
Tests target stability, peers, and metrics. |
OpenSearchServerTargets.java |
Stores initial targets via VirtualField. |
OpenSearchServerTarget.java |
Models and sanitizes configured endpoints. |
OpenSearchRestResponse.java |
Removes response-based peer lookup. |
OpenSearchRestRequest.java |
Carries target and peer state. |
OpenSearchRestAttributesGetter.java |
Extracts server and peer attributes. |
OpenSearchRest3Test.java |
Supports configurable 3.x test nodes. |
v3_0/RestClientInstrumentation.java |
Propagates target and peer state. |
v3_0/RestClientConstructorInstrumentation.java |
Captures initial 3.x nodes. |
v3_0/OpenSearchRestSingletons.java |
Simplifies response conversion. |
v3_0/OpenSearchRestInstrumentationModule.java |
Registers constructor instrumentation. |
OpenSearchRestTest.java |
Supports configurable 1.x/2.x test nodes. |
v1_0/RestClientInstrumentation.java |
Propagates target and peer state. |
v1_0/RestClientConstructorInstrumentation.java |
Captures initial 1.x/2.x nodes. |
v1_0/OpenSearchRestSingletons.java |
Simplifies response conversion. |
v1_0/OpenSearchRestInstrumentationModule.java |
Registers constructor instrumentation. |
ApacheHttpAsyncClientInstrumentation.java |
Captures Apache HttpClient 5 peer endpoints. |
💡 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
force-pushed
the
trask-opensearch-rest-targets
branch
from
August 29, 2026 00:55
069cbdf to
7830b7c
Compare
trask
force-pushed
the
trask-opensearch-rest-targets
branch
from
August 29, 2026 19:30
7830b7c to
9348e92
Compare
trask
force-pushed
the
trask-opensearch-rest-targets
branch
from
August 30, 2026 15:15
f2c8b8b to
10d492e
Compare
trask
force-pushed
the
trask-opensearch-rest-targets
branch
from
August 30, 2026 22:32
10d492e to
9c6b787
Compare
trask
force-pushed
the
trask-opensearch-rest-targets
branch
from
August 31, 2026 00:21
0178703 to
dae2e68
Compare
trask
force-pushed
the
trask-opensearch-rest-targets
branch
from
August 31, 2026 01:11
dae2e68 to
c19a9f7
Compare
Review finding: AbstractOpenSearchRestTest adds the field as protected, but nothing outside the class reads it. Only openSearchAttributes() in the same class uses peerAddress; OpenSearchRestTest and OpenSearchRest3Test read only the opensearch container field. The repository style instruction .github/instructions/java-style.instructions.md requires the principle of least access, using the most restrictive modifier that still works, and the sibling Elasticsearch tests declare the same field as private static String peerAddress (ElasticsearchRest5Test line 66, ElasticsearchRest7Test line 63). Fix: declare the field private String peerAddress. Analysis: peerAddress holds the resolved container address that openSearchAttributes() compares against network.peer.address and network.type. It is written once in setUp() and read only inside AbstractOpenSearchRestTest, at lines 261 and 265. The two subclasses, OpenSearchRestTest and OpenSearchRest3Test, inherit the field but never mention it; they use only the opensearch container field. The neighbouring fields are protected because they predate this change, which does not make protected right for a field this pull request introduces. Compiling both subclass test source sets confirms nothing outside the class needs the wider access. Upsides: The field matches the visibility rule the repository states, and the class now says which of its fields a subclass may rely on. A later subclass cannot silently start depending on a field the base class owns. Downsides: The field's modifier differs from the protected fields declared beside it, so the block is no longer uniform. Co-authored-by: Copilot <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>
Review finding: SearchPeerResponseConsumer.failed(Exception) calls capture(parentContext, httpContext), but WrappedFutureCallback.failed(Exception) already makes that exact call with the same parentContext and the same HttpContext instance. Both objects are created together in ClientAdvice.methodEnter under the same captureSearchPeer flag and are installed as arguments 1 and 4 of the same execute() call, so whenever this consumer is in place the wrapped future callback is too. Unlike consumeResponse, which captures from the live HttpContext the client passes in, this override has no context of its own and reads the stored field, so it can never observe endpoint details the callback would miss. The sibling apache-httpasyncclient-4.1 instrumentation makes the same distinction deliberately: its SearchPeerResponseConsumer.failed only delegates, and the capture lives in WrappedFutureCallback.failed and cancelled. Fix: drop the capture call from SearchPeerResponseConsumer.failed in the 5.0 instrumentation, leaving it a plain delegate like its 4.1 sibling, and keep the captures in WrappedFutureCallback.failed and cancelled. Analysis: Apache HttpClient 5.x notifies the future callback on every path that also fails the response consumer, so the consumer capture can never be the only one. Full clients call responseConsumer.failed() and future.failed() together, and minimal clients do the same through BasicClientExchangeHandler. Both captures read the same stored HttpContext field and pass the same parent context, so they resolve identical endpoint details. The live capture stays in consumeResponse, where the client supplies its own HttpContext argument that can differ from the stored one. With the failure-path capture gone, the httpContext field and its constructor argument have no remaining reader, so they go too; the 4.1 sibling keeps its field only because its responseReceived callback has no context parameter. Upsides: Removes a capture that cannot change the recorded peer, drops a field and constructor argument that nothing reads, and makes the 5.0 consumer match the capture points its 4.1 sibling already uses. Downsides: No material downside identified. Co-authored-by: Copilot <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>
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 <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>
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>
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>
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>
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>
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-opensearch-rest-targets
branch
from
September 9, 2026 18:40
6c37fed to
5134fcc
Compare
laurit
pushed a commit
to laurit/opentelemetry-java-instrumentation
that referenced
this pull request
Sep 21, 2026
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.
Captures each OpenSearch REST client's initially configured node list and reports it through stable database semantic conventions.
Stable spans use the configured target for
server.address,server.port, and span naming. The target is a sorted snapshot capped at five endpoints; sniffing and latersetNodescalls do not change it. A single non-default port is reported separately, while multi-endpoint targets omit all-default ports or inline every effective port when any endpoint uses a non-default port.Legacy conventions retain peer address and network type sourced directly from the OpenSearch response. Lower-level HTTP spans retain their independently sourced transport attributes.
This is the third layer of #19974 and is stacked on #19902.