Hibernate query span naming (open-telemetry#3106)

* Hibernate query span naming * remove commented out code * modify query sanitizer to accept queries that start with from clause * add sql sanitizer test for queries starting with from * rename hibernate-4.3 to hibernate-procedure-call-4.3
robododge · Jun 17, 2021 · ed1e9d9 · ed1e9d9
1 parent 9b77940
commit ed1e9d9
Show file tree

Hide file tree

Showing 29 changed files with 534 additions and 96 deletions.
diff --git a/instrumentation-api/src/main/jflex/SqlSanitizer.flex b/instrumentation-api/src/main/jflex/SqlSanitizer.flex
@@ -295,6 +295,11 @@ WHITESPACE        = [ \t\r\n]+
 
   "FROM" {
           if (!insideComment && !extractionDone) {
+            if (operation == NoOp.INSTANCE) {
+              // hql/jpql queries may skip SELECT and start with FROM clause
+              // treat such queries as SELECT queries
+              setOperation(new Select());
+            }
             extractionDone = operation.handleFrom();
           }
           appendCurrentFragment();

diff --git a/.../src/test/groovy/io/opentelemetry/instrumentation/api/db/SqlStatementSanitizerTest.groovy b/.../src/test/groovy/io/opentelemetry/instrumentation/api/db/SqlStatementSanitizerTest.groovy
@@ -80,6 +80,9 @@ class SqlStatementSanitizerTest extends Specification {
 
     // whitespace normalization
     "SELECT    *    \t\r\nFROM  TABLE WHERE FIELD1 = 12344 AND FIELD2 = 5678"  | "SELECT * FROM TABLE WHERE FIELD1 = ? AND FIELD2 = ?"
+
+    // hibernate/jpa query language
+    "FROM TABLE WHERE FIELD=1234"                                              | "FROM TABLE WHERE FIELD=?"
   }
 
   @Unroll
@@ -110,6 +113,9 @@ class SqlStatementSanitizerTest extends Specification {
     '/* update comment */ select * from table1'                       | SqlStatementInfo.create(sql, 'SELECT', 'table1')
     'select /*((*/abc from table'                                     | SqlStatementInfo.create(sql, 'SELECT', 'table')
     'SeLeCT * FrOm TAblE'                                             | SqlStatementInfo.create(sql, 'SELECT', 'TAblE')
+    // hibernate/jpa
+    'FROM schema.table'                                               | SqlStatementInfo.create(sql, 'SELECT', 'schema.table')
+    '/* update comment */ from table1'                                | SqlStatementInfo.create(sql, 'SELECT', 'table1')
     // Insert
     ' insert into table where lalala'                                 | SqlStatementInfo.create(sql, 'INSERT', 'table')
     'insert insert into table where lalala'                           | SqlStatementInfo.create(sql, 'INSERT', 'table')

diff --git a/instrumentation/hibernate/hibernate-3.3/javaagent/hibernate-3.3-javaagent.gradle b/instrumentation/hibernate/hibernate-3.3/javaagent/hibernate-3.3-javaagent.gradle
@@ -24,7 +24,7 @@ dependencies {
   testInstrumentation project(':instrumentation:jdbc:javaagent')
   // Added to ensure cross compatibility:
   testInstrumentation project(':instrumentation:hibernate:hibernate-4.0:javaagent')
-  testInstrumentation project(':instrumentation:hibernate:hibernate-4.3:javaagent')
+  testInstrumentation project(':instrumentation:hibernate:hibernate-procedure-call-4.3:javaagent')
 
   testLibrary "org.hibernate:hibernate-core:3.3.0.SP1"
   testImplementation "org.hibernate:hibernate-annotations:3.4.0.GA"

diff --git a/.../java/io/opentelemetry/javaagent/instrumentation/hibernate/v3_3/QueryInstrumentation.java b/.../java/io/opentelemetry/javaagent/instrumentation/hibernate/v3_3/QueryInstrumentation.java
@@ -53,7 +53,7 @@ public static void startMethod(
       ContextStore<Query, Context> contextStore =
           InstrumentationContext.get(Query.class, Context.class);
 
-      context = SessionMethodUtils.startSpanFrom(contextStore, query, query.getQueryString(), null);
+      context = SessionMethodUtils.startSpanFromQuery(contextStore, query, query.getQueryString());
       if (context != null) {
         scope = context.makeCurrent();
       }

diff --git a/instrumentation/hibernate/hibernate-3.3/javaagent/src/test/groovy/QueryTest.groovy b/instrumentation/hibernate/hibernate-3.3/javaagent/src/test/groovy/QueryTest.groovy
@@ -105,25 +105,25 @@ class QueryTest extends AbstractHibernateTest {
 
     where:
     queryMethodName       | expectedSpanName            | requiresTransaction | queryInteraction
-    "Query.list"          | "from Value"                | false               | { sess ->
+    "Query.list"          | "SELECT Value"              | false               | { sess ->
       Query q = sess.createQuery("from Value")
       q.list()
     }
-    "Query.executeUpdate" | "update Value set name = ?" | true                | { sess ->
+    "Query.executeUpdate" | "UPDATE Value"              | true                | { sess ->
       Query q = sess.createQuery("update Value set name = ?")
       q.setParameter(0, "alyx")
       q.executeUpdate()
     }
-    "Query.uniqueResult"  | "from Value where id = ?"   | false               | { sess ->
+    "Query.uniqueResult"  | "SELECT Value"              | false               | { sess ->
       Query q = sess.createQuery("from Value where id = ?")
       q.setParameter(0, 1L)
       q.uniqueResult()
     }
-    "Query.iterate"       | "from Value"                | false               | { sess ->
+    "Query.iterate"       | "SELECT Value"              | false               | { sess ->
       Query q = sess.createQuery("from Value")
       q.iterate()
     }
-    "Query.scroll"        | "from Value"                | false               | { sess ->
+    "Query.scroll"        | "SELECT Value"              | false               | { sess ->
       Query q = sess.createQuery("from Value")
       q.scroll()
     }
@@ -153,7 +153,7 @@ class QueryTest extends AbstractHibernateTest {
           }
         }
         span(1) {
-          name "from Value"
+          name "SELECT Value"
           kind INTERNAL
           childOf span(0)
           attributes {

diff --git a/instrumentation/hibernate/hibernate-3.3/javaagent/src/test/groovy/SessionTest.groovy b/instrumentation/hibernate/hibernate-3.3/javaagent/src/test/groovy/SessionTest.groovy
@@ -440,10 +440,10 @@ class SessionTest extends AbstractHibernateTest {
     }
 
     where:
-    queryMethodName  | expectedSpanName      | queryBuildMethod
-    "createQuery"    | "from Value"          | { sess -> sess.createQuery("from Value") }
-    "getNamedQuery"  | "from Value"          | { sess -> sess.getNamedQuery("TestNamedQuery") }
-    "createSQLQuery" | "SELECT * FROM Value" | { sess -> sess.createSQLQuery("SELECT * FROM Value") }
+    queryMethodName  | expectedSpanName | queryBuildMethod
+    "createQuery"    | "SELECT Value"   | { sess -> sess.createQuery("from Value") }
+    "getNamedQuery"  | "SELECT Value"   | { sess -> sess.getNamedQuery("TestNamedQuery") }
+    "createSQLQuery" | "SELECT Value"   | { sess -> sess.createSQLQuery("SELECT * FROM Value") }
   }
 
 

diff --git a/instrumentation/hibernate/hibernate-4.0/javaagent/hibernate-4.0-javaagent.gradle b/instrumentation/hibernate/hibernate-4.0/javaagent/hibernate-4.0-javaagent.gradle
@@ -1,4 +1,5 @@
 apply from: "$rootDir/gradle/instrumentation.gradle"
+apply plugin: 'org.unbroken-dome.test-sets'
 
 muzzle {
   pass {
@@ -9,21 +10,53 @@ muzzle {
   }
 }
 
+testSets {
+  version5Test {
+    dirName = 'test'
+  }
+  version6Test {
+    dirName = 'hibernate6Test'
+  }
+
+  latestDepTest {
+    dirName = 'test'
+  }
+}
+
+test.dependsOn version5Test, version6Test
+
 dependencies {
-  library "org.hibernate:hibernate-core:4.0.0.Final"
+  compileOnly "org.hibernate:hibernate-core:4.0.0.Final"
 
   implementation project(':instrumentation:hibernate:hibernate-common:javaagent')
 
   testInstrumentation project(':instrumentation:jdbc:javaagent')
   // Added to ensure cross compatibility:
   testInstrumentation project(':instrumentation:hibernate:hibernate-3.3:javaagent')
-  testInstrumentation project(':instrumentation:hibernate:hibernate-4.3:javaagent')
+  testInstrumentation project(':instrumentation:hibernate:hibernate-procedure-call-4.3:javaagent')
 
   testImplementation "com.h2database:h2:1.4.197"
   testImplementation "javax.xml.bind:jaxb-api:2.2.11"
   testImplementation "com.sun.xml.bind:jaxb-core:2.2.11"
   testImplementation "com.sun.xml.bind:jaxb-impl:2.2.11"
   testImplementation "javax.activation:activation:1.1.1"
+  testImplementation "org.hsqldb:hsqldb:2.0.0"
+  //First version to work with Java 14
+  testImplementation "org.springframework.data:spring-data-jpa:1.8.0.RELEASE"
+
+  testImplementation "org.hibernate:hibernate-core:4.0.0.Final"
+  testImplementation "org.hibernate:hibernate-entitymanager:4.0.0.Final"
+
+  version5TestImplementation "org.hibernate:hibernate-core:5.0.0.Final"
+  version5TestImplementation "org.hibernate:hibernate-entitymanager:5.0.0.Final"
+  version5TestImplementation "org.springframework.data:spring-data-jpa:2.3.0.RELEASE"
+
+  version6TestImplementation "org.hibernate:hibernate-core:6.0.0.Alpha6"
+  version6TestImplementation "org.hibernate:hibernate-entitymanager:6.0.0.Alpha6"
+  version6TestImplementation "org.springframework.data:spring-data-jpa:2.3.0.RELEASE"
 
-  latestDepTestLibrary "org.hibernate:hibernate-core:4.2.+"
+  // hibernate 6 is alpha so use 5 as latest version
+  latestDepTestImplementation "org.hibernate:hibernate-core:5.+"
+  latestDepTestImplementation "org.hibernate:hibernate-entitymanager:5.+"
+  latestDepTestImplementation "org.springframework.data:spring-data-jpa:(2.4.0,)"
 }
diff --git a/...gent/src/test/groovy/SpringJpaTest.groovy → ...ibernate6Test/groovy/SpringJpaTest.groovy b/...gent/src/test/groovy/SpringJpaTest.groovy → ...ibernate6Test/groovy/SpringJpaTest.groovy
diff --git a/instrumentation/hibernate/hibernate-4.0/javaagent/src/hibernate6Test/java/Value.java b/instrumentation/hibernate/hibernate-4.0/javaagent/src/hibernate6Test/java/Value.java
@@ -0,0 +1,45 @@
+/*
+ * Copyright The OpenTelemetry Authors
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+import javax.persistence.Entity;
+import javax.persistence.GeneratedValue;
+import javax.persistence.Id;
+import javax.persistence.Table;
+import org.hibernate.annotations.GenericGenerator;
+import org.hibernate.annotations.NamedQuery;
+
+@Entity
+@Table
+@NamedQuery(name = "TestNamedQuery", query = "FROM Value")
+public class Value {
+
+  private Long id;
+  private String name;
+
+  public Value() {}
+
+  public Value(String name) {
+    this.name = name;
+  }
+
+  @Id
+  @GeneratedValue(generator = "increment")
+  @GenericGenerator(name = "increment", strategy = "increment")
+  public Long getId() {
+    return id;
+  }
+
+  private void setId(Long id) {
+    this.id = id;
+  }
+
+  public String getName() {
+    return name;
+  }
+
+  public void setName(String title) {
+    name = title;
+  }
+}
diff --git a/...nt/src/test/java/spring/jpa/Customer.java → ...ernate6Test/java/spring/jpa/Customer.java b/...nt/src/test/java/spring/jpa/Customer.java → ...ernate6Test/java/spring/jpa/Customer.java
diff --git a/...t/java/spring/jpa/CustomerRepository.java → ...t/java/spring/jpa/CustomerRepository.java b/...t/java/spring/jpa/CustomerRepository.java → ...t/java/spring/jpa/CustomerRepository.java
diff --git a/...st/java/spring/jpa/PersistenceConfig.java → ...st/java/spring/jpa/PersistenceConfig.java b/...st/java/spring/jpa/PersistenceConfig.java → ...st/java/spring/jpa/PersistenceConfig.java
@@ -57,9 +57,6 @@ private Properties additionalProperties() {
     properties.setProperty("hibernate.show_sql", "true");
     properties.setProperty("hibernate.hbm2ddl.auto", "create");
     properties.setProperty("hibernate.dialect", "org.hibernate.dialect.HSQLDialect");
-    //    properties.setProperty(
-    //        "hibernate.format_sql",
-    // env.getProperty("spring.jpa.properties.hibernate.format_sql"));
     return properties;
   }
 }
diff --git a/.../java/io/opentelemetry/javaagent/instrumentation/hibernate/v4_0/QueryInstrumentation.java b/.../java/io/opentelemetry/javaagent/instrumentation/hibernate/v4_0/QueryInstrumentation.java
@@ -53,7 +53,7 @@ public static void startMethod(
       ContextStore<Query, Context> contextStore =
           InstrumentationContext.get(Query.class, Context.class);
 
-      context = SessionMethodUtils.startSpanFrom(contextStore, query, query.getQueryString(), null);
+      context = SessionMethodUtils.startSpanFromQuery(contextStore, query, query.getQueryString());
       if (context != null) {
         scope = context.makeCurrent();
       }

diff --git a/instrumentation/hibernate/hibernate-4.0/javaagent/src/test/groovy/QueryTest.groovy b/instrumentation/hibernate/hibernate-4.0/javaagent/src/test/groovy/QueryTest.groovy
@@ -105,25 +105,25 @@ class QueryTest extends AbstractHibernateTest {
 
     where:
     queryMethodName       | expectedSpanName            | requiresTransaction | queryInteraction
-    "query/list"          | "from Value"                | false               | { sess ->
+    "query/list"          | "SELECT Value"              | false               | { sess ->
       Query q = sess.createQuery("from Value")
       q.list()
     }
-    "query/executeUpdate" | "update Value set name = ?" | true                | { sess ->
-      Query q = sess.createQuery("update Value set name = ?")
-      q.setParameter(0, "alyx")
+    "query/executeUpdate" | "UPDATE Value"              | true                | { sess ->
+      Query q = sess.createQuery("update Value set name = :name")
+      q.setParameter("name", "alyx")
       q.executeUpdate()
     }
-    "query/uniqueResult"  | "from Value where id = ?"   | false               | { sess ->
-      Query q = sess.createQuery("from Value where id = ?")
-      q.setParameter(0, 1L)
+    "query/uniqueResult"  | "SELECT Value"              | false               | { sess ->
+      Query q = sess.createQuery("from Value where id = :id")
+      q.setParameter("id", 1L)
       q.uniqueResult()
     }
-    "iterate"             | "from Value"                | false               | { sess ->
+    "iterate"             | "SELECT Value"              | false               | { sess ->
       Query q = sess.createQuery("from Value")
       q.iterate()
     }
-    "query/scroll"        | "from Value"                | false               | { sess ->
+    "query/scroll"        | "SELECT Value"              | false               | { sess ->
       Query q = sess.createQuery("from Value")
       q.scroll()
     }
@@ -153,7 +153,7 @@ class QueryTest extends AbstractHibernateTest {
           }
         }
         span(1) {
-          name "from Value"
+          name "SELECT Value"
           kind INTERNAL
           childOf span(0)
           attributes {

diff --git a/instrumentation/hibernate/hibernate-4.0/javaagent/src/test/groovy/SessionTest.groovy b/instrumentation/hibernate/hibernate-4.0/javaagent/src/test/groovy/SessionTest.groovy
@@ -379,10 +379,10 @@ class SessionTest extends AbstractHibernateTest {
     }
 
     where:
-    queryMethodName  | resource              | queryBuildMethod
-    "createQuery"    | "from Value"          | { sess -> sess.createQuery("from Value") }
-    "getNamedQuery"  | "from Value"          | { sess -> sess.getNamedQuery("TestNamedQuery") }
-    "createSQLQuery" | "SELECT * FROM Value" | { sess -> sess.createSQLQuery("SELECT * FROM Value") }
+    queryMethodName  | resource       | queryBuildMethod
+    "createQuery"    | "SELECT Value" | { sess -> sess.createQuery("from Value") }
+    "getNamedQuery"  | "SELECT Value" | { sess -> sess.getNamedQuery("TestNamedQuery") }
+    "createSQLQuery" | "SELECT Value" | { sess -> sess.createSQLQuery("SELECT * FROM Value") }
   }