Report affected rows for DML statements by SylvainJuge · Pull Request #883 · elastic/apm-agent-java

SylvainJuge · 2019-10-11T14:22:04Z

Fixes #707

Reports the number of rows affected by a DML statement (INSERT,DELETE,UPDATE) in database span.

Checklist

Implement code
Add tests
~~Update documentation~~
Update CHANGELOG.md
~~Update supported-technologies.asciidoc~~
~~Added an API method or config option? Document in which version this will be introduced.~~

codecov-io · 2019-10-11T14:31:49Z

Codecov Report

Merging #883 into master will decrease coverage by 0.41%.
The diff coverage is n/a.

@@             Coverage Diff              @@
##             master     #883      +/-   ##
============================================
- Coverage     65.13%   64.71%   -0.42%     
  Complexity       85       85              
============================================
  Files           232      231       -1     
  Lines          9878     9662     -216     
  Branches       1326     1282      -44     
============================================
- Hits           6434     6253     -181     
+ Misses         3050     3025      -25     
+ Partials        394      384      -10

Impacted Files	Coverage Δ	Complexity Δ
...ava/co/elastic/apm/agent/jmx/JmxMetricTracker.java	`77.33% <0%> (-9%)`	`0% <0%> (ø)`
...m/agent/util/DependencyInjectingServiceLoader.java	`77.77% <0%> (-7.41%)`	`0% <0%> (ø)`
...lastic/apm/agent/impl/ElasticApmTracerBuilder.java	`79.01% <0%> (-2.19%)`	`0% <0%> (ø)`
...va/co/elastic/apm/agent/impl/ElasticApmTracer.java	`72.14% <0%> (-2.1%)`	`0% <0%> (ø)`
...c/apm/agent/context/AbstractLifecycleListener.java
...co/elastic/apm/agent/report/ApmServerReporter.java	`60.95% <0%> (+3.8%)`	`0% <0%> (ø)`	⬇️
...va/co/elastic/apm/agent/report/ReportingEvent.java	`96.66% <0%> (+10%)`	`0% <0%> (ø)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c8309f8...763a4a3. Read the comment docs.

felixbarny

Looks good overall. Some minor comments. Also, please format the code you added.

felixbarny · 2019-10-12T07:49:25Z

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

+                        for (int i = 0; i < array.length; i++) {
+                            affectedCount += array[i];
+                        }
+                    }


Extract public static method, annotate with @VisibleForAdvice and reuse in ExecuteWithoutQueryInstrumentation. As only Java core types are used, there are no classloader issues.

I don't get that- those APIs return either int or long, not arrays...
When fixing, you will need to create two separate advices.

should be fixed now

eyalkoren · 2019-10-16T05:49:11Z

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

+                                          @Advice.Thrown @Nullable Throwable t) throws SQLException {
+            if (span != null) {
+                if( t == null){
+                    span.getContext().getDb().withAffectedRowsCount(statement.getUpdateCount());


The javadoc for getUpdateCount says: "This method should be called only once per result". I believe this means we cannot call it here.

eyalkoren · 2019-10-16T06:21:20Z

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

+        Db db = reporter.getFirstSpan().getContext().getDb();
+        assertThat(db.getStatement()).isEqualTo(insert);
+        assertThat(db.getAffectedRowsCount())
+            .isEqualTo(statement.getUpdateCount())


Hmm, it seems all drivers DO support multiple invocations of getUpdateCount despite the javadoc. Still not sure if it is safe enough to use it...

eyalkoren · 2019-10-16T06:22:11Z

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

+                        for (int i = 0; i < array.length; i++) {
+                            affectedCount += array[i];
+                        }
+                    }


I don't get that- those APIs return either int or long, not arrays...
When fixing, you will need to create two separate advices.

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

eyalkoren · 2019-10-16T06:48:57Z

Also - this new value should be serialized into DB span events as part of the implementation of elastic/apm#112

Adding a label for each datasource provider makes it easier to know which datasource connection pool is being tested

eyalkoren

Awesome!! Well done!
Only not 100% about invoking getUpdateCount() in the instrumentation...
Otherwise LGTM, so approving and leave that to you.

eyalkoren · 2019-10-24T07:01:38Z

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

+
+        long expectedAffected = 2;
+        if (isKnownDatabase("MySQL", "10.")) {
+            // for an unknown reason mariadb 10 has unexpected but somehow consistent behavior here


The javadoc says:

* <LI>A value of <code>SUCCESS_NO_INFO</code> -- indicates that the command was * processed successfully but that the number of rows affected is * unknown

where the constant java.sql.Statement#SUCCESS_NO_INFO is -2.

eyalkoren · 2019-10-24T07:16:10Z

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

+                span.getContext()
+                    .getDb()
+                    // getUpdateCount javadoc indicates that this method should be called only once
+                    // however in practice adding this extra call seem to not have noticeable side effects


I am still not sure about that. On the one hand, the testMultipleRowsModifiedStatement passes, so should be OK for at least most of the cases. On the other hand, breaking the application in the other rare cases is embarrassing...
@felixbarny WDYT? Does the added value of this field worth the risk?

I have found this SO post which seems relevant: https://stackoverflow.com/questions/9984468/getresultset-should-be-called-only-once-per-result

Yes, with ResulstSets that hold open cursors, concurrent usages can be very unpleasant. It would even make sense to enforce not allowing multiple invocations of this API.
However, the update count can be easily cached and I assume that's what most drivers do, so not sure why it has the same limitation.

SylvainJuge added 2 commits October 11, 2019 16:03

capture affected rows by statement

b2f9c11

update changelog

4222878

SylvainJuge requested a review from eyalkoren October 11, 2019 14:25

SylvainJuge added [zube]: In Progress and removed [zube]: In Progress labels Oct 11, 2019

felixbarny reviewed Oct 12, 2019

View reviewed changes

eyalkoren suggested changes Oct 16, 2019

View reviewed changes

SylvainJuge added 3 commits October 18, 2019 16:02

improve naming for jdbc datasource tests

86c852d

Adding a label for each datasource provider makes it easier to know which datasource connection pool is being tested

improve testing & fix impl.

fe456d9

Merge branch 'master' into jdbc-capture-modified-records

763a4a3

SylvainJuge requested review from eyalkoren and felixbarny October 24, 2019 07:01

eyalkoren approved these changes Oct 24, 2019

View reviewed changes

SylvainJuge merged commit b258d29 into elastic:master Oct 24, 2019

SylvainJuge deleted the jdbc-capture-modified-records branch October 24, 2019 14:55

zube bot added [zube]: Done and removed [zube]: In Review labels Oct 24, 2019

mdindoffer mentioned this pull request Nov 7, 2019

[Regression] Agent fails to create spans for DML SQL commands #917

Closed

lreuven removed the [zube]: Done label Nov 18, 2019

Conversation

SylvainJuge commented Oct 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

codecov-io commented Oct 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

felixbarny left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

eyalkoren commented Oct 16, 2019

Uh oh!

eyalkoren left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

SylvainJuge commented Oct 11, 2019 •

edited

Loading

codecov-io commented Oct 11, 2019 •

edited

Loading