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 PullingGauge gauge type #405

Merged
merged 1 commit into from
Nov 15, 2022
Merged

Conversation

JanBerktold
Copy link
Contributor

First of all, thank you for this crate! We've been using it in production for a bit and are super happy.

As part of this, we've written a few additional metric types that allow us to instrument our app in an easier fashion. This PR attempts to merge the first of these back to the upstream project. If there is interest, then we'd be happy to send more PRs with other helper types (i.e. we have a MaximumOverIntervalGauge that allows monitoring of quickly changing values).

The metric type that this PR is adding is the PullingGauge. It is useful in scenarios where we want to expose a particular value but it is infeasible to keep the metric up to date all of the time (i.e. it changes a lot in hot code paths). The most common example for us internally is to expose the size of a Vector or similiar collections. In those cases, we create one of these and pass a move || vector.len() as f64 as the lambda.

I've more or less just lifted this out of our internal codebase -- please let me know in case of any changes required to make it fit better in this crate!

@JanBerktold
Copy link
Contributor Author

@lucab hi! would you mind taking a look at this PR?

@jmorag
Copy link

jmorag commented Sep 30, 2021

Just wanted to second that this functionality would be very nice to have. It exists in the go client and we miss it in rust.

@lucab lucab self-requested a review September 30, 2021 15:46
@anacrolix
Copy link

We've been using this in production for a very long time. Please consider for merging!

@JanBerktold
Copy link
Contributor Author

Hey @lucab, would it be possible for you to take a look here? Thanks!

@JanBerktold
Copy link
Contributor Author

@lucab Hey there! We'd love to get this upstreamed if possible. Is there anything else I can do to help with this?

src/pulling_gauge.rs Outdated Show resolved Hide resolved
src/pulling_gauge.rs Show resolved Hide resolved
src/pulling_gauge.rs Outdated Show resolved Hide resolved
src/pulling_gauge.rs Outdated Show resolved Hide resolved
src/pulling_gauge.rs Show resolved Hide resolved
@JanBerktold JanBerktold force-pushed the add-pulling-gauge branch 3 times, most recently from b778969 to 7afaa4b Compare November 12, 2022 00:55
@JanBerktold JanBerktold requested a review from lucab November 12, 2022 01:07
@JanBerktold
Copy link
Contributor Author

Thanks for the comments! I think I've addressed all of them now -- Please take a look, @lucab.

@anacrolix
Copy link

Thanks @lucab and @JanBerktold !

Copy link
Member

@lucab lucab left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this, just one last minor nit on the doc example.

After that, it should be ready to merge, feel free to squash all of this into a single commit.

src/pulling_gauge.rs Outdated Show resolved Hide resolved
Signed-off-by: Jan Berktold <jberktold@roblox.com>
@JanBerktold
Copy link
Contributor Author

Thanks for this, just one last minor nit on the doc example.

After that, it should be ready to merge, feel free to squash all of this into a single commit.

Should be good to go now! Changed the example & squashed all commits.

Copy link
Member

@lucab lucab left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@lucab lucab merged commit 8b462b1 into tikv:master Nov 15, 2022
@JanBerktold JanBerktold deleted the add-pulling-gauge branch November 15, 2022 17:42
@lucab lucab mentioned this pull request May 4, 2024
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.

4 participants