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

contrib/database/sql: Fix sample code in document to prevent panic on OpenDB #1635

Merged
merged 4 commits into from
Mar 16, 2023

Conversation

tsbkw
Copy link
Contributor

@tsbkw tsbkw commented Dec 22, 2022

What does this PR do?

Fix document to prevent to misuse Register function.
Issue: #1634

Motivation

In the example below, driver.Driver is passed as struct, and reflect.TypeOf(driver) is added to map.

// sqltrace.Register("pq", pq.Driver{})
// db, err := sqltrace.Open("pq", "postgres://pqgotest:password@localhost...")

Later when user call OpenDB, reflect.TypeOf(driver) is looked up in d.keys, therefore driver added and looked up must have same type. For example, if added type is struct, looked up by struct. And if added type is pointer of struct, looked up by pointer of struct.

name, ok := d.keys[reflect.TypeOf(driver)]

This behavior is not a problem when driver doesn't implement driver.DriverContext interface, because in this case driver get from map driver is used when lookup and type of these always same.

return OpenDB(&dsnConnector{dsn: dataSourceName, driver: d}, opts...), nil

However when driver implement driver.DriverContext, user must care whether type is pointer or not when calling Register.

Driver in lib/pq currently not implement driver.DriverContext but soon it will implement. see: lib/pq#900

Ideally the way look up should be better, but at least document should be fixed.

Describe how to test/QA your changes

The problem can be reproduced by code below.

	sqltrace.Register("mysql", mysql.MySQLDriver{})
	_, err := sqltrace.Open("mysql", "user:password@tcp(localhost:3306)/db")
	fmt.Printf("err with mysql.MySQLDriver{} %v\n", err)

This print panic: sqltrace.OpenDB: driver is not registered via sqltrace.Register with stacktrace.

The error can be avoided by passing by pointer like below.

	sqltrace.Register("mysql", &mysql.MySQLDriver{})
	_, err2 := sqltrace.Open("mysql", "user:password@tcp(localhost:3306)/dbe")
	fmt.Printf("err with &mysql.MySQLDriver{} %v\n", err2)

This print err with &mysql.MySQLDriver{} <nil>.

Reviewer's Checklist

  • If known, an appropriate milestone has been selected; otherwise the Triage milestone is set.
  • Changed code has unit tests for its functionality.
  • If this interacts with the agent in a new way, a system test has been added.

@tsbkw tsbkw marked this pull request as ready for review January 16, 2023 08:30
@tsbkw tsbkw requested a review from a team January 16, 2023 08:30
@dianashevchenko dianashevchenko added this to the v1.49.0 milestone Mar 16, 2023
@dianashevchenko dianashevchenko merged commit 3895e95 into DataDog:main Mar 16, 2023
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.

None yet

3 participants