Influxdb client: don't fill db.statement for create/drop database and…

… write operations
open-telemetry · Jun 12, 2024 · 2dd66b4 · 2dd66b4
1 parent dc8822c
commit 2dd66b4
Show file tree

Hide file tree

Showing 7 changed files with 64 additions and 81 deletions.
diff --git a/instrumentation/influxdb-2.4/javaagent/build.gradle.kts b/instrumentation/influxdb-2.4/javaagent/build.gradle.kts
@@ -17,6 +17,8 @@ dependencies {
   compileOnly("com.google.auto.value:auto-value-annotations")
   annotationProcessor("com.google.auto.value:auto-value")
 
+  testInstrumentation(project(":instrumentation:okhttp:okhttp-3.0:javaagent"))
+
   // we use methods that weren't present before 2.14 in tests
   testLibrary("org.influxdb:influxdb-java:2.14")
 }
@@ -44,3 +46,9 @@ tasks {
     }
   }
 }
+
+tasks.withType<Test>().configureEach {
+  // we disable the okhttp instrumentation, so we don't need to assert on the okhttp spans
+  // from the okhttp instrumentation we need OkHttp3IgnoredTypesConfigurer to fix context leaks
+  jvmArgs("-Dotel.instrumentation.okhttp.enabled=false")
+}
diff --git a/...va/io/opentelemetry/javaagent/instrumentation/influxdb/v2_4/InfluxDbAttributesGetter.java b/...va/io/opentelemetry/javaagent/instrumentation/influxdb/v2_4/InfluxDbAttributesGetter.java
@@ -19,11 +19,10 @@ public String getStatement(InfluxDbRequest request) {
   @Nullable
   @Override
   public String getOperation(InfluxDbRequest request) {
-    if (request.getSqlStatementInfo() != null) {
-      String operation = request.getSqlStatementInfo().getOperation();
-      return operation == null ? request.getSql() : operation;
+    if (request.getOperation() != null) {
+      return request.getOperation();
     }
-    return null;
+    return request.getSqlStatementInfo().getOperation();
   }
 
   @Override

diff --git a/...main/java/io/opentelemetry/javaagent/instrumentation/influxdb/v2_4/InfluxDbConstants.java b/...main/java/io/opentelemetry/javaagent/instrumentation/influxdb/v2_4/InfluxDbConstants.java
diff --git a/...io/opentelemetry/javaagent/instrumentation/influxdb/v2_4/InfluxDbImplInstrumentation.java b/...io/opentelemetry/javaagent/instrumentation/influxdb/v2_4/InfluxDbImplInstrumentation.java
@@ -6,9 +6,6 @@
 package io.opentelemetry.javaagent.instrumentation.influxdb.v2_4;
 
 import static io.opentelemetry.javaagent.bootstrap.Java8BytecodeBridge.currentContext;
-import static io.opentelemetry.javaagent.instrumentation.influxdb.v2_4.InfluxDbConstants.CREATE_DATABASE_STATEMENT_NEW;
-import static io.opentelemetry.javaagent.instrumentation.influxdb.v2_4.InfluxDbConstants.CREATE_DATABASE_STATEMENT_OLD;
-import static io.opentelemetry.javaagent.instrumentation.influxdb.v2_4.InfluxDbConstants.DELETE_DATABASE_STATEMENT;
 import static io.opentelemetry.javaagent.instrumentation.influxdb.v2_4.InfluxDbSingletons.instrumenter;
 import static net.bytebuddy.matcher.ElementMatchers.isEnum;
 import static net.bytebuddy.matcher.ElementMatchers.isMethod;
@@ -94,7 +91,7 @@ public static void onEnter(
       HttpUrl httpUrl = retrofit.baseUrl();
       influxDbRequest =
           InfluxDbRequest.create(
-              httpUrl.host(), httpUrl.port(), query.getDatabase(), query.getCommand());
+              httpUrl.host(), httpUrl.port(), query.getDatabase(), null, query.getCommand());
 
       if (!instrumenter().shouldStart(parentContext, influxDbRequest)) {
         return;
@@ -142,7 +139,6 @@ public static class InfluxDbModifyAdvice {
 
     @Advice.OnMethodEnter(suppress = Throwable.class)
     public static void onEnter(
-        @Advice.This InfluxDBImpl influxDbImpl,
         @Advice.Origin("#m") String methodName,
         @Advice.Argument(0) Object arg0,
         @Advice.FieldValue(value = "retrofit") Retrofit retrofit,
@@ -168,17 +164,17 @@ public static void onEnter(
               // write data by UDP protocol, in this way, can't get database name.
               : arg0 instanceof Integer ? "" : String.valueOf(arg0);
 
-      String sql = methodName;
+      String operation;
       if ("createDatabase".equals(methodName)) {
-        sql =
-            influxDbImpl.version().startsWith("0.")
-                ? String.format(CREATE_DATABASE_STATEMENT_OLD, database)
-                : String.format(CREATE_DATABASE_STATEMENT_NEW, database);
+        operation = "CREATE DATABASE";
       } else if ("deleteDatabase".equals(methodName)) {
-        sql = String.format(DELETE_DATABASE_STATEMENT, database);
+        operation = "DROP DATABASE";
+      } else {
+        operation = "WRITE";
       }
 
-      influxDbRequest = InfluxDbRequest.create(httpUrl.host(), httpUrl.port(), database, sql);
+      influxDbRequest =
+          InfluxDbRequest.create(httpUrl.host(), httpUrl.port(), database, operation, null);
 
       if (!instrumenter().shouldStart(parentContext, influxDbRequest)) {
         return;

diff --git a/...c/main/java/io/opentelemetry/javaagent/instrumentation/influxdb/v2_4/InfluxDbRequest.java b/...c/main/java/io/opentelemetry/javaagent/instrumentation/influxdb/v2_4/InfluxDbRequest.java
@@ -9,15 +9,17 @@
 import io.opentelemetry.instrumentation.api.incubator.semconv.db.SqlStatementInfo;
 import io.opentelemetry.instrumentation.api.incubator.semconv.db.SqlStatementSanitizer;
 import io.opentelemetry.javaagent.bootstrap.internal.CommonConfig;
+import javax.annotation.Nullable;
 
 @AutoValue
 public abstract class InfluxDbRequest {
 
   private static final SqlStatementSanitizer sanitizer =
       SqlStatementSanitizer.create(CommonConfig.get().isStatementSanitizationEnabled());
 
-  public static InfluxDbRequest create(String host, Integer port, String dbName, String sql) {
-    return new AutoValue_InfluxDbRequest(host, port, dbName, sql, sanitizer.sanitize(sql));
+  public static InfluxDbRequest create(
+      String host, Integer port, String dbName, String operation, String sql) {
+    return new AutoValue_InfluxDbRequest(host, port, dbName, operation, sanitizer.sanitize(sql));
   }
 
   public abstract String getHost();
@@ -26,7 +28,8 @@ public static InfluxDbRequest create(String host, Integer port, String dbName, S
 
   public abstract String getDbName();
 
-  public abstract String getSql();
+  @Nullable
+  public abstract String getOperation();
 
   public abstract SqlStatementInfo getSqlStatementInfo();
 }
diff --git a/...est/java/io/opentelemetry/javaagent/instrumentation/influxdb/v2_4/InfluxDbClientTest.java b/...est/java/io/opentelemetry/javaagent/instrumentation/influxdb/v2_4/InfluxDbClientTest.java
@@ -5,8 +5,6 @@
 
 package io.opentelemetry.javaagent.instrumentation.influxdb.v2_4;
 
-import static io.opentelemetry.javaagent.instrumentation.influxdb.v2_4.InfluxDbConstants.CREATE_DATABASE_STATEMENT_NEW;
-import static io.opentelemetry.javaagent.instrumentation.influxdb.v2_4.InfluxDbConstants.DELETE_DATABASE_STATEMENT;
 import static io.opentelemetry.sdk.testing.assertj.OpenTelemetryAssertions.equalTo;
 import static java.util.Arrays.asList;
 import static org.assertj.core.api.Assertions.assertThat;
@@ -108,16 +106,13 @@ void testQueryAndModifyWithOneArgument() {
                     span.hasName("CREATE DATABASE " + dbName)
                         .hasKind(SpanKind.CLIENT)
                         .hasAttributesSatisfying(
-                            attributeAssertions(
-                                String.format(CREATE_DATABASE_STATEMENT_NEW, dbName),
-                                "CREATE DATABASE",
-                                dbName))),
+                            attributeAssertions(null, "CREATE DATABASE", dbName))),
         trace ->
             trace.hasSpansSatisfyingExactly(
                 span ->
-                    span.hasName("write " + dbName)
+                    span.hasName("WRITE " + dbName)
                         .hasKind(SpanKind.CLIENT)
-                        .hasAttributesSatisfying(attributeAssertions("write", "write", dbName))),
+                        .hasAttributesSatisfying(attributeAssertions(null, "WRITE", dbName))),
         trace ->
             trace.hasSpansSatisfyingExactly(
                 span ->
@@ -131,10 +126,7 @@ void testQueryAndModifyWithOneArgument() {
                     span.hasName("DROP DATABASE " + dbName)
                         .hasKind(SpanKind.CLIENT)
                         .hasAttributesSatisfying(
-                            attributeAssertions(
-                                String.format(DELETE_DATABASE_STATEMENT, dbName),
-                                "DROP DATABASE",
-                                dbName))));
+                            attributeAssertions(null, "DROP DATABASE", dbName))));
   }
 
   @Test
@@ -279,10 +271,10 @@ void testWriteWithFourArguments() {
         trace ->
             trace.hasSpansSatisfyingExactly(
                 span ->
-                    span.hasName("write " + databaseName)
+                    span.hasName("WRITE " + databaseName)
                         .hasKind(SpanKind.CLIENT)
                         .hasAttributesSatisfying(
-                            attributeAssertions("write", "write", databaseName))));
+                            attributeAssertions(null, "WRITE", databaseName))));
   }
 
   @Test
