-
Notifications
You must be signed in to change notification settings - Fork 419
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/jinzhu/gorm: add gorm integration #268
Conversation
|
||
// Open opens a new (traced) database connection. The used dialect must be formerly registered | ||
// using (gopkg.in/DataDog/dd-trace-go.v1/contrib/database/sql).Register. | ||
func Open(dialect, source string) (*gorm.DB, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
gorm.Open
supports both a database string or passing the actual database through... I was wondering if we should try to do the same, but I guess there's really no value in supporting both since there's no way to take an existing db and add tracing too it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. I will try and investigate it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
I'm looking to add tracing to our database layer and we use gorm. Any updates on this and working with v1? |
@bwiggs Does this solution work for you? Am happy to merge it and get it out for 1.3.0 along with some other integration PRs that are open. |
Yip, I loaded your gbbr/gorm branch into my app and updated my |
Nice! Thanks for confirming that. |
Previous prototype of Pipeline would yield an error when trying to use the client as a redis.Cmdable. Now Pipeline returns an interface and not a concrete type.
Closes #100