Avoid side-effects of calling Statement.getUpdateCount

[SylvainJuge]

What does this PR do?

Current instrumentation of java.sql.Statement calls Statement.getUpdateCount() just after SQL statement execution in order to retrieve the number of modified rows by the statement.

While there is a warning in Javadoc about calling this method only once, in practice most implementations allow to call it multiple times without any problem. We had integration tests that covered most common databases thus we were confident on that hypothesis.

However, it seems that some database servers (or JDBC drivers) like Oracle strictly follow the specification and just allow for a single call to getUpdateCount. As a result, our agent would capture the first value, and then an invalid value would then be returned.

Impacted applications would be those for which both statements are true

• call Statement.getUpdateCount()
• use Oracle database (or one that have similar behavior)

Checklist

• My code follows the style guidelines of this project
• I have rebased my changes on top of the latest master branch
• I have made corresponding changes to the documentation
• [ x I have added tests that prove my fix is effective or that my feature works
• New and existing unit tests pass locally with my changes
• I have updated CHANGELOG.asciidoc
• I have updated supported-technologies.asciidoc
• Added an API method or config option? Document in which version this will be introduced
• Added an instrumentation plugin? How did you make sure that old, non-supported versions are not instrumented by accident?

Author's Checklist

• find an usable public oracle DB image for integration tests
• run benchmarks (especially allocation rate) and ensure that there isn't too much extra overhead.

[SylvainJuge]

Allocation rate is definitely higher with this change:

• about 0.05 Mb/s without it
• about 2.3 Mb/s with it

[SylvainJuge]

Detecting non-idempotent implementations at runtime allows to avoid this workaround for most implementations, which allows to keep our very low allocation rate 🎉

[felixbarny]

I wonder if we should disable update count monitoring for non-idempotent drivers but I don't have a strong preference.

[felixbarny]

Enter advice not needed when moving this to exit advice?

[SylvainJuge]

Good point, there would be no difference to move this to the exit advice.

However, we could bypass the original method call when we have a stored value, just in case there are other side-effects of calling getUpdateCount further in the driver or DB implementation.

[SylvainJuge]

I've finally took the "move everything to the exit advice" path, we can still improve this implementation later only if needed, as skipping default method implementation brings some extra complexity, especially on testing side.