Database queries tracing with datasource-proxy.#1095
Conversation
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
|
One thing that I am not convinced about are the two new starters: 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:
I think it's safer in long run. What do you think @bruno-garcia @marandaneto? |
|
Yeah I agree it's not ideal to bring those dependencies directly. But we surely should have the core of the code and main packages dependency-free, as much as possible. |
|
Lets discuss this further before merging |
|
Please consider it a draft for now as there are quite a few things we need to adjust. |
|
@bruno-garcia @marandaneto please take another look. I updated the description to reflect the changes in the PR. |
sentry-p6spy/src/main/java/io/sentry/p6spy/SentryJdbcEventListener.java
Outdated
Show resolved
Hide resolved
sentry-p6spy/src/main/java/io/sentry/p6spy/SentryJdbcEventListener.java
Outdated
Show resolved
Hide resolved
| final ISpan parent = hub.getSpan(); | ||
| if (parent != null) { | ||
| final ISpan span = parent.startChild("db.query", resolveSpanDescription(queryInfoList)); | ||
| spans.put(execInfo.getConnectionId(), span); |
There was a problem hiding this comment.
would be possible that there are multiple spans for a single connectionId?
There was a problem hiding this comment.
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..?
There was a problem hiding this comment.
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
...ng-boot-starter/src/main/java/io/sentry/spring/boot/jdbc/SentryDsProxyAutoConfiguration.java
Outdated
Show resolved
Hide resolved
...oot-starter/src/test/kotlin/io/sentry/spring/boot/jdbc/SentryDsProxyAutoConfigurationTest.kt
Show resolved
Hide resolved
...oot-starter/src/test/kotlin/io/sentry/spring/boot/jdbc/SentryDsProxyAutoConfigurationTest.kt
Show resolved
Hide resolved
...-boot-starter/src/test/kotlin/io/sentry/spring/boot/jdbc/SentryP6SpyAutoConfigurationTest.kt
Show resolved
Hide resolved
|
@maciejwalkowiak 1st pass is done, a few points to work on but overall pretty good, thanks! |
|
Docs issue getsentry/sentry-docs#3337 |
sentry-datasource-proxy/src/main/java/io/sentry/dsproxy/SentryQueryExecutionListener.java
Show resolved
Hide resolved
sentry-samples/sentry-samples-spring-boot/src/main/resources/application.properties
Show resolved
Hide resolved
marandaneto
left a comment
There was a problem hiding this comment.
left a question, but other than that, LGTM, well done
This reverts commit 972a01b.
|
Is there a plan to create a new merge request to get this fixed and merged back in? |
📢 Type of change
📜 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-decoratorproject, 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