captured span.sync attribute for ES restclient spans plugins (#3329)

* captured span.sync attribute for ES restclient spans plugins --------- Co-authored-by: Sylvain Juge <[email protected]>
elastic · Oct 5, 2023 · 38150ba · 38150ba
1 parent 1529c45
commit 38150ba
Show file tree

Hide file tree

Showing 10 changed files with 55 additions and 12 deletions.
diff --git a/CHANGELOG.asciidoc b/CHANGELOG.asciidoc
@@ -45,6 +45,7 @@ Use subheadings with the "=====" level for adding notes for unreleased changes:
 * Switched to OpenTelemetry compatible context propagation for Kafka - {pull}3300[#3300]
 * Changed `cloud.project.id` collected in Google Cloud (GCP) to be the `project-id` - {issues}3311[#3311]
 * Allow running the IntelliJ debug agent in parallel - {pull}3315[#3315]
+* Capture `span.sync` = `false` for ES restclient async spans plugins
 
 [float]
 ===== Bug fixes

diff --git a/.../java/co/elastic/apm/agent/esrestclient/v5_6/ElasticsearchClientAsyncInstrumentation.java b/.../java/co/elastic/apm/agent/esrestclient/v5_6/ElasticsearchClientAsyncInstrumentation.java
@@ -75,7 +75,7 @@ public static Object[] onBeforeExecute(@Advice.Argument(0) String method,
                                                @Advice.Argument(3) @Nullable HttpEntity entity,
                                                @Advice.Argument(5) ResponseListener responseListener) {
 
-            Span<?> span = helper.createClientSpan(method, endpoint, entity);
+            Span<?> span = helper.createClientSpan(method, endpoint, entity, false);
             if (span != null) {
                 Object[] ret = new Object[2];
                 ret[0] = span;

diff --git a/...n/java/co/elastic/apm/agent/esrestclient/v5_6/ElasticsearchClientSyncInstrumentation.java b/...n/java/co/elastic/apm/agent/esrestclient/v5_6/ElasticsearchClientSyncInstrumentation.java
@@ -47,7 +47,7 @@ public static class ElasticsearchRestClientAdvice {
         public static Object onBeforeExecute(@Advice.Argument(0) String method,
                                             @Advice.Argument(1) String endpoint,
                                             @Advice.Argument(3) @Nullable HttpEntity entity) {
-            return helper.createClientSpan(method, endpoint, entity);
+            return helper.createClientSpan(method, endpoint, entity, true);
         }
 
         @Advice.OnMethodExit(suppress = Throwable.class, onThrowable = Throwable.class, inline = false)

diff --git a/.../java/co/elastic/apm/agent/esrestclient/v6_4/ElasticsearchClientAsyncInstrumentation.java b/.../java/co/elastic/apm/agent/esrestclient/v6_4/ElasticsearchClientAsyncInstrumentation.java
@@ -66,7 +66,7 @@ public static class ElasticsearchRestClientAsyncAdvice {
         @Advice.OnMethodEnter(suppress = Throwable.class, inline = false)
         public static Object[] onBeforeExecute(@Advice.Argument(0) Request request,
                                                @Advice.Argument(1) ResponseListener responseListener) {
-            Span<?> span = helper.createClientSpan(request.getMethod(), request.getEndpoint(), request.getEntity());
+            Span<?> span = helper.createClientSpan(request.getMethod(), request.getEndpoint(), request.getEntity(), false);
             if (span != null) {
                 Object[] ret = new Object[2];
                 ret[0] = span;

diff --git a/...n/java/co/elastic/apm/agent/esrestclient/v6_4/ElasticsearchClientSyncInstrumentation.java b/...n/java/co/elastic/apm/agent/esrestclient/v6_4/ElasticsearchClientSyncInstrumentation.java
@@ -60,7 +60,7 @@ public static class ElasticsearchRestClientSyncAdvice {
         @Nullable
         @Advice.OnMethodEnter(suppress = Throwable.class, inline = false)
         public static Object onBeforeExecute(@Advice.Argument(0) Request request) {
-            return helper.createClientSpan(request.getMethod(), request.getEndpoint(), request.getEntity());
+            return helper.createClientSpan(request.getMethod(), request.getEndpoint(), request.getEntity(), true);
         }
 
         @Advice.OnMethodExit(suppress = Throwable.class, onThrowable = Throwable.class, inline = false)

diff --git a/...java/co/elastic/apm/agent/esrestclient/v7_x/ElasticsearchRestClientInstrumentationIT.java b/...java/co/elastic/apm/agent/esrestclient/v7_x/ElasticsearchRestClientInstrumentationIT.java
@@ -143,6 +143,7 @@ public void onFailure(Exception e) {
             .statusCode(-1)
             .disableHttpUrlCheck()
             .expectAnyStatement()
+            .expectAsync(true)
             .check();
 
         assertThat(searchSpan.getOutcome())

diff --git a/...plugin-8_x/src/test/java/co/elastic/apm/agent/esrestclient/v8_x/Elasticsearch8JavaIT.java b/...plugin-8_x/src/test/java/co/elastic/apm/agent/esrestclient/v8_x/Elasticsearch8JavaIT.java
@@ -276,6 +276,7 @@ public void testDocumentScenario() throws Exception {
                 .endpointName("delete")
                 .expectPathPart("index", INDEX)
                 .expectPathPart("id", DOC_ID)
+                .expectAsync(false) // delete is a sync operation
                 .check();
         }
     }

diff --git a/.../java/co/elastic/apm/agent/esrestclient/ElasticsearchRestClientInstrumentationHelper.java b/.../java/co/elastic/apm/agent/esrestclient/ElasticsearchRestClientInstrumentationHelper.java
@@ -86,7 +86,7 @@ public void clearCurrentEndpoint() {
     }
 
     @Nullable
-    public Span<?> createClientSpan(String method, String httpPath, @Nullable HttpEntity httpEntity) {
+    public Span<?> createClientSpan(String method, String httpPath, @Nullable HttpEntity httpEntity, boolean isSync) {
         ElasticsearchEndpointDefinition endpoint = currentRequestEndpoint.getAndRemove();
 
         Span<?> span = tracer.currentContext().createExitSpan();
@@ -98,7 +98,8 @@ public Span<?> createClientSpan(String method, String httpPath, @Nullable HttpEn
 
         span.withType(SPAN_TYPE)
             .withSubtype(ELASTICSEARCH)
-            .withAction(SPAN_ACTION);
+            .withAction(SPAN_ACTION)
+            .withSync(isSync);
 
         StringBuilder name = span.getAndOverrideName(AbstractSpan.PRIORITY_HIGH_LEVEL_FRAMEWORK);
         if (endpoint != null) {

diff --git a/.../src/test/java/co/elastic/apm/agent/esrestclient/AbstractEsClientInstrumentationTest.java b/.../src/test/java/co/elastic/apm/agent/esrestclient/AbstractEsClientInstrumentationTest.java
@@ -101,7 +101,7 @@ public void assertThatErrorsExistWhenDeleteNonExistingIndex() {
     }
 
     protected EsSpanValidationBuilder validateSpan(Span spanToValidate) {
-        return new EsSpanValidationBuilder(spanToValidate);
+        return new EsSpanValidationBuilder(spanToValidate, async);
     }
 
     protected EsSpanValidationBuilder validateSpan() {
@@ -138,8 +138,11 @@ protected static class EsSpanValidationBuilder {
         @Nullable
         private String expectedHttpUrl = "http://" + container.getHttpHostAddress();
 
-        public EsSpanValidationBuilder(Span spanToValidate) {
+        private boolean isAsyncRequest;
+
+        public EsSpanValidationBuilder(Span spanToValidate, boolean isAsyncRequest) {
             this.span = spanToValidate;
+            this.isAsyncRequest = isAsyncRequest;
         }
 
         public EsSpanValidationBuilder expectNoStatement() {
@@ -154,6 +157,11 @@ public EsSpanValidationBuilder expectAnyStatement() {
             return this;
         }
 
+        public EsSpanValidationBuilder expectAsync(boolean async) {
+            isAsyncRequest = async;
+            return this;
+        }
+
         public EsSpanValidationBuilder expectStatement(String statement) {
             try {
                 this.expectedStatement = jackson.readTree(statement);
@@ -221,6 +229,11 @@ public void check() {
             checkDbContext();
             checkPathPartAttributes();
             checkDestinationContext();
+            if (isAsyncRequest) {
+                assertThat(span).isAsync();
+            } else {
+                assertThat(span).isSync();
+            }
         }
 
 

diff --git a/...a/co/elastic/apm/agent/esrestclient/ElasticsearchRestClientInstrumentationHelperTest.java b/...a/co/elastic/apm/agent/esrestclient/ElasticsearchRestClientInstrumentationHelperTest.java
@@ -24,6 +24,7 @@
 import co.elastic.apm.agent.impl.transaction.Span;
 import co.elastic.apm.agent.impl.transaction.Transaction;
 import co.elastic.apm.agent.impl.transaction.TransactionTest;
+import co.elastic.apm.agent.testutils.assertions.SpanAssert;
 import org.apache.http.HttpHost;
 import org.apache.http.HttpVersion;
 import org.apache.http.entity.ByteArrayEntity;
@@ -36,6 +37,9 @@
 
 import java.util.List;
 import java.util.Map;
+import java.util.function.Consumer;
+import java.util.function.Function;
+import java.util.function.Supplier;
 
 import static co.elastic.apm.agent.esrestclient.ElasticsearchRestClientInstrumentationHelper.ELASTICSEARCH;
 import static co.elastic.apm.agent.testutils.assertions.Assertions.assertThat;
@@ -62,7 +66,7 @@ void after(){
 
     @Test
     void testCreateSpan() {
-        Span span = (Span) helper.createClientSpan("GET", "/_test", null);
+        Span span = (Span) helper.createClientSpan("GET", "/_test", null, true);
         assertThat(span).isNotNull();
 
         assertThat(tracer.getActive()).isEqualTo(span);
@@ -87,7 +91,7 @@ void testCreateSpan() {
 
     @Test
     void testCreateSpanWithClusterName() {
-        Span span = (Span) helper.createClientSpan("GET", "/_test", null);
+        Span span = (Span) helper.createClientSpan("GET", "/_test", null, true);
         assertThat(span).isNotNull();
 
         assertThat(tracer.getActive()).isEqualTo(span);
@@ -127,7 +131,7 @@ private static Response mockResponse(Map<String,String> headers) {
     @Test
     void testNonSampledSpan() {
         TransactionTest.setRecorded(false, transaction);
-        Span esSpan = (Span) helper.createClientSpan("SEARCH", "/test", null);
+        Span esSpan = (Span) helper.createClientSpan("SEARCH", "/test", null, true);
         assertThat(esSpan).isNotNull();
         try {
             assertThat(esSpan.isSampled()).isFalse();
@@ -157,7 +161,7 @@ public void testCaptureBodyUrls(boolean captureEverything) throws Exception {
         }
 
         Span span = (Span) helper.createClientSpan("GET", "/_test",
-            new ByteArrayEntity(new byte[0]));
+            new ByteArrayEntity(new byte[0]), true);
         assertThat(span).isNotNull();
         assertThat(tracer.getActive()).isEqualTo(span);
 
@@ -179,4 +183,26 @@ public void testCaptureBodyUrls(boolean captureEverything) throws Exception {
             assertThat((CharSequence) db.getStatementBuffer()).isNull();
         }
     }
+
+    @Test
+    public void testSpanIsAsync() {
+        testSpanSyncAttribute(false, (span -> assertThat(span).isAsync()));
+    }
+
+    @Test
+    public void testSpanIsSync() {
+        testSpanSyncAttribute(true, (span -> assertThat(span).isSync()));
+    }
+
+    private void testSpanSyncAttribute(boolean isSync, Function<Span, SpanAssert> checkSyncAttribute) {
+        Span span = (Span) helper.createClientSpan("GET", "/_test", null, isSync);
+        assertThat(span).isNotNull();
+
+        assertThat(tracer.getActive()).isEqualTo(span);
+        checkSyncAttribute.apply(span);
+
+        Response response = mockResponse(Map.of());
+        helper.finishClientSpan(response, span, null);
+        span.deactivate();
+    }
 }