Database queries tracing with datasource-proxy.

[maciejwalkowiak]

📢 Type of change

• Bugfix
• New feature
• Enhancement
• Refactoring

📜 Description

Database queries tracing providing hooks for 2 industry standard ways to proxy JDBC DataSource: datasource-proxy and p6spy.

Since Spring Boot does not provide out of the box integration with p6spy nor datasource-proxy, in the sample project we use https://github.com/gavlyukovskiy/spring-boot-data-source-decorator - and I also believe we should link this project in the docs.

I've decided to not provide a dedicated starters for p6spy or datasource-proxy that would depend on the spring-boot-data-source-decorator project, as this is a 3rd party project and very likely Spring eventually will come up with 1st class citizen proxying solution working for Spring Framework and Spring Boot.

💡 Motivation and Context

Database queries are often the bottlenecks so having an option to trace it easily is a must.

💚 How did you test it?

Integration test, updated sample.

📝 Checklist

• I reviewed submitted code
• I added tests to verify changes
• All tests passing
• No breaking changes

[codecov-io]

Codecov Report

Merging #1095 (5a84409) into main (fc4b457) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##               main    #1095   +/-   ##
=========================================
  Coverage     73.68%   73.69%           
- Complexity     1553     1556    +3     
=========================================
  Files           162      164    +2     
  Lines          5540     5542    +2     
  Branches        563      562    -1     
=========================================
+ Hits           4082     4084    +2     
  Misses         1171     1171           
  Partials        287      287           

Impacted Files	Coverage Δ	Complexity Δ
...source/dsproxy/SentryDsProxyAutoConfiguration.java	100.00% <100.00%> (ø)	2.00 <2.00> (?)
...datasource/p6spy/SentryP6SpyAutoConfiguration.java	100.00% <100.00%> (ø)	2.00 <2.00> (?)
sentry/src/main/java/io/sentry/Sentry.java	41.86% <0.00%> (-0.89%)	17.00% <0.00%> (-1.00%)
sentry/src/main/java/io/sentry/SentryOptions.java	83.84% <0.00%> (ø)	106.00% <0.00%> (ø%)

---

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 fc4b457...5a84409. Read the comment docs.

[maciejwalkowiak]

One thing that I am not convinced about are the two new starters: sentry-spring-boot-starter-p6spy and sentry-spring-boot-starter-datasource-proxy. With them, in order to use Sentry error handling, tracing in general and tracing database calls, users would have to add just a single dependency, but both of them depend on the 3rd party: com.github.gavlyukovskiy:datasource-proxy-spring-boot-starter:1.6.2 which works well, but I am not sure if we want to tie ourselves so much to a 3rd party starter with unknown future.

If we get rid of starters, we could compensate it with docs saying that in order to use database tracing (for example with p6spy), you need to add:

• io.sentry:sentry-spring-boot-starter
• io.sentry:sentry-p6spy

And configure p6spy to use the class we provide in sentry-p6spy package.

To make it easier, we recommend using com.github.gavlyukovskiy:datasource-proxy-spring-boot-starter:1.6.2.

I think it's safer in long run. What do you think @bruno-garcia @marandaneto?

[bruno-garcia]

Yeah I agree it's not ideal to bring those dependencies directly.
We could have a meta-package sentry-performance-all that would drag everything in without worry of dependency which could serve as a good entry-level, demo, or "I don't care about dependencies" alternative.

But we surely should have the core of the code and main packages dependency-free, as much as possible.

[bruno-garcia]

Lets discuss this further before merging

[maciejwalkowiak]

Please consider it a draft for now as there are quite a few things we need to adjust.

[maciejwalkowiak]

@bruno-garcia @marandaneto please take another look. I updated the description to reflect the changes in the PR.

[marandaneto]

would be possible that there are multiple spans for a single connectionId?

[maciejwalkowiak]

If they are executed in single statement or batch then I don't think we can create multiple queries. Also, query itself does not have an identifier so we would need to calculate hash from parameters to match begin and end..?

[marandaneto]

yeah I was just wondering how this lib batch them (as its a list), think about the use case, select A and later select B, the library decides to match them within the List<QueryInfo> and we won't be able to capture spans for each of them but together, this would affect the average time of the span when doing select A and select B separately.
but as I don't know how the QueryInfo is batched, this is just an assumption

[marandaneto]

@maciejwalkowiak 1st pass is done, a few points to work on but overall pretty good, thanks!

[marandaneto]

Docs issue getsentry/sentry-docs#3337

[marandaneto]

left a question, but other than that, LGTM, well done

[tlf30]

Is there a plan to create a new merge request to get this fixed and merged back in?