-
Notifications
You must be signed in to change notification settings - Fork 295
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
initial cut at database/sql compatible pgx integration #363
Conversation
// Use this package to instrument your PostgreSQL calls using the pgx | ||
// library. | ||
// | ||
// USING WITH PGX AS A DATABASE/SQL DRIVER |
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.
You think perhaps we need another section here to describe how it works with SQLx as well?
{ | ||
dsn: "host=host_string port=port_string dbname=dbname_string", | ||
expHost: "host_string", | ||
expPortPathOrID: "5432", | ||
expDatabaseName: "dbname_string", | ||
env: map[string]string{ | ||
"PGHOST": "host_env", | ||
"PGPORT": "port_env", | ||
"PGDATABASE": "dbname_env", | ||
}, |
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 wonder why the expected Port, Path or ID is "5432", while we have an environment variable PGPORT that's set to "port_env."
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.
Same with the test below this one.
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.
What the integration code does is fall back to the default port if the provided one is not a valid port number. In this case, it's not an integer so the library uses the default Postgres port instead.
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.
Looks good! I only made a few comments. Also, tests pass and seem pretty sane.
The database/sql example works without a running database. However, the sqlx example needs a running database to function properly. I've suggested a change in this PR, Steve's working with it.
We're very interested in this integration. I have a few questions for you all. First, we use pgx
Does this New Relic pgx integration work with Second, we use https://github.com/georgysavva/scany instead of
Note that it passes an aforementioned Any idea if the New Relic integration will work with Third, this is a general question and please don't take this as a criticism! In the past I've used New Relic with Python/Django apps, and the database integration was practically transparent. Is it simply the case that that kind of non-intrusive integration isn't possible with something like Thanks for all this work! We're excited to use it. |
@marcesher wrote:
Thanks! We hope to provide what features will be useful and needed by our user community, and interactions like this are a great way for us to know what you'd most like to see.
This However, the
Since this uses pools, the same answer above would apply.
In general terms, this has more to do with the differences between Python and Go. |
// fullParamPattern is a regular expression to match the key=value pairs of a | ||
// parameterized DSN string. |
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.
Love these comments.
This adds a new integration for the PGX Postgres database driver. It instruments database transactions using PGX in the same way as we do with the PQ integration. Future work is still needed to support the other modes of operation offered by PGX.
Fixes Issue #179