OpenSearch Transport v3.0 Implementation #14823

mznet · 2025-09-28T06:39:31Z

This PR adds tracing for OpenSearch Transport interface, enabling in-depth tracing of OpenSearch requests across all Transport implementations.

Closes #14652

jaydeluca · 2025-09-30T09:29:06Z

...emetry/javaagent/instrumentation/opensearch/java/v3_0/OpenSearchJavaInstrumenterFactory.java

+import io.opentelemetry.instrumentation.api.instrumenter.Instrumenter;
+import io.opentelemetry.instrumentation.api.instrumenter.SpanKindExtractor;
+
+public final class OpenSearchJavaInstrumenterFactory {


if this is only used in the OpenSearchJavaSingletons class, perhaps just move it there instead of having it's own class? Often we'll have factories in the library instrumentation when it's also needed in the javaagent, reducing duplication, but as far as I can tell, it's only used in one place here

jaydeluca · 2025-09-30T09:29:38Z

...a/io/opentelemetry/javaagent/instrumentation/opensearch/java/v3_0/OpenSearchJavaRequest.java

+import com.google.auto.value.AutoValue;
+
+@AutoValue
+public abstract class OpenSearchJavaRequest {


This comment applies to many of these classes, but I'm not sure including "Java" in the class names is necessary

Suggested change

public abstract class OpenSearchJavaRequest {

public abstract class OpenSearchRequest {

@jaydeluca I also struggled with this. The reason for the OpenSearchJava* prefix is clear (distinguishing from OpenTelemetryRest* and it comes from opentelemtry-java instrumentation).

Still, I'd like to settle on a direction although both have solid reasons:

Which one is the better choice

Do not use Java in the class names, even if it comes from opentelemetry-java instrumentation.

Keep using Java in the class names, even if it feels a bit awkward.

In other instrumentations we don't have a java package and we also don't use Java in class names (with the exception of JavaHttpServer... and JavaHttpClient..). There are module names such as graphql-java and openai-java-1.1 for libraries that java in their name though other libraries that have a similar naming scheme don't have java in their name. I'd drop the java package and the Java in class name, in the context of this project we are dealing only with java frameworks anyway.

jaydeluca · 2025-09-30T09:31:20Z

...agent/instrumentation/opensearch/java/v3_0/OpenSearchJavaApacheHttpClient5TransportTest.java

+import org.opensearch.client.transport.OpenSearchTransport;
+import org.opensearch.client.transport.httpclient5.ApacheHttpClient5TransportBuilder;
+
+public class OpenSearchJavaApacheHttpClient5TransportTest extends AbstractOpenSearchJavaTest {


Suggested change

public class OpenSearchJavaApacheHttpClient5TransportTest extends AbstractOpenSearchJavaTest {

class OpenSearchJavaApacheHttpClient5TransportTest extends AbstractOpenSearchJavaTest {

jaydeluca · 2025-09-30T09:32:35Z

...metry/javaagent/instrumentation/opensearch/java/v3_0/OpenSearchJavaAwsSdk2TransportTest.java

+import software.amazon.awssdk.utils.AttributeMap;
+
+@SuppressWarnings("deprecation") // using deprecated semconv
+public class OpenSearchJavaAwsSdk2TransportTest extends AbstractOpenSearchJavaTest {


Suggested change

public class OpenSearchJavaAwsSdk2TransportTest extends AbstractOpenSearchJavaTest {

class OpenSearchJavaAwsSdk2TransportTest extends AbstractOpenSearchJavaTest {

jaydeluca · 2025-09-30T09:33:40Z

...ry/javaagent/instrumentation/opensearch/java/v3_0/OpenSearchJavaRestClientTransportTest.java

+@SuppressWarnings(
+    "deprecation") // RestClientTransport is deprecated but still the correct way for OpenSearch
+// Java 3.0
+public class OpenSearchJavaRestClientTransportTest extends AbstractOpenSearchJavaTest {


Suggested change

public class OpenSearchJavaRestClientTransportTest extends AbstractOpenSearchJavaTest {

class OpenSearchJavaRestClientTransportTest extends AbstractOpenSearchJavaTest {

SylvainJuge · 2025-09-30T19:37:38Z

Because you are adding a new instrumentation, I think it would be a good idea to make it already "indy-ready" to make it future-proof and prevent having to rework it later with a PR like #14842.

I haven't looked closely, but this should be quite close to the equivalent instrumentation for Elasticsearch, which has already been made "indy-compliant".

mznet · 2025-10-01T08:44:24Z

@SylvainJuge good point, i haven't thought about that, i will look at "indy-ready"

laurit · 2025-10-01T10:20:11Z

Because you are adding a new instrumentation, I think it would be a good idea to make it already "indy-ready" to make it future-proof and prevent having to rework it later with a PR like #14842.

I haven't looked closely, but this should be quite close to the equivalent instrumentation for Elasticsearch, which has already been made "indy-compliant".

@mznet you can view this as an optional task, we'll merge this PR even if you don't do this

laurit · 2025-10-01T10:24:07Z

...telemetry/javaagent/instrumentation/opensearch/java/v3_0/OpenSearchJavaAttributesGetter.java

+  @SuppressWarnings("deprecation") // using deprecated DbSystemIncubatingValues
+  @Override
+  public String getDbSystem(OpenSearchJavaRequest request) {
+    return DbIncubatingAttributes.DbSystemIncubatingValues.OPENSEARCH;


consider using the non-deprecated DbSystemNameIncubatingValues instead

laurit · 2025-10-01T10:25:18Z

...telemetry/javaagent/instrumentation/opensearch/java/v3_0/OpenSearchJavaAttributesGetter.java

+  @Deprecated
+  @Override
+  @Nullable
+  public String getUser(OpenSearchJavaRequest request) {
+    return null;
+  }


you can remove this, there is a default implementation in DbClientCommonAttributesGetter

laurit · 2025-10-01T10:25:25Z

...telemetry/javaagent/instrumentation/opensearch/java/v3_0/OpenSearchJavaAttributesGetter.java

+  @Deprecated
+  @Override
+  @Nullable
+  public String getConnectionString(OpenSearchJavaRequest request) {
+    return null;
+  }


you can remove this, there is a default implementation in DbClientCommonAttributesGetter

laurit · 2025-10-01T10:47:35Z

...telemetry/javaagent/instrumentation/opensearch/java/v3_0/OpenSearchJavaAttributesGetter.java

+  @Override
+  @Nullable
+  public String getDbQueryText(OpenSearchJavaRequest request) {
+    return request.getMethod() + " " + request.getOperation();


This probably isn't correct, ideally this should return the full query that is sanitized.

@laurit when i first wrote the implementation, extracting the full query text seemed impossible because the request only provided limited information. on second thought, it might actually be possible, so i’ll take another look.

Using the full query isn't always desirable either because it needs to be sanitized. If it can't be sanitized then it can't be enabled by default. Unfortunately sanitizing might not be easy. I think that since the existing opensearch-rest instrumentation uses the same query text we can use it here too for now. In the future we could use the same approach to fill the query summary (https://opentelemetry.io/docs/specs/semconv/database/database-spans/#generating-a-summary-of-the-query) instead and set the query text to null. Before we can do that we have to implement support for the db.query.summary attribute. Also since db.query.summary is only in the stable semconv would need to figure out what to do with the old semconv (continue filling the query text as is or set it to null). cc @trask

@laurit I was finally able to extract the full query text, but composing the sanitization logic will take more time. The existing sanitization logic can’t be reused since this isn’t just sql or sql-like but it’s a json query. I’ve roughly figured out how to approach opensearch query sanitization, but I’ll need more time to validate that it’s the right solution.

So I agree with your review. Let’s leave it as is for now, and I’ll come back once I’ve implemented the sanitization logic and can extract the full query text with it.

mznet · 2025-10-03T04:51:42Z

@SylvainJuge Since the review has already progressed this far, it might not be a good idea to change the code at this point. It would be better to add the indy-compatible instrumentation code in a separate PR. Thank you for pointing out. cc @laurit

linux-foundation-easycla · 2025-10-03T04:56:37Z

The committers listed above are authorized under a signed CLA.

✅ login: mznet / name: Minje Park (1ec0416, e4d95fa, bde4f45, 4ed0def, e90f611, e66488d, 6451368, a7663d1, ab8ca71, 4443006, 8ea5cb4)

mznet · 2025-10-03T04:58:26Z

Looks like I did something wrong on this branch. I’ll redo it.

mznet · 2025-10-03T05:41:28Z

I did the rebase wrong, so other people’s commits ended up in this branch’s history. I force pushed to the pre-rebase commit and cherry-picked just the commits I wanted on top.

laurit · 2025-10-03T11:42:30Z

/easycla

mznet added 7 commits September 26, 2025 14:17

add opentelemetry-java 3.0 instrumentation

ab8ca71

add opentelemetry-java 3.0 instrumentation

1ec0416

add OpenSearchJavaResponseHandler

6451368

remove unused CompletableFutureWrapper.java

bde4f45

add network span assertions to OpenSearchJavaAwsSdk2TransportTest

8ea5cb4

update .fossa.yml for OpenSearch Java

4ed0def

remove unused OpenSearchJavaResponse.java

a7663d1

mznet requested a review from a team as a code owner September 28, 2025 06:39

Merge branch 'main' into opensearch-transport-v3.0

e66488d

jaydeluca reviewed Sep 30, 2025

View reviewed changes

laurit reviewed Oct 1, 2025

View reviewed changes

github-actions bot added the test native This label can be applied to PRs to trigger them to run native tests label Oct 3, 2025

mznet force-pushed the opensearch-transport-v3.0 branch 2 times, most recently from 2e37420 to e66488d Compare October 3, 2025 05:36

mznet added 3 commits October 3, 2025 14:37

remove public in test classes

e4d95fa

remove java keyword in opensearch instrumentation

e90f611

remove OpenSearchInstrumenterFactory.java

4443006

mznet requested review from laurit and jaydeluca October 3, 2025 09:34

	public abstract class OpenSearchJavaRequest {
	public abstract class OpenSearchRequest {

	public class OpenSearchJavaApacheHttpClient5TransportTest extends AbstractOpenSearchJavaTest {
	class OpenSearchJavaApacheHttpClient5TransportTest extends AbstractOpenSearchJavaTest {

	public class OpenSearchJavaAwsSdk2TransportTest extends AbstractOpenSearchJavaTest {
	class OpenSearchJavaAwsSdk2TransportTest extends AbstractOpenSearchJavaTest {

	public class OpenSearchJavaRestClientTransportTest extends AbstractOpenSearchJavaTest {
	class OpenSearchJavaRestClientTransportTest extends AbstractOpenSearchJavaTest {

OpenSearch Transport v3.0 Implementation #14823

Are you sure you want to change the base?

OpenSearch Transport v3.0 Implementation #14823

Uh oh!

Conversation

mznet commented Sep 28, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SylvainJuge commented Sep 30, 2025

Uh oh!

mznet commented Oct 1, 2025

Uh oh!

laurit commented Oct 1, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mznet commented Oct 3, 2025

Uh oh!

linux-foundation-easycla bot commented Oct 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mznet commented Oct 3, 2025

Uh oh!

mznet commented Oct 3, 2025

Uh oh!

laurit commented Oct 3, 2025

Uh oh!

Uh oh!

linux-foundation-easycla bot commented Oct 3, 2025 •

edited

Loading