Update SQL span name for procedures (#7557)

This PR includes updates to the SQLSanitizer, DbClientSpanNameExtractor
and SqlStatementInfo to name spans according to procedure name for CALL
statements. The updates to the naming logic are in the SqlSanitizer and
table has been renamed to identifier as using the table variable for the
procedure name would not be idiomatic. SqlStatementInfo has been updated
so that the db.sql.table attribute is not included for procedures.

Author: akats7

Diff for: instrumentation-api-semconv/src/main/java/io/opentelemetry/instrumentation/api/db/SqlStatementInfo.java

@@ -6,19 +6,14 @@
 package io.opentelemetry.instrumentation.api.db;
 
 import com.google.auto.value.AutoValue;
-import java.util.function.Function;
 import javax.annotation.Nullable;
 
 @AutoValue
 public abstract class SqlStatementInfo {
 
   public static SqlStatementInfo create(
-      @Nullable String fullStatement, @Nullable String operation, @Nullable String table) {
-    return new AutoValue_SqlStatementInfo(fullStatement, operation, table);
-  }
-
-  public SqlStatementInfo mapTable(Function<String, String> mapper) {
-    return SqlStatementInfo.create(getFullStatement(), getOperation(), mapper.apply(getTable()));
+      @Nullable String fullStatement, @Nullable String operation, @Nullable String identifier) {
+    return new AutoValue_SqlStatementInfo(fullStatement, operation, identifier);
   }
 
   @Nullable
@@ -28,5 +23,5 @@ public SqlStatementInfo mapTable(Function<String, String> mapper) {
   public abstract String getOperation();
 
   @Nullable
-  public abstract String getTable();
+  public abstract String getMainIdentifier();
 }

Diff for: instrumentation-api-semconv/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/db/DbClientSpanNameExtractor.java

@@ -25,11 +25,12 @@ public static <REQUEST> SpanNameExtractor<REQUEST> create(
 
   /**
    * Returns a {@link SpanNameExtractor} that constructs the span name according to DB semantic
-   * conventions: {@code <db.operation> <db.name>.<table>}.
+   * conventions: {@code <db.operation> <db.name>.<identifier>}.
    *
    * @see SqlStatementInfo#getOperation() used to extract {@code <db.operation>}.
    * @see DbClientAttributesGetter#name(Object) used to extract {@code <db.name>}.
-   * @see SqlStatementInfo#getTable() used to extract {@code <db.table>}.
+   * @see SqlStatementInfo#getMainIdentifier() used to extract {@code <db.table>} or stored
+   *     procedure name.
    */
   public static <REQUEST> SpanNameExtractor<REQUEST> create(
       SqlClientAttributesGetter<REQUEST> getter) {
@@ -40,24 +41,24 @@ public static <REQUEST> SpanNameExtractor<REQUEST> create(
 
   private DbClientSpanNameExtractor() {}
 
-  protected String computeSpanName(String dbName, String operation, String table) {
+  protected String computeSpanName(String dbName, String operation, String mainIdentifier) {
     if (operation == null) {
       return dbName == null ? DEFAULT_SPAN_NAME : dbName;
     }
 
     StringBuilder name = new StringBuilder(operation);
-    if (dbName != null || table != null) {
+    if (dbName != null || mainIdentifier != null) {
       name.append(' ');
     }
-    // skip db name if table already has a db name prefixed to it
-    if (dbName != null && (table == null || table.indexOf('.') == -1)) {
+    // skip db name if identifier already has a db name prefixed to it
+    if (dbName != null && (mainIdentifier == null || mainIdentifier.indexOf('.') == -1)) {
       name.append(dbName);
-      if (table != null) {
+      if (mainIdentifier != null) {
         name.append('.');
       }
     }
-    if (table != null) {
-      name.append(table);
+    if (mainIdentifier != null) {
+      name.append(mainIdentifier);
     }
     return name.toString();
   }
@@ -82,7 +83,7 @@ public String extract(REQUEST request) {
   private static final class SqlClientSpanNameExtractor<REQUEST>
       extends DbClientSpanNameExtractor<REQUEST> {
 
-    // a dedicated sanitizer just for extracting the operation and table name
+    // a dedicated sanitizer just for extracting the operation and identifier name
     private static final SqlStatementSanitizer sanitizer = SqlStatementSanitizer.create(true);
 
     private final SqlClientAttributesGetter<REQUEST> getter;
@@ -96,7 +97,7 @@ public String extract(REQUEST request) {
       String dbName = getter.name(request);
       SqlStatementInfo sanitizedStatement = sanitizer.sanitize(getter.rawStatement(request));
       return computeSpanName(
-          dbName, sanitizedStatement.getOperation(), sanitizedStatement.getTable());
+          dbName, sanitizedStatement.getOperation(), sanitizedStatement.getMainIdentifier());
     }
   }
 }

Diff for: instrumentation-api-semconv/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/db/SqlClientAttributesExtractor.java

@@ -43,6 +43,8 @@ public static <REQUEST, RESPONSE> SqlClientAttributesExtractorBuilder<REQUEST, R
     return new SqlClientAttributesExtractorBuilder<>(getter);
   }
 
+  private static final String SQL_CALL = "CALL";
+
   private final AttributeKey<String> dbTableAttribute;
   private final SqlStatementSanitizer sanitizer;
 
@@ -60,8 +62,11 @@ public void onStart(AttributesBuilder attributes, Context parentContext, REQUEST
     super.onStart(attributes, parentContext, request);
 
     SqlStatementInfo sanitizedStatement = sanitizer.sanitize(getter.rawStatement(request));
+    String operation = sanitizedStatement.getOperation();
     internalSet(attributes, SemanticAttributes.DB_STATEMENT, sanitizedStatement.getFullStatement());
-    internalSet(attributes, SemanticAttributes.DB_OPERATION, sanitizedStatement.getOperation());
-    internalSet(attributes, dbTableAttribute, sanitizedStatement.getTable());
+    internalSet(attributes, SemanticAttributes.DB_OPERATION, operation);
+    if (!SQL_CALL.equals(operation)) {
+      internalSet(attributes, dbTableAttribute, sanitizedStatement.getMainIdentifier());
+    }
   }
 }

Diff for: instrumentation-api-semconv/src/main/jflex/SqlSanitizer.jflex

@@ -63,13 +63,13 @@ WHITESPACE          = [ \t\r\n]+
   }
 
   /** @return text matched by current token without enclosing double quotes or backticks */
-  private String readTableName() {
-    String tableName = yytext();
-    if (tableName != null && ((tableName.startsWith("\"") && tableName.endsWith("\""))
-        || (tableName.startsWith("`") && tableName.endsWith("`")))) {
-      tableName = tableName.substring(1, tableName.length() - 1);
+  private String readIdentifierName() {
+    String identifierName = yytext();
+    if (identifierName != null && ((identifierName.startsWith("\"") && identifierName.endsWith("\""))
+        || (identifierName.startsWith("`") && identifierName.endsWith("`")))) {
+      identifierName = identifierName.substring(1, identifierName.length() - 1);
     }
-    return tableName;
+    return identifierName;
   }
 
   // you can reference a table in the FROM clause in one of the following ways:
@@ -92,7 +92,7 @@ WHITESPACE          = [ \t\r\n]+
   }
 
   private static abstract class Operation {
-    String mainTable = null;
+    String mainIdentifier = null;
 
     /** @return true if all statement info is gathered */
     boolean handleFrom() {
@@ -119,8 +119,13 @@ WHITESPACE          = [ \t\r\n]+
       return false;
     }
 
+    /** @return true if all statement info is gathered */
+    boolean handleNext() {
+      return false;
+     }
+
     SqlStatementInfo getResult(String fullStatement) {
-      return SqlStatementInfo.create(fullStatement, getClass().getSimpleName().toUpperCase(java.util.Locale.ROOT), mainTable);
+      return SqlStatementInfo.create(fullStatement, getClass().getSimpleName().toUpperCase(java.util.Locale.ROOT), mainIdentifier);
     }
   }
 
@@ -152,13 +157,13 @@ WHITESPACE          = [ \t\r\n]+
       }
 
       // subquery in WITH or SELECT clause, before main FROM clause; skipping
-      mainTable = null;
+      mainIdentifier = null;
       return true;
     }
 
     boolean handleJoin() {
       // for SELECT statements with joined tables there's no main table
-      mainTable = null;
+      mainIdentifier = null;
       return true;
     }
 
@@ -173,17 +178,17 @@ WHITESPACE          = [ \t\r\n]+
 
       // SELECT FROM (subquery) case
       if (parenLevel != 0) {
-        mainTable = null;
+        mainIdentifier = null;
         return true;
       }
 
       // whenever >1 table is used there is no main table (e.g. unions)
       if (mainTableSetAlready) {
-        mainTable = null;
+        mainIdentifier = null;
         return true;
       }
 
-      mainTable = readTableName();
+      mainIdentifier = readIdentifierName();
       mainTableSetAlready = true;
       expectingTableName = false;
       // start counting identifiers after encountering main from clause
@@ -199,7 +204,7 @@ WHITESPACE          = [ \t\r\n]+
       // any other list that can appear later needs at least 4 idents)
       if (identifiersAfterMainFromClause > 0
           && identifiersAfterMainFromClause <= FROM_TABLE_REF_MAX_IDENTIFIERS) {
-        mainTable = null;
+        mainIdentifier = null;
         return true;
       }
       return false;
@@ -219,7 +224,7 @@ WHITESPACE          = [ \t\r\n]+
         return false;
       }
 
-      mainTable = readTableName();
+      mainIdentifier = readIdentifierName();
       return true;
     }
   }
@@ -237,21 +242,33 @@ WHITESPACE          = [ \t\r\n]+
         return false;
       }
 
-      mainTable = readTableName();
+      mainIdentifier = readIdentifierName();
       return true;
     }
   }
 
   private class Update extends Operation {
     boolean handleIdentifier() {
-      mainTable = readTableName();
+      mainIdentifier = readIdentifierName();
+      return true;
+    }
+  }
+
+  private class Call extends Operation {
+    boolean handleIdentifier() {
+      mainIdentifier = readIdentifierName();
+      return true;
+    }
+
+    boolean handleNext() {
+      mainIdentifier = null;
       return true;
     }
   }
 
   private class Merge extends Operation {
     boolean handleIdentifier() {
-      mainTable = readTableName();
+      mainIdentifier = readIdentifierName();
       return true;
     }
   }
