Merge pull request #27 from signalfx/do-not-use-db.statement-as-span-name-for-sql-statements

Rename JDBC spans to lower cardinality names (operation name instead of statement)

Author: Mateusz Rzeszutek

README.md

@@ -112,6 +112,7 @@ System property values take priority over corresponding environment variables.
 | System property                                    | Environment variable                              | Default value | Notes                                                                                                                                         |
 | -------------------------------------------------- | ------------------------------------------------- | --------------| --------------------------------------------------------------------------------------------------------------------------------------------- |
 | splunk.otel.config.span.processor.instrlib.enabled | SPLUNK_OTEL_CONFIG_SPAN_PROCESSOR_INSTRLIB_ENABLED| `false`       | Enables span processing adding library instrumentation properties. Deprecated feature for customers not on the newest OpenTelemetry Collector |
+| splunk.jdbc.low.cardinality.span.name.enabled      | SPLUNK_JDBC_LOW_CARDINALITY_SPAN_NAME_ENABLED     | `true`        | Enables low cardinality span names for spans generated by the JDBC auto-instrumentation. By default JDBC spans use full SQL queries as their names; when this property is on just the SQL statement type should be used (e.g. `SELECT`, `INSERT`). |
 
 ## Manually instrument a Java application
 

build.gradle.kts

@@ -37,8 +37,10 @@ subprojects {
   }
 
   dependencies {
-    testImplementation("org.mockito:mockito-core:3.3.3")
+    testImplementation("org.mockito:mockito-core:3.5.15")
+    testImplementation("org.mockito:mockito-junit-jupiter:3.5.15")
     testImplementation("org.junit.jupiter:junit-jupiter-api:5.6.2")
+    testImplementation("org.junit.jupiter:junit-jupiter-params:5.6.2")
     testRuntimeOnly("org.junit.jupiter:junit-jupiter-engine:5.6.2")
   }
 

custom/src/main/java/com/splunk/opentelemetry/InstrumentationLibraryTracerCustomizer.java

@@ -24,6 +24,9 @@ public class InstrumentationLibraryTracerCustomizer implements TracerCustomizer
   static final String PROPERTY_SPAN_PROCESSOR_INSTR_LIB_ENABLED =
       "splunk.otel.config.span.processor.instrlib.enabled";
 
+  static final String ENABLE_JDBC_SPAN_LOW_CARDINALITY_NAME_PROPERTY =
+      "splunk.jdbc.low.cardinality.span.name.enabled";
+
   private static String propertyToEnv(String property) {
     return property.replace(".", "_").toUpperCase();
   }
@@ -33,14 +36,25 @@ private static boolean spanProcessorInstrumentationLibraryEnabled() {
     if (value == null) {
       value = System.getenv(propertyToEnv(PROPERTY_SPAN_PROCESSOR_INSTR_LIB_ENABLED));
     }
-    return ("true".equalsIgnoreCase(value));
+    return Boolean.parseBoolean(value);
+  }
+
+  private static boolean jdbcSpanLowCardinalityNameEnabled() {
+    String value = System.getProperty(ENABLE_JDBC_SPAN_LOW_CARDINALITY_NAME_PROPERTY);
+    if (value == null) {
+      value = System.getenv(propertyToEnv(ENABLE_JDBC_SPAN_LOW_CARDINALITY_NAME_PROPERTY));
+    }
+    // enabled by default
+    return value == null || Boolean.parseBoolean(value);
   }
 
   @Override
   public void configure(TracerSdkManagement tracerManagement) {
-
     if (spanProcessorInstrumentationLibraryEnabled()) {
       tracerManagement.addSpanProcessor(new InstrumentationLibrarySpanProcessor());
     }
+    if (jdbcSpanLowCardinalityNameEnabled()) {
+      tracerManagement.addSpanProcessor(new JdbcSpanRenamingProcessor());
+    }
   }
 }

custom/src/main/java/com/splunk/opentelemetry/JdbcSpanRenamingProcessor.java