@@ -297,10 +289,10 @@ void testWriteWithFiveArguments() {
         trace ->
             trace.hasSpansSatisfyingExactly(
                 span ->
-                    span.hasName("write " + databaseName)
+                    span.hasName("WRITE " + databaseName)
                         .hasKind(SpanKind.CLIENT)
                         .hasAttributesSatisfying(
-                            attributeAssertions("write", "write", databaseName))));
+                            attributeAssertions(null, "WRITE", databaseName))));
   }
 
   @Test
@@ -316,19 +308,24 @@ void testWriteWithUdp() {
         trace ->
             trace.hasSpansSatisfyingExactly(
                 span ->
-                    span.hasName("write")
+                    span.hasName("WRITE")
                         .hasKind(SpanKind.CLIENT)
-                        .hasAttributesSatisfying(attributeAssertions("write", "write", null))));
+                        .hasAttributesSatisfying(attributeAssertions(null, "WRITE", null))));
   }
 
   private static List<AttributeAssertion> attributeAssertions(
       String statement, String operation, String databaseName) {
-    return asList(
-        equalTo(DbIncubatingAttributes.DB_SYSTEM, "influxdb"),
-        equalTo(DbIncubatingAttributes.DB_NAME, databaseName),
-        equalTo(ServerAttributes.SERVER_ADDRESS, host),
-        equalTo(ServerAttributes.SERVER_PORT, port),
-        equalTo(DbIncubatingAttributes.DB_STATEMENT, statement),
-        equalTo(DbIncubatingAttributes.DB_OPERATION, operation));
+    List<AttributeAssertion> result = new ArrayList<>();
+    result.addAll(
+        asList(
+            equalTo(DbIncubatingAttributes.DB_SYSTEM, "influxdb"),
+            equalTo(DbIncubatingAttributes.DB_NAME, databaseName),
+            equalTo(ServerAttributes.SERVER_ADDRESS, host),
+            equalTo(ServerAttributes.SERVER_PORT, port),
+            equalTo(DbIncubatingAttributes.DB_OPERATION, operation)));
+    if (statement != null) {
+      result.add(equalTo(DbIncubatingAttributes.DB_STATEMENT, statement));
+    }
+    return result;
   }
 }
