-
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: add support for github.com/go-pg/pg #686
Conversation
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.
We need an example_test.go
file too, along with documentation and copyright headers.
CI should be better once you rebase on |
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.
Thanks for contributing this.
bbdde7d
to
e5250c4
Compare
Hi guys, thank you for your feedback, hope all your feedback are now fixed, if i missed something please let me know, i'll try to fix. For now CI passed, hope everything is done. |
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.
Thanks for making the changes. Left a few more comments.
Please remember to add an example file as suggested in #686 (review) |
Will add later. |
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.
Thanks for making further changes. I hope you'll be patient with us until we go through a couple of iterations in the review process.
Can you lastly try out this integration with an actual application and perhaps post a screenshot of what the result would be in the Datadog application?
I added example file. |
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 left some comments on your example file but please spend some time and go through all comments because you have not addressed everything (e.g. @knusbaum's comments)
For example this one: #686 (comment). Best way to not miss any is to just go through them one by one, address them, and once done resolve them on Github. Please note that there will be more comments with regards to the docs and styling, I hope that's ok with you and you'll have the patience to go through them with us. |
f8292b6
to
54e6bbf
Compare
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.
@patriczek, CI shows the issue with the package path/name:
# gopkg.in/DataDog/dd-trace-go.v1/contrib/go-pg
package gopkg.in/DataDog/dd-trace-go.v1/contrib/go-pg (test)
imports gopkg.in/DataDog/dd-trace-go.v1/contrib/go-pg/pg: cannot find module providing package gopkg.in/DataDog/dd-trace-go.v1/contrib/go-pg/pg
FAIL gopkg.in/DataDog/dd-trace-go.v1/contrib/go-pg [setup failed]
For the package to be gopkg.in/DataDog/dd-trace-go.v1/contrib/go-pg/pg
, the files need to be under directory contrib/go-pg/pg/
rather than what you have currently: contrib/go-pg/
Should by fixed, i forget push last commit from 2nd pc |
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.
Thank you for working through this with us, @patriczek
I have just a few more comments around the example. We should improve the language in the comments to make sure it's very clear for users. Please see our garyburd/redigo
example for a good demonstration of a clear example with good documentation.
dd-trace-go/contrib/garyburd/redigo/example_test.go
Lines 19 to 39 in 9495abe
// To start tracing Redis commands, use the TracedDial function to create a connection, | |
// passing in a service name of choice. | |
func Example() { | |
c, err := redigotrace.Dial("tcp", "127.0.0.1:6379") | |
if err != nil { | |
log.Fatal(err) | |
} | |
// Emit spans per command by using your Redis connection as usual | |
c.Do("SET", "vehicle", "truck") | |
// Use a context to pass information down the call chain | |
root, ctx := tracer.StartSpanFromContext(context.Background(), "parent.request", | |
tracer.ServiceName("web"), | |
tracer.ResourceName("/home"), | |
) | |
// When passed a context as the final argument, c.Do will emit a span inheriting from 'parent.request' | |
c.Do("SET", "food", "cheese", ctx) | |
root.Finish() | |
} |
Hi, @patriczek. Will you have time to finish this up? I think we're nearly there. |
Hi, @knusbaum, i hope tomorrow i will have some spare time to finish it. |
28db8c1
to
45b2028
Compare
Signed-off-by: Patrik Helia <patrik.helia@kiwi.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.
This is great code-wise! Thanks for the effort. One last question about the example.
contrib/go-pg/pg.v10/example_test.go
Outdated
|
||
// Wrap the connection with the APM hook | ||
pgtrace.Wrap(conn) | ||
// For correct tracing, must be query execute with context. |
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.
Why? Won't it work without context? If not, let's correct the spelling a bit:
// For correct tracing, must be query execute with context. | |
// For queries to be traced, context must be used because ... |
If it will, let's just remove this coment.
Co-authored-by: Gabriel Aszalos <gabriel.aszalos@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.
Nice work! Thank you!
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.
🎉
In this MR i would add gopg tracing.
Signed-off-by: Patrik Helia patashelia@gmail.com