@@ -0,0 +1,103 @@
+/*
+ * Copyright Splunk Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package com.splunk.opentelemetry;
+
+import io.opentelemetry.sdk.common.CompletableResultCode;
+import io.opentelemetry.sdk.trace.ReadWriteSpan;
+import io.opentelemetry.sdk.trace.ReadableSpan;
+import io.opentelemetry.sdk.trace.SpanProcessor;
+import io.opentelemetry.trace.attributes.SemanticAttributes;
+import java.util.HashSet;
+import java.util.Set;
+
+/**
+ * This span processor is responsible for renaming spans produced by the JDBC auto-instrumentation,
+ * which have the full SQL query in their names. To lower the cardinality and make those spans
+ * easier to analyze this processor renames spans so that they only use the SQL operation type (e.g.
+ * {@code SELECT}, {@code INSERT}, ...) as their names. Full SQL query is still available in the
+ * {@code db.statement} attribute.
+ */
+public class JdbcSpanRenamingProcessor implements SpanProcessor {
+  private static final Set<String> SQL_SYSTEMS;
+
+  static {
+    Set<String> sqlSystems = new HashSet<>();
+    sqlSystems.add("db2");
+    sqlSystems.add("derby");
+    sqlSystems.add("h2");
+    sqlSystems.add("hsqldb");
+    sqlSystems.add("mariadb");
+    sqlSystems.add("mssql");
+    sqlSystems.add("mysql");
+    sqlSystems.add("oracle");
+    sqlSystems.add("other_sql");
+    sqlSystems.add("postgresql");
+    SQL_SYSTEMS = sqlSystems;
+  }
+
+  @Override
+  public void onStart(ReadWriteSpan span) {
+    String dbSystem = span.toSpanData().getAttributes().get(SemanticAttributes.DB_SYSTEM);
+    if (isJdbcSpan(dbSystem)) {
+      String query = span.getName().trim().toUpperCase();
+      span.updateName(getStatementType(query));
+    }
+  }
+
+  private boolean isJdbcSpan(String dbSystem) {
+    return SQL_SYSTEMS.contains(dbSystem);
+  }
+
+  private String getStatementType(String query) {
+    if (query.startsWith("SELECT")) {
+      return "SELECT";
+    } else if (query.startsWith("UPDATE")) {
+      return "UPDATE";
+    } else if (query.startsWith("DELETE")) {
+      return "DELETE";
+    } else if (query.startsWith("INSERT")) {
+      return "INSERT";
+    } else if (query.startsWith("MERGE")) {
+      return "MERGE";
+    }
+
+    return "JDBC";
+  }
+
+  @Override
+  public boolean isStartRequired() {
+    return true;
+  }
+
+  @Override
+  public void onEnd(ReadableSpan span) {}
+
+  @Override
+  public boolean isEndRequired() {
+    return false;
+  }
+
+  @Override
+  public CompletableResultCode shutdown() {
+    return CompletableResultCode.ofSuccess();
+  }
+
+  @Override
+  public CompletableResultCode forceFlush() {
+    return CompletableResultCode.ofSuccess();
+  }
+}

custom/src/test/java/com/splunk/opentelemetry/InstrumentationLibraryTracerCustomizerTest.java

@@ -16,24 +16,29 @@
 
 package com.splunk.opentelemetry;
 
+import static com.splunk.opentelemetry.InstrumentationLibraryTracerCustomizer.ENABLE_JDBC_SPAN_LOW_CARDINALITY_NAME_PROPERTY;
 import static com.splunk.opentelemetry.InstrumentationLibraryTracerCustomizer.PROPERTY_SPAN_PROCESSOR_INSTR_LIB_ENABLED;
 import static org.mockito.ArgumentMatchers.isA;
 import static org.mockito.BDDMockito.then;
-import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.never;
 
 import io.opentelemetry.sdk.trace.TracerSdkProvider;
 import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.extension.ExtendWith;
+import org.mockito.Mock;
+import org.mockito.junit.jupiter.MockitoExtension;
 
