Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make it possible to use OpenTelemetryDriver without DriverManager #7028

Closed
michalvavrik opened this issue Nov 2, 2022 · 15 comments · Fixed by #7089
Closed

Make it possible to use OpenTelemetryDriver without DriverManager #7028

michalvavrik opened this issue Nov 2, 2022 · 15 comments · Fixed by #7089
Assignees
Labels
enhancement New feature or request

Comments

@michalvavrik
Copy link
Contributor

michalvavrik commented Nov 2, 2022

Is your feature request related to a problem? Please describe.

Using static global registry as DriverManager cause some issues, some context here: quarkusio/quarkus#28921

When using OT JDBC instrumentation with a native image (using GraalVM), most SQL drivers as OracleDriver register themselves with DriverManager in a way that won't don't work with build time initialization. Marking drivers as "runtime initialized" makes them sometimes incompatible with other systems (e.g. in my case, with DB2 drivers) as it makes them incompatible with the JDK DriverManager integrations, DriverManager typically need to load all drivers in a different phase. OpenTelemetryDriver rely on DriverManager and there is no way to register drivers directly to OT driver instance.

Describe the solution you'd like
OpenTelemetryDriver shouldn't rely on DriverManager. DriverManager could still serve as a fallback (so that change is not breaking), but give an option to provide your own registry or service provider.

Describe alternatives you've considered
#5012 would probably do the job as well, provided there is an option to override hardcoded drivers in case the list is not exhaustive (or for special cases).

Additional context
I could have a look if what's described above is valid reasoning.

@mateuszrzeszutek
Copy link
Member

OpenTelemetryDriver shouldn't rely on DriverManager. DriverManager could still serve as a fallback (so that change is not breaking), but give an option to provide your own registry or service provider.

Sounds good to me -- we'd like to preserve the current behavior for JVM users, so as long as you provide an alternative API for creating a driver without involving the DriverManager it will be a great addition.

@michalvavrik should I assign this issue to you?

@michalvavrik
Copy link
Contributor Author

@mateuszrzeszutek sure, I'll have a look. Thanks for feedback.

@brianmolinaspring
Copy link

@michalvavrik This topic has already been corrected directly in OpenTelemetry ?, to update the lib from mvn. I see that the problem is still open. Please can you confirm me?

@michalvavrik
Copy link
Contributor Author

Hello @brianmolinaspring ,

I'm not sure I copy that, but AFAICT the issue is still opened. I provided PR #7089, but so far there are no reactions on it (apart of kind Bruno).

So, not fixed.

Michal

@brianmolinaspring
Copy link

@michalvavrik thanks for answering, a query how to register the manual driver in my code?

@michalvavrik
Copy link
Contributor Author

@brianmolinaspring Ah, yeah. I just saw your Quarkus issue :-) Now I understand. You need to register it manually on startup event, e.g.

public void methodName(@Observes StartupEvent ev) {
  DriverManager.registerDriver(new OracleDriver());
}

You can also watch quarkusio/quarkus#28921.

Michal

@michalvavrik
Copy link
Contributor Author

for context - I think it shouldn't cause any issues, but be aware you are registering driver that can be used from anywhere!

@michalvavrik
Copy link
Contributor Author

in general, it's not something we want to do.

@brianmolinaspring
Copy link

@michalvavrik Perfect, thank you very much. Would this event go anywhere in the code? either in the controller, or service, or better put in the dao class?

@michalvavrik
Copy link
Contributor Author

@brianmolinaspring here to help! Here is docs https://quarkus.io/guides/lifecycle#listening-for-startup-and-shutdown-events, personally I'd not add it to dao (my dao models are mostly POJOs).

@michalvavrik
Copy link
Contributor Author

@brianmolinaspring btw. I think this should handle most situations, but I'm not 100 % sure some tasks are not triggered before startup event is triggered. If so, I'm afraid you'll have to register the driver in the constructor of some runtime initialized class, here is how to set the class as runtime initialized https://quarkus.io/guides/writing-native-applications-tips#delay-class-init-in-your-app. I'd suggest it's bit dangerous to mark the driver as runtime initialized, but depending on your use case it could also work.

@brianmolinaspring
Copy link

@michalvavrik thank you very much, put the event in the controller, do the native compilation and try and excellent there were no details. successful execution

@brianmolinaspring
Copy link

brianmolinaspring commented Nov 9, 2022

@michalvavrik a question, OpenTelemtry is compatible with EntityManager JPA from Quarkus ?, try to register the driver manually with another service, but it gives the following errors:

Caused by: javax.persistence.PersistenceException: org.hibernate.exception.GenericJDBCException: Error calling CallableStatement.getMoreResults

Caused by: java.sql.SQLException: operation not allowed: Ordinal binding and Named binding cannot be combined!
at oracle.jdbc.driver.OracleCallableStatement.execute(OracleCallableStatement.java:4276)

All is mode native, in mode dev work ok

@michalvavrik
Copy link
Contributor Author

@brianmolinaspring I can't tell without seeing the code, but OT is compatible. Without reproducer I can't tell. Anyway, I'd suggest this is really a Quarkus issue. Let's open issue in Quarkus project or try Zulip communication with Quarkus community. Thx

@brianmolinaspring
Copy link

@michalvavrik I already solved, we have to remove the set by name from the fields of the stored procedure, set it by index, and this solved the error "The ordinal union and the union with name cannot be combined!"

trask pushed a commit that referenced this issue Nov 28, 2022
#7089)

closes: #7028

For reasons explained in the linked issue, it might be handy to register
drivers directly against `OpenTelemetryDriver` rather than against
`DriverManager`. I decided to also go with static registry as drivers
are often instantiated connect pools (like Agroal) and it could be
difficult to use instance varibles. This PR adds additional `Driver`
collection where drivers can be registered. If driver is registered both
with `OpenTelemtryDriver` and `DriverManager`, drivers registered with
`OpenTelemetryDriver` are preferred.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants