From d97f80146b0f5e0ea4f8f0809fdfa76bb49357f5 Mon Sep 17 00:00:00 2001 From: Toby Lawrence Date: Wed, 30 Oct 2024 10:31:52 -0400 Subject: [PATCH] util: change some layers to require recorders that are Sync (#538) --- metrics-util/CHANGELOG.md | 5 ++ metrics-util/src/layers/fanout.rs | 17 ++++-- metrics-util/src/layers/router.rs | 21 +++++-- metrics/src/recorder/mod.rs | 94 ++++++++++++++----------------- 4 files changed, 74 insertions(+), 63 deletions(-) diff --git a/metrics-util/CHANGELOG.md b/metrics-util/CHANGELOG.md index c3a1b0ce..33b1fc5c 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 +### Changed + +- `FanoutBuilder` and `RouterBuilder` now both require recorders to be `Sync` to facilitate usage with being installed + as the global recorder. + ## [0.18.0] - 2024-10-12 ### Added diff --git a/metrics-util/src/layers/fanout.rs b/metrics-util/src/layers/fanout.rs index 60c67227..9402f7d0 100644 --- a/metrics-util/src/layers/fanout.rs +++ b/metrics-util/src/layers/fanout.rs @@ -100,7 +100,7 @@ impl From for Histogram { /// Fans out metrics to multiple recorders. pub struct Fanout { - recorders: Vec>, + recorders: Vec>, } impl fmt::Debug for Fanout { @@ -163,7 +163,7 @@ impl Recorder for Fanout { /// More information on the behavior of the layer can be found in [`Fanout`]. #[derive(Default)] pub struct FanoutBuilder { - recorders: Vec>, + recorders: Vec>, } impl fmt::Debug for FanoutBuilder { @@ -178,7 +178,7 @@ impl FanoutBuilder { /// Adds a recorder to the fanout list. pub fn add_recorder(mut self, recorder: R) -> FanoutBuilder where - R: Recorder + 'static, + R: Recorder + Sync + 'static, { self.recorders.push(Box::new(recorder)); self @@ -194,11 +194,20 @@ impl FanoutBuilder { mod tests { use super::FanoutBuilder; use crate::test_util::*; - use metrics::{Counter, Gauge, Histogram, Unit}; + use metrics::{Counter, Gauge, Histogram, Recorder, Unit}; static METADATA: metrics::Metadata = metrics::Metadata::new(module_path!(), metrics::Level::INFO, Some(module_path!())); + #[test] + fn sync() { + #[allow(dead_code)] + fn assert_sync_recorder(_t: &T) {} + + let recorder = FanoutBuilder::default().build(); + assert_sync_recorder(&recorder); + } + #[test] fn test_basic_functionality() { let operations = vec![ diff --git a/metrics-util/src/layers/router.rs b/metrics-util/src/layers/router.rs index 448ae5da..45bff052 100644 --- a/metrics-util/src/layers/router.rs +++ b/metrics-util/src/layers/router.rs @@ -9,9 +9,9 @@ use crate::{MetricKind, MetricKindMask}; /// /// More information on the behavior of the layer can be found in [`RouterBuilder`]. pub struct Router { - default: Box, + default: Box, global_mask: MetricKindMask, - targets: Vec>, + targets: Vec>, counter_routes: Trie, gauge_routes: Trie, histogram_routes: Trie, @@ -92,9 +92,9 @@ impl Recorder for Router { /// /// A default route (recorder) is always present and used in the case that no specific route exists. pub struct RouterBuilder { - default: Box, + default: Box, global_mask: MetricKindMask, - targets: Vec>, + targets: Vec>, counter_routes: Trie, gauge_routes: Trie, histogram_routes: Trie, @@ -118,7 +118,7 @@ impl RouterBuilder { /// The given recorder is used as the default route when no other specific route exists. pub fn from_recorder(recorder: R) -> Self where - R: Recorder + 'static, + R: Recorder + Sync + 'static, { RouterBuilder { default: Box::new(recorder), @@ -144,7 +144,7 @@ impl RouterBuilder { ) -> &mut RouterBuilder where P: AsRef, - R: Recorder + 'static, + R: Recorder + Sync + 'static, { let target_idx = self.targets.len(); self.targets.push(Box::new(recorder)); @@ -214,6 +214,15 @@ mod tests { } } + #[test] + fn sync() { + #[allow(dead_code)] + fn assert_sync_recorder(_t: &T) {} + + let recorder = RouterBuilder::from_recorder(MockTestRecorder::new()).build(); + assert_sync_recorder(&recorder); + } + #[test] fn test_construction() { let _ = RouterBuilder::from_recorder(MockTestRecorder::new()).build(); diff --git a/metrics/src/recorder/mod.rs b/metrics/src/recorder/mod.rs index 20c7b7d7..ca251635 100644 --- a/metrics/src/recorder/mod.rs +++ b/metrics/src/recorder/mod.rs @@ -20,31 +20,28 @@ thread_local! { /// A trait for registering and recording metrics. /// -/// This is the core trait that allows interoperability between exporter implementations and the -/// macros provided by `metrics`. +/// This is the core trait that allows interoperability between exporter implementations and the macros provided by +/// `metrics`. pub trait Recorder { /// Describes a counter. /// - /// Callers may provide the unit or a description of the counter being registered. Whether or - /// not a metric can be re-registered to provide a unit/description, if one was already passed - /// or not, as well as how units/descriptions are used by the underlying recorder, is an - /// implementation detail. + /// Callers may provide the unit or a description of the counter being registered. Whether or not a metric can be + /// re-registered to provide a unit/description, if one was already passed or not, as well as how units/descriptions + /// are used by the underlying recorder, is an implementation detail. fn describe_counter(&self, key: KeyName, unit: Option, description: SharedString); /// Describes a gauge. /// - /// Callers may provide the unit or a description of the gauge being registered. Whether or - /// not a metric can be re-registered to provide a unit/description, if one was already passed - /// or not, as well as how units/descriptions are used by the underlying recorder, is an - /// implementation detail. + /// Callers may provide the unit or a description of the gauge being registered. Whether or not a metric can be + /// re-registered to provide a unit/description, if one was already passed or not, as well as how units/descriptions + /// are used by the underlying recorder, is an implementation detail. fn describe_gauge(&self, key: KeyName, unit: Option, description: SharedString); /// Describes a histogram. /// - /// Callers may provide the unit or a description of the histogram being registered. Whether or - /// not a metric can be re-registered to provide a unit/description, if one was already passed - /// or not, as well as how units/descriptions are used by the underlying recorder, is an - /// implementation detail. + /// Callers may provide the unit or a description of the histogram being registered. Whether or not a metric can be + /// re-registered to provide a unit/description, if one was already passed or not, as well as how units/descriptions + /// are used by the underlying recorder, is an implementation detail. fn describe_histogram(&self, key: KeyName, unit: Option, description: SharedString); /// Registers a counter. @@ -125,19 +122,16 @@ impl_recorder!(T, std::sync::Arc); /// Guard for setting a local recorder. /// -/// When using a local recorder, we take a reference to the recorder and only hold it for as long as -/// the duration of the closure. However, we must store this reference in a static variable -/// (thread-local storage) so that it can be accessed by the macros. This guard ensures that the -/// pointer we store to the reference is cleared when the guard is dropped, so that it can't be used -/// after the closure has finished, even if the closure panics and unwinds the stack. +/// When using a local recorder, we take a reference to the recorder and only hold it for as long as the duration of the +/// closure. However, we must store this reference in a static variable (thread-local storage) so that it can be +/// accessed by the macros. This guard ensures that the pointer we store to the reference is cleared when the guard is +/// dropped, so that it can't be used after the closure has finished, even if the closure panics and unwinds the stack. /// /// ## Note /// -/// The guard has a lifetime parameter `'a` that is bounded using a -/// `PhantomData` type. This upholds the guard's contravariance, it must live -/// _at most as long_ as the recorder it takes a reference to. The bounded -/// lifetime prevents accidental use-after-free errors when using a guard -/// directly through [`crate::set_default_local_recorder`]. +/// The guard has a lifetime parameter `'a` that is bounded using a `PhantomData` type. This upholds the guard's +/// contravariance, it must live _at most as long_ as the recorder it takes a reference to. The bounded lifetime +/// prevents accidental use-after-free errors when using a guard directly through [`crate::set_default_local_recorder`]. pub struct LocalRecorderGuard<'a> { prev_recorder: Option>, phantom: PhantomData<&'a dyn Recorder>, @@ -146,10 +140,9 @@ pub struct LocalRecorderGuard<'a> { impl<'a> LocalRecorderGuard<'a> { /// Creates a new `LocalRecorderGuard` and sets the thread-local recorder. fn new(recorder: &'a dyn Recorder) -> Self { - // SAFETY: While we take a lifetime-less pointer to the given reference, the reference we - // derive _from_ the pointer is given the same lifetime of the reference - // used to construct the guard -- captured in the guard type itself -- - // and so derived references never outlive the source reference. + // SAFETY: While we take a lifetime-less pointer to the given reference, the reference we derive _from_ the + // pointer is given the same lifetime of the reference used to construct the guard -- captured in the guard type + // itself -- and so derived references never outlive the source reference. let recorder_ptr = unsafe { NonNull::new_unchecked(recorder as *const _ as *mut _) }; let prev_recorder = @@ -168,11 +161,11 @@ impl<'a> Drop for LocalRecorderGuard<'a> { /// Sets the global recorder. /// -/// This function may only be called once in the lifetime of a program. Any metrics recorded -/// before this method is called will be completely ignored. +/// This function may only be called once in the lifetime of a program. Any metrics recorded before this method is +/// called will be completely ignored. /// -/// This function does not typically need to be called manually. Metrics implementations should -/// provide an initialization method that installs the recorder internally. +/// This function does not typically need to be called manually. Metrics implementations should provide an +/// initialization method that installs the recorder internally. /// /// # Errors /// @@ -184,25 +177,21 @@ where GLOBAL_RECORDER.set(recorder) } -/// Sets the recorder as the default for the current thread for the duration of -/// the lifetime of the returned [`LocalRecorderGuard`]. +/// Sets the recorder as the default for the current thread for the duration of the lifetime of the returned +/// [`LocalRecorderGuard`]. /// -/// This function is suitable for capturing metrics in asynchronous code, in particular -/// when using a single-threaded runtime. Any metrics registered prior to the returned -/// guard will remain attached to the recorder that was present at the time of registration, -/// and so this cannot be used to intercept existing metrics. +/// This function is suitable for capturing metrics in asynchronous code, in particular when using a single-threaded +/// runtime. Any metrics registered prior to the returned guard will remain attached to the recorder that was present at +/// the time of registration, and so this cannot be used to intercept existing metrics. /// -/// Additionally, local recorders can be used in a nested fashion. When setting a new -/// default local recorder, the previous default local recorder will be captured if one -/// was set, and will be restored when the returned guard drops. +/// Additionally, local recorders can be used in a nested fashion. When setting a new default local recorder, the +/// previous default local recorder will be captured if one was set, and will be restored when the returned guard drops. /// the lifetime of the returned [`LocalRecorderGuard`]. /// -/// Any metrics recorded before a guard is returned will be completely ignored. -/// Metrics implementations should provide an initialization method that -/// installs the recorder internally. +/// Any metrics recorded before a guard is returned will be completely ignored. Metrics implementations should provide +/// an initialization method that installs the recorder internally. /// -/// The function is suitable for capturing metrics in asynchronous code that -/// uses a single threaded runtime. +/// The function is suitable for capturing metrics in asynchronous code that uses a single threaded runtime. /// /// If a global recorder is set, it will be restored once the guard is dropped. #[must_use] @@ -222,19 +211,18 @@ pub fn with_local_recorder(recorder: &dyn Recorder, f: impl FnOnce() -> T) -> /// Runs the closure with a reference to the current recorder for this scope. /// -/// If a local recorder has been set, it will be used. Otherwise, the global recorder will be used. -/// If neither a local recorder or global recorder have been set, a no-op recorder will be used. +/// If a local recorder has been set, it will be used. Otherwise, the global recorder will be used. If neither a local +/// recorder or global recorder have been set, a no-op recorder will be used. /// /// It should typically not be necessary to call this function directly, as it is used primarily by generated code. You /// should prefer working with the macros provided by `metrics` instead: `counter!`, `gauge!`, `histogram!`, etc. pub fn with_recorder(f: impl FnOnce(&dyn Recorder) -> T) -> T { LOCAL_RECORDER.with(|local_recorder| { if let Some(recorder) = local_recorder.get() { - // SAFETY: If we have a local recorder, we know that it is valid because it can only be - // set during the duration of a closure that is passed to `with_local_recorder`, which - // is the only time this method can be called and have a local recorder set. This - // ensures that the lifetime of the recorder is valid for the duration of this method - // call. + // SAFETY: If we have a local recorder, we know that it is valid because it can only be set during the + // duration of a closure that is passed to `with_local_recorder`, which is the only time this method can be + // called and have a local recorder set. This ensures that the lifetime of the recorder is valid for the + // duration of this method call. unsafe { f(recorder.as_ref()) } } else if let Some(global_recorder) = GLOBAL_RECORDER.try_load() { f(global_recorder)