quarkusio / quarkus

Quarkus: Supersonic Subatomic Java.
https://quarkus.io
Apache License 2.0
13.77k stars 2.68k forks source link

Datasource jdbc driver property initialization from external sources #25005

Open OleKsimov opened 2 years ago

OleKsimov commented 2 years ago

Describe the bug

Hi guys!

I've noticed that right now it's not possible to initialize datasource.jdbc.driver property from external sources (env, vault). Do you have plans to make it possible?

In our project, we configure connections to the database using Vault and for some services, it is a default driver, for some we use opentracing driver, for others - opentelemetry. And it's a bit uncomfortable to migrate the services from one driver to another without external properties.

It's not working even with the default value, in the next case datasource.jdbc.driver = ${DATABASE_DRIVER:io.opentracing.contrib.jdbc.TracingDriver} DATABASE_DRIVER value is ignored

Expected behavior

It's possible to init jdbc driver property next way datasource.jdbc.driver = ${DATABASE_DRIVER}

Actual behavior

No response

How to Reproduce?

No response

Output of uname -a or ver

No response

Output of java -version

No response

GraalVM version (if different from Java)

No response

Quarkus version or git rev

2.6.2

Build tool (ie. output of mvnw --version or gradlew --version)

No response

Additional information

No response

knutwannheden commented 2 years ago

@OleKsimov You may have noticed that the quarkus. prefix is missing in your configuration properties. Further, you may have missed that this is a build-time config, which means that you would have to set the DATABASE_DRIVER environment variable when running the Maven build, in order to get the desired database driver at runtime. If this doesn't help, then further investigation will be required.

OleKsimov commented 2 years ago

@knutwannheden We have a huge multi-module project, so I would have noticed that "quarkus" prefix is missed. But it's not :) Does "build-time config" means I can't set this property from external sources like Vault? Because I can't. Do you need a reproducer?

yrodiere commented 2 years ago

Because I can't. Do you need a reproducer?

No, this is expected behavior.

Does "build-time config" means I can't set this property from external sources like Vault?

Build-time config means the property is read and taken into account by Quarkus when you build your application, literally. I.e. when you run ./mvnw package or ./gradlew build. When you start your application, it's simply too late to change this configuration property, as Quarkus already pre-configured the application to take that property into account; that includes things that cannot easily/efficiently be done at runtime, such as generating bytecode. You may find more information here.

That's not a problem for the JDBC driver in general as it's rather unusual to change the driver at runtime.

In your case, though, this is about wrapping the JDBC driver for tracing, which I suppose you want to be able to enable/disable at runtime, which is a very reasonable use case. I suspect the only way to do that would be to always use the tracing driver, and use some configuration option of that driver to disable tracing at runtime (thus making the tracing driver delegate to the real one, without tracing). EDIT: I read your message in full now, you want to switch between opentracing and opentelemetry. No idea if that can be done, maybe if you use both? Anyway, still worth pinging OpenTelemetry people.

Let's ping the folks working on OpenTelemetry support, they'll probably know more.

quarkus-bot[bot] commented 2 years ago

/cc @Ladicek, @brunobat, @radcortez

brunobat commented 2 years ago

We don't have a migration guide yet, in the meantime you can check the following: https://opentelemetry.io/docs/migration/opentracing/ There is an artifact called shim, that will allow you to output OpenTracing in OpenTelemetry format: https://github.com/open-telemetry/opentelemetry-java/tree/main/opentracing-shim In the case of reimplementation with OpenTelemetry on Quarkus, you need to check this page: https://quarkus.io/guides/opentelemetry

OleKsimov commented 2 years ago

@brunobat @yrodiere Guys, please, read the topic name and the first message. Looks like you missed the point, tracing has nothing to do with the problem. For different environments, we want to use different datasource drivers. The driver right now can't be configured on startup from external resources. OpenTelemetry is just an example that driver value is hardcoded and can't be taken from Vault even though the property name is configured (configuration priority). For example, we have opentelemetry configured on one environment, but we don't have it on the other one. So what I'm asking is why can't the driver be configured the same way we configure database connection and can only be hardcoded?

yrodiere commented 2 years ago

