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.

Sign up for GitHub

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

Jump to bottom

contrib/sql/driver: use the connector provided by the sql driver if possible #1502

Merged
merged 3 commits into from Oct 7, 2022
Merged

contrib/sql/driver: use the connector provided by the sql driver if possible #1502

merged 3 commits into from Oct 7, 2022

Conversation

david-ds
Copy link
Contributor

@david-ds david-ds commented Oct 6, 2022

Currently, when the sql tracer opens a new database with the Open(...) function, it uses a default dsnConnector. However, if the driver implements the DriverContext interface, the one provided by the OpenConnector(...) method on the driver should be used instead. And the documentation on the sql library says that the DriverContext should be implemented :

The driver interface has evolved over time. Drivers should implement Connector and DriverContext interfaces.

(from: https://pkg.go.dev/database/sql/driver@go1.19.2)

This commit uses the connector provided by the sql driver if it implements the DriverContext and follows the same logic as the std lib sql library: https://github.com/golang/go/blob/515e3de2999b23da28e6d15ac485bfdd299ec83a/src/database/sql/sql.go#L821-L829

@david-ds david-ds requested a review from a team October 6, 2022 20:44
@ajgajg1134 ajgajg1134 added this to the v1.44.0 milestone Oct 6, 2022
@alexandre-normand
Copy link
Contributor

@ajgajg1134 Sorry to get my nose in here but any chance we could have this one be included in v1.43.0 instead of 1.44.0? This would be really helpful in unblocking more internal users of sql tracing.

@david-ds
contrib/sql/driver: use the connector provided by the sql driver if p…
2def902 
…ossible

Currently, when the sql tracer opens a new database with the Open(...) function, it uses a default dsnConnector.
However, if the driver implements the `DriverContext` interface, the one provided by the `OpenConnector(...)` method on the driver should be used instead.
And the documentation on the sql library says that the `DriverContext` should be implemented :

> The driver interface has evolved over time. Drivers should implement Connector and DriverContext interfaces.

(from: https://pkg.go.dev/database/sql/driver@go1.19.2)

This commit uses the connector provided by the sql driver if it implements the `DriverContext` and follows the same logic as the std lib sql library: https://github.com/golang/go/blob/515e3de2999b23da28e6d15ac485bfdd299ec83a/src/database/sql/sql.go#L821-L829
@david-ds david-ds force-pushed the david-ds/handle-driver-context branch from a0bbe3f to 2def902 Compare October 6, 2022 22:58
@ajgajg1134
Copy link
Contributor

@ajgajg1134 Sorry to get my nose in here but any chance we could have this one be included in v1.43.0 instead of 1.44.0? This would be really helpful in unblocking more internal users of sql tracing.

@Julio-Guerra do you think there's still time to get this change into v1.43?

It is possible to do, but since we've already created the release branch for v1.43 you'll need to get this change to both main and to the release branch. After that we can create a new rc to begin testing it and ensure there's no issues. (Of course, this all assumes that it's not too late to add changes to v1.43)

@Julio-Guerra
Copy link
Contributor

Yes, I'll try merging it into the release branch and see on Monday if everything is okay with it. Feel free to merge that one to main when you think it's ready.

@ajgajg1134
Copy link
Contributor

It looks like this change breaks the sql tests in the contrib packages? Let me know if you'd like an extra pair of 👀 to take a look at the test failures

@david-ds david-ds force-pushed the david-ds/handle-driver-context branch from 9e2b3d0 to 7f5bd7c Compare October 7, 2022 14:39
@alexandre-normand alexandre-normand modified the milestones: v1.44.0, 1.43.0 Oct 7, 2022
@alexandre-normand
Copy link
Contributor

Yes, I'll try merging it into the release branch and see on Monday if everything is okay with it. Feel free to merge that one to main when you think it's ready.

Thanks for being open to including this on the 1.43.0 branch, @Julio-Guerra ! Much appreciated 🙇

@ajgajg1134 ajgajg1134 merged commit dbab956 into main Oct 7, 2022
@ajgajg1134 ajgajg1134 deleted the david-ds/handle-driver-context branch October 7, 2022 19:48
@Julio-Guerra Julio-Guerra restored the david-ds/handle-driver-context branch October 10, 2022 09:18
@Julio-Guerra Julio-Guerra deleted the david-ds/handle-driver-context branch October 10, 2022 13:23
@Julio-Guerra
Copy link
Contributor

v1.43.0-rc.4 is out and includes that fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ajgajg1134 ajgajg1134 ajgajg1134 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
1.43.0
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@david-ds @alexandre-normand @ajgajg1134 @Julio-Guerra