-
Notifications
You must be signed in to change notification settings - Fork 435
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/gorm.io/gorm.v1: Add err check #940
Conversation
When will this PR be merged, it's very usefull |
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.
👋 @vmlellis sorry for the delay on this. I work on the profiler, but I just took a quick look to see if I can help getting this unstuck. PTAL at my comments, addressing them should help with getting this merged.
contrib/gorm.io/gorm.v1/gorm.go
Outdated
span.Finish(tracer.WithError(db.Error)) | ||
defer span.Finish() | ||
if cfg.errCheck(db.Error) { | ||
span.SetTag(ext.Error, 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.
Is there a reason for using SetTag()
instead of tracer.WithError()
here? If not, the latter would probably be preferable as it reduces the burden on the reviewer to verify that the two are interchangable.
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.
@felixge Maybe there ref
dd-trace-go/contrib/jinzhu/gorm/gorm.go
Line 141 in 63dea35
span.SetTag(ext.Error, scope.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.
I changed to this:
var dbErr error
if cfg.errCheck(db.Error) {
dbErr = db.Error
}
span.Finish(tracer.WithError(dbErr))
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.
I have a few comments.
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
@felixge Hi, Can you help to take a look at this or? At present, datadog will record db |
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.
Sorry it took a while to get back to this.
There is one nitpick I have and then I will merge this.
Co-authored-by: Kyle Nusbaum <kyle.nusbaum@datadoghq.com>
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.
Add
WithErrorCheck
option.An implementation of #915