Guy, please read my answer :)

So what I'm asking is why can't the driver be configured the same way we configure database connection and can only be hardcoded?

Does "build-time config" means I can't set this property from external sources like Vault?

Build-time config means the property is read and taken into account by Quarkus when you build your application, literally. I.e. when you run ./mvnw package or ./gradlew build. When you start your application, it's simply too late to change this configuration property, as Quarkus already pre-configured the application to take that property into account; that includes things that cannot easily/efficiently be done at runtime, such as generating bytecode. You may find more information here.

The rest of our answers were because we consider the tracing use case to be specific and deserving a separate discussion. Changing your JDBC driver at runtime from e.g. PostgreSQL to Oracle is not something we consider supporting (EDIT: because it's too complex, see above). Changing the tracing wrapper, on the other hand, is a more reasonable thing to consider (EDIT: because it might be simpler).

radcortez commented 2 years ago

For OpenTelemetry / OpenTracing it is the same situation right now because it is also a JDBC Driver that wraps the original one. We don't distinguish the driver types, so they all behave similarly.

To change this, we would need a way to set the OpenTelemetry / OpenTracing JDBC support differently because, as @yrodiere explained, the driver configuration is fixed during the build, and we cannot change it during runtime.

yrodiere commented 2 years ago

we would need a way to set the OpenTelemetry / OpenTracing JDBC support differently

Right.

That being said, we don't seem to be doing much at build-time with the JDBC driver... Merely doing some configuration checks (which I guess are not even relevant for OpenTelemetry/OpenTracing wrappers), and enabling reflection on the JDBC driver class (duh).

So, assuming people don't try to change the DB type or switch the actual JDBC driver... swapping the OpenTelemetry/OpenTracing wrappers at runtime might be possible, even in native mode.

I'm wondering whether we could have OpenTelemetry/OpenTracing use a separate runtime property, e.g. datasource.jdbc.driver-wrapper, to let Agroal know of the wrapper, while keeping datasource.jdbc.driver as a build-time property reserved for actual JDBC drivers (which generally can be determined automatically by Quarkus based on quarkus.datasource.db-kind). Then every DB-specific things could be done at build-time, while the OpenTelemetry/OpenTracing wrapping could possibly be done at runtime (assuming there are no constraints preventing that in OpenTelemetry/OpenTracing).

However, I'm not really sure how we would enforce that the "actual" JDBC driver doesn't change, since that seems to be in the hands of the OpenTelemetry/OpenTracing wrappers (which infer the actual driver from the JDBC URL, I think). Maybe Quarkus can force that by setting a JDBC property that would be passed to the wrapper?

brunobat commented 2 years ago

If the DB driver is for the same DB, except for tracing support, no need to change the driver in the runtime env. Always use the driver with tracing and then disable tracing in runtime. Currently it's not possible but we are working to support this very soon.

yrodiere commented 2 years ago

If the DB driver is for the same DB, except for tracing support, no need to change the driver in the runtime env. Always use the driver with tracing and then disable tracing in runtime. Currently it's not possible but we are working to support this very soon.

Right, that's even simpler, but the original issue description asks about switching between OpenTracing and OpenTelemetry at runtime (well, on startup), which is a bit more complex.

radcortez commented 2 years ago

We don't allow switching between OpenTracing and OpenTelemetry, at the extension level. Both extensions are mutually exclusive.

We don't control the driver since it's user configured and external. OpenTracing is already an archived project by the CNCF, and our recommendation is for users to move to OpenTelemetry. We don't have any plans to enhance the OpenTracing extension. We plan to deprecate it and remove it in the future.

yrodiere commented 2 years ago

@radcortez I think @OleKsimov is aware OpenTracing is deprecated, as (from what I understand) they wanted this precisely for migration purposes, from OpenTracing to OpenTelemetry.

Anyway, if you consider this is too complex to be worth the trouble, should we close this issue as "not planned"?

radcortez commented 2 years ago

I haven't looked at what is required for this yet.

My point is that if we move this forward, it will only be for OpenTelemetry, meaning that you will be able to change the runtime driver to the OpenTelemetry one from a regular database driver, but not from / to the OpenTracing one.