proposal: integrate with github.com/jackc/pgx

[rakshans1]

How to integrate with https://github.com/jackc/pgx ?

[gbbr]

We'd have to look at that library and see how it would be possible to integrate. If you want to help, best way to start is:

1. Look at our current integrations
2. Check if our current database/sql integration doesn't already supply everything needed to be used with this library.
3. Come back here and propose a plan in the issue.
4. Once we all agree, work can be started.

I'm not sure when we'll have time to look at all this, but contributions are welcome if you wish to help. Once you've found a way, just let us know here in the issue and we'll figure out a plan for a PR.

[petejkim]

👍

[drmaples]

Any idea how to convert the traced db connection to a pgx flavor? I thought Conn.Raw would do the trick, but I am probably doing something wrong.

package main

import (
	"context"
	"log"

	"github.com/jackc/pgx/v4"
	"github.com/jackc/pgx/v4/stdlib"
	sqltrace "gopkg.in/DataDog/dd-trace-go.v1/contrib/database/sql"
)

func main() {
	// docker run -it -p 5432:5432 postgres:10.11
	sqltrace.Register("postgres", stdlib.GetDefaultDriver())

	db, err := sqltrace.Open("postgres", "postgres://postgres:postgres@localhost:5432/postgres")
	if err != nil {
		log.Fatalf("problem opening db: %v", err)
	}

	conn, err := db.Conn(context.Background())
	if err != nil {
		log.Fatalf("problem getting conn: %v", err)
	}

	conn.Raw(func(driverConn interface{}) error {
		log.Printf("type=%T", driverConn) // *sql.tracedConn

		c, ok := driverConn.(stdlib.Conn) // nope :(
		log.Printf("type=%T, ok=%v", c, ok)

		c2, ok2 := driverConn.(pgx.Conn) // nope :(
		log.Printf("type=%T, ok=%v", c2, ok2)

		// i need to get a pgx conn -> conn.PgConn().CopyTo(...)
		return nil
	})
}

[felixge]

@drmaples I'm not sure this is possible right now.

That being said, I totally understand the problem and think we should figure out a way to fix it. Perhaps one thing we could do is make *sql.tracedConn a public type that exposes a method for getting the real underlaying pgx connection. You wouldn't get APM tracing on it, but your CopyTo() use case should work.

Anyway, I haven't fully thought this through, Go's sql interface abstractions are a little gnarly and adding pgx/dd into the mix doesn't make it easier 🙈.

[meatballhat]

I'm attempting to make this, although I feel like I'm learning more about how pgx could be modified to allow a less gnarly integration 😂 https://github.com/DataDog/dd-trace-go/compare/v1...rstudio:contrib-jackc-pgx. If anyone else sees this comment and is already doing the work, I would love to know how I can help. Alternatively, I welcome help at the linked fork ❤️

[meatballhat]

The fully wrapped version works, but is an anxiety-inducing amount of code. This attempt uses the existing pgx logger capabilities: https://github.com/DataDog/dd-trace-go/compare/v1...rstudio:contrib-jackc-pgx-logger

[felixge]

@meatballhat IDK, doesn't seem to be too bad in terms of LoC 🙃 . Quick Q: How do the spans created in the wrapped logger know the start time? Is that getting injected in the context already somehow? Do you have a full example that's using this?

[meatballhat]

@felixge I'm glad you're less irked by the LoC 😂! The fully wrapped approach is (AFAICT?) going to provide deeper insights than the wrapped logger approach.

You're absolutely right about the timings being fishy in the logger-based version -- it's going to be an approximation at best. I'm seeing behaviors where the pgx-level spans are basically offering no insights into time percentage of parent span 🙁 I could see it being a reasonable option to offer for folks who want some tracing information, but with significantly less risk of encountering buggy behaviors specific to the tracing layer.

I've got full examples in a private repo, and I can get public examples up for both branches 👍🏼

[felixge]

@felixge I'm glad you're less irked by the LoC 😂! The fully wrapped approach is (AFAICT?) going to provide deeper insights than the wrapped logger approach.

Oh wait, I clicked only one of the two links : p. The full wrapper is pretty excessive indeed 🙈. From my PoV that would only be acceptable if it would be automatically generated in order to keep up with upstream.

the timings being fishy in the logger-based version

What timings are you seeing? Are they plausible at all? I'm still not sure how you get any timings without instrumenting both start + end of the operation?

[knusbaum]

@drmaples I don't think there's a way to get at the underlying driver.Conn inside the traced one. We should export that type and provide access to the connection.

[meatballhat]

@felixge Sorry for my delay. The timings are plausible only in that they aren't longer than the parent spans 😐 It's not great. I'd be up for trying a generated fully-wrapped version.

[felixge]

@meatballhat ok, let's see what @knusbaum thinks. I'm technically working on the profiler package and not the tracer. But in my previous job I used PostgreSQL and pgx heavily, so I'd love to see better support for it at Datadog : )

[knusbaum]

It would be preferable not to have to maintain a complete wrapper for the pgx. We tend to do that as a last resort. It looks like pgx has an open issue for opencensus looking for similar functionality.

I've added my own comment there. https://github.com/jackc/pgx/issues/853

Let's see if they have any insight.

[felixge]

@knusbaum good call. I've added a thumbs up on your comment.

[duxing]

any update on this thread? I would love to use ddtrace to trace pgx but nothing is provided out of the box.

[vadimi]

pgx v5 finally added tracing capabilities - https://github.com/jackc/pgx/blob/master/tracer.go, so it should be now possible to implement this integration

[mrkagelui]

if no one else has time, I'll try to tackle this

[knusbaum]

Based on this comment, this may be possible in pgx v5: https://github.com/jackc/pgx/issues/853#issuecomment-1312454035

[mrkagelui]

I've done some preliminary work here: https://github.com/DataDog/dd-trace-go/pull/1537

but dd-trace-go supports two Go versions back, i.e., 1.17, but pgx v5 requires 1.18, so we need to wait until 1.20 comes out...

[knusbaum]

@mrkagelui You should be able to use build constraints to only build for go1.18+

[mrkagelui]

@mrkagelui

You should be able to use build constraints to only build for go1.18+

Yes, however it's this lib's policy that two versions back are supported. Even if I manage to finish everything, it can't be merged

[darccio]

#2410 was merged yesterday into main. It'll be released in the next weeks as part of v1.61.0.