OpenTelemetry JDBC instrumentation - fix Oracle and DB2 in native mode

[michalvavrik]

fixes: #28915
fixes: #30176

Oracle driver is not registered with DriverManager in native mode. When Oracle driver is used directly, e.g. by Agroal here it's not a problem, however OpenTelemetry driver uses DriverManager and thus the Oracle driver is missing. I wonder if we should initialize it at runtime, however there is a warning in OracleMetadataOverrides that deferred me from doing it. Instead, this PR register driver when quarkus-oracle-jdbc extension is used together with OpenTelemetry extension. Way I understand java.sql.DriverManager#registerDriver(java.sql.Driver) implementation, it's simply ignored if the driver is already there, however it would be nice to find better way in the future (or now if you have one!).

DB2 driver wasn't registered for reflection when used with OT driver as then Agroal registered OT driver and not DB2 driver in native mode.

[quarkus-bot]

/cc @brunobat, @radcortez

[michalvavrik]

Could you have a look @Sanne, please?

[michalvavrik]

(Windows) failures are not related.

[Sanne]

Interesting, unfortunately my options are limited as the Oracle driver is not open source.

In general I would suggest to not rely on the DriverManager - such static global registries are very ancient code patterns, I'm sure we can do better today. We're not relying on DriverManager in other code in Quarkus (as far as I know - clearly I didn't know about OpenTelemetry using it) and actively trying to not trigger it.

Would that be possible? It also happens to solve this issue, but I also believe it would be better overall.

[michalvavrik]

OpenTelemetry driver is relying on DriverManager https://github.com/open-telemetry/opentelemetry-java-instrumentation/blob/main/instrumentation/jdbc/library/src/main/java/io/opentelemetry/instrumentation/jdbc/OpenTelemetryDriver.java#L103 and do not offer other way to set drivers, so there are 2 ways we can work around this:

• byte code transformation of either one of drivers (Oracle/ OT)
• forcing runtime initialization of Oracle driver (didn't do that according to your comments)

Registering Oracle driver for now and opening issue in OT (yet to do) felt like the easiest way to work around the issue.

[geoand]

I am also -1 for this unless there is absolutely no other way to go about it.

@radcortez @brunobat can't we contribute something to OTel around this?

[Sanne]

This might indeed be our only option though.

Anyone knows why it's a problem specific for Oracle and why we shuouldn't register all drivers on DriverManager if OTel requires it?

[radcortez]

We can undoubtedly contribute something to OTel. We did that in the past successfully.

[michalvavrik]

Anyone knows why it's a problem specific for Oracle and why we shuouldn't register all drivers on DriverManager if OTel requires it?

Adjusted reproducer for Postgresql and the issue is not there even though -H:+PrintClassInitialization shows org.postgresql.Driver, BUILD_TIME. I can't find direct call to org.postgresql.Driver#register except in static init block & PGBundleActivator (in pgjdbc project, can't see anything in quarkus/ot projects either so far). Don't have the answer, but can't see how there could be other explanation than that something calls org.postgresql.Driver#register. Update: sorry, wrong assumptions, don't know yet.

[michalvavrik]

Hey @brunobat & @radcortez ,

I hope I didn't block solution with this PR - e.g. hope you don't wait for me. Here is my suggestion: I'll open an issue in OpenTelemetry and also provide a PR that close it. The issue should allow us to register drivers without java.sql.DriverManager#registerDriver(java.sql.Driver).

I propose it takes a while to get this fixed in OT and get it released, so it would be nice to have workaround till then. This PR does that, should you prefer different one, please, bring it on!

[brunobat]

Hi @michalvavrik,
Thanks for the PR. I was on PTO, therefore the delay.
I agree on having this provisional solution on, with some tweaks. The PR looks good to me but later, will the final solution we should move this instrumentation wiring elsewhere and create specific test projects per DB... We should assert also do PostgreSQL and MariaDB, at least.
If you want to provide a link for the OTel side PR, I'm happy to review and help with it.

[michalvavrik]

Hi @michalvavrik, Thanks for the PR. I was on PTO, therefore the delay. I agree on having this provisional solution on, with some tweaks. The PR looks good to me but later, will the final solution we should move this instrumentation wiring elsewhere and create specific test projects per DB...

Yeah, but that will mean 3 new IT modules, right? (fine with me, just checking).

We should assert also do PostgreSQL and MariaDB, at least.

I checked manually https://github.com/michalvavrik/quarkus-native-oracle-otel-reproducer and PG and MariaDB are fine, which confuses me heavily as they all (including OracleDriver) works pretty same way - register themselves in static initializer block. Not sure what's different and will try to identify exact reason, but avoiding DriverManager still seems like a good way around this (with OT PR).

If you want to provide a link for the OTel side PR, I'm happy to review and help with it.

Thank you, I opened an issue open-telemetry/opentelemetry-java-instrumentation#7028 and will draft a PR over weekend and ask you for review/kindly provide help if I go in a wrong direction etc.

[brunobat]

Hi @michalvavrik, Thanks for the PR. I was on PTO, therefore the delay. I agree on having this provisional solution on, with some tweaks. The PR looks good to me but later, will the final solution we should move this instrumentation wiring elsewhere and create specific test projects per DB...

Yeah, but that will mean 3 new IT modules, right? (fine with me, just checking).

We should assert also do PostgreSQL and MariaDB, at least.

We can go with TestContainers and have just one JDBC Driver related project with different test classes. One different test resource for each test class: https://quarkus.io/guides/getting-started-testing#quarkus-test-resource

I checked manually https://github.com/michalvavrik/quarkus-native-oracle-otel-reproducer and PG and MariaDB are fine, which confuses me heavily as they all (including OracleDriver) works pretty same way - register themselves in static initializer block. Not sure what's different and will try to identify exact reason, but avoiding DriverManager still seems like a good way around this (with OT PR).

If you want to provide a link for the OTel side PR, I'm happy to review and help with it.

Thank you, I opened an issue open-telemetry/opentelemetry-java-instrumentation#7028 and will draft a PR over weekend and ask you for review/kindly provide help if I go in a wrong direction etc.

[michalvavrik]

We can go with TestContainers and have just one JDBC Driver related project with different test classes. One different test resource for each test class: https://quarkus.io/guides/getting-started-testing#quarkus-test-resource

Thanks, done.

[brunobat]

This looks really good @michalvavrik. Thanks very much.
Left a small comment.

[michalvavrik]

@michalvavrik michalvavrik#48 fixes the issue for me.

Thanks, help really appreciated! I can see you added it to OT module, I'm really interested whether I could reproduce without OT, but I'm busy today. Let me have a look next week, or if you submit Quarkus PR, I will test it ex post.

[jerboaa]

I'll let you run with it.

[michalvavrik]

fyi @jerboaa - the issue is not related to OT, but comes from combination of jdbc-oracle and jdbc-db2, I don't want to create addition integration test modules as this PR already adds tests that verifies fix, so I included your fix into this PR as second commit with description. And thanks.

[michalvavrik]

Quarkus QE experienced exactly same native build failures as described above, logs are here https://github.com/quarkus-qe/quarkus-test-suite/actions/runs/3814140863/jobs/6488338890; I verified it's fixed by this PR too.

I won't open an issue as there is a chance that @brunobat will review this PR and we can have it fixed in no time; in case that's a false hope, I'll open an issue later this week.

[brunobat]

I checked out the PR and executed the db tests in my M1, in native mode and they pass...
This must be a CI issue.

[michalvavrik]

I checked out the PR and executed the db tests in my M1, in native mode and they pass... This must be a CI issue.

I think so, thank you for checking. Please review the PR (or merge it) when times is right for you. I'm sure you've got a lot to catch up after holidays period. Thank you very much.

[geoand]

What's the status of this one?

[michalvavrik]

What's the status of this one?

I guess we are waiting for additional reviewers.

[brunobat]

@gsmet needs to review it

[gsmet]

I added one small question for something I don't understand fully. Could you check?

[michalvavrik]

Rebased the PR to resolve merge conflicts. No changes made, still waiting for the review.

[quarkus-bot]

Failing Jobs - Building 24735ab

Status	Name	Step	Failures	Logs	Raw logs
⌛	Native Tests - Misc4	Build	⚠️ Check →	Logs	Raw logs