From 4c245510c9663844c572b67fdca013c3b1aadcd2 Mon Sep 17 00:00:00 2001 From: katelyn martin Date: Wed, 15 Jan 2025 00:00:00 +0000 Subject: [PATCH 1/5] feat(metrics/family): add `Family::get_or_create_clone()` fixes #231. this introduces a new method to `Family` to help alleviate the risk of deadlocks when accessing multiple series within a given metrics family. this returns a plain `M`, rather than the `MappedRwLockReadGuard` RAII guard returned by `get_or_create()`. a test case is introduced in this commit to demonstrate that structures accessing multiple series within a single expression will not accidentally create a deadlock. Signed-off-by: katelyn martin --- CHANGELOG.md | 5 ++++ src/metrics/family.rs | 57 +++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 62 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index a0e3205..8e34392 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Added - `EncodeLabelSet` is now implemented for tuples `(A: EncodeLabelSet, B: EncodeLabelSet)`. + See [PR 257]. + +- `Family::get_or_create_clone` can access a metric in a labeled family. This + method avoids the risk of runtime deadlocks at the expense of creating an + owned type. See [PR 244]. ### Changed diff --git a/src/metrics/family.rs b/src/metrics/family.rs index 1a76cf8..928bec4 100644 --- a/src/metrics/family.rs +++ b/src/metrics/family.rs @@ -207,6 +207,40 @@ impl Family { } } +impl> Family +where + S: Clone + std::hash::Hash + Eq, + M: Clone, + C: MetricConstructor, +{ + /// Access a metric with the given label set, creating it if one does not yet exist. + /// + /// ``` + /// # use prometheus_client::metrics::counter::{Atomic, Counter}; + /// # use prometheus_client::metrics::family::Family; + /// # + /// let family = Family::, Counter>::default(); + /// + /// // Will create and return the metric with label `method="GET"` when first called. + /// family.get_or_create_clone(&vec![("method".to_owned(), "GET".to_owned())]).inc(); + /// + /// // Will return a clone of the existing metric on all subsequent calls. + /// family.get_or_create_clone(&vec![("method".to_owned(), "GET".to_owned())]).inc(); + /// ``` + /// + /// Callers wishing to avoid a clone of the metric `M` can call [`Family::get_or_create()`] to + /// return a reference to the metric instead. + pub fn get_or_create_clone(&self, label_set: &S) -> M { + use std::ops::Deref; + + let guard = self.get_or_create(label_set); + let metric = guard.deref().to_owned(); + drop(guard); + + metric + } +} + impl> Family { /// Access a metric with the given label set, creating it if one does not /// yet exist. @@ -225,6 +259,10 @@ impl> Family MappedRwLockReadGuard { if let Some(metric) = self.get(label_set) { return metric; @@ -510,4 +548,23 @@ mod tests { let non_existent_string = string_family.get(&"non_existent".to_string()); assert!(non_existent_string.is_none()); } + + /// Tests that [`Family::get_or_create_clone()`] does not cause deadlocks. + #[test] + fn counter_family_does_not_deadlock() { + /// A structure we'll place two counters into, within a single expression. + struct S { + apples: Counter, + oranges: Counter, + } + + let family = Family::<(&str, &str), Counter>::default(); + let s = S { + apples: family.get_or_create_clone(&("kind", "apple")), + oranges: family.get_or_create_clone(&("kind", "orange")), + }; + + s.apples.inc(); + s.oranges.inc_by(2); + } } From cec1a34c83fef29ee1986600edc1f9cf5ab197b0 Mon Sep 17 00:00:00 2001 From: katelyn martin Date: Wed, 15 Jan 2025 00:00:00 +0000 Subject: [PATCH 2/5] review: fix doc comment typo this linked to the wrong method. this is now fixed. Signed-off-by: katelyn martin --- src/metrics/family.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/metrics/family.rs b/src/metrics/family.rs index 928bec4..3557d78 100644 --- a/src/metrics/family.rs +++ b/src/metrics/family.rs @@ -261,7 +261,7 @@ impl> Family MappedRwLockReadGuard { if let Some(metric) = self.get(label_set) { From 4f110065b4ce768a6a5bc16850aba96c214f858c Mon Sep 17 00:00:00 2001 From: katelyn martin Date: Wed, 15 Jan 2025 00:00:00 +0000 Subject: [PATCH 3/5] review: rename method to `get_or_create_owned()` https://github.com/prometheus/client_rust/pull/244#discussion_r1948077955 Signed-off-by: katelyn martin --- src/metrics/family.rs | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/metrics/family.rs b/src/metrics/family.rs index 3557d78..d3d55c6 100644 --- a/src/metrics/family.rs +++ b/src/metrics/family.rs @@ -222,15 +222,15 @@ where /// let family = Family::, Counter>::default(); /// /// // Will create and return the metric with label `method="GET"` when first called. - /// family.get_or_create_clone(&vec![("method".to_owned(), "GET".to_owned())]).inc(); + /// family.get_or_create_owned(&vec![("method".to_owned(), "GET".to_owned())]).inc(); /// /// // Will return a clone of the existing metric on all subsequent calls. - /// family.get_or_create_clone(&vec![("method".to_owned(), "GET".to_owned())]).inc(); + /// family.get_or_create_owned(&vec![("method".to_owned(), "GET".to_owned())]).inc(); /// ``` /// /// Callers wishing to avoid a clone of the metric `M` can call [`Family::get_or_create()`] to /// return a reference to the metric instead. - pub fn get_or_create_clone(&self, label_set: &S) -> M { + pub fn get_or_create_owned(&self, label_set: &S) -> M { use std::ops::Deref; let guard = self.get_or_create(label_set); @@ -261,7 +261,7 @@ impl> Family MappedRwLockReadGuard { if let Some(metric) = self.get(label_set) { @@ -549,7 +549,7 @@ mod tests { assert!(non_existent_string.is_none()); } - /// Tests that [`Family::get_or_create_clone()`] does not cause deadlocks. + /// Tests that [`Family::get_or_create_owned()`] does not cause deadlocks. #[test] fn counter_family_does_not_deadlock() { /// A structure we'll place two counters into, within a single expression. @@ -560,8 +560,8 @@ mod tests { let family = Family::<(&str, &str), Counter>::default(); let s = S { - apples: family.get_or_create_clone(&("kind", "apple")), - oranges: family.get_or_create_clone(&("kind", "orange")), + apples: family.get_or_create_owned(&("kind", "apple")), + oranges: family.get_or_create_owned(&("kind", "orange")), }; s.apples.inc(); From a8a1003e658f394b21098958abe4b00c49c3792e Mon Sep 17 00:00:00 2001 From: Max Inden Date: Sun, 9 Feb 2025 21:43:28 +0100 Subject: [PATCH 4/5] Apply suggestions from code review Signed-off-by: Max Inden --- CHANGELOG.md | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8e34392..e2a1cce 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,9 +11,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - `EncodeLabelSet` is now implemented for tuples `(A: EncodeLabelSet, B: EncodeLabelSet)`. See [PR 257]. -- `Family::get_or_create_clone` can access a metric in a labeled family. This +- `Family::get_or_create_owned` can access a metric in a labeled family. This method avoids the risk of runtime deadlocks at the expense of creating an owned type. See [PR 244]. + + [PR 244]: https://github.com/prometheus/client_rust/pull/244 ### Changed From f1449324beda4fba6f1903d5c17d677cd1ea4ef9 Mon Sep 17 00:00:00 2001 From: Max Inden Date: Sun, 9 Feb 2025 21:45:23 +0100 Subject: [PATCH 5/5] Update CHANGELOG.md Signed-off-by: Max Inden --- CHANGELOG.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e2a1cce..c798c59 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,7 +15,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 method avoids the risk of runtime deadlocks at the expense of creating an owned type. See [PR 244]. - [PR 244]: https://github.com/prometheus/client_rust/pull/244 +[PR 244]: https://github.com/prometheus/client_rust/pull/244 +[PR 257]: https://github.com/prometheus/client_rust/pull/257 ### Changed