Add distributed tracing support - #639
Conversation
fabianfett
left a comment
There was a problem hiding this comment.
Hi @0xpablo, thanks for taking the time to build this feature. This PR is massive and will require significant attention from my side to land.
However, I would love if we landed the following features before this:
- structured query support, so that we don't rely on
deinits for object lifecycles going forward. This will mean that we havequeryfunctions that use a closure - a clear document describing, which spans, we want to see. I image a doc outlining what the root span for the
PostgresClientshould be and what the sub spans should be.
We should then land the changes one by one.
|
Hi @fabianfett, thanks for the feedback. That makes sense 馃憤 I opened this mainly because we need this level of observability in production downstream, and I wanted to get early upstream feedback on whether this could fit here and in what shape. I agree it should not land as one large PR. I also agree that structured query support sounds like the right foundation first. I'm happy to help with that work first. If you already have a preferred direction for the closure based query API, I'd appreciate a bit of guidance so I can align with it. Otherwise, I'm happy waiting until that work happens and rework this on top of that (splitting things in smaller PRs as well). Thanks! |
This PR adds initial distributed tracing support to PostgresNIO.
The main goal here is to make it easier for applications to see database work inside end-to-end traces without forcing a specific observability backend or adding much complexity to the default path.
What's included
COPY FROM, and row streaming where the operation crosses async boundariesA couple of behavior notes
exactby default, sodb.operation.nameis only emitted when the library knows the operation exactly.inferredfor applications that want compatibility with observability backends that group queries by verbI tried to keep the implementation conservative by default and avoid introducing SQL parsing complexity into the main path, while still leaving room for compatibility with existing tracing setups.
We've been testing these changes for about a week now with a DataDog OTel collector in a service that has ~2K queries per minute and it's been super useful to improve performance.
I realize this may make more sense once the package has moved further toward full Swift concurrency adoption. If you'd prefer to hold off on merging tracing until then, I completely understand. I'd be happy to revisit and update this PR once that work is in place.