Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Address various JDBC instrumentation issues #2561

Merged
merged 4 commits into from
Mar 31, 2021
Merged

Conversation

richardstartin
Copy link
Member

@richardstartin richardstartin commented Mar 30, 2021

This PR makes a few changes to the JDBC instrumentation

  1. Caches connection URL parsing since some applications connect frequently, the parsing is quite slow, and connecting often takes a driver level lock. This should fix an issue for Apache Phoenix users where connection creation is more common than in applications using traditional connection pools.
  2. Separates out and namespaces DB2 Connection and PreparedStatement instrumentations, which have expensive type matchers. This means that unless DB2 is being used, JDBC matching is about as cheap as it can get.
  3. Add Redshift's CallableStatement implementation, which was missed in Add support for redshift JDBC #2511

@richardstartin richardstartin requested a review from a team as a code owner March 30, 2021 11:11
@richardstartin richardstartin force-pushed the rgs/jdbc-issues branch 2 times, most recently from ebea00f to deb7546 Compare March 30, 2021 11:48
@@ -1,6 +1,7 @@
muzzle {
pass {
coreJdk()
extraDependency 'com.ibm.db2:jcc:11.1.4.4'
Copy link
Member Author

Choose a reason for hiding this comment

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

Adding this allows to muzzle the instrumentation now that DB2 is matched separately from the rest for startup performance reasons.

@richardstartin richardstartin changed the title JDBC instrumentation issues Address various JDBC instrumentation issues Mar 30, 2021
Copy link
Contributor

@bantonsson bantonsson left a comment

Choose a reason for hiding this comment

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

👍

@@ -84,15 +88,18 @@ public AgentSpan onConnection(
*/
{
Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, it's not relevant to this PR, but why is there an extra { indentation here?

@richardstartin
Copy link
Member Author

I think it's actually possible to do this another way without needing to trust unwrapping or to know anything but the connection names, which would simplify both statement and prepared statement instrumentations, which would just be to inject the DBInfo into the statements when they are created, and if we can't find DBInfo, we don't span the execute call, because it means the statement was not created by one of the connection types we target.

@richardstartin
Copy link
Member Author

@bantonsson I have dropped the statement instrumentation changes and the recursive connection unwrapping because I want to solve this problem a different way later.

@richardstartin richardstartin merged commit d57f678 into master Mar 31, 2021
@richardstartin richardstartin deleted the rgs/jdbc-issues branch March 31, 2021 10:10
@github-actions github-actions bot added this to the 0.78.0 milestone Mar 31, 2021
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.

None yet

3 participants