contrib/jackc/pgx: add initial support #1537

mrkagelui · 2022-10-25T14:00:10Z

fixes #697

mrkagelui · 2022-10-25T14:02:26Z

this is merely the first step, before I add tests and implement other tracer interface, I want to gather some feedback on the API as well as the data being traced

mrkagelui · 2022-10-25T14:03:30Z

one more question is, this requires pgx v5+ which requires go 1.18+, is this ok, or do we need to wait for go 1.20 as the lib still supports 1.17?

ajgajg1134 · 2022-10-25T14:11:40Z

If it doesn't build / work at all with Go 1.17 then unfortunately this will need to wait until we remove support for Go 1.17 with the release of Go 1.20.

mrkagelui · 2022-10-25T14:15:25Z

If it doesn't build / work at all with Go 1.17 then unfortunately this will need to wait until we remove support for Go 1.17 with the release of Go 1.20.

I see, as generics is used, I suppose this will have to wait... but do you mind taking a look at the API or the data collected?

ajgajg1134

Overall looks like a step in a good direction for supporting pgx v5 directly. I've left a couple comments here and there.

I wonder if there's a way to reduce the duplication introduced between this and the contrib/database/sql package, I'm not sure there's a clean way to do it, but it would make future changes to SQL support easier/less brittle.

ajgajg1134 · 2022-10-25T14:30:42Z

contrib/jackc/pgx/pgx.go

+)
+
+// TracedConn returns a traced *pgx.Conn
+// note that connect trace are not effective this way


I'm not sure what's meant by this comment?

see https://github.com/jackc/pgx/blob/987de3874e0fc18b8c468d27cb9de9b2cd67540c/conn.go#L106. basically, the connection string used in Connect simply can't specify the tracer. the connection has been established before adding the tracer, hence the "Connect" span wouldn't be added in this way. I hesitated when providing this, but decided to include it to provide "compatibility"

Hi, I've corrected the implementation, now it parses the conn string and update the tracer so the comment is obsolete and removed. FYI

ajgajg1134 · 2022-10-25T14:49:55Z

contrib/jackc/pgx/trace.go

