-
Notifications
You must be signed in to change notification settings - Fork 828
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 #4974
Comments
On the one hand, this is by design: we do want to suppress nested instrumentations that would result in nested spans with same semantic conventions, e.g. nested HTTP or DB client spans. On the other hand, there is #3965 to give end-users an option to disable this suppression. Even after implementing #3965 this specific use-case will remain unsolved, because it uses different way of suppression. All in all, I think this is a valid request, but we should solve using usual semantic suppression, not by the specific change suggested. |
Although I do understand, however even if I exclude the top-level JDBC driver from instrumentation I don't get the underlying drivers to be instrumented because of this secondary suppression mechanism which leads me to believe this is incorrect behaviour. I think regardless of #3965 that this particular situation should be addressed anyway and I understand my proposal for fix might not be the best one though. |
@iNikem , would you please give me some details on your vision and I'll do my best to submit a PR aligned with that vision. |
@marcobjorge I took a closer look and my assumption was wrong. We already implement semantic suppression there and the code you referenced explicitly says why is it needed in addition to the semantic suppression. I have some doubts that your suggestion is the best final solution, but it seems like a good enough step in the right direction. So if that works for you, go ahead and submit a PR for this :) @mateuszrzeszutek WDYT? |
Yeah, I think the proposed solution is a step in a good direction - you'd still have to turn off the semantic suppression though. |
See pull/5004. |
In my tests was just not excluding ALL Calcite This was caused by my incorrect usage of the javaagent - the JDBC driver call depth calculation is correct and nothing needs to be fixed. |
@marcobjorge thanks for the insights! it would definitely be nice if the javaagent can capture the "best" telemetry out-of-the-box without additional configuration. I've opened #5012 to track this further. |
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 on the concrete class being instrumented rather than the generic JDBC interface.
Something like:
The text was updated successfully, but these errors were encountered: