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 retain functions to Registry #461

Merged
merged 1 commit into from
Mar 16, 2024
Merged

Add retain functions to Registry #461

merged 1 commit into from
Mar 16, 2024

Conversation

kathoum
Copy link
Contributor

@kathoum kathoum commented Mar 10, 2024

This adds public functions retain_counters, retain_gauges, retain_histograms, that work like HashMap::retain

@kathoum
Copy link
Contributor Author

kathoum commented Mar 10, 2024

Design-wise, I'm not sure about the choice on the function signature:
FnMut(&K, &mut S::Counter) -> bool or FnMut(&K, &S::Counter) -> bool

I went with &mut because there's already a write lock, so the code has exclusive access to the underlying S::Counter, and it shouldn't make much difference in the common case that Counter = Arc<impl CounterFn>.

But I understand if we prefer not allowing mut access to objects inside the storage.

@tobz
Copy link
Member

tobz commented Mar 11, 2024

Design-wise, I'm not sure about the choice on the function signature: FnMut(&K, &mut S::Counter) -> bool or FnMut(&K, &S::Counter) -> bool

Following the standard library, it might be best just to make it an immutable reference. Thinking more about it as well, mutable access really shouldn't even be a big deal since all metric types inevitably have to support concurrent immutable access for normal usage via Counter, etc.

@kathoum
Copy link
Contributor Author

kathoum commented Mar 11, 2024

PR updated, changed the visitors to accept immutable references

Comment on lines 501 to 511
registry.get_or_create_counter(&Key::from_name("baz"), |_| ());

let mut n = 0;
registry.retain_counters(|k, _| {
n += 1;
k.name().starts_with("foo")
});
assert_eq!(n, 2);

Copy link
Member

Choose a reason for hiding this comment

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

I think you'd want to copy the assert from lines 512-513 and stick them after adding the baz counter, to show that we now have two counters in the registry. After that, do the same thing after the retain_counters call to show it goes back down to one counter after it deletes baz.

Copy link
Member

@tobz tobz 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 to me. 👍🏻

@tobz tobz added C-util Component: utility classes and helpers. E-simple Effort: simple. T-enhancement Type: enhancement. labels Mar 16, 2024
@tobz tobz merged commit d48fab7 into metrics-rs:main Mar 16, 2024
12 checks passed
@tobz tobz added the S-awaiting-release Status: awaiting a release to be considered fixed/implemented. label Mar 16, 2024
@tobz
Copy link
Member

tobz commented Mar 16, 2024

Released in metrics-util@v0.16.3.

Thanks again for your contribution!

@tobz tobz removed the S-awaiting-release Status: awaiting a release to be considered fixed/implemented. label Mar 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-util Component: utility classes and helpers. E-simple Effort: simple. T-enhancement Type: enhancement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants