contrib/jackc/pgx/v5: add initial support #2160
Conversation
Add support for utilizing the integrated tracing functionality in github.com/jackc/pgx/v5, rather than having to go through the database/sql interface.
|
We definitely have our eyes on this after several requests for a proper pgx integration. We definitely want to see this through. Just wanted to ask @j-sv if it's ok to contribute directly to your branch to format things and add other pieces of code that we require in all our integrations. |
|
@zarirhamza, of course! |
There was a problem hiding this comment.
This is just a very initial review, @zarirhamza and @rarguelloF will give a more complete review next week. Thanks again for opening this contribution!!
contrib/jackc/pgx/v5/example_test.go
Outdated
| defer db.Close(ctx) | ||
|
|
||
| // Any calls made to the database will be traced as expected. | ||
| rows, err := db.Query(ctx, "SELECT name FROM users WHERE age=?", 27) |
There was a problem hiding this comment.
| rows, err := db.Query(ctx, "SELECT name FROM users WHERE age=?", 27) | |
| rows, err := db.Query(ctx, "SELECT name FROM users WHERE age=$1", 27) |
contrib/jackc/pgx/v5/options.go
Outdated
|
|
||
| type Option func(*tracer) | ||
|
|
||
| func WithServiceName(name string) Option { |
There was a problem hiding this comment.
Let's add godoc comments to all these public functions
There was a problem hiding this comment.
Updated with comments and tried to add some examples.
contrib/jackc/pgx/v5/trace.go
Outdated
| return ctx | ||
| } | ||
|
|
||
| opts := t.spanOptions( |
There was a problem hiding this comment.
The now time is the default so no need to create and use this option here
There was a problem hiding this comment.
Removed the start time from all span options.
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
|
@ajgajg1134 Thank you for your attention, how's it coming? Additionally, I appreciate to your contribution, @j-sv |
|
Just an update for everyone watching, we decided to continue this on another PR #2236 because it required a bit more work to be more complete especially on the testing side. We will continue using the bulk of the tracing capabilities from this PR which was a massive contribution, so thank you! |
|
@zarirhamza should we just go ahead and close this PR then, to clean up the backlog a bit? |
|
Moved to separate PR #2236 so closing this one. |
What does this PR do?
Like #1840, this is based on #1537 in an attempt to get support for this merged. It implements all the tracer interfaces in
jackc/pgx/v5in addition to theQueryTracer. It adds unit tests and examples for the connection and pool usage.Tracers other than
QueryTracerare disabled by default but can be enabled by options.This PR should close #697.
Motivation
Reviewer's Checklist