Add R2dbc statement javaagent instrumentation #7977

PhilKes · 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:

PhilKes · 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?

...vaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/r2dbc/v1_0/DbExecution.java

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

...n/java/io/opentelemetry/javaagent/instrumentation/r2dbc/v1_0/R2dbcInstrumentationModule.java

+
+  @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 PhilKes#1

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

...rc/main/java/io/opentelemetry/javaagent/instrumentation/r2dbc/v1_0/R2dbcInstrumentation.java

...vaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/r2dbc/v1_0/DbExecution.java

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 !

...rary/src/main/java/io/opentelemetry/javaagent/instrumentation/r2dbc/v1_0/R2dbcTelemetry.java

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

...c/main/java/io/opentelemetry/javaagent/instrumentation/r2dbc/v1_0/R2dbcTelemetryBuilder.java

+    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

...io/opentelemetry/javaagent/instrumentation/r2dbc/v1_0/internal/R2dbcInstrumenterBuilder.java

+                .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

...rc/main/java/io/opentelemetry/javaagent/instrumentation/r2dbc/v1_0/R2dbcInstrumentation.java

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

...n/java/io/opentelemetry/javaagent/instrumentation/r2dbc/v1_0/R2dbcInstrumentationModule.java

+
+  @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.

...ary/src/test/java/io/opentelemetry/javaagent/instrumentation/r2dbc/v1_0/DbExecutionTest.java

...n/java/io/opentelemetry/javaagent/instrumentation/r2dbc/v1_0/AbstractR2dbcStatementTest.java

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

PhilKes · 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

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

...n/java/io/opentelemetry/javaagent/instrumentation/r2dbc/v1_0/AbstractR2dbcStatementTest.java

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

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.

PhilKes · 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

PhilKes requested a review from a team as a code owner 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

PhilKes 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

PhilKes 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

PhilKes 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

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

PhilKes added 2 commits March 14, 2023 16:49

Add named argument matcher to R2dbcInstrumentation

db4fe91

Shade r2dbc-proxy for instrumentation

5cf39b6

PhilKes 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

.../src/main/java/io/opentelemetry/javaagent/instrumentation/r2dbc/v1_0/TraceProxyListener.java Outdated Show resolved Hide resolved

laurit reviewed Mar 16, 2023

View reviewed changes

...n/java/io/opentelemetry/javaagent/instrumentation/r2dbc/v1_0/AbstractR2dbcStatementTest.java Outdated Show resolved Hide resolved

Minor refactorings

a2146cb

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

laurit reviewed Mar 20, 2023

View reviewed changes

...rc/main/java/io/opentelemetry/javaagent/instrumentation/r2dbc/v1_0/internal/DbExecution.java Show resolved Hide resolved

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

PhilKes 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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add R2dbc statement javaagent instrumentation #7977

Add R2dbc statement javaagent instrumentation #7977

PhilKes commented Mar 4, 2023

PhilKes commented Mar 4, 2023 •

edited

Loading

laurit Mar 6, 2023

PhilKes Mar 6, 2023 •

edited

Loading

laurit Mar 6, 2023

marcingrzejszczak Mar 8, 2023

mateuszrzeszutek Mar 9, 2023

PhilKes Mar 12, 2023

laurit Mar 14, 2023 •

edited

Loading

marcingrzejszczak commented Mar 6, 2023

mateuszrzeszutek commented Mar 9, 2023

mateuszrzeszutek left a comment

mateuszrzeszutek Mar 9, 2023

PhilKes Mar 12, 2023

mateuszrzeszutek Mar 9, 2023

PhilKes Mar 12, 2023

mateuszrzeszutek Mar 9, 2023

mateuszrzeszutek commented Mar 13, 2023

PhilKes commented Mar 14, 2023

laurit commented Mar 15, 2023 •

edited

Loading

mateuszrzeszutek left a comment

juandaco commented Apr 3, 2023

trask commented Apr 3, 2023

pkgonan commented Apr 14, 2023

PhilKes commented Apr 22, 2023

Add R2dbc statement javaagent instrumentation #7977

Add R2dbc statement javaagent instrumentation #7977

Conversation

PhilKes commented Mar 4, 2023

PhilKes commented Mar 4, 2023 • edited Loading

Choose a reason for hiding this comment

PhilKes Mar 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

laurit Mar 14, 2023 • edited Loading

Choose a reason for hiding this comment

marcingrzejszczak commented Mar 6, 2023

mateuszrzeszutek commented Mar 9, 2023

mateuszrzeszutek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mateuszrzeszutek commented Mar 13, 2023

PhilKes commented Mar 14, 2023

laurit commented Mar 15, 2023 • edited Loading

mateuszrzeszutek left a comment

Choose a reason for hiding this comment

juandaco commented Apr 3, 2023

trask commented Apr 3, 2023

pkgonan commented Apr 14, 2023

PhilKes commented Apr 22, 2023

PhilKes commented Mar 4, 2023 •

edited

Loading

PhilKes Mar 6, 2023 •

edited

Loading

laurit Mar 14, 2023 •

edited

Loading

laurit commented Mar 15, 2023 •

edited

Loading