Add R2dbc statement javaagent instrumentation by Crustack · Pull Request #7977 · open-telemetry/opentelemetry-java-instrumentation

Crustack · 2023-03-04T13:01:10Z

This PR resolves #2515 .
It adds javaagent instrumentation for r2dbc-spi >= v1.0

As suggested by @mp911de in #2515 (comment) I used the r2dbc-proxy ProxyConnectionFactory to intercept Database query executions and create according spans.

Example span from example project using spring-boot-starter-data-r2dbc Application:

Crustack · 2023-03-04T13:01:22Z

Open Questions:

I had a look at the jdbc instrumentation and for the same example as above the spans look like this:

I am guessing the r2dbc example should have the same amount/type of spans as jdbc, but I am a bit confused of all the spans.
The first database span (db 167.08µs) contains set autocommit = ?, for the r2dbc example auto-commit is set to true by default and there doesnt seem to be an additional query for setting the autocommit, so this span should not exist in r2dbc?

Same question for the Transaction.commit span, it seems the ProxyMethodExecutionListener#before/afterCommitTransactionOnConnection() are not called if auto-commit is true, so do I have to add a Transaction.commit span manually after any query if auto-commit is true?

Also, is the SELECT CustomerJDBC simply a wrapper-span around the actual sql-query span?

laurit · 2023-03-06T13:52:41Z

