fixed review comments round 2

Signed-off-by: Alexander Wert <alexander.wert@elastic.co>
open-telemetry · Jul 4, 2023 · 240f218 · 240f218
1 parent 273a785
commit 240f218
Show file tree

Hide file tree

Showing 7 changed files with 588 additions and 403 deletions.
diff --git a/...telemetry/javaagent/instrumentation/elasticsearch/apiclient/ElasticsearchEndpointMap.java b/...telemetry/javaagent/instrumentation/elasticsearch/apiclient/ElasticsearchEndpointMap.java
diff --git a/...search/elasticsearch-api-client-7.16/javaagent/src/test/java/ElasticsearchClientTest.java b/...search/elasticsearch-api-client-7.16/javaagent/src/test/java/ElasticsearchClientTest.java
@@ -3,6 +3,10 @@
  * SPDX-License-Identifier: Apache-2.0
  */
 
+import static io.opentelemetry.instrumentation.testing.GlobalTraceUtil.runWithSpan;
+import static io.opentelemetry.sdk.testing.assertj.OpenTelemetryAssertions.equalTo;
+import static io.opentelemetry.sdk.testing.assertj.OpenTelemetryAssertions.satisfies;
+
 import co.elastic.clients.elasticsearch.ElasticsearchAsyncClient;
 import co.elastic.clients.elasticsearch.ElasticsearchClient;
 import co.elastic.clients.elasticsearch.core.InfoResponse;
@@ -15,6 +19,9 @@
 import io.opentelemetry.instrumentation.testing.junit.AgentInstrumentationExtension;
 import io.opentelemetry.instrumentation.testing.junit.InstrumentationExtension;
 import io.opentelemetry.semconv.trace.attributes.SemanticAttributes;
+import java.io.IOException;
+import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.TimeUnit;
 import org.apache.http.HttpHost;
 import org.assertj.core.api.AbstractLongAssert;
 import org.elasticsearch.client.RestClient;
@@ -24,15 +31,8 @@
 import org.junit.jupiter.api.Test;
 import org.junit.jupiter.api.extension.RegisterExtension;
 import org.testcontainers.elasticsearch.ElasticsearchContainer;
-import java.io.IOException;
-import java.util.concurrent.CountDownLatch;
-import java.util.concurrent.TimeUnit;
-
-import static io.opentelemetry.instrumentation.testing.GlobalTraceUtil.runWithSpan;
-import static io.opentelemetry.sdk.testing.assertj.OpenTelemetryAssertions.equalTo;
-import static io.opentelemetry.sdk.testing.assertj.OpenTelemetryAssertions.satisfies;
 
-public class ElasticsearchClientTest {
+class ElasticsearchClientTest {
   @RegisterExtension
   static final InstrumentationExtension testing = AgentInstrumentationExtension.create();
 
@@ -180,8 +180,8 @@ public void elasticsearchStatusAsync() throws Exception {
                         runWithSpan(
                             "callback",
                             () -> {
-                              countDownLatch.countDown();
                               request.setResponse(infoResponse);
+                              countDownLatch.countDown();
                             })));
     //noinspection ResultOfMethodCallIgnored
     countDownLatch.await(10, TimeUnit.SECONDS);
@@ -226,7 +226,7 @@ public void elasticsearchStatusAsync() throws Exception {
   }
 
   private static class AsyncRequest {
-    InfoResponse response = null;
+    volatile InfoResponse response = null;
 
     public InfoResponse getResponse() {
       return response;

diff --git a/.../elasticsearch/elasticsearch-rest-5.0/javaagent/src/test/java/ElasticsearchRest5Test.java b/.../elasticsearch/elasticsearch-rest-5.0/javaagent/src/test/java/ElasticsearchRest5Test.java
@@ -25,7 +25,7 @@
 import org.junit.jupiter.api.extension.RegisterExtension;
 import org.testcontainers.elasticsearch.ElasticsearchContainer;
 
-public class ElasticsearchRest5Test {
+class ElasticsearchRest5Test {
 
   @RegisterExtension
   static final InstrumentationExtension testing = AgentInstrumentationExtension.create();

diff --git a/.../elasticsearch/elasticsearch-rest-6.4/javaagent/src/test/java/ElasticsearchRest6Test.java b/.../elasticsearch/elasticsearch-rest-6.4/javaagent/src/test/java/ElasticsearchRest6Test.java
@@ -25,7 +25,7 @@
 import org.junit.jupiter.api.extension.RegisterExtension;
 import org.testcontainers.elasticsearch.ElasticsearchContainer;
 
-public class ElasticsearchRest6Test {
+class ElasticsearchRest6Test {
   @RegisterExtension
   static final InstrumentationExtension testing = AgentInstrumentationExtension.create();
 

diff --git a/.../elasticsearch/elasticsearch-rest-7.0/javaagent/src/test/java/ElasticsearchRest7Test.java b/.../elasticsearch/elasticsearch-rest-7.0/javaagent/src/test/java/ElasticsearchRest7Test.java
@@ -29,7 +29,7 @@
 import org.testcontainers.elasticsearch.ElasticsearchContainer;
 import org.testcontainers.shaded.com.fasterxml.jackson.databind.ObjectMapper;
 
-public class ElasticsearchRest7Test {
+class ElasticsearchRest7Test {
   @RegisterExtension
   static final InstrumentationExtension testing = AgentInstrumentationExtension.create();
 
@@ -189,8 +189,8 @@ public void onFailure(Exception e) {
   }
 
   private static class AsyncRequest {
-    Response requestResponse = null;
-    Exception exception = null;
+    volatile Response requestResponse = null;
+    volatile Exception exception = null;
 
     public Response getRequestResponse() {
       return requestResponse;

diff --git a/...y/javaagent/instrumentation/elasticsearch/rest/ElasticsearchClientAttributeExtractor.java b/...y/javaagent/instrumentation/elasticsearch/rest/ElasticsearchClientAttributeExtractor.java
@@ -14,6 +14,7 @@
 import io.opentelemetry.instrumentation.api.instrumenter.network.internal.NetworkAttributes;
 import io.opentelemetry.instrumentation.api.instrumenter.url.internal.UrlAttributes;
 import io.opentelemetry.instrumentation.api.internal.SemconvStability;
+import io.opentelemetry.instrumentation.api.internal.cache.Cache;
 import io.opentelemetry.semconv.trace.attributes.SemanticAttributes;
 import javax.annotation.Nullable;
 import org.apache.http.HttpHost;
@@ -24,6 +25,8 @@ public class ElasticsearchClientAttributeExtractor
 
   private static final String PATH_PARTS_ATTRIBUTE_PREFIX = "db.elasticsearch.path_parts.";
 
+  private static final Cache<String, AttributeKey<String>> pathPartKeysCache = Cache.weak();
+
   private static void setServerAttributes(AttributesBuilder attributes, Response response) {
     HttpHost host = response.getHost();
     if (host != null) {
@@ -33,8 +36,7 @@ private static void setServerAttributes(AttributesBuilder attributes, Response r
       }
       if (SemconvStability.emitOldHttpSemconv()) {
         internalSet(attributes, SemanticAttributes.NET_PEER_NAME, host.getHostName());
-        internalSet(
-            attributes, SemanticAttributes.NET_PEER_PORT, (long) host.getPort());
+        internalSet(attributes, SemanticAttributes.NET_PEER_PORT, (long) host.getPort());
       }
     }
   }
@@ -63,8 +65,10 @@ private static void setPathPartsAttributes(
     endpointDef.processPathParts(
         request.getEndpoint(),
         (key, value) -> {
-          String attributeKey = PATH_PARTS_ATTRIBUTE_PREFIX + key;
-          internalSet(attributes, AttributeKey.stringKey(attributeKey), value);
+          AttributeKey<String> attributeKey =
+              pathPartKeysCache.computeIfAbsent(
+                  key, k -> AttributeKey.stringKey(PATH_PARTS_ATTRIBUTE_PREFIX + k));
+          internalSet(attributes, attributeKey, value);
         });
   }
 

diff --git a/...lemetry/javaagent/instrumentation/elasticsearch/rest/ElasticsearchEndpointDefinition.java b/...lemetry/javaagent/instrumentation/elasticsearch/rest/ElasticsearchEndpointDefinition.java
@@ -5,6 +5,8 @@
 
 package io.opentelemetry.javaagent.instrumentation.elasticsearch.rest;
 
+import static java.util.Collections.unmodifiableList;
+
 import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.Collections;
@@ -15,7 +17,7 @@
 import java.util.stream.Collectors;
 import javax.annotation.Nullable;
 
-public class ElasticsearchEndpointDefinition {
+public final class ElasticsearchEndpointDefinition {
 
   private static final String UNDERSCORE_REPLACEMENT = "0";
 
@@ -27,7 +29,8 @@ public class ElasticsearchEndpointDefinition {
   public ElasticsearchEndpointDefinition(
       String endpointName, String[] routes, boolean isSearchEndpoint) {
     this.endpointName = endpointName;
-    this.routes = Arrays.stream(routes).map(Route::new).collect(Collectors.toList());
+    this.routes =
+        unmodifiableList(Arrays.stream(routes).map(Route::new).collect(Collectors.toList()));
     this.isSearchEndpoint = isSearchEndpoint;
   }
 
@@ -66,6 +69,7 @@ List<Route> getRoutes() {
   static final class Route {
     private final String name;
     private final boolean hasParameters;
+
     private volatile EndpointPattern epPattern;
 
     public Route(String name) {
@@ -90,6 +94,8 @@ Matcher createMatcher(String urlPath) {
     }
 
     private EndpointPattern getEndpointPattern() {
+      // Intentionally NOT synchronizing here to avoid synchronization overhead.
+      // Main purpose here is to cache the pattern without the need for strict thread-safety.
       if (epPattern == null) {
         epPattern = new EndpointPattern(this);
       }