@@ -298,14 +315,20 @@ WHITESPACE          = [ \t\r\n]+
           appendCurrentFragment();
           if (isOverLimit()) return YYEOF;
       }
+  "CALL" {
+          if (!insideComment) {
+            setOperation(new Call());
+          }
+          appendCurrentFragment();
+          if (isOverLimit()) return YYEOF;
+      }
   "MERGE" {
           if (!insideComment) {
             setOperation(new Merge());
           }
           appendCurrentFragment();
           if (isOverLimit()) return YYEOF;
       }
-
   "FROM" {
           if (!insideComment && !extractionDone) {
             if (operation == NoOp.INSTANCE) {
@@ -332,6 +355,13 @@ WHITESPACE          = [ \t\r\n]+
           appendCurrentFragment();
           if (isOverLimit()) return YYEOF;
       }
+  "NEXT" {
+          if (!insideComment && !extractionDone) {
+              extractionDone = operation.handleNext();
+            }
+          appendCurrentFragment();
+          if (isOverLimit()) return YYEOF;
+      }
   {COMMA} {
           if (!insideComment && !extractionDone) {
             extractionDone = operation.handleComma();
@@ -407,4 +437,4 @@ WHITESPACE          = [ \t\r\n]+
           appendCurrentFragment();
           if (isOverLimit()) return YYEOF;
       }
-}
+}

Diff for: instrumentation-api-semconv/src/test/java/io/opentelemetry/instrumentation/api/db/SqlStatementSanitizerTest.java

@@ -232,12 +232,13 @@ public Stream<? extends Arguments> provideArguments(ExtensionContext context) th
 
   static class SimplifyArgs implements ArgumentsProvider {
 
-    static Function<String, SqlStatementInfo> expect(String operation, String table) {
-      return sql -> SqlStatementInfo.create(sql, operation, table);
+    static Function<String, SqlStatementInfo> expect(String operation, String identifier) {
+      return sql -> SqlStatementInfo.create(sql, operation, identifier);
     }
 
-    static Function<String, SqlStatementInfo> expect(String sql, String operation, String table) {
-      return ignored -> SqlStatementInfo.create(sql, operation, table);
+    static Function<String, SqlStatementInfo> expect(
+        String sql, String operation, String identifier) {
+      return ignored -> SqlStatementInfo.create(sql, operation, identifier);
     }
 
     @Override
@@ -275,6 +276,7 @@ public Stream<? extends Arguments> provideArguments(ExtensionContext context) th
           Arguments.of("/* update comment */ select * from table1", expect("SELECT", "table1")),
           Arguments.of("select /*((*/abc from table", expect("SELECT", "table")),
           Arguments.of("SeLeCT * FrOm TAblE", expect("SELECT", "table")),
+          Arguments.of("select next value in hibernate_sequence", expect("SELECT", null)),
 
           // hibernate/jpa
           Arguments.of("FROM schema.table", expect("SELECT", "schema.table")),
@@ -308,6 +310,12 @@ public Stream<? extends Arguments> provideArguments(ExtensionContext context) th
               expect("update \"my table\" set answer=?", "UPDATE", "my table")),
           Arguments.of("update /*table", expect("UPDATE", null)),
 
+          // Call
+          Arguments.of("call test_proc()", expect("CALL", "test_proc")),
+          Arguments.of("call test_proc", expect("CALL", "test_proc")),
+          Arguments.of("call next value in hibernate_sequence", expect("CALL", null)),
+          Arguments.of("call db.test_proc", expect("CALL", "db.test_proc")),
+
           // Merge
           Arguments.of("merge into table", expect("MERGE", "table")),
           Arguments.of("merge into `my table`", expect("MERGE", "my table")),

Diff for: instrumentation/hibernate/hibernate-4.0/javaagent/src/test/groovy/SpringJpaTest.groovy

@@ -112,7 +112,7 @@ class SpringJpaTest extends AgentInstrumentationSpecification {
         }
         if (!isHibernate4) {
           span(2) {
-            name "test"
+            name "CALL test"
             kind CLIENT
             childOf span(1)
             attributes {
@@ -121,6 +121,7 @@ class SpringJpaTest extends AgentInstrumentationSpecification {
               "$SemanticAttributes.DB_USER" "sa"
               "$SemanticAttributes.DB_STATEMENT" "call next value for hibernate_sequence"
               "$SemanticAttributes.DB_CONNECTION_STRING" "hsqldb:mem:"
+              "$SemanticAttributes.DB_OPERATION" "CALL"
             }
           }
           span(3) {

Diff for: instrumentation/hibernate/hibernate-common/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/hibernate/OperationNameUtil.java

@@ -22,8 +22,8 @@ public static String getOperationNameForQuery(String query) {
     SqlStatementInfo info = sanitizer.sanitize(query);
     if (info.getOperation() != null) {
       operation = info.getOperation();
-      if (info.getTable() != null) {
-        operation += " " + info.getTable();
+      if (info.getMainIdentifier() != null) {
+        operation += " " + info.getMainIdentifier();
       }
     }
     return operation;

Diff for: instrumentation/hibernate/hibernate-procedure-call-4.3/javaagent/src/test/groovy/ProcedureCallTest.groovy

@@ -104,7 +104,7 @@ class ProcedureCallTest extends AgentInstrumentationSpecification {
           }
         }
         span(2) {
-          name "test"
+          name "CALL test.TEST_PROC"
           kind CLIENT
           childOf span(1)
           attributes {
@@ -113,6 +113,7 @@ class ProcedureCallTest extends AgentInstrumentationSpecification {
             "$SemanticAttributes.DB_USER" "sa"
             "$SemanticAttributes.DB_STATEMENT" "{call TEST_PROC()}"
             "$SemanticAttributes.DB_CONNECTION_STRING" "hsqldb:mem:"
+            "$SemanticAttributes.DB_OPERATION" "CALL"
           }
         }
         span(3) {

Diff for: instrumentation/spring/spring-data/spring-data-common/testing/src/main/groovy/AbstractSpringJpaTest.groovy

@@ -111,7 +111,7 @@ abstract class AbstractSpringJpaTest<ENTITY, REPOSITORY extends JpaRepository<EN
         if (!isHibernate4) {
           offset = 1
           span(1) {
-            name "test"
+            name "CALL test"
             kind CLIENT
             childOf span(0)
             attributes {
@@ -120,6 +120,7 @@ abstract class AbstractSpringJpaTest<ENTITY, REPOSITORY extends JpaRepository<EN
               "$SemanticAttributes.DB_USER" "sa"
               "$SemanticAttributes.DB_CONNECTION_STRING" "hsqldb:mem:"
               "$SemanticAttributes.DB_STATEMENT" ~/^call next value for /
+              "$SemanticAttributes.DB_OPERATION" "CALL"
             }
           }
         }