Skip to content

Commit

Permalink
Remove calls to Statement.getUpdateCount (#1147)
Browse files Browse the repository at this point in the history 
Remove calls to getUpdateCount
  • Loading branch information
@SylvainJuge
SylvainJuge authored Apr 20, 2020
1 parent 110da2a commit 5f3a4f9
Show file tree
Hide file tree
Showing 7 changed files with 26 additions and 206 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.asciidoc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ This is not a breaking change, so former
* Fix breakdown metrics span sub-types {pull}1113[#1113]
* Fix flaky gRPC server instrumentation {pull}1122[#1122]
* Fix side effect of calling `Statement.getUpdateCount` more than once {pull}1139[#1139]
* Stop capturing JDBC affected rows count using `Statement.getUpdateCount` to prevent unreliable side-effects {pull}1147[#1147]
[[release-notes-1.x]]
Expand Down
77 changes: 8 additions & 69 deletions ...ins/apm-jdbc-plugin/src/main/java/co/elastic/apm/agent/jdbc/StatementInstrumentation.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -126,19 +126,6 @@ public static void onAfterExecute(@Advice.This Statement statement,
return;
}

if (t == null && jdbcHelperManager != null) {
JdbcHelper helper = jdbcHelperManager.getForClassLoaderOfClass(Statement.class);
if (helper != null) {
int count = helper.getAndStoreUpdateCount(statement);
if (count != Integer.MIN_VALUE) {
span.getContext()
.getDb()
.withAffectedRowsCount(count);
}
}

}

span.captureException(t)
.deactivate()
.end();
Expand Down Expand Up @@ -237,10 +224,10 @@ public static void storeSql(@Advice.This Statement statement, @Advice.Argument(0

/**
* Instruments:
* <ul>
* <li>{@link Statement#executeBatch()} </li>
* <li>{@link Statement#executeLargeBatch()} (java8)</li>
* </ul>
* <ul>
* <li>{@link Statement#executeBatch()} </li>
* <li>{@link Statement#executeLargeBatch()} (java8)</li>
* </ul>
*/
public static class ExecuteBatchInstrumentation extends StatementInstrumentation {
public ExecuteBatchInstrumentation(ElasticApmTracer tracer) {
Expand Down Expand Up @@ -306,10 +293,10 @@ public static void onAfterExecute(@Advice.Enter @Nullable Span span,

/**
* Instruments:
* <ul>
* <li>{@link PreparedStatement#executeUpdate()} </li>
* <li>{@link PreparedStatement#executeLargeUpdate()} ()} (java8)</li>
* </ul>
* <ul>
* <li>{@link PreparedStatement#executeUpdate()} </li>
* <li>{@link PreparedStatement#executeLargeUpdate()} ()} (java8)</li>
* </ul>
*/
public static class ExecuteUpdateNoQueryInstrumentation extends StatementInstrumentation {
public ExecuteUpdateNoQueryInstrumentation(ElasticApmTracer tracer) {
Expand Down Expand Up @@ -396,59 +383,11 @@ public static void onAfterExecute(@Advice.This Statement statement,
return;
}

if (t == null && jdbcHelperManager != null) {
JdbcHelper jdbcHelper = jdbcHelperManager.getForClassLoaderOfClass(Statement.class);
if (jdbcHelper != null) {
span.getContext()
.getDb()
.withAffectedRowsCount(jdbcHelper.getAndStoreUpdateCount(statement));
}
}

span.captureException(t)
.deactivate()
.end();
}

}


/**
* Instruments:
* <ul>
* <li>{@link Statement#getUpdateCount()}</li>
* </ul>
*/
public static class GetUpdateCountInstrumentation extends StatementInstrumentation {

public GetUpdateCountInstrumentation(ElasticApmTracer tracer) {
super(tracer,
named("getUpdateCount")
.and(takesArguments(0))
.and(isPublic())
);
}

@Advice.OnMethodExit(suppress = Throwable.class, onThrowable = Throwable.class)
private static void onExit(@Advice.This Statement statement,
@Advice.Thrown @Nullable Throwable thrown,
@Advice.Return(readOnly = false) int returnValue) {

if (tracer == null || jdbcHelperManager == null) {
return;
}

JdbcHelper helperImpl = jdbcHelperManager.getForClassLoaderOfClass(Statement.class);
if (helperImpl == null) {
return;
}

int storedValue = helperImpl.getAndClearStoredUpdateCount(statement);

if (thrown == null && storedValue != Integer.MIN_VALUE) {
returnValue = storedValue;
}

}
}
}
20 changes: 0 additions & 20 deletions ...nt-plugins/apm-jdbc-plugin/src/main/java/co/elastic/apm/agent/jdbc/helper/JdbcHelper.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -69,24 +69,4 @@ public String retrieveSqlForStatement(Object statement) {
@Nullable
public abstract Span createJdbcSpan(@Nullable String sql, Object statement, @Nullable TraceContextHolder<?> parent, boolean preparedStatement);

/**
* Safely wraps calls to {@link java.sql.Statement#getUpdateCount()} and stores last result.
* <p>
* getUpdateCount javadoc indicates that this method should be called only once.
* In practice, adding this extra call seem to not have noticeable side effects on most databases but Oracle.
* </p>
*
* @param statement {@code java.sql.Statement} instance
* @return {@link Integer#MIN_VALUE} if statement does not support this feature, returned value otherwise
*/
public abstract int getAndStoreUpdateCount(Object statement);

/**
* Get and clears stored update count (if any) for a given statement.
*
* @param statement {@code java.sql.Statement} instance
* @return {@link Integer#MIN_VALUE} if there is no stored value for this statement
*/
public abstract int getAndClearStoredUpdateCount(Object statement);

}
58 changes: 0 additions & 58 deletions ...lugins/apm-jdbc-plugin/src/main/java/co/elastic/apm/agent/jdbc/helper/JdbcHelperImpl.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -52,16 +52,9 @@ public class JdbcHelperImpl extends JdbcHelper {
// have the expected behavior, thus, any direct reference to `JdbcHelperImpl` should only be obtained from the
// HelperClassManager<JdbcHelper> instance.
private final WeakConcurrentMap<Connection, ConnectionMetaData> metaDataMap = DataStructures.createWeakConcurrentMapWithCleanerThread();
private final WeakConcurrentMap<Class<?>, Boolean> updateCountSupported = new WeakConcurrentMap.WithInlinedExpunction<Class<?>, Boolean>();
private final WeakConcurrentMap<Class<?>, Boolean> metadataSupported = new WeakConcurrentMap.WithInlinedExpunction<Class<?>, Boolean>();
private final WeakConcurrentMap<Class<?>, Boolean> connectionSupported = new WeakConcurrentMap.WithInlinedExpunction<Class<?>, Boolean>();

// keeps track of non-idempotent implementations of getUpdateCount
private final WeakConcurrentMap<Class<?>, Boolean> idempotentGetUpdateCount = new WeakConcurrentMap.WithInlinedExpunction<Class<?>, Boolean>();

// stores statements update count to avoid side-effects of calling Statement.getUpdateCount()
private final WeakConcurrentMap<Object, Integer> statementsUpdateCount = new WeakConcurrentMap.WithInlinedExpunction<>();

@VisibleForAdvice
public final ThreadLocal<SignatureParser> SIGNATURE_PARSER_THREAD_LOCAL = new ThreadLocal<SignatureParser>() {
@Override
Expand All @@ -73,10 +66,8 @@ protected SignatureParser initialValue() {
@Override
public void clearInternalStorage() {
metaDataMap.clear();
updateCountSupported.clear();
metadataSupported.clear();
connectionSupported.clear();
statementsUpdateCount.clear();
}

@Override
Expand Down Expand Up @@ -189,55 +180,6 @@ private Connection safeGetConnection(Statement statement) {
return connection;
}


@Override
public int getAndStoreUpdateCount(Object statement) {
int result = Integer.MIN_VALUE;
if (!(statement instanceof Statement)) {
return result;
}

Class<?> type = statement.getClass();
Boolean supported = isSupported(updateCountSupported, type);
if (supported == Boolean.FALSE) {
return result;
}

try {
result = ((Statement) statement).getUpdateCount();
if (supported == null) {
markSupported(updateCountSupported, type);
}

// in order to minimize allocation, we keep track of implementation classes that are not idempotent,
// that allows avoiding extra work for the most common idempotent case.
Boolean isIdempotent = isSupported(idempotentGetUpdateCount, type);
if (isIdempotent == null) {
int secondResult = ((Statement) statement).getUpdateCount();
isIdempotent = (secondResult == result) ? Boolean.TRUE : Boolean.FALSE;
idempotentGetUpdateCount.put(type, isIdempotent);
}

if (isIdempotent != Boolean.TRUE) {
// save the value to be restored for the next call to getUpdateCount()
statementsUpdateCount.put(statement, result);
}

} catch (SQLException e) {
markNotSupported(updateCountSupported, type, e);
}
return result;
}

@Override
public int getAndClearStoredUpdateCount(Object statement) {
Integer value = null;
if (Boolean.FALSE == idempotentGetUpdateCount.get(statement.getClass())) {
value = statementsUpdateCount.remove(statement);
}
return value != null ? value : Integer.MIN_VALUE;
}

@Nullable
private static Boolean isSupported(WeakConcurrentMap<Class<?>, Boolean> featureMap, Class<?> type) {
return featureMap.get(type);
Expand Down
1 change: 0 additions & 1 deletion ...n/src/main/resources/META-INF/services/co.elastic.apm.agent.bci.ElasticApmInstrumentation
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,4 +5,3 @@ co.elastic.apm.agent.jdbc.StatementInstrumentation$ExecuteUpdateNoQueryInstrumen
co.elastic.apm.agent.jdbc.StatementInstrumentation$AddBatchInstrumentation
co.elastic.apm.agent.jdbc.StatementInstrumentation$ExecuteBatchInstrumentation
co.elastic.apm.agent.jdbc.StatementInstrumentation$ExecutePreparedStatementInstrumentation
co.elastic.apm.agent.jdbc.StatementInstrumentation$GetUpdateCountInstrumentation
67 changes: 17 additions & 50 deletions ...-jdbc-plugin/src/test/java/co/elastic/apm/agent/jdbc/AbstractJdbcInstrumentationTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -114,8 +114,8 @@ public void tearDown() throws SQLException {
@Test
public void test() throws SQLException {
executeTest(this::testStatement);
executeTest(this::testUpdateStatement);
executeTest(this::testStatementNotSupportingUpdateCount);
executeTest(() -> testUpdateStatement(false));
executeTest(() -> testUpdateStatement(true));
executeTest(this::testStatementNotSupportingConnection);
executeTest(this::testStatementWithoutConnectionMetadata);

Expand Down Expand Up @@ -179,43 +179,28 @@ private void testStatement() throws SQLException {
Statement statement = connection.createStatement();
ResultSet resultSet = statement.executeQuery(sql);

checkUpdateCount(statement, -1);

assertThat(resultSet.next()).isTrue();
assertThat(resultSet.getInt("foo")).isEqualTo(1);
assertThat(resultSet.getString("BAR")).isEqualTo("APM");

assertSpanRecorded(sql, false, -1);
}

private void testUpdateStatement() throws SQLException {
private void testUpdateStatement(boolean executeUpdate) throws SQLException {
final String sql = "UPDATE ELASTIC_APM SET BAR='AFTER' WHERE FOO=11";
Statement statement = connection.createStatement();
boolean isResultSet = statement.execute(sql);
assertThat(isResultSet).isFalse();

checkUpdateCount(statement, 1);

assertSpanRecorded(sql, false, 1);
}

private void testStatementNotSupportingUpdateCount() throws SQLException {
final String sql = "UPDATE ELASTIC_APM SET BAR='AFTER1' WHERE FOO=11";
TestStatement statement = new TestStatement(connection.createStatement());
statement.setGetUpdateCountSupported(false);
assertThat(statement.getUnsupportedThrownCount()).isZero();

boolean isResultSet = statement.execute(sql);
assertThat(statement.getUnsupportedThrownCount()).isEqualTo(1);
assertThat(isResultSet).isFalse();

assertSpanRecorded(sql, false, -1);
int expectedAffectedRows;
if (executeUpdate) {
statement.executeUpdate(sql);
// executeUpdate allows to capture affected rows without side-effects
expectedAffectedRows = 1;
} else {
boolean isResultSet = statement.execute(sql);
assertThat(isResultSet).isFalse();
expectedAffectedRows = -1;
}

// try to execute statement again, should not throw any exception
statement.execute(sql);
assertThat(statement.getUnsupportedThrownCount())
.describedAs("unsupported exception should only be thrown once")
.isEqualTo(1);
assertSpanRecorded(sql, false, expectedAffectedRows);
}

private void testStatementNotSupportingConnection() throws SQLException {
Expand All @@ -235,7 +220,7 @@ private void testStatementWithoutConnectionMetadata() throws SQLException {
checkWithoutConnectionMetadata(statement, testConnection::getUnsupportedThrownCount);
}

private interface ThrownCountCheck{
private interface ThrownCountCheck {
int getThrownCount();
}

Expand All @@ -247,7 +232,7 @@ private void checkWithoutConnectionMetadata(TestStatement statement, ThrownCount
assertThat(check.getThrownCount()).isEqualTo(1);
assertThat(isResultSet).isFalse();

assertSpanRecordedWithoutConnection(sql, false, 1);
assertSpanRecordedWithoutConnection(sql, false, -1);

// try to execute statement again, should not throw again
statement.execute(sql);
Expand Down Expand Up @@ -349,7 +334,7 @@ private void testUpdatePreparedStatement() throws SQLException {
if (updatePreparedStatement != null) {
updatePreparedStatement.setString(1, "UPDATED1");
updatePreparedStatement.execute();
assertSpanRecorded(UPDATE_PREPARED_STATEMENT_SQL, true, 1);
assertSpanRecorded(UPDATE_PREPARED_STATEMENT_SQL, true, -1);
}
}

Expand Down Expand Up @@ -399,11 +384,6 @@ private void testMultipleRowsModifiedStatement() throws SQLException {
assertThat(reporter.getSpans()).hasSize(1);
Db db = reporter.getFirstSpan().getContext().getDb();
assertThat(db.getStatement()).isEqualTo(insert);
if (!isKnownDatabase("Oracle", "")) {
assertThat(db.getAffectedRowsCount())
.isEqualTo(statement.getUpdateCount())
.isEqualTo(1);
}
}

private void assertQuerySucceededAndSpanRecorded(ResultSet resultSet, String rawSql, boolean preparedStatement) throws SQLException {
Expand Down Expand Up @@ -517,17 +497,4 @@ private static boolean executePotentiallyUnsupportedFeature(JdbcTask task) throw
return true;
}

/**
* Calls and asserts returned value of {@link Statement#getUpdateCount()}.
*
* @param statement statement
* @param expectedValue expected value
* @throws SQLException if something bad happens
*/
private static void checkUpdateCount(Statement statement, int expectedValue) throws SQLException {
int updateCount = statement.getUpdateCount();
assertThat(updateCount)
.describedAs("unexpected update count value")
.isEqualTo(expectedValue);
}
}
8 changes: 0 additions & 8 deletions apm-agent-plugins/apm-jdbc-plugin/src/test/java/co/elastic/apm/agent/jdbc/TestStatement.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,6 @@ class TestStatement implements Statement {
private final Statement delegate;

private boolean isGetConnectionSupported;
private boolean isGetUpdateCountSupported;
private int unsupportedThrownCount;

private Connection connection;
Expand All @@ -43,7 +42,6 @@ public TestStatement(Statement delegate) {
this.delegate = delegate;
this.unsupportedThrownCount = 0;
this.isGetConnectionSupported = true;
this.isGetUpdateCountSupported = true;
}

private void unsupportedCheck(boolean isFeatureSupported) throws SQLException {
Expand All @@ -57,10 +55,6 @@ public void setGetConnectionSupported(boolean supported) {
this.isGetConnectionSupported = supported;
}

public void setGetUpdateCountSupported(boolean supported) {
this.isGetUpdateCountSupported = supported;
}

int getUnsupportedThrownCount(){
return unsupportedThrownCount;
}
Expand All @@ -69,7 +63,6 @@ void setConnection(Connection connection) {
this.connection = connection;
}


public ResultSet executeQuery(String sql) throws SQLException {
return delegate.executeQuery(sql);
}
Expand Down Expand Up @@ -135,7 +128,6 @@ public ResultSet getResultSet() throws SQLException {
}

public int getUpdateCount() throws SQLException {
unsupportedCheck(isGetUpdateCountSupported);
return delegate.getUpdateCount();
}

Expand Down

0 comments on commit 5f3a4f9

Please sign in to comment.