expansiveworlds / instrumentedsql Goto Github PK

I have found this library very useful; thanks for creating it!

However, when I use with Jaeger on ElasticSearch, I find the SQL query tag value is truncated to 256 characters when I look at it in the Jaeger UI.

I couldn't find any definitive info on whether this is expected behaviour in Jaeger, however I did find some comments that tags should be things you can filter on, so the full SQL query is not really suitable as a tag value. It could be done as a 'log' rather than a tag.

Posting this mainly for comment; I'm not really sure what I should expect.

I tried using this library with sqlx, but I cannot get it working.
Any ideas how to solve this problem? My goal is to make instrumentedsql work with named queries.

My code
bebfore:

db, err := sqlx.Open("postgres", config.DB.URL)

after:

	sql.Register("instrumented-postgres", instrumentedsql.WrapDriver(&pq.Driver{}, instrumentedsql.WithTracer(opentracingsql.NewTracer())))
	db, err := sql.Open("instrumented-postgres", config.DB.URL)

Tracing works just fine, but now I get "pq: syntax error at end of input" errors.

Hi,

at work we use X-Ray, and using the AWS X-Ray for SQL tracing would force us to rewrite all of our code.

We are investigating the use of this package (and keeping an eye on @luna-duclos fork), but we have some questions about some of the decisions.

1. some calls that receive a query string do not include it as a label. For example, PrepareContext . Can someone elaborate why not? Is there a reason for this decision?
2. On Open doesn't it make sense to keep track of the name parameter to include in the Spans?

This is all for now :), thank you for your time.

\cc @pcruz7

Seems https://github.com/luna-duclos/instrumentedsql/ should be used instead.

This would be useful in case anyone wants to hook in something specific to their application, such as custom metrics or other stuff

Go modules is becoming the standard way of tracking dependencies, and this project could use one. For one, it seems the import cloud.google.com/go/trace is dead and needs to be tidied up/replaced with cloud.google.com/go/trace.

I just did something like this myself last night. Couple thoughts about your approach:

1. I chose to wrap the driver.Conn, rather than the driver.Driver, to deal with pq not exporting it's driver impl.
2. I was nervous about implementing all the optional driver interfaces in my wrapper, and taking on the responsibility of adapting to drivers which didn't implement all of them. Seems like golang's own sql package may add improvements or bug fixes to their adaptation code that you'll need to keep up with. But the only alternative I can see is implementing a bunch of structs which implement every possible combination of the optional interfaces.
3. I started to add more stuff to my wrapper (like metrics, logging, tracing, etc), and came to the same conclusion you did: it would be more flexible to have the wrapper call hooks. Which made me think of httptrace.

Just wanted to say thanks for starting this project!

I was exploring doing something similar by wrapping sql.DB at some point, but it turned out to be a PITA to wrap. Wrapping the drivers makes more sense!

I'll probably try to use this and contribute once I get a chance to work on this again.

Cheers
Felix