diff --git a/...4/java/io/opentelemetry/javaagent/instrumentation/influxdb/v2_4/InfluxDbClient24Test.java b/...4/java/io/opentelemetry/javaagent/instrumentation/influxdb/v2_4/InfluxDbClient24Test.java
@@ -5,8 +5,6 @@
 
 package io.opentelemetry.javaagent.instrumentation.influxdb.v2_4;
 
-import static io.opentelemetry.javaagent.instrumentation.influxdb.v2_4.InfluxDbConstants.CREATE_DATABASE_STATEMENT_NEW;
-import static io.opentelemetry.javaagent.instrumentation.influxdb.v2_4.InfluxDbConstants.DELETE_DATABASE_STATEMENT;
 import static io.opentelemetry.sdk.testing.assertj.OpenTelemetryAssertions.equalTo;
 import static java.util.Arrays.asList;
 import static org.assertj.core.api.Assertions.assertThat;
@@ -17,6 +15,7 @@
 import io.opentelemetry.sdk.testing.assertj.AttributeAssertion;
 import io.opentelemetry.semconv.ServerAttributes;
 import io.opentelemetry.semconv.incubating.DbIncubatingAttributes;
+import java.util.ArrayList;
 import java.util.List;
 import java.util.concurrent.TimeUnit;
 import org.influxdb.InfluxDB;
