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 tokio-tracing #174

Open
bikeshedder opened this issue Jan 3, 2022 · 10 comments
Open

Add tokio-tracing #174

bikeshedder opened this issue Jan 3, 2022 · 10 comments
Labels
A-core Area: Core / deadpool enhancement New feature or request
Milestone

Comments

@bikeshedder
Copy link
Owner

This is related to #67

@bikeshedder bikeshedder added enhancement New feature or request A-core Area: Core / deadpool labels Jan 3, 2022
@bikeshedder bikeshedder added this to the 1.0.0 milestone Jan 3, 2022
@elpiel
Copy link
Contributor

elpiel commented Jul 14, 2022

Another consideration for this issue would be to allow tracing on the postgres Pool.
I'm having issue in our async application because we can't use #[instrument] attr. macro to instrument methods and functions which use the postgres Pool because ClientWrapper & Manager do not impl Debug.

I saw in the code that StatementCache does not impl Debug because of the Statement.

What would be a good way to go around this?

@bikeshedder
Copy link
Owner Author

@elpiel I just added a PR adding Debug to all deadpool-postgres types. Would this be helpful to you?

@elpiel
Copy link
Contributor

elpiel commented Jul 14, 2022

Yes! That's actually a great start, thank you! I was looking into implementing Debug as well but you were very fast to do it 😅

Here's also an idea - rather than not showing the fields that don't have Debug, we can just use a string representation of the type?

@HigherOrderLogic
Copy link
Contributor

Any update on this?

@elpiel
Copy link
Contributor

elpiel commented Nov 25, 2022

@HigherOrderLogic it should be possible to replace log with tracing (https://docs.rs/tracing) in the necessary crates.
E.g. for deadpool-postgres there are exactly 3 places that need to be replaces:

https://github.com/bikeshedder/deadpool/blob/ddd9eafa39e0c823315c3b2bff3767abafa0b05f/postgres/src/lib.rs

@bikeshedder
Copy link
Owner Author

I'm a tracing-noob so to say. What tracing/logging messages would be needed in deadpool to be more useable?

@bikeshedder
Copy link
Owner Author

I'm currently in process of a 0.10 release and if replacing log by tracing solves 90% of the pain I'm more than happy to include it in this release: https://github.com/bikeshedder/deadpool/tree/milestone/v0.10

@elpiel
Copy link
Contributor

elpiel commented Nov 25, 2022

A good starting point IMO is to just replace log with tracing everywhere.
I don't know what new logging messages would be beneficial though.

@HigherOrderLogic
Copy link
Contributor

I'm currently in process of a 0.10 release and if replacing log by tracing solves 90% of the pain I'm more than happy to include it in this release: milestone/v0.10

Thanks for the update. Can I open a PR to replace log with tracing as the first step?

@bikeshedder
Copy link
Owner Author

Sure. Please create a PR against the milestone branch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-core Area: Core / deadpool enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants