From 4b71a2148749359138d0964212e90447f9868830 Mon Sep 17 00:00:00 2001 From: Gary Huang Date: Fri, 11 May 2018 18:26:18 -0400 Subject: [PATCH 1/3] Fixed unknown database issue from JDBC Connection constructor exception Client will now report correct database if JDBC connection is recovered from exception. --- .../datadog/trace/bootstrap/JDBCMaps.java | 36 +++ .../instrumentation/jdbc/jdbc.gradle | 6 + .../jdbc/ConnectionInstrumentation.java | 35 --- .../PreparedStatementInstrumentation.java | 7 +- .../jdbc/StatementInstrumentation.java | 6 +- .../groovy/DummyThrowingConnection.groovy | 296 ++++++++++++++++++ .../groovy/JDBCInstrumentationTest.groovy | 87 +++++ 7 files changed, 428 insertions(+), 45 deletions(-) create mode 100644 dd-java-agent/instrumentation/jdbc/src/test/groovy/DummyThrowingConnection.groovy create mode 100644 dd-java-agent/instrumentation/jdbc/src/test/groovy/JDBCInstrumentationTest.groovy diff --git a/dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/JDBCMaps.java b/dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/JDBCMaps.java index 062597276d..df0ed21833 100644 --- a/dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/JDBCMaps.java +++ b/dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/JDBCMaps.java @@ -2,6 +2,7 @@ package datadog.trace.bootstrap; import java.sql.Connection; import java.sql.PreparedStatement; +import java.sql.SQLException; import java.util.Collections; import java.util.Map; import java.util.WeakHashMap; @@ -39,4 +40,39 @@ public class JDBCMaps { private static String propToEnvName(final String name) { return name.toUpperCase().replace(".", "_"); } + + /** + * Utility function to get the DBInfo from a JDBC Connection, if the connection was never seen + * before, the connectionInfo map will return null and will attempt to extract DBInfo from the + * connection. If the DBInfo can't be extracted, then the connection will be stored with the + * UNKNOWN DBInfo as the value in the connectionInfo map to avoid retry overhead. + * + * @param connection The JDBC connection + * @return A DBInfo that contains JDBC connection info + */ + public static DBInfo getDBInfo(Connection connection) { + DBInfo dbInfo = connectionInfo.get(connection); + if (dbInfo == null) { + try { + final String url = connection.getMetaData().getURL(); + if (url != null) { + // Remove end of url to prevent passwords from leaking: + final String sanitizedURL = url.replaceAll("[?;].*", ""); + final String type = url.split(":", -1)[1]; + String user = connection.getMetaData().getUserName(); + if (user != null && user.trim().equals("")) { + user = null; + } + dbInfo = new JDBCMaps.DBInfo(sanitizedURL, type, user); + } else { + dbInfo = DBInfo.DEFAULT; + } + } catch (SQLException se) { + dbInfo = DBInfo.DEFAULT; + } + connectionInfo.put(connection, dbInfo); + } + + return dbInfo; + } } diff --git a/dd-java-agent/instrumentation/jdbc/jdbc.gradle b/dd-java-agent/instrumentation/jdbc/jdbc.gradle index 00ec61f5fc..e9dbe55750 100644 --- a/dd-java-agent/instrumentation/jdbc/jdbc.gradle +++ b/dd-java-agent/instrumentation/jdbc/jdbc.gradle @@ -7,4 +7,10 @@ dependencies { compile deps.opentracing annotationProcessor deps.autoservice implementation deps.autoservice + + testCompile project(':dd-java-agent:testing') + // jdbc unit testing + testCompile group: 'com.h2database', name: 'h2', version: '1.4.197' + testCompile group: 'org.apache.derby', name: 'derby', version: '10.12.1.1' + } diff --git a/dd-java-agent/instrumentation/jdbc/src/main/java/datadog/trace/instrumentation/jdbc/ConnectionInstrumentation.java b/dd-java-agent/instrumentation/jdbc/src/main/java/datadog/trace/instrumentation/jdbc/ConnectionInstrumentation.java index 28549d06d3..f530387627 100644 --- a/dd-java-agent/instrumentation/jdbc/src/main/java/datadog/trace/instrumentation/jdbc/ConnectionInstrumentation.java +++ b/dd-java-agent/instrumentation/jdbc/src/main/java/datadog/trace/instrumentation/jdbc/ConnectionInstrumentation.java @@ -1,7 +1,6 @@ package datadog.trace.instrumentation.jdbc; import static net.bytebuddy.matcher.ElementMatchers.failSafe; -import static net.bytebuddy.matcher.ElementMatchers.isConstructor; import static net.bytebuddy.matcher.ElementMatchers.isInterface; import static net.bytebuddy.matcher.ElementMatchers.isSubTypeOf; import static net.bytebuddy.matcher.ElementMatchers.nameStartsWith; @@ -13,11 +12,9 @@ import com.google.auto.service.AutoService; import datadog.trace.agent.tooling.DDAdvice; import datadog.trace.agent.tooling.DDTransformers; import datadog.trace.agent.tooling.Instrumenter; -import datadog.trace.bootstrap.CallDepthThreadLocalMap; import datadog.trace.bootstrap.JDBCMaps; import java.sql.Connection; import java.sql.PreparedStatement; -import java.sql.SQLException; import net.bytebuddy.agent.builder.AgentBuilder; import net.bytebuddy.asm.Advice; @@ -40,8 +37,6 @@ public final class ConnectionInstrumentation extends Instrumenter.Configurable { .and(takesArgument(0, String.class)) .and(returns(PreparedStatement.class)), ConnectionPrepareAdvice.class.getName())) - .transform( - DDAdvice.create().advice(isConstructor(), ConnectionConstructorAdvice.class.getName())) .asDecorator(); } @@ -52,34 +47,4 @@ public final class ConnectionInstrumentation extends Instrumenter.Configurable { JDBCMaps.preparedStatements.put(statement, sql); } } - - public static class ConnectionConstructorAdvice { - @Advice.OnMethodEnter(suppress = Throwable.class) - public static int constructorEnter() { - // We use this to make sure we only apply the exit instrumentation - // after the constructors are done calling their super constructors. - return CallDepthThreadLocalMap.incrementCallDepth(Connection.class); - } - - // Since we're instrumenting the constructor, we can't add onThrowable. - @Advice.OnMethodExit(suppress = Throwable.class) - public static void constructorExit( - @Advice.Enter final int depth, @Advice.This final Connection connection) - throws SQLException { - if (depth == 0) { - CallDepthThreadLocalMap.reset(Connection.class); - final String url = connection.getMetaData().getURL(); - if (url != null) { - // Remove end of url to prevent passwords from leaking: - final String sanitizedURL = url.replaceAll("[?;].*", ""); - final String type = url.split(":", -1)[1]; - String user = connection.getMetaData().getUserName(); - if (user != null && user.trim().equals("")) { - user = null; - } - JDBCMaps.connectionInfo.put(connection, new JDBCMaps.DBInfo(sanitizedURL, type, user)); - } - } - } - } } diff --git a/dd-java-agent/instrumentation/jdbc/src/main/java/datadog/trace/instrumentation/jdbc/PreparedStatementInstrumentation.java b/dd-java-agent/instrumentation/jdbc/src/main/java/datadog/trace/instrumentation/jdbc/PreparedStatementInstrumentation.java index e4487e5ae7..230e702f1b 100644 --- a/dd-java-agent/instrumentation/jdbc/src/main/java/datadog/trace/instrumentation/jdbc/PreparedStatementInstrumentation.java +++ b/dd-java-agent/instrumentation/jdbc/src/main/java/datadog/trace/instrumentation/jdbc/PreparedStatementInstrumentation.java @@ -29,7 +29,6 @@ import net.bytebuddy.asm.Advice; @AutoService(Instrumenter.class) public final class PreparedStatementInstrumentation extends Instrumenter.Configurable { - public PreparedStatementInstrumentation() { super("jdbc"); } @@ -64,10 +63,8 @@ public final class PreparedStatementInstrumentation extends Instrumenter.Configu return NoopScope.INSTANCE; } - JDBCMaps.DBInfo dbInfo = JDBCMaps.connectionInfo.get(connection); - if (dbInfo == null) { - dbInfo = JDBCMaps.DBInfo.DEFAULT; - } + JDBCMaps.DBInfo dbInfo = JDBCMaps.getDBInfo(connection); + final Scope scope = GlobalTracer.get().buildSpan(dbInfo.getType() + ".query").startActive(true); diff --git a/dd-java-agent/instrumentation/jdbc/src/main/java/datadog/trace/instrumentation/jdbc/StatementInstrumentation.java b/dd-java-agent/instrumentation/jdbc/src/main/java/datadog/trace/instrumentation/jdbc/StatementInstrumentation.java index 9a20e27b8d..45cc483d15 100644 --- a/dd-java-agent/instrumentation/jdbc/src/main/java/datadog/trace/instrumentation/jdbc/StatementInstrumentation.java +++ b/dd-java-agent/instrumentation/jdbc/src/main/java/datadog/trace/instrumentation/jdbc/StatementInstrumentation.java @@ -64,16 +64,12 @@ public final class StatementInstrumentation extends Instrumenter.Configurable { return NoopScope.INSTANCE; } - JDBCMaps.DBInfo dbInfo = JDBCMaps.connectionInfo.get(connection); - if (dbInfo == null) { - dbInfo = JDBCMaps.DBInfo.DEFAULT; - } + JDBCMaps.DBInfo dbInfo = JDBCMaps.getDBInfo(connection); final Scope scope = GlobalTracer.get().buildSpan(dbInfo.getType() + ".query").startActive(true); final Span span = scope.span(); - Tags.DB_TYPE.set(span, dbInfo.getType()); Tags.SPAN_KIND.set(span, Tags.SPAN_KIND_CLIENT); Tags.COMPONENT.set(span, "java-jdbc-statement"); diff --git a/dd-java-agent/instrumentation/jdbc/src/test/groovy/DummyThrowingConnection.groovy b/dd-java-agent/instrumentation/jdbc/src/test/groovy/DummyThrowingConnection.groovy new file mode 100644 index 0000000000..018c27c172 --- /dev/null +++ b/dd-java-agent/instrumentation/jdbc/src/test/groovy/DummyThrowingConnection.groovy @@ -0,0 +1,296 @@ +import java.sql.Array +import java.sql.Blob +import java.sql.CallableStatement +import java.sql.Clob +import java.sql.Connection +import java.sql.DatabaseMetaData +import java.sql.NClob +import java.sql.PreparedStatement +import java.sql.SQLClientInfoException +import java.sql.SQLException +import java.sql.SQLWarning +import java.sql.SQLXML +import java.sql.Savepoint +import java.sql.Statement +import java.sql.Struct +import java.util.concurrent.Executor + + +/** + * A JDBC connection class that throws an exception in the constructor, used to test + */ +class DummyThrowingConnection implements Connection { + DummyThrowingConnection() { + throw new RuntimeException("Dummy exception") + } + + @Override + Statement createStatement() throws SQLException { + return null + } + + @Override + PreparedStatement prepareStatement(String sql) throws SQLException { + return null + } + + @Override + CallableStatement prepareCall(String sql) throws SQLException { + return null + } + + @Override + String nativeSQL(String sql) throws SQLException { + return null + } + + @Override + void setAutoCommit(boolean autoCommit) throws SQLException { + + } + + @Override + boolean getAutoCommit() throws SQLException { + return false + } + + @Override + void commit() throws SQLException { + + } + + @Override + void rollback() throws SQLException { + + } + + @Override + void close() throws SQLException { + + } + + @Override + boolean isClosed() throws SQLException { + return false + } + + @Override + DatabaseMetaData getMetaData() throws SQLException { + return null + } + + @Override + void setReadOnly(boolean readOnly) throws SQLException { + + } + + @Override + boolean isReadOnly() throws SQLException { + return false + } + + @Override + void setCatalog(String catalog) throws SQLException { + + } + + @Override + String getCatalog() throws SQLException { + return null + } + + @Override + void setTransactionIsolation(int level) throws SQLException { + + } + + @Override + int getTransactionIsolation() throws SQLException { + return 0 + } + + @Override + SQLWarning getWarnings() throws SQLException { + return null + } + + @Override + void clearWarnings() throws SQLException { + + } + + @Override + Statement createStatement(int resultSetType, int resultSetConcurrency) throws SQLException { + return null + } + + @Override + PreparedStatement prepareStatement(String sql, int resultSetType, int resultSetConcurrency) throws SQLException { + return null + } + + @Override + CallableStatement prepareCall(String sql, int resultSetType, int resultSetConcurrency) throws SQLException { + return null + } + + @Override + Map> getTypeMap() throws SQLException { + return null + } + + @Override + void setTypeMap(Map> map) throws SQLException { + + } + + @Override + void setHoldability(int holdability) throws SQLException { + + } + + @Override + int getHoldability() throws SQLException { + return 0 + } + + @Override + Savepoint setSavepoint() throws SQLException { + return null + } + + @Override + Savepoint setSavepoint(String name) throws SQLException { + return null + } + + @Override + void rollback(Savepoint savepoint) throws SQLException { + + } + + @Override + void releaseSavepoint(Savepoint savepoint) throws SQLException { + + } + + @Override + Statement createStatement(int resultSetType, int resultSetConcurrency, int resultSetHoldability) throws SQLException { + return null + } + + @Override + PreparedStatement prepareStatement(String sql, int resultSetType, int resultSetConcurrency, int resultSetHoldability) throws SQLException { + return null + } + + @Override + CallableStatement prepareCall(String sql, int resultSetType, int resultSetConcurrency, int resultSetHoldability) throws SQLException { + return null + } + + @Override + PreparedStatement prepareStatement(String sql, int autoGeneratedKeys) throws SQLException { + return null + } + + @Override + PreparedStatement prepareStatement(String sql, int[] columnIndexes) throws SQLException { + return null + } + + @Override + PreparedStatement prepareStatement(String sql, String[] columnNames) throws SQLException { + return null + } + + @Override + Clob createClob() throws SQLException { + return null + } + + @Override + Blob createBlob() throws SQLException { + return null + } + + @Override + NClob createNClob() throws SQLException { + return null + } + + @Override + SQLXML createSQLXML() throws SQLException { + return null + } + + @Override + boolean isValid(int timeout) throws SQLException { + return false + } + + @Override + void setClientInfo(String name, String value) throws SQLClientInfoException { + + } + + @Override + void setClientInfo(Properties properties) throws SQLClientInfoException { + + } + + @Override + String getClientInfo(String name) throws SQLException { + return null + } + + @Override + Properties getClientInfo() throws SQLException { + return null + } + + @Override + Array createArrayOf(String typeName, Object[] elements) throws SQLException { + return null + } + + @Override + Struct createStruct(String typeName, Object[] attributes) throws SQLException { + return null + } + + @Override + void setSchema(String schema) throws SQLException { + + } + + @Override + String getSchema() throws SQLException { + return null + } + + @Override + void abort(Executor executor) throws SQLException { + + } + + @Override + void setNetworkTimeout(Executor executor, int milliseconds) throws SQLException { + + } + + @Override + int getNetworkTimeout() throws SQLException { + return 0 + } + + @Override + def T unwrap(Class iface) throws SQLException { + return null + } + + @Override + boolean isWrapperFor(Class iface) throws SQLException { + return false + } +} diff --git a/dd-java-agent/instrumentation/jdbc/src/test/groovy/JDBCInstrumentationTest.groovy b/dd-java-agent/instrumentation/jdbc/src/test/groovy/JDBCInstrumentationTest.groovy new file mode 100644 index 0000000000..a039e13469 --- /dev/null +++ b/dd-java-agent/instrumentation/jdbc/src/test/groovy/JDBCInstrumentationTest.groovy @@ -0,0 +1,87 @@ +import datadog.trace.agent.test.AgentTestRunner +import org.apache.derby.jdbc.EmbeddedDriver +import org.h2.Driver +import spock.lang.Shared +import spock.lang.Unroll + +import java.sql.Connection +import java.sql.ResultSet +import java.sql.Statement + +class JDBCInstrumentationTest extends AgentTestRunner { + @Shared + def dbName = "jdbcUnitTest" + + @Unroll + def "Connection constructor throwing then generating correct spans after recovery using #db connection (prepare statement = #prepareStatement)"() { + setup: + Connection connection = null + + when: + try { + connection = new DummyThrowingConnection() + } catch (Exception e) { + connection = driver.connect(url, null) + } + + Statement statement = null + ResultSet rs = null + if (prepareStatement) { + statement = connection.prepareStatement(query) + rs = statement.executeQuery() + } else { + statement = connection.createStatement() + rs = statement.executeQuery(query) + } + + then: + rs.next() + rs.getInt(1) == 3 + TEST_WRITER.size() == 1 + + def trace = TEST_WRITER.firstTrace() + trace.size() == 1 + + and: + def span = trace[0] + span.context().operationName == "${db}.query" + span.serviceName == db + span.resourceName == query + span.type == "sql" + !span.context().getErrorFlag() + span.context().parentId == 0 + + def tags = span.context().tags + tags["db.type"] == db + tags["db.user"] == user + tags["span.kind"] == "client" + if (prepareStatement) { + tags["component"] == "java-jdbc-prepared_statement" + } else { + tags["component"] == "java-jdbc-statement" + } + + tags["db.jdbc.url"].contains(db) + tags["span.origin.type"] != null + + tags["thread.name"] != null + tags["thread.id"] != null + tags.size() == user == null ? 7 : 8 + + cleanup: + if (statement != null) { + statement.close() + } + if (connection != null) { + connection.close() + } + + where: + prepareStatement | db | driver | url | user | query + true | "h2" | new Driver() | "jdbc:h2:mem:" + dbName | null | "SELECT 3;" + true | "derby" | new EmbeddedDriver() | "jdbc:derby:memory:" + dbName + ";create=true" | "APP" | "SELECT 3 FROM SYSIBM.SYSDUMMY1" + false | "h2" | new Driver() | "jdbc:h2:mem:" + dbName | null | "SELECT 3;" + false | "derby" | new EmbeddedDriver() | "jdbc:derby:memory:" + dbName + ";create=true" | "APP" | "SELECT 3 FROM SYSIBM.SYSDUMMY1" + } + +} From 38dfe96eb0df514abc985663561cefc19a44c0be Mon Sep 17 00:00:00 2001 From: Gary Huang Date: Tue, 15 May 2018 15:14:33 -0400 Subject: [PATCH 2/3] Fixed java 9 JDBC integration test issue. Refactored JDBCMaps's getDBInfo utlity function because JDBCMaps is in the bootstrap classloader, and the use of java.sql.* packages in getDBInfo is failing because java.sql.* packages are part of the platform classloader in java 9. --- .../datadog/trace/bootstrap/JDBCMaps.java | 36 ------------------- .../PreparedStatementInstrumentation.java | 35 +++++++++++++++++- .../jdbc/StatementInstrumentation.java | 35 +++++++++++++++++- 3 files changed, 68 insertions(+), 38 deletions(-) diff --git a/dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/JDBCMaps.java b/dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/JDBCMaps.java index df0ed21833..062597276d 100644 --- a/dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/JDBCMaps.java +++ b/dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/JDBCMaps.java @@ -2,7 +2,6 @@ package datadog.trace.bootstrap; import java.sql.Connection; import java.sql.PreparedStatement; -import java.sql.SQLException; import java.util.Collections; import java.util.Map; import java.util.WeakHashMap; @@ -40,39 +39,4 @@ public class JDBCMaps { private static String propToEnvName(final String name) { return name.toUpperCase().replace(".", "_"); } - - /** - * Utility function to get the DBInfo from a JDBC Connection, if the connection was never seen - * before, the connectionInfo map will return null and will attempt to extract DBInfo from the - * connection. If the DBInfo can't be extracted, then the connection will be stored with the - * UNKNOWN DBInfo as the value in the connectionInfo map to avoid retry overhead. - * - * @param connection The JDBC connection - * @return A DBInfo that contains JDBC connection info - */ - public static DBInfo getDBInfo(Connection connection) { - DBInfo dbInfo = connectionInfo.get(connection); - if (dbInfo == null) { - try { - final String url = connection.getMetaData().getURL(); - if (url != null) { - // Remove end of url to prevent passwords from leaking: - final String sanitizedURL = url.replaceAll("[?;].*", ""); - final String type = url.split(":", -1)[1]; - String user = connection.getMetaData().getUserName(); - if (user != null && user.trim().equals("")) { - user = null; - } - dbInfo = new JDBCMaps.DBInfo(sanitizedURL, type, user); - } else { - dbInfo = DBInfo.DEFAULT; - } - } catch (SQLException se) { - dbInfo = DBInfo.DEFAULT; - } - connectionInfo.put(connection, dbInfo); - } - - return dbInfo; - } } diff --git a/dd-java-agent/instrumentation/jdbc/src/main/java/datadog/trace/instrumentation/jdbc/PreparedStatementInstrumentation.java b/dd-java-agent/instrumentation/jdbc/src/main/java/datadog/trace/instrumentation/jdbc/PreparedStatementInstrumentation.java index 230e702f1b..32fd3501ad 100644 --- a/dd-java-agent/instrumentation/jdbc/src/main/java/datadog/trace/instrumentation/jdbc/PreparedStatementInstrumentation.java +++ b/dd-java-agent/instrumentation/jdbc/src/main/java/datadog/trace/instrumentation/jdbc/PreparedStatementInstrumentation.java @@ -23,6 +23,7 @@ import io.opentracing.tag.Tags; import io.opentracing.util.GlobalTracer; import java.sql.Connection; import java.sql.PreparedStatement; +import java.sql.SQLException; import java.util.Collections; import net.bytebuddy.agent.builder.AgentBuilder; import net.bytebuddy.asm.Advice; @@ -63,7 +64,39 @@ public final class PreparedStatementInstrumentation extends Instrumenter.Configu return NoopScope.INSTANCE; } - JDBCMaps.DBInfo dbInfo = JDBCMaps.getDBInfo(connection); + JDBCMaps.DBInfo dbInfo = JDBCMaps.connectionInfo.get(connection); + /** + * Logic to get the DBInfo from a JDBC Connection, if the connection was never seen before, + * the connectionInfo map will return null and will attempt to extract DBInfo from the + * connection. If the DBInfo can't be extracted, then the connection will be stored with the + * DEFAULT DBInfo as the value in the connectionInfo map to avoid retry overhead. + * + *

This should be a util method to be shared between PreparedStatementInstrumentation and + * StatementInstrumentation class, but java.sql.* are on the platform classloaders in Java 9, + * which prevents us from referencing them in the bootstrap utils. + */ + { + if (dbInfo == null) { + try { + final String url = connection.getMetaData().getURL(); + if (url != null) { + // Remove end of url to prevent passwords from leaking: + final String sanitizedURL = url.replaceAll("[?;].*", ""); + final String type = url.split(":", -1)[1]; + String user = connection.getMetaData().getUserName(); + if (user != null && user.trim().equals("")) { + user = null; + } + dbInfo = new JDBCMaps.DBInfo(sanitizedURL, type, user); + } else { + dbInfo = JDBCMaps.DBInfo.DEFAULT; + } + } catch (SQLException se) { + dbInfo = JDBCMaps.DBInfo.DEFAULT; + } + JDBCMaps.connectionInfo.put(connection, dbInfo); + } + } final Scope scope = GlobalTracer.get().buildSpan(dbInfo.getType() + ".query").startActive(true); diff --git a/dd-java-agent/instrumentation/jdbc/src/main/java/datadog/trace/instrumentation/jdbc/StatementInstrumentation.java b/dd-java-agent/instrumentation/jdbc/src/main/java/datadog/trace/instrumentation/jdbc/StatementInstrumentation.java index 45cc483d15..61790f4d83 100644 --- a/dd-java-agent/instrumentation/jdbc/src/main/java/datadog/trace/instrumentation/jdbc/StatementInstrumentation.java +++ b/dd-java-agent/instrumentation/jdbc/src/main/java/datadog/trace/instrumentation/jdbc/StatementInstrumentation.java @@ -22,6 +22,7 @@ import io.opentracing.noop.NoopScopeManager.NoopScope; import io.opentracing.tag.Tags; import io.opentracing.util.GlobalTracer; import java.sql.Connection; +import java.sql.SQLException; import java.sql.Statement; import java.util.Collections; import net.bytebuddy.agent.builder.AgentBuilder; @@ -64,7 +65,39 @@ public final class StatementInstrumentation extends Instrumenter.Configurable { return NoopScope.INSTANCE; } - JDBCMaps.DBInfo dbInfo = JDBCMaps.getDBInfo(connection); + JDBCMaps.DBInfo dbInfo = JDBCMaps.connectionInfo.get(connection); + /** + * Logic to get the DBInfo from a JDBC Connection, if the connection was never seen before, + * the connectionInfo map will return null and will attempt to extract DBInfo from the + * connection. If the DBInfo can't be extracted, then the connection will be stored with the + * DEFAULT DBInfo as the value in the connectionInfo map to avoid retry overhead. + * + *

This should be a util method to be shared between PreparedStatementInstrumentation and + * StatementInstrumentation class, but java.sql.* are on the platform classloaders in Java 9, + * which prevents us from referencing them in the bootstrap utils. + */ + { + if (dbInfo == null) { + try { + final String url = connection.getMetaData().getURL(); + if (url != null) { + // Remove end of url to prevent passwords from leaking: + final String sanitizedURL = url.replaceAll("[?;].*", ""); + final String type = url.split(":", -1)[1]; + String user = connection.getMetaData().getUserName(); + if (user != null && user.trim().equals("")) { + user = null; + } + dbInfo = new JDBCMaps.DBInfo(sanitizedURL, type, user); + } else { + dbInfo = JDBCMaps.DBInfo.DEFAULT; + } + } catch (SQLException se) { + dbInfo = JDBCMaps.DBInfo.DEFAULT; + } + JDBCMaps.connectionInfo.put(connection, dbInfo); + } + } final Scope scope = GlobalTracer.get().buildSpan(dbInfo.getType() + ".query").startActive(true); From 71396aee01afcb1a2173ddefee28b4223d4d32a5 Mon Sep 17 00:00:00 2001 From: Gary Huang Date: Tue, 15 May 2018 16:12:31 -0400 Subject: [PATCH 3/3] Moved JDBC integration tests to become unit tests --- .../dd-java-agent-ittests.gradle | 4 - .../jdbc/JDBCInstrumentationTest.groovy | 273 ------------------ .../instrumentation/jdbc/jdbc.gradle | 2 + .../groovy/JDBCInstrumentationTest.groovy | 268 ++++++++++++++++- 4 files changed, 263 insertions(+), 284 deletions(-) delete mode 100644 dd-java-agent-ittests/src/test/groovy/datadog/trace/agent/integration/jdbc/JDBCInstrumentationTest.groovy diff --git a/dd-java-agent-ittests/dd-java-agent-ittests.gradle b/dd-java-agent-ittests/dd-java-agent-ittests.gradle index c829b08b10..783c243c90 100644 --- a/dd-java-agent-ittests/dd-java-agent-ittests.gradle +++ b/dd-java-agent-ittests/dd-java-agent-ittests.gradle @@ -23,10 +23,6 @@ dependencies { testCompile group: 'org.apache.httpcomponents', name: 'httpclient', version: '4.3' testCompile group: 'com.squareup.okhttp3', name: 'okhttp', version: '3.6.0' - // JDBC tests: - testCompile group: 'com.h2database', name: 'h2', version: '1.4.196' - testCompile group: 'org.hsqldb', name: 'hsqldb', version: '2.3.+' - testCompile group: 'org.apache.derby', name: 'derby', version: '10.12.1.1' } tasks.withType(Test) { diff --git a/dd-java-agent-ittests/src/test/groovy/datadog/trace/agent/integration/jdbc/JDBCInstrumentationTest.groovy b/dd-java-agent-ittests/src/test/groovy/datadog/trace/agent/integration/jdbc/JDBCInstrumentationTest.groovy deleted file mode 100644 index 3005ab017e..0000000000 --- a/dd-java-agent-ittests/src/test/groovy/datadog/trace/agent/integration/jdbc/JDBCInstrumentationTest.groovy +++ /dev/null @@ -1,273 +0,0 @@ -package datadog.trace.agent.integration.jdbc - -import datadog.opentracing.DDTracer -import datadog.trace.agent.test.IntegrationTestUtils -import datadog.trace.common.writer.ListWriter -import org.apache.derby.jdbc.EmbeddedDriver -import org.h2.Driver -import org.hsqldb.jdbc.JDBCDriver -import spock.lang.Shared -import spock.lang.Specification - -import java.sql.Connection -import java.sql.PreparedStatement -import java.sql.ResultSet -import java.sql.Statement - -class JDBCInstrumentationTest extends Specification { - - final ListWriter writer = new ListWriter() - final DDTracer tracer = new DDTracer(writer) - - @Shared - private Map connections - - def setupSpec() { - Connection h2Connection = new Driver().connect("jdbc:h2:mem:integ-test", null) - Connection hsqlConnection = new JDBCDriver().connect("jdbc:hsqldb:mem:integTest", null) - Connection derbyConnection = new EmbeddedDriver().connect("jdbc:derby:memory:integTest;create=true", null) - - connections = [ - h2 : h2Connection, - derby : derbyConnection, - hsqldb: hsqlConnection, - ] - } - - def cleanupSpec() { - connections.values().each { - it.close() - } - } - - def setup() { - IntegrationTestUtils.registerOrReplaceGlobalTracer(tracer) - writer.start() - } - - def "basic statement on #driver generates spans"() { - setup: - Statement statement = connection.createStatement() - ResultSet resultSet = statement.executeQuery(query) - - expect: - resultSet.next() - resultSet.getInt(1) == 3 - writer.size() == 1 - - def trace = writer.firstTrace() - trace.size() == 1 - def span = trace[0] - - span.context().operationName == "${driver}.query" - span.serviceName == driver - span.resourceName == query - span.type == "sql" - !span.context().getErrorFlag() - span.context().parentId == 0 - - - def tags = span.context().tags - tags["db.type"] == driver - tags["db.user"] == username - tags["span.kind"] == "client" - tags["component"] == "java-jdbc-statement" - - tags["db.jdbc.url"].contains(driver) - tags["span.origin.type"] != null - - tags["thread.name"] != null - tags["thread.id"] != null - tags.size() == username == null ? 7 : 8 - - cleanup: - statement.close() - - where: - driver | connection | username | query - "h2" | connections.get("h2") | null | "SELECT 3" - "derby" | connections.get("derby") | "APP" | "SELECT 3 FROM SYSIBM.SYSDUMMY1" - "hsqldb" | connections.get("hsqldb") | "SA" | "SELECT 3 FROM INFORMATION_SCHEMA.SYSTEM_USERS" - } - - def "prepared statement execute on #driver generates a span"() { - setup: - PreparedStatement statement = connection.prepareStatement(query) - assert statement.execute() - ResultSet resultSet = statement.resultSet - - expect: - resultSet.next() - resultSet.getInt(1) == 3 - writer.size() == 1 - - def trace = writer.firstTrace() - trace.size() == 1 - def span = trace[0] - - span.context().operationName == "${driver}.query" - span.serviceName == driver - span.resourceName == query - span.type == "sql" - !span.context().getErrorFlag() - span.context().parentId == 0 - - - def tags = span.context().tags - tags["db.type"] == driver - tags["db.user"] == username - tags["span.kind"] == "client" - tags["component"] == "java-jdbc-prepared_statement" - - tags["db.jdbc.url"].contains(driver) - tags["span.origin.type"] != null - - tags["thread.name"] != null - tags["thread.id"] != null - tags.size() == username == null ? 7 : 8 - - cleanup: - statement.close() - - where: - driver | connection | username | query - "h2" | connections.get("h2") | null | "SELECT 3" - "derby" | connections.get("derby") | "APP" | "SELECT 3 FROM SYSIBM.SYSDUMMY1" - "hsqldb" | connections.get("hsqldb") | "SA" | "SELECT 3 FROM INFORMATION_SCHEMA.SYSTEM_USERS" - } - - def "prepared statement query on #driver generates a span"() { - setup: - PreparedStatement statement = connection.prepareStatement(query) - ResultSet resultSet = statement.executeQuery() - - expect: - resultSet.next() - resultSet.getInt(1) == 3 - writer.size() == 1 - - def trace = writer.firstTrace() - trace.size() == 1 - def span = trace[0] - - span.context().operationName == "${driver}.query" - span.serviceName == driver - span.resourceName == query - span.type == "sql" - !span.context().getErrorFlag() - span.context().parentId == 0 - - - def tags = span.context().tags - tags["db.type"] == driver - tags["db.user"] == username - tags["span.kind"] == "client" - tags["component"] == "java-jdbc-prepared_statement" - - tags["db.jdbc.url"].contains(driver) - tags["span.origin.type"] != null - - tags["thread.name"] != null - tags["thread.id"] != null - tags.size() == username == null ? 7 : 8 - - cleanup: - statement.close() - - where: - driver | connection | username | query - "h2" | connections.get("h2") | null | "SELECT 3" - "derby" | connections.get("derby") | "APP" | "SELECT 3 FROM SYSIBM.SYSDUMMY1" - "hsqldb" | connections.get("hsqldb") | "SA" | "SELECT 3 FROM INFORMATION_SCHEMA.SYSTEM_USERS" - } - - def "statement update on #driver generates a span"() { - setup: - Statement statement = connection.createStatement() - def sql = connection.nativeSQL(query) - - expect: - !statement.execute(sql) - statement.updateCount == 0 - - writer.size() == 1 - - def trace = writer.firstTrace() - trace.size() == 1 - def span = trace[0] - - span.context().operationName == "${driver}.query" - span.serviceName == driver - span.resourceName == query - span.type == "sql" - !span.context().getErrorFlag() - span.context().parentId == 0 - - - def tags = span.context().tags - tags["db.type"] == driver - tags["db.user"] == username - tags["span.kind"] == "client" - tags["component"] == "java-jdbc-statement" - - tags["db.jdbc.url"].contains(driver) - tags["span.origin.type"] != null - - tags["thread.name"] != null - tags["thread.id"] != null - tags.size() == username == null ? 7 : 8 - - cleanup: - statement.close() - - where: - driver | connection | username | query - "h2" | connections.get("h2") | null | "CREATE TABLE S_H2 (id INTEGER not NULL, PRIMARY KEY ( id ))" - "derby" | connections.get("derby") | "APP" | "CREATE TABLE S_DERBY (id INTEGER not NULL, PRIMARY KEY ( id ))" - "hsqldb" | connections.get("hsqldb") | "SA" | "CREATE TABLE PUBLIC.S_HSQLDB (id INTEGER not NULL, PRIMARY KEY ( id ))" - } - - def "prepared statement update on #driver generates a span"() { - setup: - def sql = connection.nativeSQL(query) - PreparedStatement statement = connection.prepareStatement(sql) - - expect: - statement.executeUpdate() == 0 - writer.size() == 1 - - def trace = writer.firstTrace() - trace.size() == 1 - def span = trace[0] - - span.context().operationName == "${driver}.query" - span.serviceName == driver - span.resourceName == query - span.type == "sql" - !span.context().getErrorFlag() - span.context().parentId == 0 - - - def tags = span.context().tags - tags["db.type"] == driver - tags["db.user"] == username - tags["span.kind"] == "client" - tags["component"] == "java-jdbc-prepared_statement" - - tags["db.jdbc.url"].contains(driver) - tags["span.origin.type"] != null - - tags["thread.name"] != null - tags["thread.id"] != null - tags.size() == username == null ? 7 : 8 - - cleanup: - statement.close() - - where: - driver | connection | username | query - "h2" | connections.get("h2") | null | "CREATE TABLE PS_H2 (id INTEGER not NULL, PRIMARY KEY ( id ))" - "derby" | connections.get("derby") | "APP" | "CREATE TABLE PS_DERBY (id INTEGER not NULL, PRIMARY KEY ( id ))" - "hsqldb" | connections.get("hsqldb") | "SA" | "CREATE TABLE PUBLIC.PS_HSQLDB (id INTEGER not NULL, PRIMARY KEY ( id ))" - } -} diff --git a/dd-java-agent/instrumentation/jdbc/jdbc.gradle b/dd-java-agent/instrumentation/jdbc/jdbc.gradle index e9dbe55750..80dba9603d 100644 --- a/dd-java-agent/instrumentation/jdbc/jdbc.gradle +++ b/dd-java-agent/instrumentation/jdbc/jdbc.gradle @@ -12,5 +12,7 @@ dependencies { // jdbc unit testing testCompile group: 'com.h2database', name: 'h2', version: '1.4.197' testCompile group: 'org.apache.derby', name: 'derby', version: '10.12.1.1' + // not using hsqldb 2.4 because org/hsqldb/jdbc/JDBCDriver : Unsupported major.minor version 52.0 + testCompile group: 'org.hsqldb', name: 'hsqldb', version: '2.3.+' } diff --git a/dd-java-agent/instrumentation/jdbc/src/test/groovy/JDBCInstrumentationTest.groovy b/dd-java-agent/instrumentation/jdbc/src/test/groovy/JDBCInstrumentationTest.groovy index a039e13469..35ad53176d 100644 --- a/dd-java-agent/instrumentation/jdbc/src/test/groovy/JDBCInstrumentationTest.groovy +++ b/dd-java-agent/instrumentation/jdbc/src/test/groovy/JDBCInstrumentationTest.groovy @@ -1,10 +1,12 @@ import datadog.trace.agent.test.AgentTestRunner import org.apache.derby.jdbc.EmbeddedDriver import org.h2.Driver +import org.hsqldb.jdbc.JDBCDriver import spock.lang.Shared import spock.lang.Unroll import java.sql.Connection +import java.sql.PreparedStatement import java.sql.ResultSet import java.sql.Statement @@ -12,8 +14,260 @@ class JDBCInstrumentationTest extends AgentTestRunner { @Shared def dbName = "jdbcUnitTest" + @Shared + private Map connections + + def setupSpec() { + Connection h2Connection = new Driver().connect("jdbc:h2:mem:" + dbName, null) + Connection hsqlConnection = new JDBCDriver().connect("jdbc:hsqldb:mem:" + dbName, null) + Connection derbyConnection = new EmbeddedDriver().connect("jdbc:derby:memory:" + dbName + ";create=true", null) + + connections = [ + h2 : h2Connection, + derby : derbyConnection, + hsqldb: hsqlConnection, + ] + } + + def cleanupSpec() { + connections.values().each { + it.close() + } + } + @Unroll - def "Connection constructor throwing then generating correct spans after recovery using #db connection (prepare statement = #prepareStatement)"() { + def "basic statement on #driver generates spans"() { + setup: + Statement statement = connection.createStatement() + ResultSet resultSet = statement.executeQuery(query) + + expect: + resultSet.next() + resultSet.getInt(1) == 3 + TEST_WRITER.size() == 1 + + def trace = TEST_WRITER.firstTrace() + trace.size() == 1 + def span = trace[0] + + span.context().operationName == "${driver}.query" + span.serviceName == driver + span.resourceName == query + span.type == "sql" + !span.context().getErrorFlag() + span.context().parentId == 0 + + + def tags = span.context().tags + tags["db.type"] == driver + tags["db.user"] == username + tags["span.kind"] == "client" + tags["component"] == "java-jdbc-statement" + + tags["db.jdbc.url"].contains(driver) + tags["span.origin.type"] != null + + tags["thread.name"] != null + tags["thread.id"] != null + tags.size() == username == null ? 7 : 8 + + cleanup: + statement.close() + + where: + driver | connection | username | query + "h2" | connections.get("h2") | null | "SELECT 3" + "derby" | connections.get("derby") | "APP" | "SELECT 3 FROM SYSIBM.SYSDUMMY1" + "hsqldb" | connections.get("hsqldb") | "SA" | "SELECT 3 FROM INFORMATION_SCHEMA.SYSTEM_USERS" + } + + @Unroll + def "prepared statement execute on #driver generates a span"() { + setup: + PreparedStatement statement = connection.prepareStatement(query) + assert statement.execute() + ResultSet resultSet = statement.resultSet + + expect: + resultSet.next() + resultSet.getInt(1) == 3 + TEST_WRITER.size() == 1 + + def trace = TEST_WRITER.firstTrace() + trace.size() == 1 + def span = trace[0] + + span.context().operationName == "${driver}.query" + span.serviceName == driver + span.resourceName == query + span.type == "sql" + !span.context().getErrorFlag() + span.context().parentId == 0 + + + def tags = span.context().tags + tags["db.type"] == driver + tags["db.user"] == username + tags["span.kind"] == "client" + tags["component"] == "java-jdbc-prepared_statement" + + tags["db.jdbc.url"].contains(driver) + tags["span.origin.type"] != null + + tags["thread.name"] != null + tags["thread.id"] != null + tags.size() == username == null ? 7 : 8 + + cleanup: + statement.close() + + where: + driver | connection | username | query + "h2" | connections.get("h2") | null | "SELECT 3" + "derby" | connections.get("derby") | "APP" | "SELECT 3 FROM SYSIBM.SYSDUMMY1" + "hsqldb" | connections.get("hsqldb") | "SA" | "SELECT 3 FROM INFORMATION_SCHEMA.SYSTEM_USERS" + } + + @Unroll + def "prepared statement query on #driver generates a span"() { + setup: + PreparedStatement statement = connection.prepareStatement(query) + ResultSet resultSet = statement.executeQuery() + + expect: + resultSet.next() + resultSet.getInt(1) == 3 + TEST_WRITER.size() == 1 + + def trace = TEST_WRITER.firstTrace() + trace.size() == 1 + def span = trace[0] + + span.context().operationName == "${driver}.query" + span.serviceName == driver + span.resourceName == query + span.type == "sql" + !span.context().getErrorFlag() + span.context().parentId == 0 + + + def tags = span.context().tags + tags["db.type"] == driver + tags["db.user"] == username + tags["span.kind"] == "client" + tags["component"] == "java-jdbc-prepared_statement" + + tags["db.jdbc.url"].contains(driver) + tags["span.origin.type"] != null + + tags["thread.name"] != null + tags["thread.id"] != null + tags.size() == username == null ? 7 : 8 + + cleanup: + statement.close() + + where: + driver | connection | username | query + "h2" | connections.get("h2") | null | "SELECT 3" + "derby" | connections.get("derby") | "APP" | "SELECT 3 FROM SYSIBM.SYSDUMMY1" + "hsqldb" | connections.get("hsqldb") | "SA" | "SELECT 3 FROM INFORMATION_SCHEMA.SYSTEM_USERS" + } + + @Unroll + def "statement update on #driver generates a span"() { + setup: + Statement statement = connection.createStatement() + def sql = connection.nativeSQL(query) + + expect: + !statement.execute(sql) + statement.updateCount == 0 + + TEST_WRITER.size() == 1 + + def trace = TEST_WRITER.firstTrace() + trace.size() == 1 + def span = trace[0] + + span.context().operationName == "${driver}.query" + span.serviceName == driver + span.resourceName == query + span.type == "sql" + !span.context().getErrorFlag() + span.context().parentId == 0 + + + def tags = span.context().tags + tags["db.type"] == driver + tags["db.user"] == username + tags["span.kind"] == "client" + tags["component"] == "java-jdbc-statement" + + tags["db.jdbc.url"].contains(driver) + tags["span.origin.type"] != null + + tags["thread.name"] != null + tags["thread.id"] != null + tags.size() == username == null ? 7 : 8 + + cleanup: + statement.close() + + where: + driver | connection | username | query + "h2" | connections.get("h2") | null | "CREATE TABLE S_H2 (id INTEGER not NULL, PRIMARY KEY ( id ))" + "derby" | connections.get("derby") | "APP" | "CREATE TABLE S_DERBY (id INTEGER not NULL, PRIMARY KEY ( id ))" + "hsqldb" | connections.get("hsqldb") | "SA" | "CREATE TABLE PUBLIC.S_HSQLDB (id INTEGER not NULL, PRIMARY KEY ( id ))" + } + + @Unroll + def "prepared statement update on #driver generates a span"() { + setup: + def sql = connection.nativeSQL(query) + PreparedStatement statement = connection.prepareStatement(sql) + + expect: + statement.executeUpdate() == 0 + TEST_WRITER.size() == 1 + + def trace = TEST_WRITER.firstTrace() + trace.size() == 1 + def span = trace[0] + + span.context().operationName == "${driver}.query" + span.serviceName == driver + span.resourceName == query + span.type == "sql" + !span.context().getErrorFlag() + span.context().parentId == 0 + + + def tags = span.context().tags + tags["db.type"] == driver + tags["db.user"] == username + tags["span.kind"] == "client" + tags["component"] == "java-jdbc-prepared_statement" + + tags["db.jdbc.url"].contains(driver) + tags["span.origin.type"] != null + + tags["thread.name"] != null + tags["thread.id"] != null + tags.size() == username == null ? 7 : 8 + + cleanup: + statement.close() + + where: + driver | connection | username | query + "h2" | connections.get("h2") | null | "CREATE TABLE PS_H2 (id INTEGER not NULL, PRIMARY KEY ( id ))" + "derby" | connections.get("derby") | "APP" | "CREATE TABLE PS_DERBY (id INTEGER not NULL, PRIMARY KEY ( id ))" + "hsqldb" | connections.get("hsqldb") | "SA" | "CREATE TABLE PUBLIC.PS_HSQLDB (id INTEGER not NULL, PRIMARY KEY ( id ))" + } + + @Unroll + def "connection constructor throwing then generating correct spans after recovery using #driver connection (prepare statement = #prepareStatement)"() { setup: Connection connection = null @@ -21,7 +275,7 @@ class JDBCInstrumentationTest extends AgentTestRunner { try { connection = new DummyThrowingConnection() } catch (Exception e) { - connection = driver.connect(url, null) + connection = driverClass.connect(url, null) } Statement statement = null @@ -44,15 +298,15 @@ class JDBCInstrumentationTest extends AgentTestRunner { and: def span = trace[0] - span.context().operationName == "${db}.query" - span.serviceName == db + span.context().operationName == "${driver}.query" + span.serviceName == driver span.resourceName == query span.type == "sql" !span.context().getErrorFlag() span.context().parentId == 0 def tags = span.context().tags - tags["db.type"] == db + tags["db.type"] == driver tags["db.user"] == user tags["span.kind"] == "client" if (prepareStatement) { @@ -61,7 +315,7 @@ class JDBCInstrumentationTest extends AgentTestRunner { tags["component"] == "java-jdbc-statement" } - tags["db.jdbc.url"].contains(db) + tags["db.jdbc.url"].contains(driver) tags["span.origin.type"] != null tags["thread.name"] != null @@ -77,7 +331,7 @@ class JDBCInstrumentationTest extends AgentTestRunner { } where: - prepareStatement | db | driver | url | user | query + prepareStatement | driver | driverClass | url | user | query true | "h2" | new Driver() | "jdbc:h2:mem:" + dbName | null | "SELECT 3;" true | "derby" | new EmbeddedDriver() | "jdbc:derby:memory:" + dbName + ";create=true" | "APP" | "SELECT 3 FROM SYSIBM.SYSDUMMY1" false | "h2" | new Driver() | "jdbc:h2:mem:" + dbName | null | "SELECT 3;"