Code review sweep (run 25177950292)

instrumentation/jaxws/jaxws-cxf-3.0/javaagent-unit-tests/build.gradle.kts

@@ -3,9 +3,6 @@ plugins {
 }
 
 dependencies {
-  compileOnly("javax.servlet:javax.servlet-api:3.0.1")
-  compileOnly("org.apache.cxf:cxf-rt-frontend-jaxws:3.0.0")
-
   testImplementation(project(":instrumentation:jaxws:jaxws-cxf-3.0:javaagent"))
 
   testImplementation("org.apache.cxf:cxf-rt-frontend-jaxws:3.0.0")

instrumentation/jaxws/jaxws-cxf-3.0/javaagent/build.gradle.kts

@@ -45,7 +45,7 @@ dependencies {
   latestDepTestLibrary("org.apache.cxf:cxf-rt-transports-http:3.+") // documented limitation
 }
 
-tasks.withType<Test>().configureEach {
+tasks.test {
   // required on jdk17
   jvmArgs("--add-opens=java.base/java.lang=ALL-UNNAMED")
   jvmArgs("-XX:+IgnoreUnrecognizedVMOptions")

instrumentation/jaxws/jaxws-jws-api-1.1/javaagent/build.gradle.kts

@@ -16,7 +16,7 @@ dependencies {
   implementation(project(":instrumentation:jaxws:jaxws-common:javaagent"))
 }
 
-tasks.withType<Test>().configureEach {
+tasks.test {
   jvmArgs("-Dotel.instrumentation.common.experimental.controller-telemetry.enabled=true")
   systemProperty("collectMetadata", otelProps.collectMetadata)
   systemProperty("metadataConfig", "otel.instrumentation.common.experimental.controller-telemetry.enabled=true")

instrumentation/jaxws/jaxws-metro-2.2/javaagent/build.gradle.kts

@@ -35,7 +35,7 @@ dependencies {
   latestDepTestLibrary("com.sun.xml.stream.buffer:streambuffer:1.+") // see jaxws-3.0-metro-3.0-testing module
 }
 
-tasks.withType<Test>().configureEach {
+tasks.test {
   // required on jdk17
   jvmArgs("--add-exports=java.xml/com.sun.org.apache.xerces.internal.dom=ALL-UNNAMED")
   jvmArgs("--add-exports=java.xml/com.sun.org.apache.xerces.internal.jaxp=ALL-UNNAMED")

instrumentation/jboss-logmanager/jboss-logmanager-appender-1.1/javaagent/build.gradle.kts

@@ -14,8 +14,6 @@ muzzle {
 dependencies {
   library("org.jboss.logmanager:jboss-logmanager:1.1.0.GA")
 
-  compileOnly(project(":javaagent-bootstrap"))
-
   // ensure no cross interference
   testInstrumentation(project(":instrumentation:java-util-logging:javaagent"))
 }
@@ -26,7 +24,7 @@ if (otelProps.testLatestDeps) {
   }
 }
 
-tasks.withType<Test>().configureEach {
+tasks.test {
   // TODO run tests both with and without experimental log attributes
   jvmArgs("-Dotel.instrumentation.jboss-logmanager.experimental.capture-mdc-attributes=*")
   jvmArgs("-Dotel.instrumentation.jboss-logmanager.experimental-log-attributes=true")

instrumentation/jboss-logmanager/jboss-logmanager-appender-1.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jbosslogmanager/appender/v1_1/JbossLogmanagerInstrumentation.java

@@ -14,6 +14,7 @@
 import io.opentelemetry.javaagent.bootstrap.CallDepth;
 import io.opentelemetry.javaagent.extension.instrumentation.TypeInstrumentation;
 import io.opentelemetry.javaagent.extension.instrumentation.TypeTransformer;
+import javax.annotation.Nullable;
 import net.bytebuddy.asm.Advice;
 import net.bytebuddy.description.type.TypeDescription;
 import net.bytebuddy.matcher.ElementMatcher;
@@ -45,14 +46,21 @@ public static CallDepth methodEnter(
       // logging framework delegates to another
       CallDepth callDepth = CallDepth.forClass(LoggerProvider.class);
       if (callDepth.getAndIncrement() == 0) {
-        LoggingEventMapper.INSTANCE.capture(logger, record);
+        try {
+          LoggingEventMapper.INSTANCE.capture(logger, record);
+        } catch (Throwable t) {
+          callDepth.decrementAndGet();
+          throw t;
+        }
       }
       return callDepth;
     }
 
     @Advice.OnMethodExit(onThrowable = Throwable.class, suppress = Throwable.class, inline = false)
-    public static void methodExit(@Advice.Enter CallDepth callDepth) {
-      callDepth.decrementAndGet();
+    public static void methodExit(@Advice.Enter @Nullable CallDepth callDepth) {
+      if (callDepth != null) {
+        callDepth.decrementAndGet();
+      }
     }
   }
 }

instrumentation/jboss-logmanager/jboss-logmanager-mdc-1.1/javaagent/src/test/java/io/opentelemetry/javaagent/instrumentation/jbosslogmanager/mdc/v1_1/JbossLogmanagerMdcTest.java

@@ -28,7 +28,7 @@ class JbossLogmanagerMdcTest {
   @RegisterExtension
   static final InstrumentationExtension testing = AgentInstrumentationExtension.create();
 
-  static class LogHandler extends Handler {
+  private static class LogHandler extends Handler {
     private final List<ExtLogRecord> logRecords;
 
     LogHandler(List<ExtLogRecord> logRecords) {

instrumentation/jdbc/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jdbc/ConnectionInstrumentation.java

@@ -114,7 +114,7 @@ public static Object[] processSql(
 
     @Advice.OnMethodExit(suppress = Throwable.class, onThrowable = Throwable.class, inline = false)
     public static void addDbInfo(
-        @Advice.Return PreparedStatement statement,
+        @Advice.Return @Nullable PreparedStatement statement,
         @Advice.Enter Object[] enterResult,
         @Advice.Thrown Throwable error) {
       Context context = Java8BytecodeBridge.currentContext();
@@ -124,6 +124,7 @@ public static void addDbInfo(
         scope.close();
       }
       if (error != null
+          || statement == null
           || prepareContext == null
           || JdbcSingletons.isWrapper(statement, PreparedStatement.class)) {
         return;

instrumentation/jdbc/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jdbc/JdbcSingletons.java

@@ -34,6 +34,7 @@ public class JdbcSingletons {
   private static final Instrumenter<DataSource, DbInfo> dataSourceInstrumenter =
       createDataSourceInstrumenter(GlobalOpenTelemetry.get(), true);
   private static final SqlCommenter sqlCommenter = configureSqlCommenter();
+  private static final Cache<Class<?>, Boolean> wrapperClassCache = Cache.weak();
   public static final boolean CAPTURE_QUERY_PARAMETERS;
 
   static {
@@ -74,8 +75,6 @@ public static Instrumenter<DataSource, DbInfo> dataSourceInstrumenter() {
     return dataSourceInstrumenter;
   }
 
-  private static final Cache<Class<?>, Boolean> wrapperClassCache = Cache.weak();
-
   /**
    * Returns true if the given object is a wrapper and shouldn't be instrumented. We'll instrument
    * the underlying object called by the wrapper instead.

instrumentation/jdbc/library/src/main/java/io/opentelemetry/instrumentation/jdbc/OpenTelemetryDriver.java

@@ -52,17 +52,17 @@
 /** JDBC driver for OpenTelemetry. */
 public final class OpenTelemetryDriver implements Driver {
 
-  // visible for testing
-  static final OpenTelemetryDriver INSTANCE = new OpenTelemetryDriver();
-
-  private volatile OpenTelemetry openTelemetry = OpenTelemetry.noop();
-
   private static final int MAJOR_VERSION;
   private static final int MINOR_VERSION;
 
   private static final String URL_PREFIX = "jdbc:otel:";
-  private static final AtomicBoolean REGISTERED = new AtomicBoolean();
-  private static final List<Driver> DRIVER_CANDIDATES = new CopyOnWriteArrayList<>();
+  private static final AtomicBoolean registered = new AtomicBoolean();
+  private static final List<Driver> driverCandidates = new CopyOnWriteArrayList<>();
+
+  // visible for testing
+  static final OpenTelemetryDriver INSTANCE = new OpenTelemetryDriver();
+
+  private volatile OpenTelemetry openTelemetry = OpenTelemetry.noop();
 
   @SuppressWarnings("deprecation") // library flat config fallback remains supported until 3.0
   private static SqlCommenter getSqlCommenter(OpenTelemetry openTelemetry) {
@@ -98,7 +98,7 @@ private static SqlCommenter getSqlCommenter(OpenTelemetry openTelemetry) {
    * @throws SQLException if registering the driver fails
    */
   public static void register() throws SQLException {
-    if (!REGISTERED.compareAndSet(false, true)) {
+    if (!registered.compareAndSet(false, true)) {
       throw new IllegalStateException(
           "Driver is already registered. It can only be registered once.");
     }
@@ -114,7 +114,7 @@ public static void register() throws SQLException {
    * @throws SQLException if deregistering the driver fails
    */
   public static void deregister() throws SQLException {
-    if (!REGISTERED.compareAndSet(true, false)) {
+    if (!registered.compareAndSet(true, false)) {
       throw new IllegalStateException(
           "Driver is not registered (or it has not been registered using Driver.register() method)");
     }
@@ -123,7 +123,7 @@ public static void deregister() throws SQLException {
 
   /** Returns {@code true} if the driver is registered against {@link DriverManager}. */
   public static boolean isRegistered() {
-    return REGISTERED.get();
+    return registered.get();
   }
 
   /**
@@ -137,7 +137,7 @@ public static boolean isRegistered() {
    */
   public static void addDriverCandidate(@Nullable Driver driver) {
     if (driver != null) {
-      DRIVER_CANDIDATES.add(driver);
+      driverCandidates.add(driver);
     }
   }
 
@@ -148,17 +148,17 @@ public static void addDriverCandidate(@Nullable Driver driver) {
    * @return true if the driver was unregistered
    */
   public static boolean removeDriverCandidate(Driver driver) {
-    return DRIVER_CANDIDATES.remove(driver);
+    return driverCandidates.remove(driver);
   }
 
   /**
-   * Find driver that accepts {@code realUrl}. Drivers registered against {@link #DRIVER_CANDIDATES}
+   * Find driver that accepts {@code realUrl}. Drivers registered against {@link #driverCandidates}
    * are preferred over {@link DriverManager} drivers.
    */
   static Driver findDriver(String realUrl) {
     Driver driver = null;
-    if (!DRIVER_CANDIDATES.isEmpty()) {
-      driver = findDriver(realUrl, DRIVER_CANDIDATES);
+    if (!driverCandidates.isEmpty()) {
+      driver = findDriver(realUrl, driverCandidates);
     }
     if (driver == null) {
       driver = findDriver(realUrl, Collections.list(DriverManager.getDrivers()));

instrumentation/jdbc/library/src/main/java/io/opentelemetry/instrumentation/jdbc/datasource/JdbcTelemetryBuilder.java

@@ -19,17 +19,17 @@
 /** A builder of {@link JdbcTelemetry}. */
 public final class JdbcTelemetryBuilder {
 
+  static {
+    Experimental.internalSetSqlCommenterBuilder(builder -> builder.sqlCommenterBuilder);
+  }
+
   private final OpenTelemetry openTelemetry;
+  private final SqlCommenterBuilder sqlCommenterBuilder = SqlCommenter.builder();
   private boolean dataSourceInstrumenterEnabled = false;
   private boolean statementInstrumenterEnabled = true;
   private boolean querySanitizationEnabled = true;
   private boolean transactionInstrumenterEnabled = false;
   private boolean captureQueryParameters = false;
-  private final SqlCommenterBuilder sqlCommenterBuilder = SqlCommenter.builder();
-
-  static {
-    Experimental.internalSetSqlCommenterBuilder(builder -> builder.sqlCommenterBuilder);
-  }
 
   JdbcTelemetryBuilder(OpenTelemetry openTelemetry) {
     this.openTelemetry = openTelemetry;

instrumentation/jdbc/library/src/main/java/io/opentelemetry/instrumentation/jdbc/internal/JdbcConnectionUrlParser.java

@@ -45,59 +45,59 @@ public final class JdbcConnectionUrlParser {
 
   private static final Logger logger = Logger.getLogger(JdbcConnectionUrlParser.class.getName());
 
-  private static final Map<String, JdbcUrlParser> TYPE_PARSERS = new HashMap<>();
+  private static final Map<String, JdbcUrlParser> typeParsers = new HashMap<>();
 
   static {
     // PostgreSQL
-    TYPE_PARSERS.put("postgresql", PostgresqlUrlParser.INSTANCE);
+    typeParsers.put("postgresql", PostgresqlUrlParser.INSTANCE);
 
     // MySQL and MariaDB
-    TYPE_PARSERS.put("mysql", MysqlUrlParser.INSTANCE);
-    TYPE_PARSERS.put("mariadb", MysqlUrlParser.INSTANCE);
+    typeParsers.put("mysql", MysqlUrlParser.INSTANCE);
+    typeParsers.put("mariadb", MysqlUrlParser.INSTANCE);
 
     // Microsoft SQL Server
-    TYPE_PARSERS.put("jtds", JtdsUrlParser.INSTANCE);
-    TYPE_PARSERS.put("microsoft", MssqlUrlParser.INSTANCE);
-    TYPE_PARSERS.put("sqlserver", MssqlUrlParser.INSTANCE);
+    typeParsers.put("jtds", JtdsUrlParser.INSTANCE);
+    typeParsers.put("microsoft", MssqlUrlParser.INSTANCE);
+    typeParsers.put("sqlserver", MssqlUrlParser.INSTANCE);
 
     // Oracle
-    TYPE_PARSERS.put("oracle", OracleUrlParser.INSTANCE);
+    typeParsers.put("oracle", OracleUrlParser.INSTANCE);
 
     // DB2 and AS400
-    TYPE_PARSERS.put("db2", Db2UrlParser.INSTANCE);
-    TYPE_PARSERS.put("as400", Db2UrlParser.INSTANCE);
+    typeParsers.put("db2", Db2UrlParser.INSTANCE);
+    typeParsers.put("as400", Db2UrlParser.INSTANCE);
 
     // H2
-    TYPE_PARSERS.put("h2", H2UrlParser.INSTANCE);
+    typeParsers.put("h2", H2UrlParser.INSTANCE);
 
     // HyperSQL (HSQLDB)
-    TYPE_PARSERS.put("hsqldb", HsqlUrlParser.INSTANCE);
+    typeParsers.put("hsqldb", HsqlUrlParser.INSTANCE);
 
     // Apache Derby
-    TYPE_PARSERS.put("derby", DerbyUrlParser.INSTANCE);
+    typeParsers.put("derby", DerbyUrlParser.INSTANCE);
 
     // SAP HANA
-    TYPE_PARSERS.put("sap", SapUrlParser.INSTANCE);
+    typeParsers.put("sap", SapUrlParser.INSTANCE);
 
     // DataDirect and TIBCO
-    TYPE_PARSERS.put("datadirect", DataDirectUrlParser.INSTANCE);
-    TYPE_PARSERS.put("tibcosoftware", DataDirectUrlParser.INSTANCE);
+    typeParsers.put("datadirect", DataDirectUrlParser.INSTANCE);
+    typeParsers.put("tibcosoftware", DataDirectUrlParser.INSTANCE);
 
     // Informix
-    TYPE_PARSERS.put("informix-sqli", InformixSqliUrlParser.INSTANCE);
-    TYPE_PARSERS.put("informix-direct", InformixDirectUrlParser.INSTANCE);
+    typeParsers.put("informix-sqli", InformixSqliUrlParser.INSTANCE);
+    typeParsers.put("informix-direct", InformixDirectUrlParser.INSTANCE);
 
     // ClickHouse
-    TYPE_PARSERS.put("clickhouse", ClickhouseUrlParser.INSTANCE);
+    typeParsers.put("clickhouse", ClickhouseUrlParser.INSTANCE);
 
     // OceanBase
-    TYPE_PARSERS.put("oceanbase", OceanbaseUrlParser.INSTANCE);
+    typeParsers.put("oceanbase", OceanbaseUrlParser.INSTANCE);
 
     // Lindorm
-    TYPE_PARSERS.put("lindorm", LindormUrlParser.INSTANCE);
+    typeParsers.put("lindorm", LindormUrlParser.INSTANCE);
 
     // PolarDB
-    TYPE_PARSERS.put("polardb", PolardbUrlParser.INSTANCE);
+    typeParsers.put("polardb", PolardbUrlParser.INSTANCE);
   }
 
   private JdbcConnectionUrlParser() {}
@@ -130,7 +130,7 @@ public static DbInfo parse(String connectionUrl, Properties props) {
     }
 
     String type = jdbcUrl.substring(0, typeLoc);
-    JdbcUrlParser parser = TYPE_PARSERS.get(type);
+    JdbcUrlParser parser = typeParsers.get(type);
     ParseContext ctx = ParseContext.of(type, props);
 
     try {

instrumentation/jdbc/library/src/main/java/io/opentelemetry/instrumentation/jdbc/internal/JdbcUtils.java

@@ -30,9 +30,9 @@ public final class JdbcUtils {
   public static Connection connectionFromStatement(Statement statement) {
     try {
       return unwrapConnection(statement.getConnection());
-    } catch (Throwable e) {
+    } catch (Throwable t) {
       // Had some problem getting the connection.
-      logger.log(FINE, "Could not get connection from a statement", e);
+      logger.log(FINE, "Could not get connection from a statement", t);
       return null;
     }
   }
@@ -53,7 +53,7 @@ public static Connection unwrapConnection(Connection connection) {
         if (connection.isWrapperFor(Connection.class)) {
           connection = connection.unwrap(Connection.class);
         }
-      } catch (Exception | AbstractMethodError e) {
+      } catch (Exception | AbstractMethodError ignored) {
         if (connection != null) {
           // Attempt to work around c3po delegating to an connection that doesn't support
           // unwrapping.
@@ -71,9 +71,9 @@ public static Connection unwrapConnection(Connection connection) {
         // or: jdts.jdbc which always throws `AbstractMethodError` (at least up to version 1.3)
         // Stick with original connection.
       }
-    } catch (Throwable e) {
+    } catch (Throwable t) {
       // Had some problem getting the connection.
-      logger.log(FINE, "Could not unwrap connection", e);
+      logger.log(FINE, "Could not unwrap connection", t);
       return null;
     }
     return connection;
@@ -116,7 +116,7 @@ public static DbInfo computeDbInfo(@Nullable Connection connection) {
       } else {
         return DbInfo.DEFAULT;
       }
-    } catch (SQLException se) {
+    } catch (SQLException ignored) {
       return DbInfo.DEFAULT;
     }
   }

instrumentation/jdbc/library/src/main/java/io/opentelemetry/instrumentation/jdbc/internal/parser/UrlParsingUtils.java

@@ -82,7 +82,7 @@ public static Map<String, String> splitQuery(String query, String separator) {
                   : null;
           queryPairs.put(key, value);
         }
-      } catch (UnsupportedEncodingException e) {
+      } catch (UnsupportedEncodingException ignored) {
         // Ignore.
       }
     }