+@ExtendWith(MockitoExtension.class)
 public class InstrumentationLibraryTracerCustomizerTest {
+  @Mock private TracerSdkProvider tracerSdkProvider;
 
   @Test
-  public void shouldAddSpanProcessorIfPropertySetToTrue() {
+  public void shouldAddSpanProcessorsIfPropertiesAreSetToTrue() {
 
     // given
-    TracerSdkProvider tracerSdkProvider = mock(TracerSdkProvider.class);
     InstrumentationLibraryTracerCustomizer underTest = new InstrumentationLibraryTracerCustomizer();
     System.setProperty(PROPERTY_SPAN_PROCESSOR_INSTR_LIB_ENABLED, "true");
+    System.setProperty(ENABLE_JDBC_SPAN_LOW_CARDINALITY_NAME_PROPERTY, "true");
 
     // when
     underTest.configure(tracerSdkProvider);
@@ -42,32 +47,32 @@ public void shouldAddSpanProcessorIfPropertySetToTrue() {
     then(tracerSdkProvider)
         .should()
         .addSpanProcessor(isA(InstrumentationLibrarySpanProcessor.class));
+    then(tracerSdkProvider).should().addSpanProcessor(isA(JdbcSpanRenamingProcessor.class));
+    then(tracerSdkProvider).shouldHaveNoMoreInteractions();
   }
 
   @Test
-  public void shouldNotAddSpanProcessorIfPropertySetToAnythingElse() {
+  public void shouldNotAddSpanProcessorsIfPropertiesAreSetToAnythingElse() {
 
     // given
-    TracerSdkProvider tracerSdkProvider = mock(TracerSdkProvider.class);
     InstrumentationLibraryTracerCustomizer underTest = new InstrumentationLibraryTracerCustomizer();
     System.setProperty(PROPERTY_SPAN_PROCESSOR_INSTR_LIB_ENABLED, "enabled");
+    System.setProperty(ENABLE_JDBC_SPAN_LOW_CARDINALITY_NAME_PROPERTY, "whatever");
 
     // when
     underTest.configure(tracerSdkProvider);
 
     // then
-    then(tracerSdkProvider)
-        .should(never())
-        .addSpanProcessor(isA(InstrumentationLibrarySpanProcessor.class));
+    then(tracerSdkProvider).shouldHaveNoInteractions();
   }
 
   @Test
-  public void shouldNotAddSpanProcessorIfPropertyNotSet() {
+  public void shouldConfigureTracerSdkForDefaultValues() {
 
     // given
-    TracerSdkProvider tracerSdkProvider = mock(TracerSdkProvider.class);
     InstrumentationLibraryTracerCustomizer underTest = new InstrumentationLibraryTracerCustomizer();
     System.clearProperty(PROPERTY_SPAN_PROCESSOR_INSTR_LIB_ENABLED);
+    System.clearProperty(ENABLE_JDBC_SPAN_LOW_CARDINALITY_NAME_PROPERTY);
 
     // when
     underTest.configure(tracerSdkProvider);
@@ -76,5 +81,7 @@ public void shouldNotAddSpanProcessorIfPropertyNotSet() {
     then(tracerSdkProvider)
         .should(never())
         .addSpanProcessor(isA(InstrumentationLibrarySpanProcessor.class));
+    then(tracerSdkProvider).should().addSpanProcessor(isA(JdbcSpanRenamingProcessor.class));
+    then(tracerSdkProvider).shouldHaveNoMoreInteractions();
   }
 }

custom/src/test/java/com/splunk/opentelemetry/JdbcSpanRenamingProcessorTest.java

@@ -0,0 +1,125 @@
+/*
+ * Copyright Splunk Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package com.splunk.opentelemetry;
+
+import static io.opentelemetry.trace.attributes.SemanticAttributes.DB_SYSTEM;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+import static org.junit.jupiter.params.provider.Arguments.arguments;
+import static org.mockito.Answers.RETURNS_DEEP_STUBS;
+import static org.mockito.ArgumentMatchers.anyString;
+import static org.mockito.BDDMockito.given;
+import static org.mockito.BDDMockito.then;
+import static org.mockito.Mockito.never;
+
+import io.opentelemetry.sdk.trace.ReadWriteSpan;
+import io.opentelemetry.sdk.trace.SpanProcessor;
+import java.util.stream.Stream;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.extension.ExtendWith;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.Arguments;
+import org.junit.jupiter.params.provider.MethodSource;
+import org.junit.jupiter.params.provider.ValueSource;
+import org.mockito.Mock;
+import org.mockito.junit.jupiter.MockitoExtension;
+
+@ExtendWith(MockitoExtension.class)
+class JdbcSpanRenamingProcessorTest {
+  @Mock(answer = RETURNS_DEEP_STUBS)
+  ReadWriteSpan span;
+
+  SpanProcessor processor = new JdbcSpanRenamingProcessor();
+
+  @Test
+  void shouldRequireOnStart() {
+    assertTrue(processor.isStartRequired());
+  }
+
+  @Test
+  void shouldIgnoreSpansThatDoNotHaveDbSystemAttribute() {
+    // given
+    given(span.toSpanData().getAttributes().get(DB_SYSTEM)).willReturn(null);
+
+    // when
+    processor.onStart(span);
+
+    // then
+    then(span).should(never()).updateName(anyString());
+  }
+
+  @Test
+  void shouldIgnoreSpansThatAreNotSql() {
+    // given
+    given(span.toSpanData().getAttributes().get(DB_SYSTEM)).willReturn("mongodb");
+
+    // when
+    processor.onStart(span);
+
+    // then
+    then(span).should(never()).updateName(anyString());
+  }
+
+  @ParameterizedTest
+  @ValueSource(
+      strings = {
+        "db2",
+        "derby",
+        "h2",
+        "hsqldb",
+        "mariadb",
+        "mssql",
+        "mysql",
+        "oracle",
+        "other_sql",
+        "postgresql"
+      })
+  void shouldUpdateJdbcSpans(String dbSystem) {
+    // given
+    given(span.toSpanData().getAttributes().get(DB_SYSTEM)).willReturn(dbSystem);
+    given(span.getName()).willReturn("select * from table");
+
+    // when
+    processor.onStart(span);
+
+    // then
+    then(span).should().updateName("SELECT");
+  }
+
+  @ParameterizedTest
+  @MethodSource("spanNameArgs")
+  void shouldGetSqlStatementTypeFromSpanName(String query, String sqlStatementType) {
+    // given
+    given(span.toSpanData().getAttributes().get(DB_SYSTEM)).willReturn("other_sql");
+    given(span.getName()).willReturn(query);
+
+    // when
+    processor.onStart(span);
+
+    // then
+    then(span).should().updateName(sqlStatementType);
+  }
+
+  private static Stream<Arguments> spanNameArgs() {
+    return Stream.of(
+        arguments(" Select * From Table", "SELECT"),
+        arguments("  \tINSert into TAble", "INSERT"),
+        arguments(" UPDATE table", "UPDATE"),
+        arguments("delete from table", "DELETE"),
+        arguments(" merge into table", "MERGE"),
+        arguments("alter table table add column", "JDBC"));
+  }
+}