opentracing-contrib / java-jdbc

OpenTracing Instrumentation for JDBC
Apache License 2.0
82 stars 56 forks source link

Prevent TracingDriver registration when not in use #110

Closed asvanberg closed 3 years ago

asvanberg commented 3 years ago

Pull request #70 caused issues since the driver will be registered and never de-register itself when not in use due to static references from JdbcTracingUtils

coveralls commented 3 years ago

Pull Request Test Coverage Report for Build 343


Changes Missing Coverage Covered Lines Changed/Added Lines %
src/main/java/io/opentracing/contrib/jdbc/JdbcTracing.java 4 5 80.0%
src/main/java/io/opentracing/contrib/jdbc/TracingDriver.java 1 2 50.0%
<!-- Total: 8 10 80.0% -->
Totals Coverage Status
Change from base Build 342: -0.003%
Covered Lines: 517
Relevant Lines: 1020

💛 - Coveralls
malafeev commented 3 years ago

@asvanberg you moved static boolean traceEnabled from TracingDriver class to JdbcTracing class; could you clarify how moving static variable solved the issue?

asvanberg commented 3 years ago

When any of the tracing wrappers (TracingConnection, TracingPreparedStatement, and so on) create a span they call JdbcTracingUtils#call which has a static reference to TracingDriver to check if tracing is enabled or not. This will cause the class to be loaded by the JVM and the static initializer to be run (TracingDriver:49) which will register itself with the DriverManager. If the tracing driver is not in use due to using TracingDataSource and another driver it will not be deregistered.

For example when undeploying an application in Tomcat the following warning is generated WARNING [main] org.apache.catalina.loader.WebappClassLoaderBase.clearReferencesJdbc The web application [ROOT] registered the JDBC driver [io.opentracing.contrib.jdbc.TracingDriver] but failed to unregister it when the web application was stopped. To prevent a memory leak, the JDBC Driver has been forcibly unregistered.

PS. I'm also wondering why this check is even there since you have to explicitly enable tracing, why should there be a flag to turn it back off again?