+
+  @Override
+  public boolean isHelperClass(String className) {
+    return className.startsWith("io.r2dbc.proxy");


The drawback of doing it this way is that when the application also contains the r2dbc-proxy library there may be incompatibilities unless the library bundled in the agent has the same version that the application has.

Thanks for your feedback.
So the alternative would be to basically reimplement the r2dbc-proxy library in order to get Query Listeners just to not use r2dbx-proxy directly? It just seemed a bit redundant to me, but I get your point.
After @marcingrzejszczak #7977 (comment), should I be simply using the existing ObservationProxyExecutionListener or stick to the manually implemented TraceProxyListener, or even move away entirely from r2dbc-proxy for the auto instrumentation?

We could shade the r2dbc-proxy library under a different package so it wouldn't conflict with whatever is in the application. I think this isn't super important, we can always fix it later. @marcingrzejszczak is one of the authors of micrometer tracing. As for his suggestion I'm not quite sure what he means. It could be either that he is suggesting to implement some sort of bridge with micrometer api so that their instrumentations could be automatically used or he might be hinting that instead of implementing this instrumentation for opentelemetry you could be using micrometer tracing where this instrumentation already exists.

Yeah I'm suggesting that since the instrumentation is there providing a bridge would mean you have nothing to do 🤷 Otherwise you can just replicate the same code but use the OTel API.

We could shade the r2dbc-proxy library under a different package so it wouldn't conflict with whatever is in the application.

We're doing a similar thing in the couchbase instrumentation. @PhilKes you could probably implement it in a somewhat similar manner.

We could shade the r2dbc-proxy library under a different package so it wouldn't conflict with whatever is in the application.

We're doing a similar thing in the couchbase instrumentation. @PhilKes you could probably implement it in a somewhat similar manner.

@mateuszrzeszutek I tried to shade the r2dbc-proxy the same way as in the couchbase instrumentation, but I ran into a problem when running the tests:

R2dbcStatementTest > testQueries(Parameter) > io.opentelemetry.javaagent.instrumentation.r2dbc.v1_0.R2dbcStatementTest.testQueries(Parameter)[8] FAILED java.util.ServiceConfigurationError: io.r2dbc.spi.ConnectionFactoryProvider: Provider io.r2dbc.proxy.ProxyConnectionFactoryProvider not found at java.base/java.util.ServiceLoader.fail(ServiceLoader.java:593) at java.base/java.util.ServiceLoader$LazyClassPathLookupIterator.nextProviderClass(ServiceLoader.java:1219) at java.base/java.util.ServiceLoader$LazyClassPathLookupIterator.hasNextService(ServiceLoader.java:1228) at java.base/java.util.ServiceLoader$LazyClassPathLookupIterator.hasNext(ServiceLoader.java:1273) at java.base/java.util.ServiceLoader$2.hasNext(ServiceLoader.java:1309) at java.base/java.util.ServiceLoader$3.hasNext(ServiceLoader.java:1393) at io.r2dbc.spi.ConnectionFactories.find(ConnectionFactories.java:108) at io.opentelemetry.javaagent.instrumentation.r2dbc.v1_0.AbstractR2dbcStatementTest.createProxyConnectionFactory(AbstractR2dbcStatementTest.java:93) at io.opentelemetry.javaagent.instrumentation.r2dbc.v1_0.R2dbcStatementTest.createProxyConnectionFactory(R2dbcStatementTest.java:44)

I dont get why ConnectionFactoryProvider looks for the io.r2dbc.proxy package, the shaded imports work everywhere else and the ServiceLoader should find the ProxyConnectionFactoryProvider in the io.opentelemetry.javaagent.instrumentation.r2dbc.v1_0.shaded.io.r2dbc.proxy package, or is there something wrong with the shaded r2dbc-proxy?

@PhilKes I think it is better to to rename packages only in the code that gets embedded in the agent and leave the library module unshaded. I created a pr that applies on top of your pr for that Crustack#1

marcingrzejszczak · 2023-03-06T15:07:30Z

BTW you can use Micrometer Observation API that is already there in R2DBC proxy (https://github.com/r2dbc/r2dbc-proxy/tree/main/src/main/java/io/r2dbc/proxy/observation) to have the instrumentation done for you.

mateuszrzeszutek · 2023-03-09T12:50:16Z

The first database span (db 167.08µs) contains set autocommit = ?, for the r2dbc example auto-commit is set to true by default and there doesnt seem to be an additional query for setting the autocommit, so this span should not exist in r2dbc?

No, you shouldn't worry about keeping it 1:1 with JDBC.

Same question for the Transaction.commit span, it seems the ProxyMethodExecutionListener#before/afterCommitTransactionOnConnection() are not called if auto-commit is true, so do I have to add a Transaction.commit span manually after any query if auto-commit is true?

Also, is the SELECT CustomerJDBC simply a wrapper-span around the actual sql-query span?

I think both of the Transaction.commit and SELECT CustomerJDBC might be Hibernate spans.

mateuszrzeszutek

Thanks @PhilKes !

mateuszrzeszutek · 2023-03-09T13:17:15Z

+    ConnectionFactory proxiedFactory =
+        ProxyConnectionFactory.builder(originalConnectionFactory)
+            .listener(new TraceProxyListener(instrumenterBuilder.build(), connectionFactoryOptions))
+            .build();


I think this part should live in the R2dbcTelemetry class - instead of the getConnectionFactory method there should be a wrapConnectionFactory(ConnectionFactory, ConnectionFactoryOptions) (or similar name) that takes any connection factory and returns a decorated instance. The R2dbcTelemetry class should include any actual connection factory, it should be possible to use R2dbcTelemetry to decorate multiple connection factories - at least that's how we're doing it in all other library instrumentations.

I moved it into the R2dbcTelemetry as suggested, so that any number of ConnectionFactory can be wrapped.

mateuszrzeszutek · 2023-03-09T13:21:27Z

+                .setStatementSanitizationEnabled(
+                    ConfigPropertiesUtil.getBoolean(
+                        "otel.instrumentation.common.db-statement-sanitizer.enabled", true))


In general, usually we don't use properties in library instrumentations - instead, we expose a builder method that allows users to configure the setting by themselves. The JDBC library instrumentation is not a good example of that because it utilizes the Driver decorator approach - where it's not possible to have a builder to configure a setting.
See the CassandraTelemetryBuilder for an example.

I added the the flag as a builder method

mateuszrzeszutek · 2023-03-09T13:31:27Z

+
+  @Override
+  public boolean isHelperClass(String className) {
+    return className.startsWith("io.r2dbc.proxy");


We could shade the r2dbc-proxy library under a different package so it wouldn't conflict with whatever is in the application.

We're doing a similar thing in the couchbase instrumentation. @PhilKes you could probably implement it in a somewhat similar manner.

mateuszrzeszutek · 2023-03-13T12:26:16Z

Hey @PhilKes , can you rebase (or clean merge) your branch with main? Right now this PR includes lots of changes that are not yours

…o.ValuesStore instead of Map

Crustack · 2023-03-14T16:06:22Z

Hey @PhilKes , can you rebase (or clean merge) your branch with main? Right now this PR includes lots of changes that are not yours

Sorry for that, I fixed the rebase

Relocate bundled r2dbc-proxy

laurit · 2023-03-15T11:46:57Z

@PhilKes to get rid of the muzzle failure https://github.com/open-telemetry/opentelemetry-java-instrumentation/actions/runs/4425709289/jobs/7761112434 Try adding extraDependency("io.projectreactor:reactor-core:3.4.12") as described in

opentelemetry-java-instrumentation/docs/contributing/muzzle.md

Line 113 in 65bf28f

extraDependency("javax.servlet:javax.servlet-api:3.0.1")

…2dbc muzzle pass

mateuszrzeszutek

Thanks @PhilKes !

juandaco · 2023-04-03T22:14:26Z

Hello all. Thank you so much for your work on supporting thi!

I'm unable to get R2DBC tracing on a Spring Boot application running with the javaagent (using the proxy). Is there any particular configuration that is required for this to work?

trask · 2023-04-03T23:46:03Z

hi @juandaco!

can you open a new issue?

you shouldn't need to make any code changes if you are using the javaagent (e.g. you shouldn't need to use a proxy)

and if you can post a minimal repro that is usually the easiest way for folks to understand and help out

pkgonan · 2023-04-14T02:19:28Z

Hi. The actual query was performed 1 time, but there seems to be a bug exposed to APM by duplicating 2 times for the same query.

Crustack · 2023-04-22T10:04:56Z

Hi. The actual query was performed 1 time, but there seems to be a bug exposed to APM by duplicating 2 times for the same query.

Hi, could you please open a new issue with this?
Are you using the r2dbc-proxy? Maybe there is an issue if you use the opentelemetry-autoinstrumentation (which already uses r2dbc-proxy) and then use another Proxy on top of it

Crustack requested a review from a team March 4, 2023 13:01

github-actions Bot requested a review from theletterf March 4, 2023 13:01

laurit reviewed Mar 6, 2023

View reviewed changes

Crustack requested review from laurit and removed request for theletterf March 8, 2023 17:49

mateuszrzeszutek reviewed Mar 9, 2023

View reviewed changes

github-actions Bot requested a review from theletterf March 12, 2023 09:50

Crustack requested review from mateuszrzeszutek and removed request for laurit and theletterf March 12, 2023 10:00

github-actions Bot requested a review from theletterf March 13, 2023 15:50

Crustack added 8 commits March 14, 2023 16:23

Add R2dbc statement javaagent instrumentation

9320c68

Add reactor instrumentation to r2dbc tests

d05ab97

Use mockito in DbExecutionTest

6f257b0

Refactor R2dbcTelemetry and stop last container in R2dbcStatementTest

a60ccb9

Shade r2dbc-proxy dependency

1aaccd5

Remove scope creation from TraceProxyListener + use QueryExecutionInf…

d6ca0ae

…o.ValuesStore instead of Map

Add r2dbc-1.0 library instrumentation and testing module

d6ffeef

Fix settings.gradle-kts

b1009e7

Crustack force-pushed the feat/r2dbc-instrumentation branch from b379e2f to b1009e7 Compare March 14, 2023 15:44

Crustack added 2 commits March 14, 2023 16:49

Add named argument matcher to R2dbcInstrumentation

db4fe91

Shade r2dbc-proxy for instrumentation

5cf39b6

Crustack and others added 3 commits March 14, 2023 17:35

Fix spotless in AbstractR2dbcStatementTest

2a4334a

Relocate bundled r2dbc-proxy

2cb34e4

Merge pull request #1 from laurit/r2dbc-javaagent-shade

8bf8d4e

Relocate bundled r2dbc-proxy

Fix supported-libraries.md and add reactor-core extra dependency to r…

e050c61

…2dbc muzzle pass

laurit reviewed Mar 16, 2023

View reviewed changes

Comment thread .../src/main/java/io/opentelemetry/javaagent/instrumentation/r2dbc/v1_0/TraceProxyListener.java Outdated

laurit reviewed Mar 16, 2023

View reviewed changes

Comment thread ...n/java/io/opentelemetry/javaagent/instrumentation/r2dbc/v1_0/AbstractR2dbcStatementTest.java Outdated

Minor refactorings

a2146cb

Crustack requested review from laurit and removed request for mateuszrzeszutek and theletterf March 20, 2023 11:43

laurit reviewed Mar 20, 2023

View reviewed changes

Comment thread ...rc/main/java/io/opentelemetry/javaagent/instrumentation/r2dbc/v1_0/internal/DbExecution.java

laurit approved these changes Mar 20, 2023

View reviewed changes

Use DbSystemValues.OTHER_SQL as fallback for r2dbc system name

8280740

github-actions Bot requested a review from theletterf March 20, 2023 17:32

Crustack requested review from mateuszrzeszutek and removed request for theletterf March 20, 2023 17:33

mateuszrzeszutek approved these changes Mar 23, 2023

View reviewed changes

mateuszrzeszutek merged commit a6bc3b1 into open-telemetry:main Mar 23, 2023

Conversation

Crustack commented Mar 4, 2023

Uh oh!

Crustack commented Mar 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Crustack Mar 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

laurit Mar 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

marcingrzejszczak commented Mar 6, 2023

Uh oh!

mateuszrzeszutek commented Mar 9, 2023

Uh oh!

mateuszrzeszutek left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mateuszrzeszutek commented Mar 13, 2023

Uh oh!

Crustack commented Mar 14, 2023

Uh oh!

laurit commented Mar 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mateuszrzeszutek left a comment

Choose a reason for hiding this comment

Uh oh!

juandaco commented Apr 3, 2023

Uh oh!

trask commented Apr 3, 2023

Uh oh!

pkgonan commented Apr 14, 2023

Uh oh!

Crustack commented Apr 22, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

Crustack commented Mar 4, 2023 •

edited

Loading

Crustack Mar 6, 2023 •

edited

Loading

laurit Mar 14, 2023 •

edited

Loading

laurit commented Mar 15, 2023 •

edited

Loading