+}
+
+// TraceQueryStart marks the start of a query, implementing pgx.QueryTracer
+func (t *tracer) TraceQueryStart(ctx context.Context, _ *pgx.Conn, data pgx.TraceQueryStartData) context.Context {


Can you add unit and integration tests for these? (See contrib/database/sql for example tests)

Also I think a short example file showing how it's expected to be used would be very helpful

will do them all once I have some feedback from more experienced user of pgx, thanks!

ajgajg1134 · 2022-10-25T14:50:27Z

contrib/jackc/pgx/trace.go

+	}
+
+	if t.traceStatus {
+		span.SetTag("pq.status", data.CommandTag.String())


should this be pgx.status?

hmm, I meant that "this is a status from postgresql", rather than specifically from pgx, what do you think?

ajgajg1134 · 2022-10-25T14:53:44Z

contrib/jackc/pgx/trace.go

+
+// tracer contains configs to tracing and would implement QueryTracer, BatchTracer,
+// CopyFromTracer, PrepareTracer and ConnectTracer from pgx.
+type tracer struct {


I think a better name here might be config to align with how the contrib/database/sql package works.

I thought of that, but the pgx interface works a bit differently 🤔 it exposes a series of interfaces and calls them in the queries/connections etc. I suppose it's a bit odd to make a config satisfy a QueryTracer/BatchTracer etc?

ajgajg1134 · 2022-10-25T15:00:03Z

contrib/jackc/pgx/pgx.go

+}
+
+// TracedPool returns a traced *pgxpool.Pool for concurrent use
+// note that connect traces are not effective this way


What is meant by "connect traces are not effective" As in the connect span doesn't work for pooled connections?

similar to the above, see https://github.com/jackc/pgx/blob/987de3874e0fc18b8c468d27cb9de9b2cd67540c/pgxpool/pool.go#L159, basically with connection strings, tracers can't be specified and hence the "connect" trace can't be collected this way

mrkagelui · 2022-10-25T15:26:35Z

Overall looks like a step in a good direction for supporting pgx v5 directly. I've left a couple comments here and there.

I wonder if there's a way to reduce the duplication introduced between this and the contrib/database/sql package, I'm not sure there's a clean way to do it, but it would make future changes to SQL support easier/less brittle.

thank you for the comments! let me address them all, in the mean time, I'd like to explain that pgx basically provides a different and more performant interface, other than database/sql. using database/sql would defeat the purpose and in fact rather unnecessary (as github.com/jackc/pgx/v5/stdlib can directly be used). feel free to continue discussing this!

mrkagelui · 2022-10-25T15:49:50Z

allow me to elaborate a bit more about pgx vs database/sql. see https://pkg.go.dev/github.com/jackc/pgx/v5/stdlib#pkg-overview, you could use pgx with that interface, but see https://pkg.go.dev/github.com/jackc/pgx/v5#pkg-overview,

pgx provides a native PostgreSQL driver and can act as a database/sql driver. The native PostgreSQL interface is similar to the database/sql interface while providing better speed and access to PostgreSQL specific features.

in other words, it provides features and performance that couldn't be utilized if you use database/sql interface through it, hence this is necessary.

mrkagelui · 2022-11-06T14:07:03Z

converting to draft since we can only talk about it when go 1.20 comes out

katiehockman · 2023-02-06T19:55:20Z

converting to draft since we can only talk about it when go 1.20 comes out

Go 1.20 is now out and is compatible with dd-trace-go, so should this PR be moved out of draft at this time?

mrkagelui · 2023-02-08T03:13:39Z

converting to draft since we can only talk about it when go 1.20 comes out

Go 1.20 is now out and is compatible with dd-trace-go, so should this PR be moved out of draft at this time?

thanks for reminding, I'll make time for this!

doron-cohen · 2023-03-15T10:07:22Z

Hey @mrkagelui tell me if you need help with pushing this PR forward

mrkagelui · 2023-03-22T11:06:25Z

Hey @mrkagelui tell me if you need help with pushing this PR forward

hey thanks for reminding @doron-cohen , been sick last week and work and life have been both crazy, I think I'd have some bandwidth next month.

anyone feel free to take over.

statik · 2023-03-29T15:22:52Z

@mrkagelui thank you for your work on this! I would like to help, I've prepared a PR that includes the content from this one over at #1840. So far it only includes a merge from main and is moved out of the draft state so that upstream CI tests can run. I'm happy to hand back over to you at any point if you get time.

mrkagelui · 2023-03-30T02:53:05Z

thank you so much @statik ! unfortunately I would only have time in the next month. please kindly carry on if you can. I'll contribute there if needed.

xzile · 2023-04-22T14:10:32Z

contrib/jackc/pgx/pgx.go

+	for _, opt := range options {
+		opt(t)
+	}
+	p.Config().ConnConfig.Tracer = t


During our implementation of dd-tracer-go and pgx, we had to do something like:

config := p.Config() config.ConnConfig.Tracer = t poolWithTracer, err := pgxpool.NewWithConfig(ctx, config)

Pretty new to Go so I can't tell you exactly why, but the pointer reference from p.Config() was different than p, so editing that config didn't affect the original pool, so recreating the pool with the updated config was necessary to get things working.

thanks for pointing this out, I think it has to do with the fact that the conn configs can't really be modified on the fly (for concurrency reasons). I'll use this.

if you are still interested @xzile , kindly see my updated implementation, thanks!

👍 Looks good.

devwells · 2023-07-27T18:45:32Z

@mrkagelui PR #1840 has been closed. Would you be open to moving this one out of the draft state?

sudotliu · 2023-09-28T13:28:25Z

This is really great and looks like it's really close! I'm an eager adopter as well and would really appreciate it if anyone has a rough idea of when it can be expected to make it in just so we can set the right expectations and avoid over-investing in other workarounds. Much thanks all around!

samcfinan · 2023-12-13T04:17:55Z

This looks great! Are there any plans to move this along into a review state? @mrkagelui

mrkagelui · 2023-12-27T02:27:39Z

Hey sorry everyone who's watching, I recently could finally find some time to work on this. let me know if anyone else could help (to review etc). I'll see if it's already done first.

mrkagelui · 2023-12-27T11:55:45Z

update: I've implemented all tracer interfaces in pgx, will add tests later. I'd appreciate it if anyone can help to see if the data points in the implementation is sufficient, or if the logic looks right.

doron-cohen · 2024-01-25T20:54:21Z

@ajgajg1134 what has to happen next for this to get into the library? We want to migrate from pq which is unmaintained but DB query tracing is very important to us. We are considering to move to the OTEL client just to allow it but I see this PR is in final steps, no?

zarirhamza · 2024-01-29T16:22:09Z

We acknowledge the significant amount of people that want to see this PR through and we'll be taking a closer look soon before trying to finalize it

darccio · 2024-02-08T11:05:30Z

Closing this PR in favour of #2410.

mrkagelui requested a review from a team October 25, 2022 14:00

mrkagelui requested a review from a team as a code owner October 25, 2022 14:00

mrkagelui force-pushed the feat-support-pgx branch from dc04df1 to df5a4e2 Compare October 25, 2022 14:01

mrkagelui mentioned this pull request Oct 25, 2022

How to improve logging and tracing support in future release? jackc/pgx#1061

Closed

mrkagelui force-pushed the feat-support-pgx branch from df5a4e2 to b300a57 Compare October 25, 2022 14:07

mrkagelui changed the title ~~contrib/jackc/pgx: add API and implement QueryTracer~~ contrib/jackc/pgx: add support Oct 25, 2022

mrkagelui changed the title ~~contrib/jackc/pgx: add support~~ contrib/jackc/pgx: add initial support Oct 25, 2022

mrkagelui force-pushed the feat-support-pgx branch from b300a57 to 91ed48b Compare October 25, 2022 14:12

ajgajg1134 reviewed Oct 25, 2022

View reviewed changes

ajgajg1134 added this to the Triage milestone Oct 25, 2022

mrkagelui marked this pull request as draft November 6, 2022 14:06

mrkagelui mentioned this pull request Dec 27, 2022

proposal: integrate with github.com/jackc/pgx #697

Closed

ahmed-mez added the apm:ecosystem contrib/* related feature requests or bugs label Mar 27, 2023

statik mentioned this pull request Mar 29, 2023

contrib/jackc/pgx: add initial support #1840

Closed

3 tasks

xzile reviewed Apr 22, 2023

View reviewed changes

j-sv mentioned this pull request Aug 1, 2023

contrib/jackc/pgx/v5: add initial support #2160

Closed

3 tasks

mrkagelui force-pushed the feat-support-pgx branch 6 times, most recently from cfb902e to b070e84 Compare December 27, 2023 03:38

mrkagelui added 2 commits December 27, 2023 21:37

contrib/jackc/pgx: add API and implement QueryTracer

c9d2cb5

contrib/jackc/pgx: implement Connect/Batch/CopyFrom/Prepare tracers

6bb79bb

mrkagelui force-pushed the feat-support-pgx branch from bbac71d to 6bb79bb Compare December 27, 2023 13:38

mrkagelui marked this pull request as ready for review December 27, 2023 13:38

mrkagelui mentioned this pull request Dec 29, 2023

why does BatchTracer not have TraceBatchQueryStart/TraceBatchQueryEnd? jackc/pgx#1857

Closed

darccio closed this Feb 8, 2024

contrib/jackc/pgx: add initial support #1537

contrib/jackc/pgx: add initial support #1537

Conversation

mrkagelui commented Oct 25, 2022

mrkagelui commented Oct 25, 2022

mrkagelui commented Oct 25, 2022

ajgajg1134 commented Oct 25, 2022

mrkagelui commented Oct 25, 2022

ajgajg1134 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrkagelui commented Oct 25, 2022

mrkagelui commented Oct 25, 2022 • edited Loading

mrkagelui commented Nov 6, 2022

katiehockman commented Feb 6, 2023

mrkagelui commented Feb 8, 2023

doron-cohen commented Mar 15, 2023

mrkagelui commented Mar 22, 2023

statik commented Mar 29, 2023

mrkagelui commented Mar 30, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devwells commented Jul 27, 2023

sudotliu commented Sep 28, 2023

samcfinan commented Dec 13, 2023

mrkagelui commented Dec 27, 2023

mrkagelui commented Dec 27, 2023

doron-cohen commented Jan 25, 2024

zarirhamza commented Jan 29, 2024

darccio commented Feb 8, 2024

mrkagelui commented Oct 25, 2022 •

edited

Loading