Simplify ClickHouse instrumentation (#16206)

Co-authored-by: Jay DeLuca <jaydeluca4@gmail.com>

Author: trask

instrumentation-api-incubator/src/main/java/io/opentelemetry/instrumentation/api/incubator/semconv/db/DbClientSpanNameExtractor.java

@@ -30,6 +30,23 @@ public static <REQUEST> SpanNameExtractor<REQUEST> create(
     return new SqlClientSpanNameExtractor<>(getter);
   }
 
+  /**
+   * Returns a {@link SpanNameExtractor} that uses SQL parsing for stable semconv span names but
+   * preserves the old semconv span name format (without collection name) for backward
+   * compatibility.
+   *
+   * <p>This is a transitional method for migrating instrumentations from {@link
+   * DbClientAttributesGetter} to {@link SqlClientAttributesGetter}. Once old database semconv are
+   * dropped, callers should switch to {@link #create(SqlClientAttributesGetter)}.
+   *
+   * @deprecated Use {@link #create(SqlClientAttributesGetter)} instead.
+   */
+  @Deprecated // to be removed in 3.0
+  public static <REQUEST> SpanNameExtractor<REQUEST> createForMigration(
+      SqlClientAttributesGetter<REQUEST, ?> getter) {
+    return new MigratingSqlClientSpanNameExtractor<>(getter);
+  }
+
   private static final String DEFAULT_SPAN_NAME = "DB Query";
 
   private DbClientSpanNameExtractor() {}
@@ -166,9 +183,15 @@ public String extract(REQUEST request) {
 
       if (rawQueryTexts.isEmpty()) {
         if (SemconvStability.emitStableDatabaseSemconv()) {
-          return computeSpanNameStable(getter, request, null, null, null);
+          String querySummary = getter.getDbQuerySummary(request);
+          if (querySummary != null) {
+            return querySummary;
+          }
+          String operationName = getter.getDbOperationName(request);
+          return computeSpanNameStable(getter, request, operationName, null, null);
         }
-        return computeSpanName(namespace, null, null, null);
+        String operationName = getter.getDbOperationName(request);
+        return computeSpanName(namespace, operationName, null, null);
       }
 
       if (!SemconvStability.emitStableDatabaseSemconv()) {
@@ -210,4 +233,42 @@ private boolean isBatch(REQUEST request) {
       return batchSize != null && batchSize > 1;
     }
   }
+
+  /**
+   * A transitional span name extractor that uses SQL parsing for stable semconv but preserves the
+   * old semconv span name format (operation + namespace, without collection name) for backward
+   * compatibility during migration from {@link DbClientAttributesGetter} to {@link
+   * SqlClientAttributesGetter}.
+   */
+  private static final class MigratingSqlClientSpanNameExtractor<REQUEST>
+      extends DbClientSpanNameExtractor<REQUEST> {
+
+    private final SqlClientAttributesGetter<REQUEST, ?> getter;
+    private final SqlClientSpanNameExtractor<REQUEST> sqlDelegate;
+
+    private MigratingSqlClientSpanNameExtractor(SqlClientAttributesGetter<REQUEST, ?> getter) {
+      this.getter = getter;
+      this.sqlDelegate = new SqlClientSpanNameExtractor<>(getter);
+    }
+
+    @Override
+    public String extract(REQUEST request) {
+      if (SemconvStability.emitStableDatabaseSemconv()) {
+        return sqlDelegate.extract(request);
+      }
+      // For old semconv, use the generic span name format (operation + namespace)
+      // without collection name to preserve backward compatibility
+      String namespace = getter.getDbNamespace(request);
+      Collection<String> rawQueryTexts = getter.getRawQueryTexts(request);
+      String operationName = null;
+      if (rawQueryTexts.size() == 1) {
+        SqlQuery sanitizedQuery = SqlQuerySanitizerUtil.sanitize(rawQueryTexts.iterator().next());
+        operationName = sanitizedQuery.getOperationName();
+      }
+      if (operationName == null) {
+        operationName = getter.getDbOperationName(request);
+      }
+      return computeSpanName(namespace, operationName, null, null);
+    }
+  }
 }

instrumentation-api-incubator/src/main/java/io/opentelemetry/instrumentation/api/incubator/semconv/db/SqlClientAttributesExtractor.java

@@ -57,13 +57,13 @@ public static <REQUEST, RESPONSE> SqlClientAttributesExtractorBuilder<REQUEST, R
   private final SqlClientAttributesGetter<REQUEST, RESPONSE> getter;
   private final InternalNetworkAttributesExtractor<REQUEST, RESPONSE> internalNetworkExtractor;
   private final ServerAttributesExtractor<REQUEST, RESPONSE> serverAttributesExtractor;
-  private final AttributeKey<String> oldSemconvTableAttribute;
+  @Nullable private final AttributeKey<String> oldSemconvTableAttribute;
   private final boolean querySanitizationEnabled;
   private final boolean captureQueryParameters;
 
   SqlClientAttributesExtractor(
       SqlClientAttributesGetter<REQUEST, RESPONSE> getter,
-      AttributeKey<String> oldSemconvTableAttribute,
+      @Nullable AttributeKey<String> oldSemconvTableAttribute,
       boolean querySanitizationEnabled,
       boolean captureQueryParameters) {
     this.getter = getter;
@@ -91,7 +91,9 @@ public void onStart(AttributesBuilder attributes, Context parentContext, REQUEST
         attributes.put(
             DB_STATEMENT, querySanitizationEnabled ? sanitizedQuery.getQueryText() : rawQueryText);
         attributes.put(DB_OPERATION, operationName);
-        attributes.put(oldSemconvTableAttribute, sanitizedQuery.getCollectionName());
+        if (oldSemconvTableAttribute != null) {
+          attributes.put(oldSemconvTableAttribute, sanitizedQuery.getCollectionName());
+        }
       }
     }
 

instrumentation-api-incubator/src/main/java/io/opentelemetry/instrumentation/api/incubator/semconv/db/SqlClientAttributesExtractorBuilder.java

@@ -5,11 +5,10 @@
 
 package io.opentelemetry.instrumentation.api.incubator.semconv.db;
 
-import static java.util.Objects.requireNonNull;
-
 import com.google.errorprone.annotations.CanIgnoreReturnValue;
 import io.opentelemetry.api.common.AttributeKey;
 import io.opentelemetry.instrumentation.api.instrumenter.AttributesExtractor;
+import javax.annotation.Nullable;
 
 /** A builder of {@link SqlClientAttributesExtractor}. */
 public final class SqlClientAttributesExtractorBuilder<REQUEST, RESPONSE> {
@@ -18,7 +17,7 @@ public final class SqlClientAttributesExtractorBuilder<REQUEST, RESPONSE> {
   private static final AttributeKey<String> DB_SQL_TABLE = AttributeKey.stringKey("db.sql.table");
 
   final SqlClientAttributesGetter<REQUEST, RESPONSE> getter;
-  AttributeKey<String> oldSemconvTableAttribute = DB_SQL_TABLE;
+  @Nullable AttributeKey<String> oldSemconvTableAttribute = DB_SQL_TABLE;
   boolean querySanitizationEnabled = true;
   boolean captureQueryParameters = false;
 
@@ -27,13 +26,16 @@ public final class SqlClientAttributesExtractorBuilder<REQUEST, RESPONSE> {
   }
 
   /**
+   * Sets the attribute key for the old semconv table attribute. Pass {@code null} to disable
+   * emitting any table attribute under old semconv.
+   *
    * @deprecated new semantic conventions always use db.collection.name
    */
   @CanIgnoreReturnValue
   @Deprecated // to be removed in 3.0
   public SqlClientAttributesExtractorBuilder<REQUEST, RESPONSE> setTableAttribute(
-      AttributeKey<String> oldSemconvTableAttribute) {
-    this.oldSemconvTableAttribute = requireNonNull(oldSemconvTableAttribute);
+      @Nullable AttributeKey<String> oldSemconvTableAttribute) {
+    this.oldSemconvTableAttribute = oldSemconvTableAttribute;
     return this;
   }
 

instrumentation-api-incubator/src/test/java/io/opentelemetry/instrumentation/api/incubator/semconv/db/DbClientSpanNameExtractorTest.java

@@ -6,6 +6,7 @@
 package io.opentelemetry.instrumentation.api.incubator.semconv.db;
 
 import static io.opentelemetry.instrumentation.api.internal.SemconvStability.emitStableDatabaseSemconv;
+import static java.util.Collections.emptyList;
 import static java.util.Collections.singleton;
 import static org.assertj.core.api.Assertions.assertThat;
 import static org.mockito.Mockito.lenient;
@@ -208,5 +209,64 @@ void shouldExtractFullSpanNameForSingleQueryBatch() {
                 : "INSERT database.table");
   }
 
+  @Test
+  void shouldFallBackToExplicitOperationNameForEmptySqlQuery() {
+    // given
+    DbRequest dbRequest = new DbRequest();
+
+    when(sqlAttributesGetter.getRawQueryTexts(dbRequest)).thenReturn(emptyList());
+    when(sqlAttributesGetter.getDbOperationName(dbRequest)).thenReturn("WRITE");
+    when(sqlAttributesGetter.getDbNamespace(dbRequest)).thenReturn("mydb");
+
+    SpanNameExtractor<DbRequest> underTest = DbClientSpanNameExtractor.create(sqlAttributesGetter);
+
+    // when
+    String spanName = underTest.extract(dbRequest);
+
+    // then
+    assertThat(spanName).isEqualTo("WRITE mydb");
+  }
+
+  @Test
+  @SuppressWarnings("deprecation") // testing deprecated method
+  void shouldPreserveOldSemconvSpanNameForMigration() {
+    // given
+    DbRequest dbRequest = new DbRequest();
+
+    when(sqlAttributesGetter.getRawQueryTexts(dbRequest))
+        .thenReturn(singleton("SELECT * from table"));
+    lenient().when(sqlAttributesGetter.getDbNamespace(dbRequest)).thenReturn("database");
+
+    SpanNameExtractor<DbRequest> underTest =
+        DbClientSpanNameExtractor.createForMigration(sqlAttributesGetter);
+
+    // when
+    String spanName = underTest.extract(dbRequest);
+
+    // then
+    assertThat(spanName)
+        .isEqualTo(emitStableDatabaseSemconv() ? "SELECT table" : "SELECT database");
+  }
+
+  @Test
+  @SuppressWarnings("deprecation") // testing deprecated method
+  void shouldFallBackToExplicitOperationForEmptySqlQueryInMigration() {
+    // given
+    DbRequest dbRequest = new DbRequest();
+
+    when(sqlAttributesGetter.getRawQueryTexts(dbRequest)).thenReturn(emptyList());
+    when(sqlAttributesGetter.getDbOperationName(dbRequest)).thenReturn("WRITE");
+    when(sqlAttributesGetter.getDbNamespace(dbRequest)).thenReturn("mydb");
+
+    SpanNameExtractor<DbRequest> underTest =
+        DbClientSpanNameExtractor.createForMigration(sqlAttributesGetter);
+
+    // when
+    String spanName = underTest.extract(dbRequest);
+
+    // then
+    assertThat(spanName).isEqualTo("WRITE mydb");
+  }
+
   static class DbRequest {}
 }

instrumentation/clickhouse/clickhouse-client-common/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/clickhouse/common/ClickHouseAttributesGetter.java

@@ -5,48 +5,25 @@
 
 package io.opentelemetry.javaagent.instrumentation.clickhouse.common;
 
-import io.opentelemetry.instrumentation.api.incubator.semconv.db.DbClientAttributesGetter;
+import io.opentelemetry.instrumentation.api.incubator.semconv.db.SqlClientAttributesGetter;
 import io.opentelemetry.semconv.incubating.DbIncubatingAttributes;
+import java.util.Collection;
+import java.util.Collections;
 import java.util.function.Function;
 import javax.annotation.Nullable;
 
 final class ClickHouseAttributesGetter
-    implements DbClientAttributesGetter<ClickHouseDbRequest, Void> {
+    implements SqlClientAttributesGetter<ClickHouseDbRequest, Void> {
 
   private final Function<Throwable, String> errorCodeExtractor;
 
   ClickHouseAttributesGetter(Function<Throwable, String> errorCodeExtractor) {
     this.errorCodeExtractor = errorCodeExtractor;
   }
 
-  @Nullable
-  @Override
-  public String getDbQueryText(ClickHouseDbRequest request) {
-    if (request.getSqlQueryWithSummary() != null) {
-      return request.getSqlQueryWithSummary().getQueryText();
-    }
-    if (request.getSqlQuery() != null) {
-      return request.getSqlQuery().getQueryText();
-    }
-    return null;
-  }
-
-  @Nullable
   @Override
-  public String getDbOperationName(ClickHouseDbRequest request) {
-    if (request.getSqlQuery() != null) {
-      return request.getSqlQuery().getOperationName();
-    }
-    return null;
-  }
-
-  @Nullable
-  @Override
-  public String getDbQuerySummary(ClickHouseDbRequest request) {
-    if (request.getSqlQueryWithSummary() != null) {
-      return request.getSqlQueryWithSummary().getQuerySummary();
-    }
-    return null;
+  public Collection<String> getRawQueryTexts(ClickHouseDbRequest request) {
+    return Collections.singletonList(request.getSql());
   }
 
   @Override
@@ -69,4 +46,14 @@ public String getDbNamespace(ClickHouseDbRequest request) {
   public String getDbResponseStatusCode(@Nullable Void response, @Nullable Throwable error) {
     return errorCodeExtractor.apply(error);
   }
+
+  @Override
+  public String getServerAddress(ClickHouseDbRequest request) {
+    return request.getHost();
+  }
+
+  @Override
+  public Integer getServerPort(ClickHouseDbRequest request) {
+    return request.getPort();
+  }
 }

instrumentation/clickhouse/clickhouse-client-common/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/clickhouse/common/ClickHouseDbRequest.java

@@ -6,24 +6,14 @@
 package io.opentelemetry.javaagent.instrumentation.clickhouse.common;
 
 import com.google.auto.value.AutoValue;
-import io.opentelemetry.instrumentation.api.incubator.semconv.db.SqlQuery;
-import io.opentelemetry.instrumentation.api.incubator.semconv.db.SqlQuerySanitizer;
-import io.opentelemetry.instrumentation.api.internal.SemconvStability;
-import io.opentelemetry.javaagent.bootstrap.internal.AgentCommonConfig;
 import javax.annotation.Nullable;
 
 @AutoValue
 public abstract class ClickHouseDbRequest {
 
-  private static final SqlQuerySanitizer sanitizer =
-      SqlQuerySanitizer.create(AgentCommonConfig.get().isQuerySanitizationEnabled());
-
   public static ClickHouseDbRequest create(
       @Nullable String host, @Nullable Integer port, @Nullable String namespace, String sql) {
-    SqlQuery sqlQuery = SemconvStability.emitOldDatabaseSemconv() ? sanitizer.sanitize(sql) : null;
-    SqlQuery sqlQueryWithSummary =
-        SemconvStability.emitStableDatabaseSemconv() ? sanitizer.sanitizeWithSummary(sql) : null;
-    return new AutoValue_ClickHouseDbRequest(host, port, namespace, sqlQuery, sqlQueryWithSummary);
+    return new AutoValue_ClickHouseDbRequest(host, port, namespace, sql);
   }
 
   @Nullable
@@ -35,9 +25,5 @@ public static ClickHouseDbRequest create(
   @Nullable
   public abstract String getNamespace();
 
-  @Nullable
-  public abstract SqlQuery getSqlQuery();
-
-  @Nullable
-  public abstract SqlQuery getSqlQueryWithSummary();
+  public abstract String getSql();
 }

instrumentation/clickhouse/clickhouse-client-common/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/clickhouse/common/ClickHouseInstrumenterFactory.java

@@ -6,16 +6,16 @@
 package io.opentelemetry.javaagent.instrumentation.clickhouse.common;
 
 import io.opentelemetry.api.GlobalOpenTelemetry;
-import io.opentelemetry.instrumentation.api.incubator.semconv.db.DbClientAttributesExtractor;
 import io.opentelemetry.instrumentation.api.incubator.semconv.db.DbClientMetrics;
 import io.opentelemetry.instrumentation.api.incubator.semconv.db.DbClientSpanNameExtractor;
+import io.opentelemetry.instrumentation.api.incubator.semconv.db.SqlClientAttributesExtractor;
 import io.opentelemetry.instrumentation.api.instrumenter.Instrumenter;
 import io.opentelemetry.instrumentation.api.instrumenter.SpanKindExtractor;
-import io.opentelemetry.instrumentation.api.semconv.network.ServerAttributesExtractor;
 import java.util.function.Function;
 
 public final class ClickHouseInstrumenterFactory {
 
+  @SuppressWarnings("deprecation") // to support old semconv
   public static Instrumenter<ClickHouseDbRequest, Void> createInstrumenter(
       String instrumenterName, Function<Throwable, String> errorCodeExtractor) {
     ClickHouseAttributesGetter dbAttributesGetter =
@@ -24,10 +24,11 @@ public static Instrumenter<ClickHouseDbRequest, Void> createInstrumenter(
     return Instrumenter.<ClickHouseDbRequest, Void>builder(
             GlobalOpenTelemetry.get(),
             instrumenterName,
-            DbClientSpanNameExtractor.create(dbAttributesGetter))
-        .addAttributesExtractor(DbClientAttributesExtractor.create(dbAttributesGetter))
+            DbClientSpanNameExtractor.createForMigration(dbAttributesGetter))
         .addAttributesExtractor(
-            ServerAttributesExtractor.create(new ClickHouseNetworkAttributesGetter()))
+            SqlClientAttributesExtractor.<ClickHouseDbRequest, Void>builder(dbAttributesGetter)
+                .setTableAttribute(null)
+                .build())
         .addOperationMetrics(DbClientMetrics.get())
         .buildInstrumenter(SpanKindExtractor.alwaysClient());
   }