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

OpenTelemetryDriver don't require DriverManager for underlying drivers #7089

Merged
merged 1 commit into from
Nov 28, 2022
Merged

OpenTelemetryDriver don't require DriverManager for underlying drivers #7089

merged 1 commit into from
Nov 28, 2022

Conversation

michalvavrik
Copy link
Contributor

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.

@michalvavrik michalvavrik requested a review from a team as a code owner November 7, 2022 10:54
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Nov 7, 2022

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: michalvavrik / name: Michal Vavřík (2b39337)

Copy link

@brunobat brunobat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @michalvavrik. It looks ok to me.

@brianmolinaspring
Copy link

@brunobat how are you? For this PR to be approved, do we need any other approver?

@brunobat
Copy link

brunobat commented Nov 8, 2022

Not mandatory, but it would be nice

@brianmolinaspring
Copy link

@brunobat ok thanks, then you could do the merge?

@michalvavrik
Copy link
Contributor Author

@brianmolinaspring I asked Bruno to review this as I needed his expertise, but I think we will have to wait for the project maintainers (rights + they know best what's desired here).

@brianmolinaspring
Copy link

@michalvavrik Perfect, this would solve, to place the driverManager manually, that is, we would not have to do anything else but only update the libraries, right?

Copy link
Member

@trask trask left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thx @michalvavrik!

Is there any use-case for this outside of building native images? I'm thinking about it from docs and public API surface perspective.

@trask
Copy link
Member

trask commented Nov 9, 2022

hey @michalvavrik,

would this be an alternative option? https://github.com/open-telemetry/opentelemetry-java-instrumentation/blob/main/instrumentation/jdbc/library/README.md#datasource-way

just trying to understand the use case and restrictions better.

if you happen to be able to join our Thu 9am UTC-8 SIG meeting, that would be a great place to discuss further, since there are a lot of gaps in knowledge on our side about native images

https://calendar.google.com/calendar/event?action=TEMPLATE&tmeid=cDl2Z29mcm0zNThlcWp2azVpOTJoODBlNzJfMjAyMjExMTBUMTcwMDAwWiBnb29nbGUuY29tX2I3OWUzZTkwajdiYnNhMm4ycDVhbjVsZjYwQGc&tmsrc=google.com_b79e3e90j7bbsa2n2p5an5lf60%40group.calendar.google.com&scp=ALL

@michalvavrik
Copy link
Contributor Author

@trask your digging is perfectly fine, I used Agroal with OpenTelemetryDriver and judging from OpenTelemetryDataSource and AgroalDataSource, it should be possible. I'll try to find out why we want with the driver and get back to you (as for meeting, can't tell ATM). Thank you for the time you spent reviewing this.

@brunobat
Copy link

@trask, For native mode, all classes that will be used need to be known at runtime. This will affect all Drivers. This means that we cannot create a new Otel Driver Wrapper object at runtime if tracing is needed. This needs to be known at build time.

@trask
Copy link
Member

trask commented Nov 14, 2022

@trask, For native mode, all classes that will be used need to be known at runtime. This will affect all Drivers. This means that we cannot create a new Otel Driver Wrapper object at runtime if tracing is needed. This needs to be known at build time.

thx @brunobat, can you wrap the underlying driver with the OTel wrapper at build time?

@brunobat
Copy link

Yes @trask ... That can be done.

@michalvavrik
Copy link
Contributor Author

I've got a feeling we are not moving forward to the mutual understanding about the issue and possible solutions, so I'll join the meeting on Thursday as Trask proposed.

@michalvavrik
Copy link
Contributor Author

michalvavrik commented Nov 21, 2022

Hey @trask , now that we have clarified the matter during the meeting, please provide feedback on the PR (required changes, suggestions, ...) once you have a time, thanks.

Copy link
Member

@trask trask left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm good with the API shape as-is 👍

@michalvavrik
Copy link
Contributor Author

michalvavrik commented Nov 22, 2022

I don't think CI failures are related to the PR; are failures known issue? My bad, there was unnecessary semicolon in one of the tests.

Copy link
Member

@trask trask left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@trask trask merged commit 6207887 into open-telemetry:main Nov 28, 2022
@michalvavrik michalvavrik deleted the feature/ot-driver-dont-require-drivermanager branch November 29, 2022 05:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make it possible to use OpenTelemetryDriver without DriverManager
4 participants