@@ -101,16 +100,13 @@ void testQueryAndModifyWithOneArgument() {
                     span.hasName("CREATE DATABASE " + dbName)
                         .hasKind(SpanKind.CLIENT)
                         .hasAttributesSatisfying(
-                            attributeAssertions(
-                                String.format(CREATE_DATABASE_STATEMENT_NEW, dbName),
-                                "CREATE DATABASE",
-                                dbName))),
+                            attributeAssertions(null, "CREATE DATABASE", dbName))),
         trace ->
             trace.hasSpansSatisfyingExactly(
                 span ->
-                    span.hasName("write " + dbName)
+                    span.hasName("WRITE " + dbName)
                         .hasKind(SpanKind.CLIENT)
-                        .hasAttributesSatisfying(attributeAssertions("write", "write", dbName))),
+                        .hasAttributesSatisfying(attributeAssertions(null, "WRITE", dbName))),
         trace ->
             trace.hasSpansSatisfyingExactly(
                 span ->
@@ -124,10 +120,7 @@ void testQueryAndModifyWithOneArgument() {
                     span.hasName("DROP DATABASE " + dbName)
                         .hasKind(SpanKind.CLIENT)
                         .hasAttributesSatisfying(
-                            attributeAssertions(
-                                String.format(DELETE_DATABASE_STATEMENT, dbName),
-                                "DROP DATABASE",
-                                dbName))));
+                            attributeAssertions(null, "DROP DATABASE", dbName))));
   }
 
   @Test
@@ -150,12 +143,17 @@ void testQueryWithTwoArguments() {
 
   private static List<AttributeAssertion> attributeAssertions(
       String statement, String operation, String databaseName) {
-    return asList(
-        equalTo(DbIncubatingAttributes.DB_SYSTEM, "influxdb"),
-        equalTo(DbIncubatingAttributes.DB_NAME, databaseName),
-        equalTo(ServerAttributes.SERVER_ADDRESS, host),
-        equalTo(ServerAttributes.SERVER_PORT, port),
-        equalTo(DbIncubatingAttributes.DB_STATEMENT, statement),
-        equalTo(DbIncubatingAttributes.DB_OPERATION, operation));
+    List<AttributeAssertion> result = new ArrayList<>();
+    result.addAll(
+        asList(
+            equalTo(DbIncubatingAttributes.DB_SYSTEM, "influxdb"),
+            equalTo(DbIncubatingAttributes.DB_NAME, databaseName),
+            equalTo(ServerAttributes.SERVER_ADDRESS, host),
+            equalTo(ServerAttributes.SERVER_PORT, port),
+            equalTo(DbIncubatingAttributes.DB_OPERATION, operation)));
+    if (statement != null) {
+      result.add(equalTo(DbIncubatingAttributes.DB_STATEMENT, statement));
+    }
+    return result;
   }
 }