Skip to content

Conversation

@felixbarny
Copy link
Member

@felixbarny felixbarny commented May 14, 2018

closes #35

@codecov-io
Copy link

codecov-io commented May 14, 2018

Codecov Report

Merging #84 into master will decrease coverage by 0.31%.
The diff coverage is 82.89%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master      #84      +/-   ##
============================================
- Coverage     74.87%   74.55%   -0.32%     
- Complexity      779      793      +14     
============================================
  Files            87       90       +3     
  Lines          2901     2975      +74     
  Branches        278      283       +5     
============================================
+ Hits           2172     2218      +46     
- Misses          594      615      +21     
- Partials        135      142       +7
Impacted Files Coverage Δ Complexity Δ
...java/co/elastic/apm/jdbc/ApmJdbcEventListener.java 52.3% <74.07%> (-9.92%) 8 <4> (-2)
...tic/apm/jdbc/PreparedStatementInstrumentation.java 82.35% <82.35%> (ø) 6 <6> (?)
.../co/elastic/apm/jdbc/StatementInstrumentation.java 87.5% <87.5%> (ø) 7 <7> (?)
...co/elastic/apm/jdbc/ConnectionInstrumentation.java 93.75% <93.75%> (ø) 6 <6> (?)
.../elastic/apm/jdbc/ApmJdbcEventListenerFactory.java 0% <0%> (-100%) 0% <0%> (-3%)
...lastic/apm/servlet/FilterChainInstrumentation.java 27.27% <0%> (-2.73%) 5% <0%> (ø)
...co/elastic/apm/servlet/ServletInstrumentation.java 31.57% <0%> (-0.86%) 5% <0%> (ø)
... and 1 more

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 274b42c...f711c8c. Read the comment docs.

closes elastic#35

Signed-off-by: Felix Barnsteiner <felix.barnsteiner@elastic.co>
@Override
public ElementMatcher<? super TypeDescription> getTypeMatcher() {
return not(isInterface())
.and(nameContains("Statement"))

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just out of curiosity, why was this needed? Is this a performance optimization to avoid the isSubTypeOf check?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See the comment in https://github.com/elastic/apm-agent-java/blob/master/apm-agent-plugins/apm-servlet-plugin/src/main/java/co/elastic/apm/servlet/ServletInstrumentation.java#L69
Basically it’s because the isSubType marcher can be pretty expensive. It causes a lot of type descriptions to be parsed by ByteBuddy. I stagemonitor, I cached TypeDescriptions to improve instrumentation performance (startup times). A user experienced an OOME because of an isSubType matcher. But I should re evaluate with a recent Byte Buddy version.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is your experience in regards to the performance of the isSubType(Class) and hasSuperType(named(String)) matchers?

Signed-off-by: Felix Barnsteiner <felix.barnsteiner@elastic.co>
@felixbarny felixbarny force-pushed the jdbc-instrumentation branch from a1f8ece to f711c8c Compare May 15, 2018 07:51
@felixbarny
Copy link
Member Author

TODO: more integration tests

@felixbarny felixbarny merged commit 02da44a into elastic:master May 15, 2018
@felixbarny felixbarny deleted the jdbc-instrumentation branch May 15, 2018 07:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Zero garbage JDBC tracing

4 participants