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

Tracing support #1053

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Tracing support #1053

wants to merge 4 commits into from

Conversation

Protryon
Copy link

@Protryon Protryon commented Jul 24, 2023

This PR:

  • Adds tracing support following OpenTelemetry database semantic conventions
  • Adds no new dependencies (tracing was already added transitively by other dependencies)
  • Will have no performance impact if no tracer or logger is setup at a DEBUG or TRACE level.

Example trace overview on real application (vaultwarden):

direct

Breaking down to find a slow query:

direct

Note that this PR does not add distributed tracing support into the database, as it is (AFAIK) experimental and not well supported -- it would also add some extra dependencies.

Fixes #642

@sfackler
Copy link
Owner

What are some examples of other database drivers that have adopted these logging conventions?

@Protryon
Copy link
Author

@sfackler I did some digging, here is a list at the OTEL registry.

A specific example in code in the first item of using the OTEL semantic conventions, go-pg.

@titanous
Copy link

This looks fantastic! I was actually just looking at implementing this exact same thing, and you've done it better than I would have. I will apply this patch and try it out in production for an app that I work on.

@titanous
Copy link

This works great, I ended up making two changes:

  1. I set all of the spans to INFO because that is the level that we send to Honeycomb. I think at the very least prepare should be bumped to DEBUG, as trace is almost always too noisy to be useful and prepare spans are useful to me (I try to have a span for every external network call).
  2. I added a span for connect, which delivered value as soon as I deployed it. Feel free to pull in this commit and/or modify it. I have already found the sub-span timings that I added to be useful as well. titanous@7e2e3a2

@Protryon
Copy link
Author

This works great, I ended up making two changes:

  1. I set all of the spans to INFO because that is the level that we send to Honeycomb. I think at the very least prepare should be bumped to DEBUG, as trace is almost always too noisy to be useful and prepare spans are useful to me (I try to have a span for every external network call).
  2. I added a span for connect, which delivered value as soon as I deployed it. Feel free to pull in this commit and/or modify it. I have already found the sub-span timings that I added to be useful as well. titanous@7e2e3a2

I merged in that commit and bumped all the trace levels as well. Thanks!

@sfackler
Copy link
Owner

sfackler commented Jul 30, 2023

From what I can tell most or all of the libraries at that OTEL reference are external libraires that you can use with a database driver to apply tracing conventions.

I am hesitant to land this for a couple of reasons:

  1. People can be using this library as a tracing appender, and this change can potentially cause some undesired infinite logging recursion. cf Feature Request: Optionally disable use of the log crate #215
  2. This seems like a convention that your particular organization wants to use, e.g. "I set all of the spans to INFO because that is the level that we send to Honeycomb", not something that is uniformly applicable.

I think you'd probably be better served by following what those other libraries do and making a separate library that can wrap tokio-postgres and apply the appropriate tracing logic.

@titanous
Copy link

I think a separate library would be fine, though it seems like a hook system would be necessary to ergonomically log everything we have in this PR. For example, there is currently no good way for a separate library to properly trace connection attempts with all of the relevant metadata, especially with timings for each phase.

@sfackler
Copy link
Owner

That approach seems like it could work in principle - I'd be interested in what the hook trait would look like.

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.

built-in tracing support
3 participants