Skip to content

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

Merged
merged 2 commits into from
Aug 19, 2021

Conversation

pietroalbini
Copy link
Member

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.

@rust-highfive
Copy link

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.
@Turbo87 Turbo87 added A-backend ⚙️ C-internal 🔧 Category: Nonessential work that would make the codebase more consistent or clear labels Aug 18, 2021
Copy link
Member

@Turbo87 Turbo87 left a 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();
Copy link
Member

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?

Copy link
Member

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.

Copy link
Member

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 😅

@jtgeibel
Copy link
Member

LGTM too!

@bors r=Turbo87

@bors
Copy link
Contributor

bors commented Aug 19, 2021

📌 Commit 5ad915f has been approved by Turbo87

@bors
Copy link
Contributor

bors commented Aug 19, 2021

⌛ Testing commit 5ad915f with merge 2676ea6...

@bors
Copy link
Contributor

bors commented Aug 19, 2021

☀️ Test successful - checks-actions
Approved by: Turbo87
Pushing 2676ea6 to master...

@bors bors merged commit 2676ea6 into rust-lang:master Aug 19, 2021
@pietroalbini pietroalbini deleted the pa-db-pool-histogram branch August 22, 2021 11:05
bors added a commit that referenced this pull request Aug 22, 2021
…ietroalbini

Revert "Track used database conns with an histogram"

Reverts #3829

This has been causing issues in production:

![image](https://user-images.githubusercontent.com/141300/130358224-673c5126-622d-4a92-bbcf-5fa95404533a.png)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-backend ⚙️ C-internal 🔧 Category: Nonessential work that would make the codebase more consistent or clear
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants