From a17f745dd99d72b088e817518eba181d61eaaaf1 Mon Sep 17 00:00:00 2001 From: Mateusz Rzeszutek Date: Thu, 8 Apr 2021 13:08:48 +0200 Subject: [PATCH 1/3] Fix JDBC instrumentation: recursive statements inside Connection#getMetaData --- .../PreparedStatementInstrumentation.java | 8 ++++ .../jdbc/StatementInstrumentation.java | 7 +++ .../groovy/JdbcInstrumentationTest.groovy | 48 +++++++++++++++++++ 3 files changed, 63 insertions(+) diff --git a/instrumentation/jdbc/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jdbc/PreparedStatementInstrumentation.java b/instrumentation/jdbc/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jdbc/PreparedStatementInstrumentation.java index 967610ecab05..b89a9fda60e1 100644 --- a/instrumentation/jdbc/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jdbc/PreparedStatementInstrumentation.java +++ b/instrumentation/jdbc/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jdbc/PreparedStatementInstrumentation.java @@ -17,8 +17,10 @@ import io.opentelemetry.context.Context; import io.opentelemetry.context.Scope; +import io.opentelemetry.javaagent.instrumentation.api.CallDepthThreadLocalMap; import io.opentelemetry.javaagent.tooling.TypeInstrumentation; import java.sql.PreparedStatement; +import java.sql.Statement; import java.util.Map; import net.bytebuddy.asm.Advice; import net.bytebuddy.description.method.MethodDescription; @@ -51,6 +53,11 @@ public static void onEnter( @Advice.This PreparedStatement statement, @Advice.Local("otelContext") Context context, @Advice.Local("otelScope") Scope scope) { + // prevent recursive Statement calls + if (CallDepthThreadLocalMap.getCallDepth(Statement.class).getAndIncrement() > 0) { + return; + } + Context parentContext = currentContext(); if (!tracer().shouldStartSpan(parentContext)) { return; @@ -68,6 +75,7 @@ public static void stopSpan( if (scope == null) { return; } + CallDepthThreadLocalMap.reset(Statement.class); scope.close(); if (throwable == null) { diff --git a/instrumentation/jdbc/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jdbc/StatementInstrumentation.java b/instrumentation/jdbc/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jdbc/StatementInstrumentation.java index b8633c0ec55c..60beed82d28f 100644 --- a/instrumentation/jdbc/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jdbc/StatementInstrumentation.java +++ b/instrumentation/jdbc/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jdbc/StatementInstrumentation.java @@ -17,6 +17,7 @@ import io.opentelemetry.context.Context; import io.opentelemetry.context.Scope; +import io.opentelemetry.javaagent.instrumentation.api.CallDepthThreadLocalMap; import io.opentelemetry.javaagent.tooling.TypeInstrumentation; import java.sql.Statement; import java.util.Map; @@ -52,6 +53,11 @@ public static void onEnter( @Advice.This Statement statement, @Advice.Local("otelContext") Context context, @Advice.Local("otelScope") Scope scope) { + // prevent recursive Statement calls + if (CallDepthThreadLocalMap.getCallDepth(Statement.class).getAndIncrement() > 0) { + return; + } + Context parentContext = currentContext(); if (!tracer().shouldStartSpan(parentContext)) { return; @@ -69,6 +75,7 @@ public static void stopSpan( if (scope == null) { return; } + CallDepthThreadLocalMap.reset(Statement.class); scope.close(); if (throwable == null) { diff --git a/instrumentation/jdbc/javaagent/src/test/groovy/JdbcInstrumentationTest.groovy b/instrumentation/jdbc/javaagent/src/test/groovy/JdbcInstrumentationTest.groovy index ed6a4c6948f2..6f533fdef960 100644 --- a/instrumentation/jdbc/javaagent/src/test/groovy/JdbcInstrumentationTest.groovy +++ b/instrumentation/jdbc/javaagent/src/test/groovy/JdbcInstrumentationTest.groovy @@ -3,6 +3,7 @@ * SPDX-License-Identifier: Apache-2.0 */ + import static io.opentelemetry.api.trace.SpanKind.CLIENT import static io.opentelemetry.instrumentation.test.utils.TraceUtils.basicSpan import static io.opentelemetry.instrumentation.test.utils.TraceUtils.runUnderTrace @@ -14,8 +15,10 @@ import io.opentelemetry.instrumentation.test.AgentInstrumentationSpecification import io.opentelemetry.semconv.trace.attributes.SemanticAttributes import java.sql.CallableStatement import java.sql.Connection +import java.sql.DatabaseMetaData import java.sql.PreparedStatement import java.sql.ResultSet +import java.sql.SQLException import java.sql.Statement import javax.sql.DataSource import org.apache.derby.jdbc.EmbeddedDataSource @@ -724,4 +727,49 @@ class JdbcInstrumentationTest extends AgentInstrumentationSpecification { "tomcat" | _ "c3p0" | _ } + + // regression test for https://github.com/open-telemetry/opentelemetry-java-instrumentation/issues/2644 + def "should handle recursive Statements inside Connection#getMetaData()"() { + given: + def connection = new DbCallingConnection() + + when: + runUnderTrace("parent") { + def statement = connection.createStatement() + return statement.executeQuery("SELECT * FROM table") + } + + then: + assertTraces(1) { + trace(0, 2) { + basicSpan(it, 0, "parent") + span(1) { + name "SELECT table" + kind CLIENT + childOf span(0) + errored false + attributes { + "$SemanticAttributes.DB_SYSTEM.key" "testdb" + "$SemanticAttributes.DB_CONNECTION_STRING.key" "testdb://localhost" + "$SemanticAttributes.DB_STATEMENT.key" "SELECT * FROM table" + "$SemanticAttributes.DB_OPERATION.key" "SELECT" + "$SemanticAttributes.DB_SQL_TABLE.key" "table" + } + } + } + } + } + + class DbCallingConnection extends TestConnection { + DbCallingConnection() { + super(false) + } + + @Override + DatabaseMetaData getMetaData() throws SQLException { + // simulate retrieving DB metadata from the DB itself + createStatement().executeQuery("SELECT * from DB_METADATA") + return super.getMetaData() + } + } } From ee1758d7df025fb2009672818bbcd48373bef317 Mon Sep 17 00:00:00 2001 From: Mateusz Rzeszutek Date: Thu, 8 Apr 2021 14:49:25 +0200 Subject: [PATCH 2/3] spotless --- .../javaagent/src/test/groovy/JdbcInstrumentationTest.groovy | 1 - 1 file changed, 1 deletion(-) diff --git a/instrumentation/jdbc/javaagent/src/test/groovy/JdbcInstrumentationTest.groovy b/instrumentation/jdbc/javaagent/src/test/groovy/JdbcInstrumentationTest.groovy index 6f533fdef960..351c909c8202 100644 --- a/instrumentation/jdbc/javaagent/src/test/groovy/JdbcInstrumentationTest.groovy +++ b/instrumentation/jdbc/javaagent/src/test/groovy/JdbcInstrumentationTest.groovy @@ -3,7 +3,6 @@ * SPDX-License-Identifier: Apache-2.0 */ - import static io.opentelemetry.api.trace.SpanKind.CLIENT import static io.opentelemetry.instrumentation.test.utils.TraceUtils.basicSpan import static io.opentelemetry.instrumentation.test.utils.TraceUtils.runUnderTrace From 096d930e5b22b98af8db8583eb352024a3a76235 Mon Sep 17 00:00:00 2001 From: Mateusz Rzeszutek Date: Fri, 9 Apr 2021 08:57:45 +0200 Subject: [PATCH 3/3] Code review comments --- .../PreparedStatementInstrumentation.java | 8 +- .../jdbc/StatementInstrumentation.java | 8 +- .../groovy/JdbcInstrumentationTest.groovy | 26 +- .../test/groovy/test/TestConnection.groovy | 2 +- .../groovy/test/TestPreparedStatement.groovy | 304 ++++++++++++++++++ 5 files changed, 339 insertions(+), 9 deletions(-) create mode 100644 instrumentation/jdbc/javaagent/src/test/groovy/test/TestPreparedStatement.groovy diff --git a/instrumentation/jdbc/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jdbc/PreparedStatementInstrumentation.java b/instrumentation/jdbc/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jdbc/PreparedStatementInstrumentation.java index b89a9fda60e1..a383520e21ca 100644 --- a/instrumentation/jdbc/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jdbc/PreparedStatementInstrumentation.java +++ b/instrumentation/jdbc/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jdbc/PreparedStatementInstrumentation.java @@ -53,7 +53,13 @@ public static void onEnter( @Advice.This PreparedStatement statement, @Advice.Local("otelContext") Context context, @Advice.Local("otelScope") Scope scope) { - // prevent recursive Statement calls + // Connection#getMetaData() may execute a Statement or PreparedStatement to retrieve DB info + // this happens before the DB CLIENT span is started (and put in the current context), so this + // instrumentation runs again and the shouldStartSpan() check always returns true - and so on + // until we get a StackOverflowError + // using CallDepth prevents this, because this check happens before Connection#getMetadata() + // is called - the first recursive Statement call is just skipped and we do not create a span + // for it if (CallDepthThreadLocalMap.getCallDepth(Statement.class).getAndIncrement() > 0) { return; } diff --git a/instrumentation/jdbc/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jdbc/StatementInstrumentation.java b/instrumentation/jdbc/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jdbc/StatementInstrumentation.java index 60beed82d28f..c90c48624d67 100644 --- a/instrumentation/jdbc/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jdbc/StatementInstrumentation.java +++ b/instrumentation/jdbc/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jdbc/StatementInstrumentation.java @@ -53,7 +53,13 @@ public static void onEnter( @Advice.This Statement statement, @Advice.Local("otelContext") Context context, @Advice.Local("otelScope") Scope scope) { - // prevent recursive Statement calls + // Connection#getMetaData() may execute a Statement or PreparedStatement to retrieve DB info + // this happens before the DB CLIENT span is started (and put in the current context), so this + // instrumentation runs again and the shouldStartSpan() check always returns true - and so on + // until we get a StackOverflowError + // using CallDepth prevents this, because this check happens before Connection#getMetadata() + // is called - the first recursive Statement call is just skipped and we do not create a span + // for it if (CallDepthThreadLocalMap.getCallDepth(Statement.class).getAndIncrement() > 0) { return; } diff --git a/instrumentation/jdbc/javaagent/src/test/groovy/JdbcInstrumentationTest.groovy b/instrumentation/jdbc/javaagent/src/test/groovy/JdbcInstrumentationTest.groovy index 351c909c8202..bed2891fce4d 100644 --- a/instrumentation/jdbc/javaagent/src/test/groovy/JdbcInstrumentationTest.groovy +++ b/instrumentation/jdbc/javaagent/src/test/groovy/JdbcInstrumentationTest.groovy @@ -728,14 +728,14 @@ class JdbcInstrumentationTest extends AgentInstrumentationSpecification { } // regression test for https://github.com/open-telemetry/opentelemetry-java-instrumentation/issues/2644 - def "should handle recursive Statements inside Connection#getMetaData()"() { + @Unroll + def "should handle recursive Statements inside Connection.getMetaData(): #desc"() { given: - def connection = new DbCallingConnection() + def connection = new DbCallingConnection(usePreparedStatementInConnection) when: runUnderTrace("parent") { - def statement = connection.createStatement() - return statement.executeQuery("SELECT * FROM table") + executeQueryFunction(connection, "SELECT * FROM table") } then: @@ -757,17 +757,31 @@ class JdbcInstrumentationTest extends AgentInstrumentationSpecification { } } } + + where: + desc | usePreparedStatementInConnection | executeQueryFunction + "getMetaData() uses Statement, test Statement" | false | { con, query -> con.createStatement().executeQuery(query) } + "getMetaData() uses PreparedStatement, test Statement" | true | { con, query -> con.createStatement().executeQuery(query) } + "getMetaData() uses Statement, test PreparedStatement" | false | { con, query -> con.prepareStatement(query).executeQuery() } + "getMetaData() uses PreparedStatement, test PreparedStatement" | true | { con, query -> con.prepareStatement(query).executeQuery() } } class DbCallingConnection extends TestConnection { - DbCallingConnection() { + final boolean usePreparedStatement + + DbCallingConnection(boolean usePreparedStatement) { super(false) + this.usePreparedStatement = usePreparedStatement } @Override DatabaseMetaData getMetaData() throws SQLException { // simulate retrieving DB metadata from the DB itself - createStatement().executeQuery("SELECT * from DB_METADATA") + if (usePreparedStatement) { + prepareStatement("SELECT * from DB_METADATA").executeQuery() + } else { + createStatement().executeQuery("SELECT * from DB_METADATA") + } return super.getMetaData() } } diff --git a/instrumentation/jdbc/javaagent/src/test/groovy/test/TestConnection.groovy b/instrumentation/jdbc/javaagent/src/test/groovy/test/TestConnection.groovy index edfe44c7465a..9adebacaf3a4 100644 --- a/instrumentation/jdbc/javaagent/src/test/groovy/test/TestConnection.groovy +++ b/instrumentation/jdbc/javaagent/src/test/groovy/test/TestConnection.groovy @@ -41,7 +41,7 @@ class TestConnection implements Connection { @Override PreparedStatement prepareStatement(String sql) throws SQLException { - return null + return new TestPreparedStatement(this) } @Override diff --git a/instrumentation/jdbc/javaagent/src/test/groovy/test/TestPreparedStatement.groovy b/instrumentation/jdbc/javaagent/src/test/groovy/test/TestPreparedStatement.groovy new file mode 100644 index 000000000000..5f2b4f1a02da --- /dev/null +++ b/instrumentation/jdbc/javaagent/src/test/groovy/test/TestPreparedStatement.groovy @@ -0,0 +1,304 @@ +/* + * Copyright The OpenTelemetry Authors + * SPDX-License-Identifier: Apache-2.0 + */ + +package test + +import java.sql.Array +import java.sql.Blob +import java.sql.Clob +import java.sql.Connection +import java.sql.Date +import java.sql.NClob +import java.sql.ParameterMetaData +import java.sql.PreparedStatement +import java.sql.Ref +import java.sql.ResultSet +import java.sql.ResultSetMetaData +import java.sql.RowId +import java.sql.SQLException +import java.sql.SQLXML +import java.sql.Time +import java.sql.Timestamp + +class TestPreparedStatement extends TestStatement implements PreparedStatement { + TestPreparedStatement(Connection connection) { + super(connection) + } + + @Override + ResultSet executeQuery() throws SQLException { + return null + } + + @Override + int executeUpdate() throws SQLException { + return 0 + } + + @Override + void setNull(int parameterIndex, int sqlType) throws SQLException { + + } + + @Override + void setBoolean(int parameterIndex, boolean x) throws SQLException { + + } + + @Override + void setByte(int parameterIndex, byte x) throws SQLException { + + } + + @Override + void setShort(int parameterIndex, short x) throws SQLException { + + } + + @Override + void setInt(int parameterIndex, int x) throws SQLException { + + } + + @Override + void setLong(int parameterIndex, long x) throws SQLException { + + } + + @Override + void setFloat(int parameterIndex, float x) throws SQLException { + + } + + @Override + void setDouble(int parameterIndex, double x) throws SQLException { + + } + + @Override + void setBigDecimal(int parameterIndex, BigDecimal x) throws SQLException { + + } + + @Override + void setString(int parameterIndex, String x) throws SQLException { + + } + + @Override + void setBytes(int parameterIndex, byte[] x) throws SQLException { + + } + + @Override + void setDate(int parameterIndex, Date x) throws SQLException { + + } + + @Override + void setTime(int parameterIndex, Time x) throws SQLException { + + } + + @Override + void setTimestamp(int parameterIndex, Timestamp x) throws SQLException { + + } + + @Override + void setAsciiStream(int parameterIndex, InputStream x, int length) throws SQLException { + + } + + @Override + void setUnicodeStream(int parameterIndex, InputStream x, int length) throws SQLException { + + } + + @Override + void setBinaryStream(int parameterIndex, InputStream x, int length) throws SQLException { + + } + + @Override + void clearParameters() throws SQLException { + + } + + @Override + void setObject(int parameterIndex, Object x, int targetSqlType) throws SQLException { + + } + + @Override + void setObject(int parameterIndex, Object x) throws SQLException { + + } + + @Override + boolean execute() throws SQLException { + return false + } + + @Override + void addBatch() throws SQLException { + + } + + @Override + void setCharacterStream(int parameterIndex, Reader reader, int length) throws SQLException { + + } + + @Override + void setRef(int parameterIndex, Ref x) throws SQLException { + + } + + @Override + void setBlob(int parameterIndex, Blob x) throws SQLException { + + } + + @Override + void setClob(int parameterIndex, Clob x) throws SQLException { + + } + + @Override + void setArray(int parameterIndex, Array x) throws SQLException { + + } + + @Override + ResultSetMetaData getMetaData() throws SQLException { + return null + } + + @Override + void setDate(int parameterIndex, Date x, Calendar cal) throws SQLException { + + } + + @Override + void setTime(int parameterIndex, Time x, Calendar cal) throws SQLException { + + } + + @Override + void setTimestamp(int parameterIndex, Timestamp x, Calendar cal) throws SQLException { + + } + + @Override + void setNull(int parameterIndex, int sqlType, String typeName) throws SQLException { + + } + + @Override + void setURL(int parameterIndex, URL x) throws SQLException { + + } + + @Override + ParameterMetaData getParameterMetaData() throws SQLException { + return null + } + + @Override + void setRowId(int parameterIndex, RowId x) throws SQLException { + + } + + @Override + void setNString(int parameterIndex, String value) throws SQLException { + + } + + @Override + void setNCharacterStream(int parameterIndex, Reader value, long length) throws SQLException { + + } + + @Override + void setNClob(int parameterIndex, NClob value) throws SQLException { + + } + + @Override + void setClob(int parameterIndex, Reader reader, long length) throws SQLException { + + } + + @Override + void setBlob(int parameterIndex, InputStream inputStream, long length) throws SQLException { + + } + + @Override + void setNClob(int parameterIndex, Reader reader, long length) throws SQLException { + + } + + @Override + void setSQLXML(int parameterIndex, SQLXML xmlObject) throws SQLException { + + } + + @Override + void setObject(int parameterIndex, Object x, int targetSqlType, int scaleOrLength) throws SQLException { + + } + + @Override + void setAsciiStream(int parameterIndex, InputStream x, long length) throws SQLException { + + } + + @Override + void setBinaryStream(int parameterIndex, InputStream x, long length) throws SQLException { + + } + + @Override + void setCharacterStream(int parameterIndex, Reader reader, long length) throws SQLException { + + } + + @Override + void setAsciiStream(int parameterIndex, InputStream x) throws SQLException { + + } + + @Override + void setBinaryStream(int parameterIndex, InputStream x) throws SQLException { + + } + + @Override + void setCharacterStream(int parameterIndex, Reader reader) throws SQLException { + + } + + @Override + void setNCharacterStream(int parameterIndex, Reader value) throws SQLException { + + } + + @Override + void setClob(int parameterIndex, Reader reader) throws SQLException { + + } + + @Override + void setBlob(int parameterIndex, InputStream inputStream) throws SQLException { + + } + + @Override + void setNClob(int parameterIndex, Reader reader) throws SQLException { + + } +}