Remove calls to Statement.getUpdateCount (#1147)

Remove calls to getUpdateCount

Author: SylvainJuge

CHANGELOG.asciidoc

@@ -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]]

apm-agent-plugins/apm-jdbc-plugin/src/main/java/co/elastic/apm/agent/jdbc/StatementInstrumentation.java

@@ -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();
@@ -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) {
@@ -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) {
@@ -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;
- }
-
- }
- }
 }

apm-agent-plugins/apm-jdbc-plugin/src/main/java/co/elastic/apm/agent/jdbc/helper/JdbcHelper.java

@@ -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);
-
 }

apm-agent-plugins/apm-jdbc-plugin/src/main/java/co/elastic/apm/agent/jdbc/helper/JdbcHelperImpl.java

@@ -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
@@ -73,10 +66,8 @@ protected SignatureParser initialValue() {
  @Override
  public void clearInternalStorage() {
  metaDataMap.clear();
- updateCountSupported.clear();
  metadataSupported.clear();
  connectionSupported.clear();
- statementsUpdateCount.clear();
  }
 
  @Override
@@ -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);

apm-agent-plugins/apm-jdbc-plugin/src/main/resources/META-INF/services/co.elastic.apm.agent.bci.ElasticApmInstrumentation

@@ -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

apm-agent-plugins/apm-jdbc-plugin/src/test/java/co/elastic/apm/agent/jdbc/AbstractJdbcInstrumentationTest.java

@@ -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);
 
@@ -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 {
@@ -235,7 +220,7 @@ private void testStatementWithoutConnectionMetadata() throws SQLException {
  checkWithoutConnectionMetadata(statement, testConnection::getUnsupportedThrownCount);
  }
 
- private interface ThrownCountCheck{
+ private interface ThrownCountCheck {
  int getThrownCount();
  }
 
@@ -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);
@@ -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);
  }
  }
 
@@ -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 {
@@ -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);
- }
 }

apm-agent-plugins/apm-jdbc-plugin/src/test/java/co/elastic/apm/agent/jdbc/TestStatement.java

@@ -34,7 +34,6 @@ class TestStatement implements Statement {
  private final Statement delegate;
 
  private boolean isGetConnectionSupported;
- private boolean isGetUpdateCountSupported;
  private int unsupportedThrownCount;
 
  private Connection connection;
@@ -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 {
@@ -57,10 +55,6 @@ public void setGetConnectionSupported(boolean supported) {
  this.isGetConnectionSupported = supported;
  }
 
- public void setGetUpdateCountSupported(boolean supported) {
- this.isGetUpdateCountSupported = supported;
- }
-
  int getUnsupportedThrownCount(){
  return unsupportedThrownCount;
  }
@@ -69,7 +63,6 @@ void setConnection(Connection connection) {
  this.connection = connection;
  }
 
-
  public ResultSet executeQuery(String sql) throws SQLException {
  return delegate.executeQuery(sql);
  }
@@ -135,7 +128,6 @@ public ResultSet getResultSet() throws SQLException {
  }
 
  public int getUpdateCount() throws SQLException {
- unsupportedCheck(isGetUpdateCountSupported);
  return delegate.getUpdateCount();
  }