Skip to content
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

Add pgx pool acquire tracer #2008

Merged
merged 7 commits into from
May 15, 2024
Merged

Conversation

ngavinsir
Copy link
Contributor

resolves #1747

@jackc
Copy link
Owner

jackc commented May 10, 2024

Looks pretty good, but I have a few suggestions.

I think that TraceAcquireStart and TraceAcquireEnd should receive the *Pool as an argument. This would match the precedent set by QueryTracer of passing the item that is being traced. This would also mean that TraceAcquireStartData would not need to include ConnConfig. TraceAcquireStartData would still exist for possible future changes, but it would be empty.

It's a little confusing at first whether TraceAcquireEnd is called when the connection is acquired or when the acquisition is complete (i.e. when it is released). I think the docs should make that clearer. TraceAcquireEndData should probably also include the *pgx.Conn.


This might be a separate feature, but it might also make sense to have a ReleaseTracer interface to allow tracing the entire time the the connection is acquired. In addition to the normal metrics usage of tracing, this would allow building diagnostics that would make it easier to track down connection leaks.

@ngavinsir
Copy link
Contributor Author

ngavinsir commented May 12, 2024

@jackc updated, but not sure how to pass the current context to the release method, so it can have the same parent trace as the acquire one. Is it ok to save it in the conn struct?

@jackc
Copy link
Owner

jackc commented May 12, 2024

@ngavinsir I guess the release tracer is a bit trickier than I thought.

It is, perhaps, a little weird to use the context from Acquire for the Release. But I suppose context is where things like opentelemetry are stored so it is necessary. 🤔 At least it should be documented that it is the Acquire Context and/or made part of TraceRelease(Start|End)Data. Storing it in the pgxpool.Conn is fine. It's private so we can refactor it if necessary.

I also wonder if release tracing needs Start and End. Release should be consistently extremely fast and non-blocking. Perhaps it only needs one hook?

@jackc
Copy link
Owner

jackc commented May 12, 2024

🤔 And on even further reflection, maybe the release tracer doesn't need context at all. As of 6f0deff there is a custom data map available on connections. Any release tracer that needed it could simply store it there.

@ngavinsir
Copy link
Contributor Author

@jackc I see, so for example someone can save the passed context on TraceAcquireEnd in the TraceAcquireEndData.Conn.PgConn().CustomData() and access it again inside trace release from TraceReleaseData.Conn.PgConn().CustomData()

@jackc jackc merged commit 532bf8f into jackc:master May 15, 2024
14 checks passed
@jackc
Copy link
Owner

jackc commented May 15, 2024

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

add the Acquire connection tracer
2 participants