From 9beb28e773f9ad8cf59129353e85335d98983766 Mon Sep 17 00:00:00 2001 From: Toby Lawrence Date: Mon, 17 Apr 2023 19:24:31 -0400 Subject: [PATCH 1/4] feat(util): new helper type for recovering recorder after installing it --- metrics-util/CHANGELOG.md | 5 + metrics-util/src/lib.rs | 3 + metrics-util/src/recoverable.rs | 244 ++++++++++++++++++++++++++++++++ metrics/Cargo.toml | 2 - metrics/benches/macros.rs | 4 +- metrics/build.rs | 13 -- metrics/src/recorder.rs | 45 ------ 7 files changed, 254 insertions(+), 62 deletions(-) create mode 100644 metrics-util/src/recoverable.rs delete mode 100644 metrics/build.rs diff --git a/metrics-util/CHANGELOG.md b/metrics-util/CHANGELOG.md index ba7e67bb..5385c4ee 100644 --- a/metrics-util/CHANGELOG.md +++ b/metrics-util/CHANGELOG.md @@ -9,6 +9,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] - ReleaseDate +### Added + +- Added a new helper type, `RecoverableRecorder`, that allows installing a recorder and then + recovering it later. + ## [0.15.0] - 2023-04-16 ### Changed diff --git a/metrics-util/src/lib.rs b/metrics-util/src/lib.rs index 42641fd7..c3052858 100644 --- a/metrics-util/src/lib.rs +++ b/metrics-util/src/lib.rs @@ -31,6 +31,9 @@ pub use kind::{MetricKind, MetricKindMask}; mod histogram; pub use histogram::Histogram; +mod recoverable; +pub use recoverable::RecoverableRecorder; + #[cfg(feature = "summary")] mod summary; #[cfg(feature = "summary")] diff --git a/metrics-util/src/recoverable.rs b/metrics-util/src/recoverable.rs new file mode 100644 index 00000000..4694ced7 --- /dev/null +++ b/metrics-util/src/recoverable.rs @@ -0,0 +1,244 @@ +use std::sync::{Arc, Weak}; + +use metrics::{Recorder, KeyName, Unit, SharedString, Key, Counter, Gauge, Histogram, SetRecorderError}; + +/// Wraps a recorder to allow for recovering it after being installed. +/// +/// Installing a recorder generally involves providing an owned value, which means that it is not +/// possible to recover the recorder after it has been installed. For some recorder implementations, +/// it can be important to perform finalization before the application exits, which is not possible +/// if the application cannot consume the recorder. +/// +/// `RecoverableRecorder` allows wrapping a recorder such that a weak reference to it is installed +/// globally, while the recorder itself is held by `RecoverableRecorder`. This allows for recovering +/// the recorder whenever the application chooses. +pub struct RecoverableRecorder { + recorder: Arc, +} + +impl RecoverableRecorder { + /// Creates a new `RecoverableRecorder` wrapper around the given recorder. + /// + /// This also installs the recorder globally, returning an error if there was already a recorder + /// installed. + pub fn from_recorder(recorder: R) -> Result { + let recorder = Arc::new(recorder); + + let wrapped = WeakRecorder::from_arc(&recorder); + metrics::set_boxed_recorder(Box::new(wrapped))?; + + Ok(Self { recorder }) + } + + /// Consumes this wrapper, returning the wrapped recorder. + /// + /// This method will loop until there are no active weak references to the recorder. It is not + /// advised to call this method under heavy load, as doing so is not deterministic or ordered + /// and may block for an indefinite amount of time. + pub fn into_inner(mut self) -> R { + loop { + match Arc::try_unwrap(self.recorder) { + Ok(recorder) => break recorder, + Err(recorder) => { + self.recorder = recorder; + } + } + } + } +} + +struct WeakRecorder { + recorder: Weak, +} + +impl WeakRecorder { + fn from_arc(recorder: &Arc) -> Self { + Self { + recorder: Arc::downgrade(recorder), + } + } +} + +impl Recorder for WeakRecorder { + fn describe_counter(&self, key: KeyName, unit: Option, description: SharedString) { + if let Some(recorder) = self.recorder.upgrade() { + recorder.describe_counter(key, unit, description); + } + } + + fn describe_gauge(&self, key: KeyName, unit: Option, description: SharedString) { + if let Some(recorder) = self.recorder.upgrade() { + recorder.describe_gauge(key, unit, description); + } + } + + fn describe_histogram(&self, key: KeyName, unit: Option, description: SharedString) { + if let Some(recorder) = self.recorder.upgrade() { + recorder.describe_histogram(key, unit, description); + } + } + + fn register_counter(&self, key: &Key) -> Counter { + if let Some(recorder) = self.recorder.upgrade() { + recorder.register_counter(key) + } else { + Counter::noop() + } + } + + fn register_gauge(&self, key: &Key) -> Gauge { + if let Some(recorder) = self.recorder.upgrade() { + recorder.register_gauge(key) + } else { + Gauge::noop() + } + } + + fn register_histogram(&self, key: &Key) -> Histogram { + if let Some(recorder) = self.recorder.upgrade() { + recorder.register_histogram(key) + } else { + Histogram::noop() + } + } +} + +#[cfg(test)] +mod tests { + use std::sync::atomic::Ordering; + + use super::*; + use metrics::{Key, Recorder, atomics::AtomicU64, CounterFn, GaugeFn, HistogramFn}; + + struct CounterWrapper(AtomicU64); + struct GaugeWrapper(AtomicU64); + struct HistogramWrapper(AtomicU64); + + impl CounterWrapper { + fn get(&self) -> u64 { + self.0.load(Ordering::Acquire) + } + } + + impl GaugeWrapper { + fn get(&self) -> u64 { + self.0.load(Ordering::Acquire) + } + } + + impl HistogramWrapper { + fn get(&self) -> u64 { + self.0.load(Ordering::Acquire) + } + } + + impl CounterFn for CounterWrapper { + fn increment(&self, value: u64) { + self.0.fetch_add(value, Ordering::Release); + } + + fn absolute(&self, value: u64) { + self.0.store(value, Ordering::Release); + } + } + + impl GaugeFn for GaugeWrapper { + fn increment(&self, value: f64) { + self.0.fetch_add(value as u64, Ordering::Release); + } + + fn decrement(&self, value: f64) { + self.0.fetch_sub(value as u64, Ordering::Release); + } + + fn set(&self, value: f64) { + self.0.store(value as u64, Ordering::Release); + } + } + + impl HistogramFn for HistogramWrapper { + fn record(&self, value: f64) { + self.0.fetch_add(value as u64, Ordering::Release); + } + } + + struct TestRecorder { + counter: Arc, + gauge: Arc, + histogram: Arc, + } + + impl TestRecorder { + fn new() -> (Self, Arc, Arc, Arc) { + let counter = Arc::new(CounterWrapper(AtomicU64::new(0))); + let gauge = Arc::new(GaugeWrapper(AtomicU64::new(0))); + let histogram = Arc::new(HistogramWrapper(AtomicU64::new(0))); + + let recorder = Self { + counter: Arc::clone(&counter), + gauge: Arc::clone(&gauge), + histogram: Arc::clone(&histogram), + }; + + (recorder, counter, gauge, histogram) + } + } + + impl Recorder for TestRecorder { + fn describe_counter(&self, _key: KeyName, _unit: Option, _description: SharedString) { + todo!() + } + + fn describe_gauge(&self, _key: KeyName, _unit: Option, _description: SharedString) { + todo!() + } + + fn describe_histogram(&self, _key: KeyName, _unit: Option, _description: SharedString) { + todo!() + } + + fn register_counter(&self, _: &Key) -> Counter { + Counter::from_arc(Arc::clone(&self.counter)) + } + + fn register_gauge(&self, _: &Key) -> Gauge { + Gauge::from_arc(Arc::clone(&self.gauge)) + } + + fn register_histogram(&self, _: &Key) -> Histogram { + Histogram::from_arc(Arc::clone(&self.histogram)) + } + } + + #[test] + fn basic() { + // Create and install the recorder. + let (recorder, counter, gauge, histogram) = TestRecorder::new(); + let recoverable = RecoverableRecorder::from_recorder(recorder) + .expect("failed to install recorder"); + + // Record some metrics, and make sure the atomics for each metric type are + // incremented as we would expect them to be. + metrics::counter!("counter", 5); + metrics::increment_gauge!("gauge", 5.0); + metrics::increment_gauge!("gauge", 5.0); + metrics::histogram!("histogram", 5.0); + metrics::histogram!("histogram", 5.0); + metrics::histogram!("histogram", 5.0); + + let _recorder = recoverable.into_inner(); + assert_eq!(counter.get(), 5); + assert_eq!(gauge.get(), 10); + assert_eq!(histogram.get(), 15); + + // Now that we've recovered the recorder, incrementing the same metrics should + // not actually increment the value of the atomics for each metric type. + metrics::counter!("counter", 7); + metrics::increment_gauge!("gauge", 7.0); + metrics::histogram!("histogram", 7.0); + + assert_eq!(counter.get(), 5); + assert_eq!(gauge.get(), 10); + assert_eq!(histogram.get(), 15); + } +} diff --git a/metrics/Cargo.toml b/metrics/Cargo.toml index 4636fc6f..88a07108 100644 --- a/metrics/Cargo.toml +++ b/metrics/Cargo.toml @@ -16,8 +16,6 @@ readme = "README.md" categories = ["development-tools::debugging"] keywords = ["metrics", "facade"] -build = "build.rs" - [lib] bench = false diff --git a/metrics/benches/macros.rs b/metrics/benches/macros.rs index b46ad89b..1c5ded0d 100644 --- a/metrics/benches/macros.rs +++ b/metrics/benches/macros.rs @@ -24,8 +24,8 @@ impl Recorder for TestRecorder { } fn reset_recorder() { - let recorder = Box::leak(Box::new(TestRecorder::default())); - unsafe { metrics::set_recorder_racy(recorder).unwrap() } + unsafe { metrics::clear_recorder(); } + metrics::set_boxed_recorder(Box::new(TestRecorder::default())).unwrap() } fn macro_benchmark(c: &mut Criterion) { diff --git a/metrics/build.rs b/metrics/build.rs deleted file mode 100644 index b5c13da8..00000000 --- a/metrics/build.rs +++ /dev/null @@ -1,13 +0,0 @@ -//! This build script detects target platforms that lack proper support for -//! atomics and sets `cfg` flags accordingly. -use std::env; - -fn main() { - // CAS is not available on thumbv6. - let target = env::var("TARGET").unwrap(); - if !target.starts_with("thumbv6") { - println!("cargo:rustc-cfg=atomic_cas"); - } - - println!("cargo:rerun-if-changed=build.rs"); -} diff --git a/metrics/src/recorder.rs b/metrics/src/recorder.rs index 728c9078..13ae93ec 100644 --- a/metrics/src/recorder.rs +++ b/metrics/src/recorder.rs @@ -31,7 +31,6 @@ mod cell { Self { recorder: UnsafeCell::new(None), state: AtomicUsize::new(UNINITIALIZED) } } - #[cfg(atomic_cas)] pub fn set(&self, recorder: &'static dyn Recorder) -> Result<(), SetRecorderError> { // Try and transition the cell from `UNINITIALIZED` to `INITIALIZING`, which would give // us exclusive access to set the recorder. @@ -77,27 +76,6 @@ mod cell { unsafe { self.recorder.get().read() } } } - - pub unsafe fn set_racy( - &self, - recorder: &'static dyn Recorder, - ) -> Result<(), SetRecorderError> { - match self.state.load(Ordering::Relaxed) { - UNINITIALIZED => { - // SAFETY: Caller guarantees that access is unique. - self.recorder.get().write(Some(recorder)); - self.state.store(INITIALIZED, Ordering::Release); - Ok(()) - } - INITIALIZING => { - // This is just plain UB, since we were racing another initialization function - unreachable!( - "set_recorder_racy must not be used with other initialization functions" - ) - } - _ => Err(SetRecorderError(())), - } - } } // SAFETY: We can only mutate through `set`, which is protected by the `state` and unsafe @@ -182,7 +160,6 @@ impl Recorder for NoopRecorder { /// # Errors /// /// An error is returned if a recorder has already been set. -#[cfg(atomic_cas)] pub fn set_recorder(recorder: &'static dyn Recorder) -> Result<(), SetRecorderError> { RECORDER.set(recorder) } @@ -196,32 +173,10 @@ pub fn set_recorder(recorder: &'static dyn Recorder) -> Result<(), SetRecorderEr /// # Errors /// /// An error is returned if a recorder has already been set. -#[cfg(atomic_cas)] pub fn set_boxed_recorder(recorder: Box) -> Result<(), SetRecorderError> { RECORDER.set(Box::leak(recorder)) } -/// A thread-unsafe version of [`set_recorder`]. -/// -/// This function is available on all platforms, even those that do not have support for atomics -/// that are needed by [`set_recorder`]. -/// -/// In almost all cases, [`set_recorder`] should be preferred. -/// -/// # Safety -/// -/// This function is only safe to call when no other metrics initialization function is called -/// while this function still executes. -/// -/// This can be upheld by (for example) making sure that **there are no other threads**, and (on -/// embedded) that **interrupts are disabled**. -/// -/// It is safe to use other metrics functions while this function runs (including all metrics -/// macros). -pub unsafe fn set_recorder_racy(recorder: &'static dyn Recorder) -> Result<(), SetRecorderError> { - RECORDER.set_racy(recorder) -} - /// Clears the currently configured recorder. /// /// This will leak the currently installed recorder, as we cannot safely drop it due to it being From b0fb4d6207d0106dd196c5cbc6c6274878125b8d Mon Sep 17 00:00:00 2001 From: Toby Lawrence Date: Mon, 17 Apr 2023 19:57:21 -0400 Subject: [PATCH 2/4] drop on drop behavior + add test for it --- .github/workflows/ci.yml | 2 +- metrics-util/src/recoverable.rs | 104 +++++++++++++++++++++++++++----- metrics/benches/macros.rs | 4 +- 3 files changed, 94 insertions(+), 16 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index c9dc8a6b..2fde46c8 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -40,7 +40,7 @@ jobs: - name: Install Rust ${{ matrix.rust_version }} run: rustup default ${{ matrix.rust_version }} - name: Run Tests - run: cargo test --all-features --workspace --exclude=metrics-observer + run: cargo test --all-features --workspace --exclude=metrics-observer -- --test-threads=1 docs: runs-on: ubuntu-latest env: diff --git a/metrics-util/src/recoverable.rs b/metrics-util/src/recoverable.rs index 4694ced7..4364a81d 100644 --- a/metrics-util/src/recoverable.rs +++ b/metrics-util/src/recoverable.rs @@ -1,6 +1,8 @@ use std::sync::{Arc, Weak}; -use metrics::{Recorder, KeyName, Unit, SharedString, Key, Counter, Gauge, Histogram, SetRecorderError}; +use metrics::{ + Counter, Gauge, Histogram, Key, KeyName, Recorder, SetRecorderError, SharedString, Unit, +}; /// Wraps a recorder to allow for recovering it after being installed. /// @@ -12,15 +14,29 @@ use metrics::{Recorder, KeyName, Unit, SharedString, Key, Counter, Gauge, Histog /// `RecoverableRecorder` allows wrapping a recorder such that a weak reference to it is installed /// globally, while the recorder itself is held by `RecoverableRecorder`. This allows for recovering /// the recorder whenever the application chooses. +/// +/// ## On drop +/// +/// While `RecoverableRecorder` provides a method to manually recover the recorder directly, one +/// particular benefit is that due to how the recorder is wrapped, when `RecoverableRecorder` is +/// dropped, and the last active reference to it is dropped, the recorder itself will be dropped. +/// +/// This allows using `RecoverableRecorder` as a drop guard, ensuring that by dropping it, the +/// recorder itself will be dropped, and any finalization logic implemented for the recorder will be +/// run. pub struct RecoverableRecorder { recorder: Arc, } impl RecoverableRecorder { - /// Creates a new `RecoverableRecorder` wrapper around the given recorder. + /// Creates a new `RecoverableRecorder`, wrapping the given recorder. /// - /// This also installs the recorder globally, returning an error if there was already a recorder - /// installed. + /// A weakly-referenced version of the recorder is installed globally, while the original + /// recorder is held within `RecoverableRecorder`, and can be recovered by calling `into_inner`. + /// + /// # Errors + /// + /// If a recorder is already installed, an error is returned. pub fn from_recorder(recorder: R) -> Result { let recorder = Arc::new(recorder); @@ -30,7 +46,7 @@ impl RecoverableRecorder { Ok(Self { recorder }) } - /// Consumes this wrapper, returning the wrapped recorder. + /// Consumes this wrapper, returning the original recorder. /// /// This method will loop until there are no active weak references to the recorder. It is not /// advised to call this method under heavy load, as doing so is not deterministic or ordered @@ -53,9 +69,7 @@ struct WeakRecorder { impl WeakRecorder { fn from_arc(recorder: &Arc) -> Self { - Self { - recorder: Arc::downgrade(recorder), - } + Self { recorder: Arc::downgrade(recorder) } } } @@ -105,10 +119,10 @@ impl Recorder for WeakRecorder { #[cfg(test)] mod tests { - use std::sync::atomic::Ordering; + use std::sync::atomic::{AtomicBool, Ordering}; use super::*; - use metrics::{Key, Recorder, atomics::AtomicU64, CounterFn, GaugeFn, HistogramFn}; + use metrics::{atomics::AtomicU64, CounterFn, GaugeFn, HistogramFn, Key, Recorder}; struct CounterWrapper(AtomicU64); struct GaugeWrapper(AtomicU64); @@ -163,6 +177,7 @@ mod tests { } struct TestRecorder { + dropped: Arc, counter: Arc, gauge: Arc, histogram: Arc, @@ -170,17 +185,26 @@ mod tests { impl TestRecorder { fn new() -> (Self, Arc, Arc, Arc) { + let (recorder, _, counter, gauge, histogram) = Self::new_with_drop(); + (recorder, counter, gauge, histogram) + } + + fn new_with_drop( + ) -> (Self, Arc, Arc, Arc, Arc) + { + let dropped = Arc::new(AtomicBool::new(false)); let counter = Arc::new(CounterWrapper(AtomicU64::new(0))); let gauge = Arc::new(GaugeWrapper(AtomicU64::new(0))); let histogram = Arc::new(HistogramWrapper(AtomicU64::new(0))); let recorder = Self { + dropped: Arc::clone(&dropped), counter: Arc::clone(&counter), gauge: Arc::clone(&gauge), histogram: Arc::clone(&histogram), }; - (recorder, counter, gauge, histogram) + (recorder, dropped, counter, gauge, histogram) } } @@ -193,7 +217,12 @@ mod tests { todo!() } - fn describe_histogram(&self, _key: KeyName, _unit: Option, _description: SharedString) { + fn describe_histogram( + &self, + _key: KeyName, + _unit: Option, + _description: SharedString, + ) { todo!() } @@ -210,12 +239,21 @@ mod tests { } } + impl Drop for TestRecorder { + fn drop(&mut self) { + self.dropped.store(true, Ordering::Release); + } + } + #[test] fn basic() { // Create and install the recorder. let (recorder, counter, gauge, histogram) = TestRecorder::new(); - let recoverable = RecoverableRecorder::from_recorder(recorder) - .expect("failed to install recorder"); + unsafe { + metrics::clear_recorder(); + } + let recoverable = + RecoverableRecorder::from_recorder(recorder).expect("failed to install recorder"); // Record some metrics, and make sure the atomics for each metric type are // incremented as we would expect them to be. @@ -241,4 +279,42 @@ mod tests { assert_eq!(gauge.get(), 10); assert_eq!(histogram.get(), 15); } + + #[test] + fn on_drop() { + // Create and install the recorder. + let (recorder, dropped, counter, gauge, histogram) = TestRecorder::new_with_drop(); + unsafe { + metrics::clear_recorder(); + } + let recoverable = + RecoverableRecorder::from_recorder(recorder).expect("failed to install recorder"); + + // Record some metrics, and make sure the atomics for each metric type are + // incremented as we would expect them to be. + metrics::counter!("counter", 5); + metrics::increment_gauge!("gauge", 5.0); + metrics::increment_gauge!("gauge", 5.0); + metrics::histogram!("histogram", 5.0); + metrics::histogram!("histogram", 5.0); + metrics::histogram!("histogram", 5.0); + + drop(recoverable.into_inner()); + assert_eq!(counter.get(), 5); + assert_eq!(gauge.get(), 10); + assert_eq!(histogram.get(), 15); + + // Now that we've recovered the recorder, incrementing the same metrics should + // not actually increment the value of the atomics for each metric type. + metrics::counter!("counter", 7); + metrics::increment_gauge!("gauge", 7.0); + metrics::histogram!("histogram", 7.0); + + assert_eq!(counter.get(), 5); + assert_eq!(gauge.get(), 10); + assert_eq!(histogram.get(), 15); + + // And we should be able to check that the recorder was indeed dropped. + assert!(dropped.load(Ordering::Acquire)); + } } diff --git a/metrics/benches/macros.rs b/metrics/benches/macros.rs index 1c5ded0d..ba7a30b3 100644 --- a/metrics/benches/macros.rs +++ b/metrics/benches/macros.rs @@ -24,7 +24,9 @@ impl Recorder for TestRecorder { } fn reset_recorder() { - unsafe { metrics::clear_recorder(); } + unsafe { + metrics::clear_recorder(); + } metrics::set_boxed_recorder(Box::new(TestRecorder::default())).unwrap() } From a1a7720efe968387daf4d7e5b9604c16bb330199 Mon Sep 17 00:00:00 2001 From: Toby Lawrence Date: Mon, 17 Apr 2023 20:12:58 -0400 Subject: [PATCH 3/4] fix docs --- metrics/src/lib.rs | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/metrics/src/lib.rs b/metrics/src/lib.rs index 0e2f1596..93062297 100644 --- a/metrics/src/lib.rs +++ b/metrics/src/lib.rs @@ -236,17 +236,13 @@ //! //! In order to actually use an exporter, it must be installed as the "global" recorder. This is a //! static recorder that the registration and emission macros refer to behind-the-scenes. `metrics` -//! provides a few methods to do so: [`set_recorder`], [`set_boxed_recorder`], and [`set_recorder_racy`]. +//! provides a few methods to do so: [`set_recorder`] and [`set_boxed_recorder`]. //! //! Primarily, you'll use [`set_boxed_recorder`] to pass a boxed version of the exporter to be //! installed. This is due to the fact that most exporters won't be able to be constructed //! statically. If you could construct your exporter statically, though, then you could instead //! choose [`set_recorder`]. //! -//! Similarly, [`set_recorder_racy`] takes a static reference, but is also not thread safe, and -//! should only be used on platforms which do not support atomic operations, such as embedded -//! environments. -//! //! As users of `metrics`, you'll typically see exporters provide methods to install themselves that //! hide the nitty gritty details. These methods will usually be aptly named, such as `install`. //! From 34021f75c8999dacbd4ab2375a854ee7f6a4817b Mon Sep 17 00:00:00 2001 From: Toby Lawrence Date: Mon, 17 Apr 2023 21:01:02 -0400 Subject: [PATCH 4/4] smol change to trigger CI --- metrics-util/src/recoverable.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/metrics-util/src/recoverable.rs b/metrics-util/src/recoverable.rs index 4364a81d..d8133e5e 100644 --- a/metrics-util/src/recoverable.rs +++ b/metrics-util/src/recoverable.rs @@ -15,7 +15,7 @@ use metrics::{ /// globally, while the recorder itself is held by `RecoverableRecorder`. This allows for recovering /// the recorder whenever the application chooses. /// -/// ## On drop +/// ## As a drop guard /// /// While `RecoverableRecorder` provides a method to manually recover the recorder directly, one /// particular benefit is that due to how the recorder is wrapped, when `RecoverableRecorder` is