-
Notifications
You must be signed in to change notification settings - Fork 649
Track used database conns with an histogram #3829
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
Conversation
r? @smarnach (rust-highfive has picked a reviewer for you, use r? to override) |
The histogram should allow us to determine more accurately how many connections are used, as the older approach of taking a sample whenever metrics are scraped does not work well.
b3c53e8
to
5ad915f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good from what I can tell. at least I didn't see any obvious flaws :D
feel free to r=me
time_to_obtain_connection_metric, | ||
} => time_to_obtain_connection_metric.observe_closure_duration(|| { | ||
// Record the number of used connections before obtaining the current one. | ||
let state = pool.state(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume accessing pool.state()
is an inexpensive operation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This just requires obtaining a mutex lock, so shouldn't be expensive.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
requires obtaining a mutex lock
sounds pretty expensive to me 😅
LGTM too! @bors r=Turbo87 |
📌 Commit 5ad915f has been approved by |
☀️ Test successful - checks-actions |
…ietroalbini Revert "Track used database conns with an histogram" Reverts #3829 This has been causing issues in production: 
While looking at the metrics, me and @jtgeibel noticed that the way we tracked used database connections before (sampling the current state every time metrics are scraped) is not really accurate, as the connections could be all used up until the scrape happens. This PR adds another metric recording the used database connections with an histogram.
This PR also refactors how histograms are defined in the source code, to account for different histograms needing different buckets.