contrib/jackc/pgx: add initial support #1840
Conversation
statik
changed the title
|
Here is a quick sample app that uses this tracing support: https://github.com/statik/pgx-ddtrace-example To set up for the sample app I run the data dog agent and Postgres on my dev machine. Then I run |
| } | ||
|
|
||
| // TraceArgs will report the arguments of the queries, if on is set to true | ||
| func TraceArgs(on bool) Option { |
There was a problem hiding this comment.
It seems the library separates the SQL statement and the arguments. However, we already inherently obfuscate the arguments from the statement to prevent security leaks and we wouldn't the same issues by exposing the arguments as a tag, so this field should be omitted
| } | ||
|
|
||
| // TraceStatus will report the status of the queries, if on is set to true | ||
| func TraceStatus(on bool) Option { |
There was a problem hiding this comment.
Is there a reason that this information is gated behind a boolean? It seems like a person would always want this information so we should either always set it as a tag or omit it as well
| ) | ||
|
|
||
| // TracedConn returns a traced *pgx.Conn | ||
| // note that connect trace are not effective this way |
| } | ||
|
|
||
| // TracedPool returns a traced *pgxpool.Pool for concurrent use | ||
| // note that connect traces are not effective this way |
| ddtracer.Tag("sql.query_type", "Query"), | ||
| ddtracer.Tag(ext.ResourceName, data.SQL), | ||
| } | ||
| if t.traceArgs { |
There was a problem hiding this comment.
As mentioned before, should be omitted
| } | ||
|
|
||
| if t.traceStatus { | ||
| span.SetTag("pq.status", data.CommandTag.String()) |
There was a problem hiding this comment.
Should be pgx so we know that it's from this library
| ddtracer.SpanType(ext.SpanTypeSQL), | ||
| ddtracer.StartTime(time.Now()), | ||
| ddtracer.Tag("sql.query_type", "Query"), | ||
| ddtracer.Tag(ext.ResourceName, data.SQL), |
There was a problem hiding this comment.
| ddtracer.Tag(ext.ResourceName, data.SQL), | |
| ddtracer.Tag(ext.ResourceName, data.SQL), | |
| ddtracer.Tag(ext.Component, "jackc/pgx"), | |
| ddtracer.Tag(ext.SpanKind, ext.SpanKindClient), |
Component and SpanKind tags are necessary for all components
| } | ||
|
|
||
| // TracedPoolWithConfig returns a traced *pgxpool.Pool with config, for concurrent use | ||
| func TracedPoolWithConfig(ctx context.Context, config *pgxpool.Config, options ...Option) (*pgxpool.Pool, error) { |
There was a problem hiding this comment.
Hey, thanks for the effort on implementing this.
Out of curiosity, what happens if someone is trying to use TraceLog?
May I suggest that the library could check if the config already has a Tracer set and wrap it if true?
| for _, opt := range options { | ||
| opt(t) | ||
| } | ||
| p.Config().ConnConfig.Tracer = t |
There was a problem hiding this comment.
Accidentally left a comment on the old draft regarding this.
#1537 (comment)
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.
|
@statik are you still able to work on this PR? |
|
@devwells unfortunately I no longer have time to work on this, closing. |
What does this PR do?
The content of this PR is from #1537. I'm trying to help move this contribution forward in response to the request for help in the original PR. DD support asked me to get a PR that is not in draft state so that the tests can run.
This PR fixes #697
Motivation
Describe how to test/QA your changes
Reviewer's Checklist
Triagemilestone is set.