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

Fix multi-layer JDBC driver call depth calculation #5004

Closed
wants to merge 1 commit into from

Conversation

marcobjorge
Copy link

The #2756 request introduced a check on the call depth of the JDBC instrumentation that limits tracing with multi-layered drivers. For example, with Apache Calcite, the Calcite JDBC driver is caught on the call stack and none of the underlying connections are traced.

For a better support of different use cases the check should be anchored to the specific connection instead of the generic JDBC interface.

@linux-foundation-easycla
Copy link

CLA Not Signed

@iNikem
Copy link
Contributor

iNikem commented Jan 4, 2022

@marcobjorge please sign CLA

@marcobjorge
Copy link
Author

@iNikem I'm on it, but I need to jump some hoops in my org first though (corporate contributor).

@laurit
Copy link
Contributor

laurit commented Jan 4, 2022

@marcobjorge just curious, would it work if you'd use -Dotel.javaagent.exclude-classes to exclude calcite classes from instrumentation when running without this change?
Perhaps an alternative would be to fix #2756 in a slightly different way. Would it work if we surrounded

with a thread local (or CallDepth) and only suppressed statement instrumentation when the call originates from there? Then you wouldn't need to worry whether statement.getConnection() throws exception or returns a new proxy on each call.

@marcobjorge
Copy link
Author

@laurit I was sure it could not be done with -Dotel.javaagent.exclude-classes, but after your ask I set out to prove it and arrived at the conclusion it can and this PR is not needed (oopsy and thank you!).

TLDR: In my tests was just not excluding ALL Calcite java.sql.{Driver,Connection,Statement,PreparedStatement} in the classpath and the javaagent instrumentation was picking up something it shouldn't and therefore instrumenting the top level driver instead of the underlying drivers.

@marcobjorge marcobjorge closed this Jan 4, 2022
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.

3 participants