From 310220b209ed0aed16453df29db0178d14ebd48f Mon Sep 17 00:00:00 2001 From: Toby Lawrence Date: Thu, 23 Nov 2023 10:16:23 -0500 Subject: [PATCH 01/17] fix(metrics): ensure boxed recorders are dropped when installation fails --- metrics/src/recorder.rs | 132 ++++++++++++++++++++++++++++++++++++++-- 1 file changed, 127 insertions(+), 5 deletions(-) diff --git a/metrics/src/recorder.rs b/metrics/src/recorder.rs index 8c351aee..e3516e1c 100644 --- a/metrics/src/recorder.rs +++ b/metrics/src/recorder.rs @@ -1,6 +1,6 @@ use std::fmt; -use self::cell::RecorderOnceCell; +use self::cell::{RecorderOnceCell, RecorderVariant}; use crate::{Counter, Gauge, Histogram, Key, KeyName, Metadata, SharedString, Unit}; @@ -20,6 +20,39 @@ mod cell { /// The recorder has been initialized successfully and can be read. const INITIALIZED: usize = 2; + pub enum RecorderVariant { + Static(&'static dyn Recorder), + Boxed(&'static dyn Recorder), + } + + impl RecorderVariant { + pub fn from_static(recorder: &'static dyn Recorder) -> Self { + Self::Static(recorder) + } + + pub fn from_boxed(recorder: Box) -> Self { + Self::Boxed(Box::leak(recorder)) + } + + pub fn into_recorder_ref(self) -> &'static dyn Recorder { + match self { + Self::Static(recorder) => recorder, + Self::Boxed(recorder) => recorder, + } + } + } + + impl Drop for RecorderVariant { + fn drop(&mut self) { + if let Self::Boxed(recorder) = self { + // SAFETY: We are the only owner of the recorder, so it is safe to drop. + unsafe { + drop(Box::from_raw(*recorder as *const dyn Recorder as *mut dyn Recorder)) + }; + } + } + } + /// An specialized version of `OnceCell` for `Recorder`. pub struct RecorderOnceCell { recorder: UnsafeCell>, @@ -32,7 +65,7 @@ mod cell { Self { recorder: UnsafeCell::new(None), state: AtomicUsize::new(UNINITIALIZED) } } - pub fn set(&self, recorder: &'static dyn Recorder) -> Result<(), SetRecorderError> { + pub fn set(&self, variant: RecorderVariant) -> Result<(), SetRecorderError> { // Try and transition the cell from `UNINITIALIZED` to `INITIALIZING`, which would give // us exclusive access to set the recorder. match self.state.compare_exchange( @@ -45,7 +78,7 @@ mod cell { unsafe { // SAFETY: Access is unique because we can only be here if we won the race // to transition from `UNINITIALIZED` to `INITIALIZING` above. - self.recorder.get().write(Some(recorder)); + self.recorder.get().write(Some(variant.into_recorder_ref())); } // Mark the recorder as initialized, which will make it visible to readers. @@ -162,7 +195,7 @@ impl Recorder for NoopRecorder { /// /// An error is returned if a recorder has already been set. pub fn set_recorder(recorder: &'static dyn Recorder) -> Result<(), SetRecorderError> { - RECORDER.set(recorder) + RECORDER.set(RecorderVariant::from_static(recorder)) } /// Sets the global recorder to a `Box`. @@ -175,7 +208,7 @@ pub fn set_recorder(recorder: &'static dyn Recorder) -> Result<(), SetRecorderEr /// /// An error is returned if a recorder has already been set. pub fn set_boxed_recorder(recorder: Box) -> Result<(), SetRecorderError> { - RECORDER.set(Box::leak(recorder)) + RECORDER.set(RecorderVariant::from_boxed(recorder)) } /// Clears the currently configured recorder. @@ -225,3 +258,92 @@ pub fn recorder() -> &'static dyn Recorder { pub fn try_recorder() -> Option<&'static dyn Recorder> { RECORDER.try_load() } + +#[cfg(test)] +mod tests { + use std::sync::{ + atomic::{AtomicBool, Ordering}, + Arc, + }; + + use super::{NoopRecorder, Recorder, RecorderOnceCell, RecorderVariant}; + + #[test] + fn boxed_recorder_dropped_on_existing_set() { + // This test simply ensures that if a boxed recorder is handed to us to install, and another + // recorder has already been installed, that we drop th new boxed recorder instead of + // leaking it. + struct TrackOnDropRecorder(Arc); + + impl TrackOnDropRecorder { + pub fn new() -> (Self, Arc) { + let arc = Arc::new(AtomicBool::new(false)); + (Self(arc.clone()), arc) + } + } + + impl Recorder for TrackOnDropRecorder { + fn describe_counter( + &self, + _: crate::KeyName, + _: Option, + _: crate::SharedString, + ) { + } + fn describe_gauge( + &self, + _: crate::KeyName, + _: Option, + _: crate::SharedString, + ) { + } + fn describe_histogram( + &self, + _: crate::KeyName, + _: Option, + _: crate::SharedString, + ) { + } + + fn register_counter(&self, _: &crate::Key, _: &crate::Metadata<'_>) -> crate::Counter { + crate::Counter::noop() + } + + fn register_gauge(&self, _: &crate::Key, _: &crate::Metadata<'_>) -> crate::Gauge { + crate::Gauge::noop() + } + + fn register_histogram( + &self, + _: &crate::Key, + _: &crate::Metadata<'_>, + ) -> crate::Histogram { + crate::Histogram::noop() + } + } + + impl Drop for TrackOnDropRecorder { + fn drop(&mut self) { + self.0.store(true, Ordering::SeqCst); + } + } + + let recorder_cell = RecorderOnceCell::new(); + + // This is the first set of the cell, so it should always succeed; + let first_recorder = NoopRecorder; + let first_set_result = + recorder_cell.set(RecorderVariant::from_boxed(Box::new(first_recorder))); + assert!(first_set_result.is_ok()); + + // Since the cell is already set, this second set should fail. We'll also then assert that + // our atomic boolean is set to `true`, indicating the drop logic ran for it. + let (second_recorder, was_dropped) = TrackOnDropRecorder::new(); + assert!(!was_dropped.load(Ordering::SeqCst)); + + let second_set_result = + recorder_cell.set(RecorderVariant::from_boxed(Box::new(second_recorder))); + assert!(second_set_result.is_err()); + assert!(was_dropped.load(Ordering::SeqCst)); + } +} From 541b1a0d2a989ba9f27f59ee073bdf2581f13dee Mon Sep 17 00:00:00 2001 From: Toby Lawrence Date: Fri, 24 Nov 2023 22:14:48 -0500 Subject: [PATCH 02/17] ended up implementing scoped (local) recorders --- metrics-benchmark/src/main.rs | 6 +- metrics-exporter-prometheus/src/builder.rs | 4 +- metrics-exporter-prometheus/src/common.rs | 4 +- metrics-exporter-prometheus/src/recorder.rs | 3 - metrics-exporter-tcp/src/lib.rs | 9 +- metrics-tracing-context/src/lib.rs | 4 +- metrics-tracing-context/tests/integration.rs | 170 +++++---- metrics-util/src/debugging.rs | 241 +------------ metrics-util/src/layers/mod.rs | 12 +- metrics-util/src/recoverable.rs | 150 ++++---- metrics/benches/macros.rs | 65 ++-- metrics/examples/basic.rs | 2 +- metrics/src/lib.rs | 60 ++-- metrics/src/macros.rs | 18 +- metrics/src/recorder.rs | 349 ------------------- metrics/src/recorder/cell.rs | 69 ++++ metrics/src/recorder/errors.rs | 28 ++ metrics/src/recorder/mod.rs | 232 ++++++++++++ metrics/src/recorder/noop.rs | 22 ++ rust-toolchain.toml | 2 +- 20 files changed, 631 insertions(+), 819 deletions(-) delete mode 100644 metrics/src/recorder.rs create mode 100644 metrics/src/recorder/cell.rs create mode 100644 metrics/src/recorder/errors.rs create mode 100644 metrics/src/recorder/mod.rs create mode 100644 metrics/src/recorder/noop.rs diff --git a/metrics-benchmark/src/main.rs b/metrics-benchmark/src/main.rs index df2b58cb..ffe03322 100644 --- a/metrics-benchmark/src/main.rs +++ b/metrics-benchmark/src/main.rs @@ -3,7 +3,7 @@ use hdrhistogram::Histogram as HdrHistogram; use log::{error, info}; use metrics::{ counter, gauge, histogram, Counter, Gauge, Histogram, Key, KeyName, Metadata, Recorder, - SharedString, Unit, + SetRecorderError, SharedString, Unit, }; use metrics_util::registry::{AtomicStorage, Registry}; use portable_atomic::AtomicU64; @@ -56,8 +56,8 @@ impl BenchmarkingRecorder { } /// Installs this recorder as the global recorder. - pub fn install(self) -> Result<(), metrics::SetRecorderError> { - metrics::set_boxed_recorder(Box::new(self)) + pub fn install(self) -> Result<(), SetRecorderError> { + metrics::set_global_recorder(self) } } diff --git a/metrics-exporter-prometheus/src/builder.rs b/metrics-exporter-prometheus/src/builder.rs index 70f5f762..f19fe907 100644 --- a/metrics-exporter-prometheus/src/builder.rs +++ b/metrics-exporter-prometheus/src/builder.rs @@ -367,7 +367,7 @@ impl PrometheusBuilder { recorder }; - metrics::set_boxed_recorder(Box::new(recorder))?; + metrics::set_global_recorder(recorder)?; Ok(()) } @@ -384,7 +384,7 @@ impl PrometheusBuilder { let recorder = self.build_recorder(); let handle = recorder.handle(); - metrics::set_boxed_recorder(Box::new(recorder))?; + metrics::set_global_recorder(recorder)?; Ok(handle) } diff --git a/metrics-exporter-prometheus/src/common.rs b/metrics-exporter-prometheus/src/common.rs index b481bd4a..b61873fb 100644 --- a/metrics-exporter-prometheus/src/common.rs +++ b/metrics-exporter-prometheus/src/common.rs @@ -1,6 +1,6 @@ use std::collections::HashMap; -use crate::distribution::Distribution; +use crate::{distribution::Distribution, PrometheusRecorder}; use crate::formatting::sanitize_metric_name; use indexmap::IndexMap; @@ -54,7 +54,7 @@ pub enum BuildError { /// Installing the recorder did not succeed. #[error("failed to install exporter as global recorder: {0}")] - FailedToSetGlobalRecorder(#[from] SetRecorderError), + FailedToSetGlobalRecorder(#[from] SetRecorderError), /// The given address could not be parsed successfully as an IP address/subnet. #[error("failed to parse address as a valid IP address/subnet: {0}")] diff --git a/metrics-exporter-prometheus/src/recorder.rs b/metrics-exporter-prometheus/src/recorder.rs index 5453c06e..73ef1316 100644 --- a/metrics-exporter-prometheus/src/recorder.rs +++ b/metrics-exporter-prometheus/src/recorder.rs @@ -198,9 +198,6 @@ impl Inner { /// A Prometheus recorder. /// -/// This recorder should be composed with other recorders or installed globally via -/// [`metrics::set_boxed_recorder`]. -/// /// Most users will not need to interact directly with the recorder, and can simply deal with the /// builder methods on [`PrometheusBuilder`](crate::PrometheusBuilder) for building and installing /// the recorder/exporter. diff --git a/metrics-exporter-tcp/src/lib.rs b/metrics-exporter-tcp/src/lib.rs index 0c4fc7c8..7a81e44d 100644 --- a/metrics-exporter-tcp/src/lib.rs +++ b/metrics-exporter-tcp/src/lib.rs @@ -104,7 +104,7 @@ pub enum Error { Io(io::Error), /// Installing the recorder did not succeed. - Recorder(SetRecorderError), + Recorder(SetRecorderError), } impl From for Error { @@ -113,8 +113,8 @@ impl From for Error { } } -impl From for Error { - fn from(e: SetRecorderError) -> Self { +impl From> for Error { + fn from(e: SetRecorderError) -> Self { Error::Recorder(e) } } @@ -284,8 +284,7 @@ impl TcpBuilder { /// installing the recorder as the global recorder. pub fn install(self) -> Result<(), Error> { let recorder = self.build()?; - metrics::set_boxed_recorder(Box::new(recorder))?; - Ok(()) + metrics::set_global_recorder(recorder).map_err(Into::into) } /// Builds and installs the exporter, but returns the recorder. diff --git a/metrics-tracing-context/src/lib.rs b/metrics-tracing-context/src/lib.rs index 656e6449..5ea0efc6 100644 --- a/metrics-tracing-context/src/lib.rs +++ b/metrics-tracing-context/src/lib.rs @@ -23,7 +23,7 @@ //! // Prepare metrics. //! # let my_recorder = DebuggingRecorder::new(); //! let recorder = TracingContextLayer::all().layer(my_recorder); -//! metrics::set_boxed_recorder(Box::new(recorder)).unwrap(); +//! metrics::set_global_recorder(recorder).unwrap(); //! ``` //! //! Then emit some metrics within spans and see the labels being injected! @@ -37,7 +37,7 @@ //! # tracing::subscriber::set_global_default(subscriber).unwrap(); //! # let myrecorder = DebuggingRecorder::new(); //! # let recorder = TracingContextLayer::all().layer(myrecorder); -//! # metrics::set_boxed_recorder(Box::new(recorder)).unwrap(); +//! # metrics::set_global_recorder(recorder).unwrap(); //! use tracing::{span, Level}; //! use metrics::counter; //! diff --git a/metrics-tracing-context/tests/integration.rs b/metrics-tracing-context/tests/integration.rs index 526b0c63..032d2e42 100644 --- a/metrics-tracing-context/tests/integration.rs +++ b/metrics-tracing-context/tests/integration.rs @@ -1,21 +1,19 @@ use itertools::Itertools; use metrics::{counter, Key, KeyName, Label}; use metrics_tracing_context::{LabelFilter, MetricsLayer, TracingContextLayer}; -use metrics_util::debugging::{DebugValue, DebuggingRecorder, Snapshotter}; +use metrics_util::debugging::{DebugValue, DebuggingRecorder, Snapshot}; use metrics_util::{layers::Layer, CompositeKey, MetricKind}; -use parking_lot::{const_mutex, Mutex, MutexGuard}; -use tracing::dispatcher::{set_default, DefaultGuard, Dispatch}; +use tracing::dispatcher::{set_default, Dispatch}; use tracing::{span, Level}; use tracing_subscriber::{layer::SubscriberExt, Registry}; -static TEST_MUTEX: Mutex<()> = const_mutex(()); -static LOGIN_ATTEMPTS: &str = "login_attempts"; -static LOGIN_ATTEMPTS_NONE: &str = "login_attempts_no_labels"; -static LOGIN_ATTEMPTS_STATIC: &str = "login_attempts_static_labels"; -static LOGIN_ATTEMPTS_DYNAMIC: &str = "login_attempts_dynamic_labels"; -static LOGIN_ATTEMPTS_BOTH: &str = "login_attempts_static_and_dynamic_labels"; -static MY_COUNTER: &str = "my_counter"; -static USER_EMAIL: &[Label] = &[ +static LOGIN_ATTEMPTS: &'static str = "login_attempts"; +static LOGIN_ATTEMPTS_NONE: &'static str = "login_attempts_no_labels"; +static LOGIN_ATTEMPTS_STATIC: &'static str = "login_attempts_static_labels"; +static LOGIN_ATTEMPTS_DYNAMIC: &'static str = "login_attempts_dynamic_labels"; +static LOGIN_ATTEMPTS_BOTH: &'static str = "login_attempts_static_and_dynamic_labels"; +static MY_COUNTER: &'static str = "my_counter"; +static USER_EMAIL: &'static [Label] = &[ Label::from_static_parts("user", "ferris"), Label::from_static_parts("user.email", "ferris@rust-lang.org"), ]; @@ -73,43 +71,34 @@ static SAME_CALLSITE_PATH_2: &[Label] = &[ Label::from_static_parts("path2_specific_dynamic", "bar_dynamic"), ]; -struct TestGuard { - _test_mutex_guard: MutexGuard<'static, ()>, - _tracing_guard: DefaultGuard, -} - -fn setup(layer: TracingContextLayer) -> (TestGuard, Snapshotter) +fn with_tracing_layer(layer: TracingContextLayer, f: impl FnOnce()) -> Snapshot where F: LabelFilter + Clone + 'static, { - let test_mutex_guard = TEST_MUTEX.lock(); let subscriber = Registry::default().with(MetricsLayer::new()); - let tracing_guard = set_default(&Dispatch::new(subscriber)); + let _tracing_guard = set_default(&Dispatch::new(subscriber)); let recorder = DebuggingRecorder::new(); let snapshotter = recorder.snapshotter(); let recorder = layer.layer(recorder); - unsafe { metrics::clear_recorder() }; - metrics::set_boxed_recorder(Box::new(recorder)).expect("failed to install recorder"); + metrics::with_local_recorder(&recorder, f); - let test_guard = - TestGuard { _test_mutex_guard: test_mutex_guard, _tracing_guard: tracing_guard }; - (test_guard, snapshotter) + snapshotter.snapshot() } #[test] fn test_basic_functionality() { - let (_guard, snapshotter) = setup(TracingContextLayer::all()); - - let user = "ferris"; - let email = "ferris@rust-lang.org"; - let span = span!(Level::TRACE, "login", user, user.email = email); - let _guard = span.enter(); + let snapshot = with_tracing_layer(TracingContextLayer::all(), || { + let user = "ferris"; + let email = "ferris@rust-lang.org"; + let span = span!(Level::TRACE, "login", user, user.email = email); + let _guard = span.enter(); - counter!("login_attempts", "service" => "login_service").increment(1); + counter!("login_attempts", "service" => "login_service").increment(1); + }); - let snapshot = snapshotter.snapshot().into_vec(); + let snapshot = snapshot.into_vec(); assert_eq!( snapshot, @@ -320,29 +309,26 @@ fn test_record_does_not_overwrite() { #[test] fn test_macro_forms() { - let (_guard, snapshotter) = setup(TracingContextLayer::all()); - - let user = "ferris"; - let email = "ferris@rust-lang.org"; - let span = span!(Level::TRACE, "login", user, user.email = email); - let _guard = span.enter(); - - // No labels. - counter!("login_attempts_no_labels").increment(1); - // Static labels only. - counter!("login_attempts_static_labels", "service" => "login_service").increment(1); - // Dynamic labels only. - let node_name = "localhost".to_string(); - counter!("login_attempts_dynamic_labels", "node_name" => node_name.clone()).increment(1); - // Static and dynamic. - counter!( - "login_attempts_static_and_dynamic_labels", - "service" => "login_service", - "node_name" => node_name, - ) - .increment(1); + let snapshot = with_tracing_layer(TracingContextLayer::all(), || { + let user = "ferris"; + let email = "ferris@rust-lang.org"; + let span = span!(Level::TRACE, "login", user, user.email = email); + let _guard = span.enter(); - let snapshot = snapshotter.snapshot().into_vec(); + // No labels. + counter!("login_attempts_no_labels").increment(1); + // Static labels only. + counter!("login_attempts_static_labels", "service" => "login_service").increment(1); + // Dynamic labels only. + let node_name = "localhost".to_string(); + counter!("login_attempts_dynamic_labels", "node_name" => node_name.clone()).increment(1); + // Static and dynamic. + counter!("login_attempts_static_and_dynamic_labels", + "service" => "login_service", "node_name" => node_name.clone()) + .increment(1); + }); + + let snapshot = snapshot.into_vec(); assert_eq!( snapshot, @@ -389,14 +375,14 @@ fn test_macro_forms() { #[test] fn test_no_labels() { - let (_guard, snapshotter) = setup(TracingContextLayer::all()); - - let span = span!(Level::TRACE, "login"); - let _guard = span.enter(); + let snapshot = with_tracing_layer(TracingContextLayer::all(), || { + let span = span!(Level::TRACE, "login"); + let _guard = span.enter(); - counter!("login_attempts").increment(1); + counter!("login_attempts").increment(1); + }); - let snapshot = snapshotter.snapshot().into_vec(); + let snapshot = snapshot.into_vec(); assert_eq!( snapshot, @@ -434,8 +420,6 @@ fn test_no_labels_record() { #[test] fn test_multiple_paths_to_the_same_callsite() { - let (_guard, snapshotter) = setup(TracingContextLayer::all()); - let shared_fn = || { counter!("my_counter").increment(1); }; @@ -466,10 +450,12 @@ fn test_multiple_paths_to_the_same_callsite() { shared_fn(); }; - path1(); - path2(); + let snapshot = with_tracing_layer(TracingContextLayer::all(), || { + path1(); + path2(); + }); - let snapshot = snapshotter.snapshot().into_vec(); + let snapshot = snapshot.into_vec(); assert_eq!( snapshot, @@ -498,8 +484,6 @@ fn test_multiple_paths_to_the_same_callsite() { #[test] fn test_nested_spans() { - let (_guard, snapshotter) = setup(TracingContextLayer::all()); - let inner = || { let inner_specific_dynamic = "foo_dynamic"; let span = span!( @@ -527,9 +511,11 @@ fn test_nested_spans() { inner(); }; - outer(); + let snapshot = with_tracing_layer(TracingContextLayer::all(), || { + outer(); + }); - let snapshot = snapshotter.snapshot().into_vec(); + let snapshot = snapshot.into_vec(); assert_eq!( snapshot, @@ -556,16 +542,16 @@ impl LabelFilter for OnlyUser { #[test] fn test_label_filtering() { - let (_guard, snapshotter) = setup(TracingContextLayer::new(OnlyUser)); - - let user = "ferris"; - let email = "ferris@rust-lang.org"; - let span = span!(Level::TRACE, "login", user, user.email_span = email); - let _guard = span.enter(); + let snapshot = with_tracing_layer(TracingContextLayer::new(OnlyUser), || { + let user = "ferris"; + let email = "ferris@rust-lang.org"; + let span = span!(Level::TRACE, "login", user, user.email_span = email); + let _guard = span.enter(); - counter!("login_attempts", "user.email" => "ferris@rust-lang.org").increment(1); + counter!("login_attempts", "user.email" => "ferris@rust-lang.org").increment(1); + }); - let snapshot = snapshotter.snapshot().into_vec(); + let snapshot = snapshot.into_vec(); assert_eq!( snapshot, @@ -583,23 +569,23 @@ fn test_label_filtering() { #[test] fn test_label_allowlist() { - let (_guard, snapshotter) = setup(TracingContextLayer::only_allow(["env", "service"])); - - let user = "ferris"; - let email = "ferris@rust-lang.org"; - let span = span!( - Level::TRACE, - "login", - user, - user.email_span = email, - service = "login_service", - env = "test" - ); - let _guard = span.enter(); + let snapshot = with_tracing_layer(TracingContextLayer::only_allow(&["env", "service"]), || { + let user = "ferris"; + let email = "ferris@rust-lang.org"; + let span = span!( + Level::TRACE, + "login", + user, + user.email_span = email, + service = "login_service", + env = "test" + ); + let _guard = span.enter(); - counter!("login_attempts").increment(1); + counter!("login_attempts").increment(1); + }); - let snapshot = snapshotter.snapshot().into_vec(); + let snapshot = snapshot.into_vec(); assert_eq!( snapshot, diff --git a/metrics-util/src/debugging.rs b/metrics-util/src/debugging.rs index 31ee34bb..9b8c1193 100644 --- a/metrics-util/src/debugging.rs +++ b/metrics-util/src/debugging.rs @@ -5,7 +5,6 @@ //! metrics in some limited cases. use std::{ - cell::RefCell, collections::HashMap, fmt::Debug, hash::Hash, @@ -19,14 +18,12 @@ use crate::{ }; use indexmap::IndexMap; -use metrics::{Counter, Gauge, Histogram, Key, KeyName, Metadata, Recorder, SharedString, Unit}; +use metrics::{ + Counter, Gauge, Histogram, Key, KeyName, Metadata, Recorder, SetRecorderError, SharedString, + Unit, +}; use ordered_float::OrderedFloat; -thread_local! { - /// A per-thread version of the debugging registry/state used only when the debugging recorder is configured to run in per-thread mode. - static PER_THREAD_INNER: RefCell> = RefCell::new(None); -} - /// A composite key name that stores both the metric key name and the metric kind. #[derive(Clone, Debug, Eq, Hash, PartialEq, PartialOrd, Ord)] struct CompositeKeyName(MetricKind, KeyName); @@ -134,58 +131,6 @@ impl Snapshotter { Snapshot(snapshot) } - - /// Takes a snapshot of the recorder for the current thread only. - /// - /// If no registry exists for the current thread, `None` is returned. Otherwise, `Some(snapshot)` is returned. - pub fn current_thread_snapshot() -> Option { - PER_THREAD_INNER.with(|maybe_inner| match maybe_inner.borrow().as_ref() { - None => None, - Some(inner) => { - let mut snapshot = Vec::new(); - - let counters = inner.registry.get_counter_handles(); - let gauges = inner.registry.get_gauge_handles(); - let histograms = inner.registry.get_histogram_handles(); - - let seen = inner.seen.lock().expect("seen lock poisoned").clone(); - let metadata = inner.metadata.lock().expect("metadata lock poisoned").clone(); - - for (ck, _) in seen.into_iter() { - let value = match ck.kind() { - MetricKind::Counter => counters - .get(ck.key()) - .map(|c| DebugValue::Counter(c.load(Ordering::SeqCst))), - MetricKind::Gauge => gauges.get(ck.key()).map(|g| { - let value = f64::from_bits(g.load(Ordering::SeqCst)); - DebugValue::Gauge(value.into()) - }), - MetricKind::Histogram => histograms.get(ck.key()).map(|h| { - let mut values = Vec::new(); - h.clear_with(|xs| { - values.extend(xs.iter().map(|f| OrderedFloat::from(*f))) - }); - DebugValue::Histogram(values) - }), - }; - - let ckn = CompositeKeyName::new(ck.kind(), ck.key().name().to_string().into()); - let (unit, desc) = metadata - .get(&ckn) - .map(|(u, d)| (u.to_owned(), Some(d.to_owned()))) - .unwrap_or_else(|| (None, None)); - - // If there's no value for the key, that means the metric was only ever described, and - // not registered, so don't emit it. - if let Some(value) = value { - snapshot.push((ck, unit, desc, value)); - } - } - - Some(Snapshot(snapshot)) - } - }) - } } /// A simplistic recorder that can be installed and used for debugging or testing. @@ -194,27 +139,12 @@ impl Snapshotter { /// to the raw values. pub struct DebuggingRecorder { inner: Arc, - is_per_thread: bool, } impl DebuggingRecorder { /// Creates a new `DebuggingRecorder`. pub fn new() -> DebuggingRecorder { - DebuggingRecorder { inner: Arc::new(Inner::new()), is_per_thread: false } - } - - /// Creates a new `DebuggingRecorder` in per-thread mode. - /// - /// This sends all metrics to a per-thread registry, such that the snapshotter will only see metrics emitted in the - /// thread that the `Snapshotter` is used from. Additionally, as keeping a reference to the original `Snapshotter` - /// around can be tricky, [`Snapshotter::current_thread_snapshot`] can be used to get all of the metrics currently - /// present in the registry for the calling thread, if any were emitted. - /// - /// Please note that this recorder must still be installed, but it can be installed multiple times (if the error - /// from `install` is ignored) without clearing or removing any of the existing per-thread metrics, so it's safe to - /// re-create and re-install multiple times in the same test binary if necessary. - pub fn per_thread() -> DebuggingRecorder { - DebuggingRecorder { inner: Arc::new(Inner::new()), is_per_thread: true } + DebuggingRecorder { inner: Arc::new(Inner::new()) } } /// Gets a `Snapshotter` attached to this recorder. @@ -223,56 +153,22 @@ impl DebuggingRecorder { } fn describe_metric(&self, rkey: CompositeKeyName, unit: Option, desc: SharedString) { - if self.is_per_thread { - PER_THREAD_INNER.with(|cell| { - // Create the inner state if it doesn't yet exist. - // - // SAFETY: It's safe to use `borrow_mut` here, even though the parent method is `&self`, as this is a - // per-thread invocation, so no other caller could possibly be holding a referenced, immutable or - // mutable, at the same time. - let mut maybe_inner = cell.borrow_mut(); - let inner = maybe_inner.get_or_insert_with(Inner::new); - - let mut metadata = inner.metadata.lock().expect("metadata lock poisoned"); - let (uentry, dentry) = metadata.entry(rkey).or_insert((None, desc.to_owned())); - if unit.is_some() { - *uentry = unit; - } - *dentry = desc.to_owned(); - }); - } else { - let mut metadata = self.inner.metadata.lock().expect("metadata lock poisoned"); - let (uentry, dentry) = metadata.entry(rkey).or_insert((None, desc.to_owned())); - if unit.is_some() { - *uentry = unit; - } - *dentry = desc; + let mut metadata = self.inner.metadata.lock().expect("metadata lock poisoned"); + let (uentry, dentry) = metadata.entry(rkey).or_insert((None, desc.to_owned())); + if unit.is_some() { + *uentry = unit; } + *dentry = desc; } fn track_metric(&self, ckey: CompositeKey) { - if self.is_per_thread { - PER_THREAD_INNER.with(|cell| { - // Create the inner state if it doesn't yet exist. - // - // SAFETY: It's safe to use `borrow_mut` here, even though the parent method is `&self`, as this is a - // per-thread invocation, so no other caller could possibly be holding a referenced, immutable or - // mutable, at the same time. - let mut maybe_inner = cell.borrow_mut(); - let inner = maybe_inner.get_or_insert_with(Inner::new); - - let mut seen = inner.seen.lock().expect("seen lock poisoned"); - seen.insert(ckey, ()); - }); - } else { - let mut seen = self.inner.seen.lock().expect("seen lock poisoned"); - seen.insert(ckey, ()); - } + let mut seen = self.inner.seen.lock().expect("seen lock poisoned"); + seen.insert(ckey, ()); } /// Installs this recorder as the global recorder. - pub fn install(self) -> Result<(), metrics::SetRecorderError> { - metrics::set_boxed_recorder(Box::new(self)) + pub fn install(self) -> Result<(), SetRecorderError> { + metrics::set_global_recorder(self) } } @@ -296,63 +192,21 @@ impl Recorder for DebuggingRecorder { let ckey = CompositeKey::new(MetricKind::Counter, key.clone()); self.track_metric(ckey); - if self.is_per_thread { - PER_THREAD_INNER.with(move |cell| { - // Create the inner state if it doesn't yet exist. - // - // SAFETY: It's safe to use `borrow_mut` here, even though the parent method is `&self`, as this is a - // per-thread invocation, so no other caller could possibly be holding a referenced, immutable or - // mutable, at the same time. - let mut maybe_inner = cell.borrow_mut(); - let inner = maybe_inner.get_or_insert_with(Inner::new); - - inner.registry.get_or_create_counter(key, |c| Counter::from_arc(c.clone())) - }) - } else { - self.inner.registry.get_or_create_counter(key, |c| Counter::from_arc(c.clone())) - } + self.inner.registry.get_or_create_counter(key, |c| Counter::from_arc(c.clone())) } fn register_gauge(&self, key: &Key, _metadata: &Metadata<'_>) -> Gauge { let ckey = CompositeKey::new(MetricKind::Gauge, key.clone()); self.track_metric(ckey); - if self.is_per_thread { - PER_THREAD_INNER.with(move |cell| { - // Create the inner state if it doesn't yet exist. - // - // SAFETY: It's safe to use `borrow_mut` here, even though the parent method is `&self`, as this is a - // per-thread invocation, so no other caller could possibly be holding a referenced, immutable or - // mutable, at the same time. - let mut maybe_inner = cell.borrow_mut(); - let inner = maybe_inner.get_or_insert_with(Inner::new); - - inner.registry.get_or_create_gauge(key, |g| Gauge::from_arc(g.clone())) - }) - } else { - self.inner.registry.get_or_create_gauge(key, |g| Gauge::from_arc(g.clone())) - } + self.inner.registry.get_or_create_gauge(key, |g| Gauge::from_arc(g.clone())) } fn register_histogram(&self, key: &Key, _metadata: &Metadata<'_>) -> Histogram { let ckey = CompositeKey::new(MetricKind::Histogram, key.clone()); self.track_metric(ckey); - if self.is_per_thread { - PER_THREAD_INNER.with(move |cell| { - // Create the inner state if it doesn't yet exist. - // - // SAFETY: It's safe to use `borrow_mut` here, even though the parent method is `&self`, as this is a - // per-thread invocation, so no other caller could possibly be holding a referenced, immutable or - // mutable, at the same time. - let mut maybe_inner = cell.borrow_mut(); - let inner = maybe_inner.get_or_insert_with(Inner::new); - - inner.registry.get_or_create_histogram(key, |h| Histogram::from_arc(h.clone())) - }) - } else { - self.inner.registry.get_or_create_histogram(key, |h| Histogram::from_arc(h.clone())) - } + self.inner.registry.get_or_create_histogram(key, |h| Histogram::from_arc(h.clone())) } } @@ -361,64 +215,3 @@ impl Default for DebuggingRecorder { DebuggingRecorder::new() } } - -#[cfg(test)] -mod tests { - use metrics::counter; - - use crate::{CompositeKey, MetricKind}; - - use super::{DebugValue, DebuggingRecorder, Snapshotter}; - - #[test] - fn per_thread() { - // Create the recorder in per-thread mode, get the snapshotter, and then spawn two threads that record some - // metrics. Neither thread should see the metrics of the other, and this main thread running the test should see - // _any_ metrics. - let recorder = DebuggingRecorder::per_thread(); - let snapshotter = recorder.snapshotter(); - - unsafe { metrics::clear_recorder() }; - recorder.install().expect("installing debugging recorder should not fail"); - - let t1 = std::thread::spawn(|| { - counter!("test_counter").increment(43); - - Snapshotter::current_thread_snapshot() - }); - - let t2 = std::thread::spawn(|| { - counter!("test_counter").increment(47); - - Snapshotter::current_thread_snapshot() - }); - - let t1_result = - t1.join().expect("thread 1 should not fail").expect("thread 1 should have metrics"); - let t2_result = - t2.join().expect("thread 2 should not fail").expect("thread 2 should have metrics"); - - let main_result = snapshotter.snapshot().into_vec(); - assert!(main_result.is_empty()); - - assert_eq!( - t1_result.into_vec(), - vec![( - CompositeKey::new(MetricKind::Counter, "test_counter".into()), - None, - None, - DebugValue::Counter(43), - )] - ); - - assert_eq!( - t2_result.into_vec(), - vec![( - CompositeKey::new(MetricKind::Counter, "test_counter".into()), - None, - None, - DebugValue::Counter(47), - )] - ); - } -} diff --git a/metrics-util/src/layers/mod.rs b/metrics-util/src/layers/mod.rs index bddafc5d..30786d4f 100644 --- a/metrics-util/src/layers/mod.rs +++ b/metrics-util/src/layers/mod.rs @@ -7,7 +7,7 @@ //! //! Here's an example of a layer that filters out all metrics that start with a specific string: //! -//! ```rust +//! ```no_run //! # use metrics::{Counter, Gauge, Histogram, Key, KeyName, Metadata, Recorder, SharedString, Unit}; //! # use metrics::NoopRecorder as BasicRecorder; //! # use metrics_util::layers::{Layer, Stack, PrefixLayer}; @@ -92,16 +92,12 @@ //! let recorder = BasicRecorder; //! let layer = StairwayDenyLayer::default(); //! let layered = layer.layer(recorder); -//! metrics::set_boxed_recorder(Box::new(layered)).expect("failed to install recorder"); -//! -//! # unsafe { metrics::clear_recorder() }; +//! metrics::set_global_recorder(layered).expect("failed to install recorder"); //! //! // Working with layers directly is a bit cumbersome, though, so let's use a `Stack`. //! let stack = Stack::new(BasicRecorder); //! stack.push(StairwayDenyLayer::default()).install().expect("failed to install stack"); //! -//! # unsafe { metrics::clear_recorder() }; -//! //! // `Stack` makes it easy to chain layers together, as well. //! let stack = Stack::new(BasicRecorder); //! stack @@ -161,8 +157,8 @@ impl Stack { /// Installs this stack as the global recorder. /// /// An error will be returned if there's an issue with installing the stack as the global recorder. - pub fn install(self) -> Result<(), SetRecorderError> { - metrics::set_boxed_recorder(Box::new(self)) + pub fn install(self) -> Result<(), SetRecorderError> { + metrics::set_global_recorder(self) } } diff --git a/metrics-util/src/recoverable.rs b/metrics-util/src/recoverable.rs index 9fb31236..ac2b311f 100644 --- a/metrics-util/src/recoverable.rs +++ b/metrics-util/src/recoverable.rs @@ -5,6 +5,31 @@ use metrics::{ Unit, }; +pub struct RecoveryHandle { + handle: Arc, +} + +impl RecoveryHandle { + /// Consumes the handle, returning the original recorder. + /// + /// This method will loop until there are no other strong references to the recorder. This means + /// that the wrapped recorder which was installed is not being actively used, as using it + /// temporarily upgrades its internal weak reference to a strong reference. + /// + /// 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.handle) { + Ok(recorder) => break recorder, + Err(handle) => { + self.handle = handle; + } + } + } + } +} + /// 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 @@ -13,52 +38,51 @@ use metrics::{ /// 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. +/// globally, while the recorder itself is held by `RecoveryHandle`. This allows the recorder to +/// be used globally so long as the recovery handle is active, keeping the original recorder alive. /// /// ## 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 -/// dropped, and the last active reference to it is dropped, the recorder itself will be dropped. +/// While `RecoveryHandle` provides a method to manually recover the recorder directly, one +/// particular benefit is that due to how the recorder is wrapped, when `RecoveryHandle` is +/// dropped, and the last active reference to the wrapped recorder is dropped, the recorder itself +/// will be dropped. /// -/// This allows using `RecoverableRecorder` as a drop guard, ensuring that by dropping it, the +/// This allows using `RecoveryHandle` 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, + handle: Arc, } impl RecoverableRecorder { - /// Creates a new `RecoverableRecorder`, wrapping the given recorder. + /// Creates a new `RecoverableRecorder` from the given recorder. + pub fn new(recorder: R) -> Self { + Self { handle: Arc::new(recorder) } + } + + /// Builds the wrapped recorder and a handle to recover the original. + pub(self) fn build(self) -> (WeakRecorder, RecoveryHandle) { + let wrapped = WeakRecorder::from_arc(&self.handle); + + (wrapped, RecoveryHandle { handle: self.handle }) + } + + /// Installs the wrapped recorder globally, returning a handle to recover it. /// /// 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); - - let wrapped = WeakRecorder::from_arc(&recorder); - metrics::set_boxed_recorder(Box::new(wrapped))?; - - Ok(Self { 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 - /// 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; - } + /// If a recorder is already installed, an error is returned containing the original recorder. + pub fn install(self) -> Result, SetRecorderError> { + let (wrapped, handle) = self.build(); + match metrics::set_global_recorder(wrapped) { + Ok(()) => Ok(handle), + Err(_) => { + let recorder = handle.into_inner(); + Err(SetRecorderError(recorder)) } } } @@ -250,31 +274,32 @@ mod tests { fn basic() { // Create and install the recorder. let (recorder, counter, gauge, histogram) = TestRecorder::new(); - unsafe { - metrics::clear_recorder(); - } - let recoverable = - RecoverableRecorder::from_recorder(recorder).expect("failed to install recorder"); + let recoverable = RecoverableRecorder::new(recorder); + let (recorder, handle) = recoverable.build(); // Record some metrics, and make sure the atomics for each metric type are // incremented as we would expect them to be. - metrics::counter!("counter").increment(5); - metrics::gauge!("gauge").increment(5.0); - metrics::gauge!("gauge").increment(5.0); - metrics::histogram!("histogram").record(5.0); - metrics::histogram!("histogram").record(5.0); - metrics::histogram!("histogram").record(5.0); - - let _recorder = recoverable.into_inner(); + metrics::with_local_recorder(&recorder, || { + metrics::counter!("counter").increment(5); + metrics::gauge!("gauge").increment(5.0); + metrics::gauge!("gauge").increment(5.0); + metrics::histogram!("histogram").record(5.0); + metrics::histogram!("histogram").record(5.0); + metrics::histogram!("histogram").record(5.0); + }); + + let _recorder = handle.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").increment(7); - metrics::gauge!("gauge").increment(7.0); - metrics::histogram!("histogram").record(7.0); + metrics::with_local_recorder(&recorder, || { + metrics::counter!("counter").increment(7); + metrics::gauge!("gauge").increment(7.0); + metrics::histogram!("histogram").record(7.0); + }); assert_eq!(counter.get(), 5); assert_eq!(gauge.get(), 10); @@ -285,31 +310,32 @@ mod tests { 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"); + let recoverable = RecoverableRecorder::new(recorder); + let (recorder, handle) = recoverable.build(); // Record some metrics, and make sure the atomics for each metric type are // incremented as we would expect them to be. - metrics::counter!("counter").increment(5); - metrics::gauge!("gauge").increment(5.0); - metrics::gauge!("gauge").increment(5.0); - metrics::histogram!("histogram").record(5.0); - metrics::histogram!("histogram").record(5.0); - metrics::histogram!("histogram").record(5.0); - - drop(recoverable.into_inner()); + metrics::with_local_recorder(&recorder, || { + metrics::counter!("counter").increment(5); + metrics::gauge!("gauge").increment(5.0); + metrics::gauge!("gauge").increment(5.0); + metrics::histogram!("histogram").record(5.0); + metrics::histogram!("histogram").record(5.0); + metrics::histogram!("histogram").record(5.0); + }); + + drop(handle.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").increment(7); - metrics::gauge!("gauge").increment(7.0); - metrics::histogram!("histogram").record(7.0); + metrics::with_local_recorder(&recorder, || { + metrics::counter!("counter").increment(7); + metrics::gauge!("gauge").increment(7.0); + metrics::histogram!("histogram").record(7.0); + }); assert_eq!(counter.get(), 5); assert_eq!(gauge.get(), 10); diff --git a/metrics/benches/macros.rs b/metrics/benches/macros.rs index 7ece0dcc..90c7d51f 100644 --- a/metrics/benches/macros.rs +++ b/metrics/benches/macros.rs @@ -25,60 +25,67 @@ impl Recorder for TestRecorder { } } -fn reset_recorder() { - unsafe { - metrics::clear_recorder(); - } - metrics::set_boxed_recorder(Box::new(TestRecorder::default())).unwrap() -} - fn macro_benchmark(c: &mut Criterion) { let mut group = c.benchmark_group("macros"); group.bench_function("uninitialized/no_labels", |b| { - unsafe { - metrics::clear_recorder(); - } b.iter(|| { counter!("counter_bench").increment(42); }) }); group.bench_function("uninitialized/with_static_labels", |b| { - unsafe { - metrics::clear_recorder(); - } b.iter(|| { counter!("counter_bench", "request" => "http", "svc" => "admin").increment(42); }) }); - group.bench_function("initialized/no_labels", |b| { - reset_recorder(); + group.bench_function("global_initialized/no_labels", |b| { + metrics::set_global_recorder(TestRecorder::default()).unwrap(); b.iter(|| { counter!("counter_bench").increment(42); }); - unsafe { - metrics::clear_recorder(); - } }); - group.bench_function("initialized/with_static_labels", |b| { - reset_recorder(); + group.bench_function("global_initialized/with_static_labels", |b| { + metrics::set_global_recorder(TestRecorder::default()).unwrap(); b.iter(|| { counter!("counter_bench", "request" => "http", "svc" => "admin").increment(42); }); - unsafe { - metrics::clear_recorder(); - } }); - group.bench_function("initialized/with_dynamic_labels", |b| { - let label_val = thread_rng().gen::().to_string(); + group.bench_function("global_initialized/with_dynamic_labels", |b| { + metrics::set_global_recorder(TestRecorder::default()).unwrap(); - reset_recorder(); + let label_val = thread_rng().gen::().to_string(); b.iter(move || { counter!("counter_bench", "request" => "http", "uid" => label_val.clone()) .increment(42); }); - unsafe { - metrics::clear_recorder(); - } + }); + group.bench_function("local_initialized/no_labels", |b| { + let recorder = TestRecorder::default(); + + metrics::with_local_recorder(&recorder, || { + b.iter(|| { + counter!("counter_bench").increment(42); + }); + }); + }); + group.bench_function("local_initialized/with_static_labels", |b| { + let recorder = TestRecorder::default(); + + metrics::with_local_recorder(&recorder, || { + b.iter(|| { + counter!("counter_bench", "request" => "http", "svc" => "admin").increment(42); + }); + }); + }); + group.bench_function("local_initialized/with_dynamic_labels", |b| { + let recorder = TestRecorder::default(); + + metrics::with_local_recorder(&recorder, || { + let label_val = thread_rng().gen::().to_string(); + b.iter(move || { + counter!("counter_bench", "request" => "http", "uid" => label_val.clone()) + .increment(42); + }); + }); }); group.finish(); } diff --git a/metrics/examples/basic.rs b/metrics/examples/basic.rs index 9b2da7e4..8d638386 100644 --- a/metrics/examples/basic.rs +++ b/metrics/examples/basic.rs @@ -91,7 +91,7 @@ impl Recorder for PrintRecorder { fn init_print_logger() { let recorder = PrintRecorder::default(); - metrics::set_boxed_recorder(Box::new(recorder)).unwrap() + metrics::set_global_recorder(recorder).unwrap() } fn main() { diff --git a/metrics/src/lib.rs b/metrics/src/lib.rs index 6182e852..6db3b022 100644 --- a/metrics/src/lib.rs +++ b/metrics/src/lib.rs @@ -157,25 +157,11 @@ //! //! # Development //! -//! The primary interface with `metrics` is through the [`Recorder`] trait, so we'll show examples -//! below of the trait and implementation notes. +//! The primary interface with `metrics` is through the [`Recorder`] trait, which is the connection +//! between the user-facing emission macros -- `counter!`, and so on -- and the actual logic for +//! handling those metrics and doing something with them, like logging them to the console or +//! sending them to a remote metrics system. //! -//! ## Installing a basic recorder -//! -//! Here's a basic example which writes metrics in text form via the `log` crate. -//! -//! ```no_run -//! # use metrics::{SetRecorderError, NoopRecorder as LogRecorder}; -//! // Recorders are installed by calling the [`set_recorder`] function. Recorders should provide a -//! // function that wraps the creation and installation of the recorder: -//! -//! static RECORDER: LogRecorder = LogRecorder; -//! -//! pub fn init() -> Result<(), SetRecorderError> { -//! metrics::set_recorder(&RECORDER) -//! } -//! # fn main() {} -//! ``` //! ## Keys //! //! All metrics are, in essence, the combination of a metric type and metric identifier, such as a @@ -243,17 +229,37 @@ //! //! ## Installing recorders //! -//! 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`] and [`set_boxed_recorder`]. +//! Recorders, also referred to as exporters, must be "installed" such that the emission macros can +//! access them. 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`. +//! +//! However, at a low level, this can happen in one of two ways: installing a recorder globally, or +//! temporarily using it locally. +//! +//! ### Global recorder +//! +//! The global recorder is the recorder that the macros use by default. It is stored in a static +//! variable accessible by all portions of the compiled application, including dependencies. This is +//! what allows us to provide the same "initialize once, benefit everywhere" behavior that users are +//! familiar with from other telemetry crates like `tracing` and `log`. +//! +//! Only one global recorder can be installed in the lifetime of the process. If a global recorder +//! has already been installed, it cannot be replaced: this is due to the fact that once installed, +//! the recorder is "leaked" so that a static reference can be obtained to it and used by subsequent +//! calls to the emission macros, and any downstream crates. +//! +//! ### Local 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`]. +//! In many scenarios, such as in unit tests, you may wish to temporarily set a recorder to +//! influence all calls to the emission macros within a specific section of code, without +//! influencing other areas of the code, or being limited by the constraints of only one global +//! recorder being allowed. //! -//! 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`. +//! [`with_local_recorder`] allows you to do this by changing the recorder used by the emission macros for +//! the duration of a given closure. While in that closure, the given recorder will act as if it was +//! the global recorder for the current thread. Once the closure returns, the true global recorder +//! takes priority again for the current thread. //! //! [metrics-exporter-tcp]: https://docs.rs/metrics-exporter-tcp //! [metrics-exporter-prometheus]: https://docs.rs/metrics-exporter-prometheus diff --git a/metrics/src/macros.rs b/metrics/src/macros.rs index 1c9b9712..ce4e7f33 100644 --- a/metrics/src/macros.rs +++ b/metrics/src/macros.rs @@ -109,7 +109,7 @@ macro_rules! counter { let metric_key = $crate::key_var!($name $(, $label_key $(=> $label_value)?)*); let metadata = $crate::metadata_var!($target, $level); - $crate::recorder().register_counter(&metric_key, metadata) + $crate::with_recorder(|recorder| recorder.register_counter(&metric_key, metadata)) }}; (target: $target:expr, $name:expr $(, $label_key:expr $(=> $label_value:expr)?)* $(,)?) => { $crate::counter!(target: $target, level: $crate::Level::INFO, $name $(, $label_key $(=> $label_value)?)*) @@ -176,7 +176,7 @@ macro_rules! gauge { let metric_key = $crate::key_var!($name $(, $label_key $(=> $label_value)?)*); let metadata = $crate::metadata_var!($target, $level); - $crate::recorder().register_gauge(&metric_key, metadata) + $crate::with_recorder(|recorder| recorder.register_gauge(&metric_key, metadata)) }}; (target: $target:expr, $name:expr $(, $label_key:expr $(=> $label_value:expr)?)* $(,)?) => { $crate::gauge!(target: $target, level: $crate::Level::INFO, $name $(, $label_key $(=> $label_value)?)*) @@ -240,7 +240,7 @@ macro_rules! histogram { let metric_key = $crate::key_var!($name $(, $label_key $(=> $label_value)?)*); let metadata = $crate::metadata_var!($target, $level); - $crate::recorder().register_histogram(&metric_key, metadata) + $crate::with_recorder(|recorder| recorder.register_histogram(&metric_key, metadata)) }}; (target: $target:expr, $name:expr $(, $label_key:expr $(=> $label_value:expr)?)* $(,)?) => { $crate::histogram!(target: $target, level: $crate::Level::INFO, $name $(, $label_key $(=> $label_value)?)*) @@ -257,22 +257,22 @@ macro_rules! histogram { #[macro_export] macro_rules! describe { ($method:ident, $name:expr, $unit:expr, $description:expr) => {{ - if let ::core::option::Option::Some(recorder) = $crate::try_recorder() { + $crate::with_recorder(|recorder| { recorder.$method( ::core::convert::Into::into($name), ::core::option::Option::Some($unit), ::core::convert::Into::into($description), - ) - } + ); + }); }}; ($method:ident, $name:expr, $description:expr) => {{ - if let ::core::option::Option::Some(recorder) = $crate::try_recorder() { + $crate::with_recorder(|recorder| { recorder.$method( ::core::convert::Into::into($name), ::core::option::Option::None, ::core::convert::Into::into($description), - ) - } + ); + }); }}; } diff --git a/metrics/src/recorder.rs b/metrics/src/recorder.rs deleted file mode 100644 index e3516e1c..00000000 --- a/metrics/src/recorder.rs +++ /dev/null @@ -1,349 +0,0 @@ -use std::fmt; - -use self::cell::{RecorderOnceCell, RecorderVariant}; - -use crate::{Counter, Gauge, Histogram, Key, KeyName, Metadata, SharedString, Unit}; - -mod cell { - use super::{Recorder, SetRecorderError}; - use std::{ - cell::UnsafeCell, - sync::atomic::{AtomicUsize, Ordering}, - }; - - /// The recorder is uninitialized. - const UNINITIALIZED: usize = 0; - - /// The recorder is currently being initialized. - const INITIALIZING: usize = 1; - - /// The recorder has been initialized successfully and can be read. - const INITIALIZED: usize = 2; - - pub enum RecorderVariant { - Static(&'static dyn Recorder), - Boxed(&'static dyn Recorder), - } - - impl RecorderVariant { - pub fn from_static(recorder: &'static dyn Recorder) -> Self { - Self::Static(recorder) - } - - pub fn from_boxed(recorder: Box) -> Self { - Self::Boxed(Box::leak(recorder)) - } - - pub fn into_recorder_ref(self) -> &'static dyn Recorder { - match self { - Self::Static(recorder) => recorder, - Self::Boxed(recorder) => recorder, - } - } - } - - impl Drop for RecorderVariant { - fn drop(&mut self) { - if let Self::Boxed(recorder) = self { - // SAFETY: We are the only owner of the recorder, so it is safe to drop. - unsafe { - drop(Box::from_raw(*recorder as *const dyn Recorder as *mut dyn Recorder)) - }; - } - } - } - - /// An specialized version of `OnceCell` for `Recorder`. - pub struct RecorderOnceCell { - recorder: UnsafeCell>, - state: AtomicUsize, - } - - impl RecorderOnceCell { - /// Creates an uninitialized `RecorderOnceCell`. - pub const fn new() -> Self { - Self { recorder: UnsafeCell::new(None), state: AtomicUsize::new(UNINITIALIZED) } - } - - pub fn set(&self, variant: RecorderVariant) -> Result<(), SetRecorderError> { - // Try and transition the cell from `UNINITIALIZED` to `INITIALIZING`, which would give - // us exclusive access to set the recorder. - match self.state.compare_exchange( - UNINITIALIZED, - INITIALIZING, - Ordering::Acquire, - Ordering::Relaxed, - ) { - Ok(UNINITIALIZED) => { - unsafe { - // SAFETY: Access is unique because we can only be here if we won the race - // to transition from `UNINITIALIZED` to `INITIALIZING` above. - self.recorder.get().write(Some(variant.into_recorder_ref())); - } - - // Mark the recorder as initialized, which will make it visible to readers. - self.state.store(INITIALIZED, Ordering::Release); - Ok(()) - } - _ => Err(SetRecorderError(())), - } - } - - /// Clears the currently installed recorder, allowing a new writer to override it. - /// - /// # Safety - /// - /// The caller must guarantee that no reader has read the state before we do this and then - /// reads the recorder after another writer has written to it after us. - pub unsafe fn clear(&self) { - // Set the state to `UNINITIALIZED` to allow the next writer to write again. This is not - // a problem for readers since their `&'static` refs will remain valid forever. - self.state.store(UNINITIALIZED, Ordering::Relaxed); - } - - pub fn try_load(&self) -> Option<&'static dyn Recorder> { - if self.state.load(Ordering::Acquire) != INITIALIZED { - None - } else { - // SAFETY: If the state is `INITIALIZED`, then we know that the recorder has been - // installed and is safe to read. - unsafe { self.recorder.get().read() } - } - } - } - - // SAFETY: We can only mutate through `set`, which is protected by the `state` and unsafe - // function where the caller has to guarantee synced-ness. - unsafe impl Send for RecorderOnceCell {} - unsafe impl Sync for RecorderOnceCell {} -} - -static RECORDER: RecorderOnceCell = RecorderOnceCell::new(); - -static SET_RECORDER_ERROR: &str = - "attempted to set a recorder after the metrics system was already initialized"; - -/// A trait for registering and recording 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 reregistered 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 reregistered 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 reregistered 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. - fn register_counter(&self, key: &Key, metadata: &Metadata<'_>) -> Counter; - - /// Registers a gauge. - fn register_gauge(&self, key: &Key, metadata: &Metadata<'_>) -> Gauge; - - /// Registers a histogram. - fn register_histogram(&self, key: &Key, metadata: &Metadata<'_>) -> Histogram; -} - -/// A no-op recorder. -/// -/// Used as the default recorder when one has not been installed yet. Useful for acting as the root -/// recorder when testing layers. -pub struct NoopRecorder; - -impl Recorder for NoopRecorder { - fn describe_counter(&self, _key: KeyName, _unit: Option, _description: SharedString) {} - fn describe_gauge(&self, _key: KeyName, _unit: Option, _description: SharedString) {} - fn describe_histogram(&self, _key: KeyName, _unit: Option, _description: SharedString) {} - fn register_counter(&self, _key: &Key, _metadata: &Metadata<'_>) -> Counter { - Counter::noop() - } - fn register_gauge(&self, _key: &Key, _metadata: &Metadata<'_>) -> Gauge { - Gauge::noop() - } - fn register_histogram(&self, _key: &Key, _metadata: &Metadata<'_>) -> Histogram { - Histogram::noop() - } -} - -/// Sets the global recorder to a `&'static Recorder`. -/// -/// This function may only be called once in the lifetime of a program. Any metrics recorded -/// before the call to `set_recorder` occurs 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. -/// -/// # Errors -/// -/// An error is returned if a recorder has already been set. -pub fn set_recorder(recorder: &'static dyn Recorder) -> Result<(), SetRecorderError> { - RECORDER.set(RecorderVariant::from_static(recorder)) -} - -/// Sets the global recorder to a `Box`. -/// -/// This is a simple convenience wrapper over `set_recorder`, which takes a `Box` -/// rather than a `&'static Recorder`. See the documentation for [`set_recorder`] for more -/// details. -/// -/// # Errors -/// -/// An error is returned if a recorder has already been set. -pub fn set_boxed_recorder(recorder: Box) -> Result<(), SetRecorderError> { - RECORDER.set(RecorderVariant::from_boxed(recorder)) -} - -/// Clears the currently configured recorder. -/// -/// This will leak the currently installed recorder, as we cannot safely drop it due to it being -/// provided via a reference with a `'static` lifetime. -/// -/// This method is typically only useful for testing or benchmarking. -/// -/// # Safety -/// -/// The caller must ensure that this method is not being called while other threads are either -/// loading a reference to the global recorder, or attempting to initialize the global recorder, as -/// it can cause a data race. -#[doc(hidden)] -pub unsafe fn clear_recorder() { - RECORDER.clear(); -} - -/// The type returned by [`set_recorder`] if [`set_recorder`] has already been called. -#[derive(Debug)] -pub struct SetRecorderError(()); - -impl fmt::Display for SetRecorderError { - fn fmt(&self, fmt: &mut fmt::Formatter) -> fmt::Result { - fmt.write_str(SET_RECORDER_ERROR) - } -} - -impl std::error::Error for SetRecorderError { - fn description(&self) -> &str { - SET_RECORDER_ERROR - } -} - -/// Returns a reference to the recorder. -/// -/// If a recorder has not been set, a no-op implementation is returned. -pub fn recorder() -> &'static dyn Recorder { - static NOOP: NoopRecorder = NoopRecorder; - try_recorder().unwrap_or(&NOOP) -} - -/// Returns a reference to the recorder. -/// -/// If a recorder has not been set, returns `None`. -pub fn try_recorder() -> Option<&'static dyn Recorder> { - RECORDER.try_load() -} - -#[cfg(test)] -mod tests { - use std::sync::{ - atomic::{AtomicBool, Ordering}, - Arc, - }; - - use super::{NoopRecorder, Recorder, RecorderOnceCell, RecorderVariant}; - - #[test] - fn boxed_recorder_dropped_on_existing_set() { - // This test simply ensures that if a boxed recorder is handed to us to install, and another - // recorder has already been installed, that we drop th new boxed recorder instead of - // leaking it. - struct TrackOnDropRecorder(Arc); - - impl TrackOnDropRecorder { - pub fn new() -> (Self, Arc) { - let arc = Arc::new(AtomicBool::new(false)); - (Self(arc.clone()), arc) - } - } - - impl Recorder for TrackOnDropRecorder { - fn describe_counter( - &self, - _: crate::KeyName, - _: Option, - _: crate::SharedString, - ) { - } - fn describe_gauge( - &self, - _: crate::KeyName, - _: Option, - _: crate::SharedString, - ) { - } - fn describe_histogram( - &self, - _: crate::KeyName, - _: Option, - _: crate::SharedString, - ) { - } - - fn register_counter(&self, _: &crate::Key, _: &crate::Metadata<'_>) -> crate::Counter { - crate::Counter::noop() - } - - fn register_gauge(&self, _: &crate::Key, _: &crate::Metadata<'_>) -> crate::Gauge { - crate::Gauge::noop() - } - - fn register_histogram( - &self, - _: &crate::Key, - _: &crate::Metadata<'_>, - ) -> crate::Histogram { - crate::Histogram::noop() - } - } - - impl Drop for TrackOnDropRecorder { - fn drop(&mut self) { - self.0.store(true, Ordering::SeqCst); - } - } - - let recorder_cell = RecorderOnceCell::new(); - - // This is the first set of the cell, so it should always succeed; - let first_recorder = NoopRecorder; - let first_set_result = - recorder_cell.set(RecorderVariant::from_boxed(Box::new(first_recorder))); - assert!(first_set_result.is_ok()); - - // Since the cell is already set, this second set should fail. We'll also then assert that - // our atomic boolean is set to `true`, indicating the drop logic ran for it. - let (second_recorder, was_dropped) = TrackOnDropRecorder::new(); - assert!(!was_dropped.load(Ordering::SeqCst)); - - let second_set_result = - recorder_cell.set(RecorderVariant::from_boxed(Box::new(second_recorder))); - assert!(second_set_result.is_err()); - assert!(was_dropped.load(Ordering::SeqCst)); - } -} diff --git a/metrics/src/recorder/cell.rs b/metrics/src/recorder/cell.rs new file mode 100644 index 00000000..5450b6f0 --- /dev/null +++ b/metrics/src/recorder/cell.rs @@ -0,0 +1,69 @@ +use super::{Recorder, SetRecorderError}; +use std::{ + cell::UnsafeCell, + sync::atomic::{AtomicUsize, Ordering}, +}; + +/// The recorder is uninitialized. +const UNINITIALIZED: usize = 0; + +/// The recorder is currently being initialized. +const INITIALIZING: usize = 1; + +/// The recorder has been initialized successfully and can be read. +const INITIALIZED: usize = 2; + +/// An specialized version of `OnceCell` for `Recorder`. +pub struct RecorderOnceCell { + recorder: UnsafeCell>, + state: AtomicUsize, +} + +impl RecorderOnceCell { + /// Creates an uninitialized `RecorderOnceCell`. + pub const fn new() -> Self { + Self { recorder: UnsafeCell::new(None), state: AtomicUsize::new(UNINITIALIZED) } + } + + pub fn set(&self, recorder: R) -> Result<(), SetRecorderError> + where + R: Recorder + 'static, + { + // Try and transition the cell from `UNINITIALIZED` to `INITIALIZING`, which would give + // us exclusive access to set the recorder. + match self.state.compare_exchange( + UNINITIALIZED, + INITIALIZING, + Ordering::Acquire, + Ordering::Relaxed, + ) { + Ok(UNINITIALIZED) => { + unsafe { + // SAFETY: Access is unique because we can only be here if we won the race + // to transition from `UNINITIALIZED` to `INITIALIZING` above. + self.recorder.get().write(Some(Box::leak(Box::new(recorder)))); + } + + // Mark the recorder as initialized, which will make it visible to readers. + self.state.store(INITIALIZED, Ordering::Release); + Ok(()) + } + _ => Err(SetRecorderError(recorder)), + } + } + + pub fn try_load(&self) -> Option<&'static dyn Recorder> { + if self.state.load(Ordering::Acquire) != INITIALIZED { + None + } else { + // SAFETY: If the state is `INITIALIZED`, then we know that the recorder has been + // installed and is safe to read. + unsafe { self.recorder.get().read() } + } + } +} + +// SAFETY: We can only mutate through `set`, which is protected by the `state` and unsafe +// function where the caller has to guarantee synced-ness. +unsafe impl Send for RecorderOnceCell {} +unsafe impl Sync for RecorderOnceCell {} diff --git a/metrics/src/recorder/errors.rs b/metrics/src/recorder/errors.rs new file mode 100644 index 00000000..10e32a92 --- /dev/null +++ b/metrics/src/recorder/errors.rs @@ -0,0 +1,28 @@ +use std::{error::Error, fmt}; + +const SET_RECORDER_ERROR: &str = + "attempted to set a recorder after the metrics system was already initialized"; + +/// The type returned by [`set_global_recorder`] if [`set_recorder`] has already been called. +pub struct SetRecorderError(pub R); + +impl SetRecorderError { + /// Returns the recorder that was attempted to be set. + pub fn into_inner(self) -> R { + self.0 + } +} + +impl fmt::Debug for SetRecorderError { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + f.debug_struct("SetRecorderError").finish_non_exhaustive() + } +} + +impl fmt::Display for SetRecorderError { + fn fmt(&self, fmt: &mut fmt::Formatter) -> fmt::Result { + fmt.write_str(SET_RECORDER_ERROR) + } +} + +impl Error for SetRecorderError {} diff --git a/metrics/src/recorder/mod.rs b/metrics/src/recorder/mod.rs new file mode 100644 index 00000000..b9d6fbf6 --- /dev/null +++ b/metrics/src/recorder/mod.rs @@ -0,0 +1,232 @@ +use std::{cell::Cell, ptr::NonNull}; + +mod cell; +use self::cell::RecorderOnceCell; + +mod errors; +pub use self::errors::SetRecorderError; + +mod noop; +pub use self::noop::NoopRecorder; + +use crate::{Counter, Gauge, Histogram, Key, KeyName, Metadata, SharedString, Unit}; + +static NOOP_RECORDER: NoopRecorder = NoopRecorder; +static GLOBAL_RECORDER: RecorderOnceCell = RecorderOnceCell::new(); + +thread_local! { + static LOCAL_RECORDER: Cell>> = Cell::new(None); +} + +/// A trait for registering and recording 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 reregistered 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 reregistered 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 reregistered 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. + fn register_counter(&self, key: &Key, metadata: &Metadata<'_>) -> Counter; + + /// Registers a gauge. + fn register_gauge(&self, key: &Key, metadata: &Metadata<'_>) -> Gauge; + + /// Registers a histogram. + fn register_histogram(&self, key: &Key, metadata: &Metadata<'_>) -> Histogram; +} + +/// 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. +struct LocalRecorderGuard; + +impl LocalRecorderGuard { + /// Creates a new `LocalRecorderGuard` and sets the thread-local recorder. + fn new(recorder: &dyn Recorder) -> Self { + // SAFETY: While we take a lifetime-less pointer to the given reference, the reference we + // derive _from_ the pointer is never given a lifetime that exceeds the lifetime of the + // input reference. + let recorder_ptr = unsafe { NonNull::new_unchecked(recorder as *const _ as *mut _) }; + + LOCAL_RECORDER.with(|local_recorder| { + local_recorder.set(Some(recorder_ptr)); + }); + + Self + } +} + +impl Drop for LocalRecorderGuard { + fn drop(&mut self) { + // Clear the thread-local recorder. + LOCAL_RECORDER.with(|local_recorder| { + local_recorder.set(None); + }); + } +} + +/// Sets the global recorder. +/// +/// This function may only be called once in the lifetime of a program. Any metrics recorded +/// before the call to `set_recorder` occurs 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. +/// +/// # Errors +/// +/// An error is returned if a recorder has already been set. +pub fn set_global_recorder(recorder: R) -> Result<(), SetRecorderError> +where + R: Recorder + 'static, +{ + GLOBAL_RECORDER.set(recorder) +} + +/// Runs the closure with the given recorder set as the global recorder for the duration. +pub fn with_local_recorder(recorder: &dyn Recorder, f: impl FnOnce() -> T) -> T { + let _local = LocalRecorderGuard::new(recorder); + f() +} + +/// 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. +/// +/// This is used primarily by the generated code from the convenience macros used to record metrics. +/// It should typically not be necessary to call this function directly. +#[doc(hidden)] +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. + unsafe { f(recorder.as_ref()) } + } else { + if let Some(global_recorder) = GLOBAL_RECORDER.try_load() { + f(global_recorder) + } else { + f(&NOOP_RECORDER) + } + } + }) +} + +#[cfg(test)] +mod tests { + use std::sync::{ + atomic::{AtomicBool, Ordering}, + Arc, + }; + + use super::{Recorder, RecorderOnceCell}; + + #[test] + fn boxed_recorder_dropped_on_existing_set() { + // This test simply ensures that if a boxed recorder is handed to us to install, and another + // recorder has already been installed, that we drop th new boxed recorder instead of + // leaking it. + struct TrackOnDropRecorder(Arc); + + impl TrackOnDropRecorder { + pub fn new() -> (Self, Arc) { + let arc = Arc::new(AtomicBool::new(false)); + (Self(arc.clone()), arc) + } + } + + impl Recorder for TrackOnDropRecorder { + fn describe_counter( + &self, + _: crate::KeyName, + _: Option, + _: crate::SharedString, + ) { + } + fn describe_gauge( + &self, + _: crate::KeyName, + _: Option, + _: crate::SharedString, + ) { + } + fn describe_histogram( + &self, + _: crate::KeyName, + _: Option, + _: crate::SharedString, + ) { + } + + fn register_counter(&self, _: &crate::Key, _: &crate::Metadata<'_>) -> crate::Counter { + crate::Counter::noop() + } + + fn register_gauge(&self, _: &crate::Key, _: &crate::Metadata<'_>) -> crate::Gauge { + crate::Gauge::noop() + } + + fn register_histogram( + &self, + _: &crate::Key, + _: &crate::Metadata<'_>, + ) -> crate::Histogram { + crate::Histogram::noop() + } + } + + impl Drop for TrackOnDropRecorder { + fn drop(&mut self) { + self.0.store(true, Ordering::SeqCst); + } + } + + let recorder_cell = RecorderOnceCell::new(); + + // This is the first set of the cell, so it should always succeed. + let (first_recorder, _) = TrackOnDropRecorder::new(); + let first_set_result = recorder_cell.set(first_recorder); + assert!(first_set_result.is_ok()); + + // Since the cell is already set, this second set should fail. We'll also then assert that + // our atomic boolean is set to `true`, indicating the drop logic ran for it. + let (second_recorder, was_dropped) = TrackOnDropRecorder::new(); + assert!(!was_dropped.load(Ordering::SeqCst)); + + let second_set_result = recorder_cell.set(second_recorder); + assert!(second_set_result.is_err()); + assert!(!was_dropped.load(Ordering::SeqCst)); + drop(second_set_result); + assert!(was_dropped.load(Ordering::SeqCst)); + } +} diff --git a/metrics/src/recorder/noop.rs b/metrics/src/recorder/noop.rs new file mode 100644 index 00000000..8c44fb6b --- /dev/null +++ b/metrics/src/recorder/noop.rs @@ -0,0 +1,22 @@ +use crate::{Counter, Gauge, Histogram, Key, KeyName, Metadata, Recorder, SharedString, Unit}; + +/// A no-op recorder. +/// +/// Used as the default recorder when one has not been installed yet. Useful for acting as the root +/// recorder when testing layers. +pub struct NoopRecorder; + +impl Recorder for NoopRecorder { + fn describe_counter(&self, _key: KeyName, _unit: Option, _description: SharedString) {} + fn describe_gauge(&self, _key: KeyName, _unit: Option, _description: SharedString) {} + fn describe_histogram(&self, _key: KeyName, _unit: Option, _description: SharedString) {} + fn register_counter(&self, _key: &Key, _metadata: &Metadata<'_>) -> Counter { + Counter::noop() + } + fn register_gauge(&self, _key: &Key, _metadata: &Metadata<'_>) -> Gauge { + Gauge::noop() + } + fn register_histogram(&self, _key: &Key, _metadata: &Metadata<'_>) -> Histogram { + Histogram::noop() + } +} diff --git a/rust-toolchain.toml b/rust-toolchain.toml index 292fe499..ead02718 100644 --- a/rust-toolchain.toml +++ b/rust-toolchain.toml @@ -1,2 +1,2 @@ [toolchain] -channel = "stable" +channel = "1.61.0" From 3e2a1d89ce8077a50665d30aa0d355dc1a6c3720 Mon Sep 17 00:00:00 2001 From: Toby Lawrence Date: Fri, 24 Nov 2023 22:15:26 -0500 Subject: [PATCH 03/17] remove single test thread limitation --- .github/workflows/ci.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 2c5dd6a0..2a57cccd 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -42,7 +42,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 -- --test-threads=1 + run: cargo test --all-features --workspace --exclude=metrics-observer docs: runs-on: ubuntu-latest env: From 1bab95b1d0393ee8c7a2f4f63a384d303a9855a0 Mon Sep 17 00:00:00 2001 From: Toby Lawrence Date: Fri, 24 Nov 2023 22:34:31 -0500 Subject: [PATCH 04/17] make benchmarks work --- metrics/benches/macros.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/metrics/benches/macros.rs b/metrics/benches/macros.rs index 90c7d51f..291d143f 100644 --- a/metrics/benches/macros.rs +++ b/metrics/benches/macros.rs @@ -38,19 +38,19 @@ fn macro_benchmark(c: &mut Criterion) { }) }); group.bench_function("global_initialized/no_labels", |b| { - metrics::set_global_recorder(TestRecorder::default()).unwrap(); + let _ = metrics::set_global_recorder(TestRecorder::default()); b.iter(|| { counter!("counter_bench").increment(42); }); }); group.bench_function("global_initialized/with_static_labels", |b| { - metrics::set_global_recorder(TestRecorder::default()).unwrap(); + let _ = metrics::set_global_recorder(TestRecorder::default()); b.iter(|| { counter!("counter_bench", "request" => "http", "svc" => "admin").increment(42); }); }); group.bench_function("global_initialized/with_dynamic_labels", |b| { - metrics::set_global_recorder(TestRecorder::default()).unwrap(); + let _ = metrics::set_global_recorder(TestRecorder::default()); let label_val = thread_rng().gen::().to_string(); b.iter(move || { From 3daa97f6058f0ee6c9b3222bb6d747d29286074d Mon Sep 17 00:00:00 2001 From: Toby Lawrence Date: Sat, 25 Nov 2023 09:53:24 -0500 Subject: [PATCH 05/17] make sure fmt can run --- .github/workflows/ci.yml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 2a57cccd..faf0d474 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -11,8 +11,8 @@ jobs: runs-on: ubuntu-latest steps: - uses: actions/checkout@v3 - - name: Install Rust Nightly - run: rustup default nightly + - name: Install Rust Stable + run: rustup default stable && rustup component add rustfmt - name: Check Formatting run: cargo fmt --all -- --check feature-check: From 75793e2930e265e84820ec020fb7f9bd93b95945 Mon Sep 17 00:00:00 2001 From: Toby Lawrence Date: Sat, 25 Nov 2023 09:56:04 -0500 Subject: [PATCH 06/17] install rustfmt for toolchain version --- .github/workflows/ci.yml | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index faf0d474..1684d1e4 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -11,8 +11,8 @@ jobs: runs-on: ubuntu-latest steps: - uses: actions/checkout@v3 - - name: Install Rust Stable - run: rustup default stable && rustup component add rustfmt + - name: Install rustfmt + run: rustup component add rustfmt - name: Check Formatting run: cargo fmt --all -- --check feature-check: @@ -21,8 +21,6 @@ jobs: - uses: actions/checkout@v3 - name: Install Protobuf Compiler run: sudo apt-get install protobuf-compiler - - name: Install Rust Stable - run: rustup default stable - name: Install cargo-hack uses: taiki-e/install-action@v2 with: From f65e5f802ca0043da93b7e9dd977cca83d7795bd Mon Sep 17 00:00:00 2001 From: Toby Lawrence Date: Sat, 25 Nov 2023 09:57:29 -0500 Subject: [PATCH 07/17] exclude metrics-benchmark from test commands --- .github/workflows/ci.yml | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 1684d1e4..981d84ff 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 --exclude=metrics-benchmark docs: runs-on: ubuntu-latest env: @@ -52,7 +52,7 @@ jobs: - name: Install Rust Nightly run: rustup default nightly - name: Check Docs - run: cargo doc --all-features --workspace --exclude=metrics-observer --no-deps + run: cargo doc --all-features --workspace --exclude=metrics-benchmark --no-deps bench: name: Bench runs-on: ubuntu-latest @@ -63,7 +63,7 @@ jobs: - name: Install Rust Stable run: rustup default stable - name: Run Benchmarks - run: cargo bench --all-features --workspace --exclude=metrics-observer + run: cargo bench --all-features --workspace --exclude=metrics-observer --exclude=metrics-benchmark clippy: name: Clippy ${{ matrix.rust_version }} runs-on: ubuntu-latest @@ -77,4 +77,4 @@ jobs: - name: Install Rust ${{ matrix.rust_version }} run: rustup default ${{ matrix.rust_version }} - name: Run Clippy - run: cargo clippy --all-features --workspace --exclude=metrics-observer + run: cargo clippy --all-features --workspace --exclude=metrics-benchmark From 850ad4905bee020043834699a7f2aa470646acf8 Mon Sep 17 00:00:00 2001 From: Toby Lawrence Date: Sat, 25 Nov 2023 10:11:38 -0500 Subject: [PATCH 08/17] gotta be more explicit to do matrix'd tests w/ rust-toolchain.toml present --- .github/workflows/ci.yml | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 981d84ff..9ec7e45c 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -28,19 +28,28 @@ jobs: - name: Check Feature Matrix run: cargo hack check --all --all-targets --feature-powerset --release test: + name: Test (rust-toolchain.toml) + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v3 + - name: Install Protobuf Compiler + run: sudo apt-get install protobuf-compiler + - name: Run Tests + run: cargo test --all-features --workspace --exclude=metrics-observer --exclude=metrics-benchmark + test-matrixed: name: Test ${{ matrix.rust_version }} runs-on: ubuntu-latest strategy: matrix: - rust_version: ['1.61.0', 'stable', 'nightly'] + rust_version: ['stable', 'nightly'] steps: - uses: actions/checkout@v3 - name: Install Protobuf Compiler run: sudo apt-get install protobuf-compiler - name: Install Rust ${{ matrix.rust_version }} - run: rustup default ${{ matrix.rust_version }} + run: rustup install ${{ matrix.rust_version }} - name: Run Tests - run: cargo test --all-features --workspace --exclude=metrics-observer --exclude=metrics-benchmark + run: cargo +${{ matrix.rust_version }} test --all-features --workspace --exclude=metrics-observer --exclude=metrics-benchmark docs: runs-on: ubuntu-latest env: From f5fc0f502a511e97f27c136b83ab6a5fc03bd845 Mon Sep 17 00:00:00 2001 From: Toby Lawrence Date: Sat, 25 Nov 2023 10:17:41 -0500 Subject: [PATCH 09/17] more explicitness --- .github/workflows/ci.yml | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 9ec7e45c..db2ad93a 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -21,12 +21,18 @@ jobs: - uses: actions/checkout@v3 - name: Install Protobuf Compiler run: sudo apt-get install protobuf-compiler + - name: Install Rust Stable + run: rustup install stable - name: Install cargo-hack uses: taiki-e/install-action@v2 with: tool: cargo-hack - name: Check Feature Matrix +<<<<<<< HEAD run: cargo hack check --all --all-targets --feature-powerset --release +======= + run: cargo +stable hack build --all --all-targets --feature-powerset +>>>>>>> f1154ef (more explicitness) test: name: Test (rust-toolchain.toml) runs-on: ubuntu-latest @@ -59,9 +65,9 @@ jobs: - name: Install Protobuf Compiler run: sudo apt-get install protobuf-compiler - name: Install Rust Nightly - run: rustup default nightly + run: rustup install nightly - name: Check Docs - run: cargo doc --all-features --workspace --exclude=metrics-benchmark --no-deps + run: cargo +nightly doc --all-features --workspace --exclude=metrics-observer --exclude=metrics-benchmark --no-deps bench: name: Bench runs-on: ubuntu-latest @@ -74,16 +80,13 @@ jobs: - name: Run Benchmarks run: cargo bench --all-features --workspace --exclude=metrics-observer --exclude=metrics-benchmark clippy: - name: Clippy ${{ matrix.rust_version }} + name: Clippy runs-on: ubuntu-latest - strategy: - matrix: - rust_version: ['1.61.0', 'stable', 'nightly'] steps: - uses: actions/checkout@v3 - name: Install Protobuf Compiler run: sudo apt-get install protobuf-compiler - - name: Install Rust ${{ matrix.rust_version }} - run: rustup default ${{ matrix.rust_version }} + - name: Install Rust Stable + run: rustup default stable - name: Run Clippy run: cargo clippy --all-features --workspace --exclude=metrics-benchmark From 2c038c7b7312ef0c026f63878b4557a248399c41 Mon Sep 17 00:00:00 2001 From: Toby Lawrence Date: Sat, 25 Nov 2023 10:26:24 -0500 Subject: [PATCH 10/17] clean up doc comment --- metrics/src/recorder/errors.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/metrics/src/recorder/errors.rs b/metrics/src/recorder/errors.rs index 10e32a92..ff8d7ca4 100644 --- a/metrics/src/recorder/errors.rs +++ b/metrics/src/recorder/errors.rs @@ -3,7 +3,7 @@ use std::{error::Error, fmt}; const SET_RECORDER_ERROR: &str = "attempted to set a recorder after the metrics system was already initialized"; -/// The type returned by [`set_global_recorder`] if [`set_recorder`] has already been called. +/// Error returned when trying to install a global recorder when another has already been installed. pub struct SetRecorderError(pub R); impl SetRecorderError { From 98715dd0c3cc6960f48eddbbe3a5c819f3b6606a Mon Sep 17 00:00:00 2001 From: Toby Lawrence Date: Sat, 25 Nov 2023 10:42:47 -0500 Subject: [PATCH 11/17] bump MSRV to 1.65.0 and adjust CI --- .github/workflows/ci.yml | 14 ++++++-------- README.md | 2 +- metrics-benchmark/Cargo.toml | 2 +- metrics-exporter-prometheus/CHANGELOG.md | 7 ++++++- metrics-exporter-prometheus/Cargo.toml | 2 +- metrics-exporter-tcp/CHANGELOG.md | 4 ++++ metrics-exporter-tcp/Cargo.toml | 2 +- metrics-observer/CHANGELOG.md | 4 ++++ metrics-observer/Cargo.toml | 2 +- metrics-tracing-context/CHANGELOG.md | 7 ++++++- metrics-tracing-context/Cargo.toml | 2 +- metrics-util/CHANGELOG.md | 4 ++++ metrics-util/Cargo.toml | 2 +- metrics/CHANGELOG.md | 1 + metrics/Cargo.toml | 2 +- rust-toolchain.toml | 2 +- 16 files changed, 40 insertions(+), 19 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index db2ad93a..03840854 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -22,17 +22,13 @@ jobs: - name: Install Protobuf Compiler run: sudo apt-get install protobuf-compiler - name: Install Rust Stable - run: rustup install stable + run: rustup default stable - name: Install cargo-hack uses: taiki-e/install-action@v2 with: tool: cargo-hack - name: Check Feature Matrix -<<<<<<< HEAD run: cargo hack check --all --all-targets --feature-powerset --release -======= - run: cargo +stable hack build --all --all-targets --feature-powerset ->>>>>>> f1154ef (more explicitness) test: name: Test (rust-toolchain.toml) runs-on: ubuntu-latest @@ -40,8 +36,10 @@ jobs: - uses: actions/checkout@v3 - name: Install Protobuf Compiler run: sudo apt-get install protobuf-compiler + - name: Install Rust Stable + run: rustup default stable - name: Run Tests - run: cargo test --all-features --workspace --exclude=metrics-observer --exclude=metrics-benchmark + run: cargo test --all-features --workspace test-matrixed: name: Test ${{ matrix.rust_version }} runs-on: ubuntu-latest @@ -55,7 +53,7 @@ jobs: - name: Install Rust ${{ matrix.rust_version }} run: rustup install ${{ matrix.rust_version }} - name: Run Tests - run: cargo +${{ matrix.rust_version }} test --all-features --workspace --exclude=metrics-observer --exclude=metrics-benchmark + run: cargo +${{ matrix.rust_version }} test --all-features --workspace docs: runs-on: ubuntu-latest env: @@ -67,7 +65,7 @@ jobs: - name: Install Rust Nightly run: rustup install nightly - name: Check Docs - run: cargo +nightly doc --all-features --workspace --exclude=metrics-observer --exclude=metrics-benchmark --no-deps + run: cargo +nightly doc --all-features --workspace --no-deps bench: name: Bench runs-on: ubuntu-latest diff --git a/README.md b/README.md index fafdce31..96b379a0 100644 --- a/README.md +++ b/README.md @@ -65,7 +65,7 @@ As well, there are also some community-maintained exporters and other integratio ## MSRV and MSRV policy -Minimum supported Rust version (MSRV) is currently **1.61.0**, enforced by CI. +Minimum supported Rust version (MSRV) is currently **1.65.0**, enforced by CI. `metrics` will always support _at least_ the latest four versions of stable Rust, based on minor version releases, and excluding patch versions. Overall, we strive to support older versions where diff --git a/metrics-benchmark/Cargo.toml b/metrics-benchmark/Cargo.toml index e48d98fc..9fc8fe89 100644 --- a/metrics-benchmark/Cargo.toml +++ b/metrics-benchmark/Cargo.toml @@ -3,7 +3,7 @@ name = "metrics-benchmark" version = "0.1.1-alpha.5" authors = ["Toby Lawrence "] edition = "2018" -rust-version = "1.61.0" +rust-version = "1.65.0" publish = false [dependencies] diff --git a/metrics-exporter-prometheus/CHANGELOG.md b/metrics-exporter-prometheus/CHANGELOG.md index 600265f2..52c85e33 100644 --- a/metrics-exporter-prometheus/CHANGELOG.md +++ b/metrics-exporter-prometheus/CHANGELOG.md @@ -10,7 +10,12 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Added -- Support for using HTTPS in Push Gateway mode. ([#392](https://github.com/metrics-rs/metrics/pull/392)) +- Support for using HTTPS in Push Gateway mode. + ([#392](https://github.com/metrics-rs/metrics/pull/392)) + +### Changed + +- Bump MSRV to 1.65.0. ## [0.12.2] - 2023-12-13 diff --git a/metrics-exporter-prometheus/Cargo.toml b/metrics-exporter-prometheus/Cargo.toml index 1360cd87..8bc2040f 100644 --- a/metrics-exporter-prometheus/Cargo.toml +++ b/metrics-exporter-prometheus/Cargo.toml @@ -3,7 +3,7 @@ name = "metrics-exporter-prometheus" version = "0.12.2" authors = ["Toby Lawrence "] edition = "2018" -rust-version = "1.61.0" +rust-version = "1.65.0" license = "MIT" diff --git a/metrics-exporter-tcp/CHANGELOG.md b/metrics-exporter-tcp/CHANGELOG.md index ce05c71e..026fb550 100644 --- a/metrics-exporter-tcp/CHANGELOG.md +++ b/metrics-exporter-tcp/CHANGELOG.md @@ -8,6 +8,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] - ReleaseDate +### Changed + +- Bump MSRV to 1.65.0. + ## [0.8.0] - 2023-04-16 ### Changed diff --git a/metrics-exporter-tcp/Cargo.toml b/metrics-exporter-tcp/Cargo.toml index 7801ea3c..9a3afd36 100644 --- a/metrics-exporter-tcp/Cargo.toml +++ b/metrics-exporter-tcp/Cargo.toml @@ -3,7 +3,7 @@ name = "metrics-exporter-tcp" version = "0.8.0" authors = ["Toby Lawrence "] edition = "2018" -rust-version = "1.61.0" +rust-version = "1.65.0" license = "MIT" diff --git a/metrics-observer/CHANGELOG.md b/metrics-observer/CHANGELOG.md index 2d3f581e..771d30e8 100644 --- a/metrics-observer/CHANGELOG.md +++ b/metrics-observer/CHANGELOG.md @@ -14,6 +14,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 order, instead of only trying the first and then giving up. ([#429](https://github.com/metrics-rs/metrics/pull/429)) +### Changed + +- Bump MSRV to 1.65.0. + ## [0.2.0] - 2023-04-16 ### Added diff --git a/metrics-observer/Cargo.toml b/metrics-observer/Cargo.toml index 9a50cd21..2cb3fb69 100644 --- a/metrics-observer/Cargo.toml +++ b/metrics-observer/Cargo.toml @@ -3,7 +3,7 @@ name = "metrics-observer" version = "0.2.0" authors = ["Toby Lawrence "] edition = "2018" -rust-version = "1.61.0" +rust-version = "1.65.0" license = "MIT" diff --git a/metrics-tracing-context/CHANGELOG.md b/metrics-tracing-context/CHANGELOG.md index 63ed2d4d..10945325 100644 --- a/metrics-tracing-context/CHANGELOG.md +++ b/metrics-tracing-context/CHANGELOG.md @@ -10,7 +10,12 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Added -- Support for dynamism using `tracing::Span::record` to add label values. ([#408](https://github.com/metrics-rs/metrics/pull/408)) +- Support for dynamism using `tracing::Span::record` to add label values. + ([#408](https://github.com/metrics-rs/metrics/pull/408)) + +### Changed + +- Bump MSRV to 1.65.0. ## [0.14.0] - 2023-04-16 diff --git a/metrics-tracing-context/Cargo.toml b/metrics-tracing-context/Cargo.toml index 96287598..e82a799d 100644 --- a/metrics-tracing-context/Cargo.toml +++ b/metrics-tracing-context/Cargo.toml @@ -3,7 +3,7 @@ name = "metrics-tracing-context" version = "0.14.0" authors = ["MOZGIII "] edition = "2018" -rust-version = "1.61.0" +rust-version = "1.65.0" license = "MIT" diff --git a/metrics-util/CHANGELOG.md b/metrics-util/CHANGELOG.md index a666c911..b0b23c96 100644 --- a/metrics-util/CHANGELOG.md +++ b/metrics-util/CHANGELOG.md @@ -14,6 +14,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Fixed the `Debug` implementation for `bucket::Block` which represented both an unsafe and logically incorrect usage of `crossbeam-epoch.` +### Changed + +- Bump MSRV to 1.65.0. + ## [0.15.1] - 2023-07-02 ### Added diff --git a/metrics-util/Cargo.toml b/metrics-util/Cargo.toml index 373960d5..698f3053 100644 --- a/metrics-util/Cargo.toml +++ b/metrics-util/Cargo.toml @@ -3,7 +3,7 @@ name = "metrics-util" version = "0.15.1" authors = ["Toby Lawrence "] edition = "2018" -rust-version = "1.61.0" +rust-version = "1.65.0" license = "MIT" diff --git a/metrics/CHANGELOG.md b/metrics/CHANGELOG.md index bc47838a..c603136b 100644 --- a/metrics/CHANGELOG.md +++ b/metrics/CHANGELOG.md @@ -20,6 +20,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Changed - Make `Unit` methods return `&'static str` (instead of `&str`) where possible. ([#392](https://github.com/metrics-rs/metrics/pull/393)) +- Bump MSRV to 1.65.0. ## [0.21.1] - 2023-07-02 diff --git a/metrics/Cargo.toml b/metrics/Cargo.toml index e16cabc3..4b6e1b1e 100644 --- a/metrics/Cargo.toml +++ b/metrics/Cargo.toml @@ -3,7 +3,7 @@ name = "metrics" version = "0.21.1" authors = ["Toby Lawrence "] edition = "2018" -rust-version = "1.61.0" +rust-version = "1.65.0" license = "MIT" diff --git a/rust-toolchain.toml b/rust-toolchain.toml index ead02718..f8c2abbb 100644 --- a/rust-toolchain.toml +++ b/rust-toolchain.toml @@ -1,2 +1,2 @@ [toolchain] -channel = "1.61.0" +channel = "1.65.0" From 34ca1c9d72aa61fe0100241428eaff2ceed5ac6c Mon Sep 17 00:00:00 2001 From: Toby Lawrence Date: Sat, 25 Nov 2023 12:50:43 -0500 Subject: [PATCH 12/17] changelog updates --- metrics-util/CHANGELOG.md | 7 +++++++ metrics/CHANGELOG.md | 6 ++++++ 2 files changed, 13 insertions(+) diff --git a/metrics-util/CHANGELOG.md b/metrics-util/CHANGELOG.md index b0b23c96..f24340e5 100644 --- a/metrics-util/CHANGELOG.md +++ b/metrics-util/CHANGELOG.md @@ -17,6 +17,13 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Changed - Bump MSRV to 1.65.0. +- `RecoverableRecorder` no longer functions as a drop guard itself, and instead returns a new + type, `RecoveryHandle`, which provides that functionality. ([#414](https://github.com/metrics-rs/metrics/pull/414)) + +### Removed + +- Support for per-thread mode in `DebuggingRecorder`. Users should now use + `metrics::with_local_recorder` instead, which is inherently per-thread. ## [0.15.1] - 2023-07-02 diff --git a/metrics/CHANGELOG.md b/metrics/CHANGELOG.md index c603136b..d25c7852 100644 --- a/metrics/CHANGELOG.md +++ b/metrics/CHANGELOG.md @@ -17,10 +17,16 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 `&'static str` or `String` were allowed. There's still work to be done to also support labels in this regard. +- Support for local recorders. ([#414](https://github.com/metrics-rs/metrics/pull/414)) + + This is a large feature, and is documented in [RELEASES.md](RELEASES.md). + ### Changed - Make `Unit` methods return `&'static str` (instead of `&str`) where possible. ([#392](https://github.com/metrics-rs/metrics/pull/393)) - Bump MSRV to 1.65.0. +- `SetRecorderError` now returns the recorder given to `set_global_recorder` if another global + recorder was already installed instead of leaking it. ([#414](https://github.com/metrics-rs/metrics/pull/414)) ## [0.21.1] - 2023-07-02 From 71f283963688b2f4ab3690191b88684c8e0e0c0d Mon Sep 17 00:00:00 2001 From: Toby Lawrence Date: Sat, 23 Dec 2023 10:21:42 -0500 Subject: [PATCH 13/17] fix missing refactor due to bad merge conflict resolution --- metrics-tracing-context/tests/integration.rs | 265 +++++++++---------- 1 file changed, 130 insertions(+), 135 deletions(-) diff --git a/metrics-tracing-context/tests/integration.rs b/metrics-tracing-context/tests/integration.rs index 032d2e42..ccceed30 100644 --- a/metrics-tracing-context/tests/integration.rs +++ b/metrics-tracing-context/tests/integration.rs @@ -116,23 +116,23 @@ fn test_basic_functionality() { #[test] fn test_basic_functionality_record() { - let (_guard, snapshotter) = setup(TracingContextLayer::all()); - - let user = "ferris"; - let email = "ferris@rust-lang.org"; - let span = span!( - Level::TRACE, - "login", - user, - user.email = email, - user.id = tracing_core::field::Empty, - ); - let _guard = span.enter(); + let snapshot = with_tracing_layer(TracingContextLayer::all(), || { + let user = "ferris"; + let email = "ferris@rust-lang.org"; + let span = span!( + Level::TRACE, + "login", + user, + user.email = email, + user.id = tracing_core::field::Empty, + ); + let _guard = span.enter(); - span.record("user.id", 42); - counter!("login_attempts", "service" => "login_service").increment(1); + span.record("user.id", 42); + counter!("login_attempts", "service" => "login_service").increment(1); + }); - let snapshot = snapshotter.snapshot().into_vec(); + let snapshot = snapshot.into_vec(); assert_eq!( snapshot, @@ -150,54 +150,48 @@ fn test_basic_functionality_record() { #[test] fn test_basic_functionality_then_record() { - let (_guard, snapshotter) = setup(TracingContextLayer::all()); - - let user = "ferris"; - let email = "ferris@rust-lang.org"; - let span = span!( - Level::TRACE, - "login", - user, - user.email = email, - user.id = tracing_core::field::Empty, - ); - let _guard = span.enter(); - let mut snapshots = vec![]; - { - counter!("login_attempts", "service" => "login_service").increment(1); - - let snapshot = snapshotter.snapshot().into_vec(); - - snapshots.push(( - CompositeKey::new( - MetricKind::Counter, - Key::from_static_parts(LOGIN_ATTEMPTS, SVC_USER_EMAIL), - ), - None, - None, - DebugValue::Counter(1), - )); + let snapshot = with_tracing_layer(TracingContextLayer::all(), || { + let user = "ferris"; + let email = "ferris@rust-lang.org"; + let span = span!( + Level::TRACE, + "login", + user, + user.email = email, + user.id = tracing_core::field::Empty, + ); - assert_eq!(snapshot, snapshots); - } - span.record("user.id", 42); - { + let _guard = span.enter(); counter!("login_attempts", "service" => "login_service").increment(1); - let snapshot = snapshotter.snapshot().into_vec(); + span.record("user.id", 42); + counter!("login_attempts", "service" => "login_service").increment(1); + }); - snapshots.push(( - CompositeKey::new( - MetricKind::Counter, - Key::from_static_parts(LOGIN_ATTEMPTS, SVC_USER_EMAIL_ID), + let snapshot = snapshot.into_vec(); + assert_eq!( + snapshot, + vec![ + ( + CompositeKey::new( + MetricKind::Counter, + Key::from_static_parts(LOGIN_ATTEMPTS, SVC_USER_EMAIL), + ), + None, + None, + DebugValue::Counter(1), ), - None, - None, - DebugValue::Counter(1), - )); - - assert_eq!(snapshot, snapshots); - } + ( + CompositeKey::new( + MetricKind::Counter, + Key::from_static_parts(LOGIN_ATTEMPTS, SVC_USER_EMAIL_ID), + ), + None, + None, + DebugValue::Counter(1), + ) + ] + ); } #[test] @@ -205,18 +199,18 @@ fn test_rerecord() { static USER_ID_42: &[Label] = &[Label::from_static_parts("user.id", "42")]; static USER_ID_123: &[Label] = &[Label::from_static_parts("user.id", "123")]; - let (_guard, snapshotter) = setup(TracingContextLayer::all()); - - let span = span!(Level::TRACE, "login", user.id = tracing_core::field::Empty); - let _guard = span.enter(); + let snapshot = with_tracing_layer(TracingContextLayer::all(), || { + let span = span!(Level::TRACE, "login", user.id = tracing_core::field::Empty); + let _guard = span.enter(); - span.record("user.id", 42); - counter!("login_attempts").increment(1); + span.record("user.id", 42); + counter!("login_attempts").increment(1); - span.record("user.id", 123); - counter!("login_attempts").increment(1); + span.record("user.id", 123); + counter!("login_attempts").increment(1); + }); - let snapshot = snapshotter.snapshot().into_vec(); + let snapshot = snapshot.into_vec(); assert_eq!( snapshot, @@ -245,25 +239,25 @@ fn test_rerecord() { #[test] fn test_loop() { - let (_guard, snapshotter) = setup(TracingContextLayer::all()); - - let user = "ferris"; - let email = "ferris@rust-lang.org"; - let span = span!( - Level::TRACE, - "login", - user, - user.email = email, - attempt = tracing_core::field::Empty, - ); - let _guard = span.enter(); + let snapshot = with_tracing_layer(TracingContextLayer::all(), || { + let user = "ferris"; + let email = "ferris@rust-lang.org"; + let span = span!( + Level::TRACE, + "login", + user, + user.email = email, + attempt = tracing_core::field::Empty, + ); + let _guard = span.enter(); - for attempt in 1..=42 { - span.record("attempt", attempt); - } - counter!("login_attempts").increment(1); + for attempt in 1..=42 { + span.record("attempt", attempt); + } + counter!("login_attempts").increment(1); + }); - let snapshot = snapshotter.snapshot().into_vec(); + let snapshot = snapshot.into_vec(); assert_eq!( snapshot, @@ -283,15 +277,15 @@ fn test_loop() { fn test_record_does_not_overwrite() { static USER_ID_42: &[Label] = &[Label::from_static_parts("user.id", "42")]; - let (_guard, snapshotter) = setup(TracingContextLayer::all()); - - let span = span!(Level::TRACE, "login", user.id = tracing_core::field::Empty); - let _guard = span.enter(); + let snapshot = with_tracing_layer(TracingContextLayer::all(), || { + let span = span!(Level::TRACE, "login", user.id = tracing_core::field::Empty); + let _guard = span.enter(); - span.record("user.id", 666); - counter!("login_attempts", "user.id" => "42").increment(1); + span.record("user.id", 666); + counter!("login_attempts", "user.id" => "42").increment(1); + }); - let snapshot = snapshotter.snapshot().into_vec(); + let snapshot = snapshot.into_vec(); assert_eq!( snapshot, @@ -397,15 +391,15 @@ fn test_no_labels() { #[test] fn test_no_labels_record() { - let (_guard, snapshotter) = setup(TracingContextLayer::all()); - - let span = span!(Level::TRACE, "login", user.id = tracing_core::field::Empty); - let _guard = span.enter(); + let snapshot = with_tracing_layer(TracingContextLayer::all(), || { + let span = span!(Level::TRACE, "login", user.id = tracing_core::field::Empty); + let _guard = span.enter(); - span.record("user.id", 42); - counter!("login_attempts").increment(1); + span.record("user.id", 42); + counter!("login_attempts").increment(1); + }); - let snapshot = snapshotter.snapshot().into_vec(); + let snapshot = snapshot.into_vec(); assert_eq!( snapshot, @@ -511,10 +505,7 @@ fn test_nested_spans() { inner(); }; - let snapshot = with_tracing_layer(TracingContextLayer::all(), || { - outer(); - }); - + let snapshot = with_tracing_layer(TracingContextLayer::all(), outer); let snapshot = snapshot.into_vec(); assert_eq!( @@ -635,48 +626,52 @@ fn test( record_field: bool, increment_before_recording: bool, ) { - let (_guard, snapshotter) = setup(TracingContextLayer::all()); + let snapshot = with_tracing_layer(TracingContextLayer::all(), || { + let parent = if span_field_same_as_metric && parent_field_same_as_span { + tracing::trace_span!("outer", user.email = "changed@domain.com") + } else { + tracing::trace_span!("outer", user.id = 999) + }; - let parent = if span_field_same_as_metric && parent_field_same_as_span { - tracing::trace_span!("outer", user.email = "changed@domain.com") - } else { - tracing::trace_span!("outer", user.id = 999) - }; + let _guard = span_has_parent.then(|| parent.enter()); + + let span = if span_has_fields { + match (span_field_same_as_metric, span_field_is_empty) { + (false, false) => tracing::trace_span!("login", user.id = 666), + (false, true) => { + tracing::trace_span!("login", user.id = tracing_core::field::Empty) + } + (true, false) => tracing::trace_span!("login", user.email = "user@domain.com"), + (true, true) => { + tracing::trace_span!("login", user.email = tracing_core::field::Empty) + } + } + } else { + tracing::trace_span!("login") + }; - let _guard = span_has_parent.then(|| parent.enter()); + let _guard = in_span.then(|| span.enter()); - let span = if span_has_fields { - match (span_field_same_as_metric, span_field_is_empty) { - (false, false) => tracing::trace_span!("login", user.id = 666), - (false, true) => tracing::trace_span!("login", user.id = tracing_core::field::Empty), - (true, false) => tracing::trace_span!("login", user.email = "user@domain.com"), - (true, true) => tracing::trace_span!("login", user.email = tracing_core::field::Empty), - } - } else { - tracing::trace_span!("login") - }; + let increment = || { + if metric_has_labels { + counter!("login_attempts", "user.email" => "ferris@rust-lang.org").increment(1); + } else { + counter!("login_attempts").increment(1); + } + }; - let _guard = in_span.then(|| span.enter()); + if increment_before_recording { + increment(); + } - let increment = || { - if metric_has_labels { - counter!("login_attempts", "user.email" => "ferris@rust-lang.org").increment(1); - } else { - counter!("login_attempts").increment(1); + if record_field { + span.record("user.id", 42); } - }; - if increment_before_recording { increment(); - } - - if record_field { - span.record("user.id", 42); - } - - increment(); + }); - let snapshot = snapshotter.snapshot().into_vec(); + let snapshot = snapshot.into_vec(); let mut expected = vec![]; From b364207e8dd2139faa8dbf80d54699b2246c47e1 Mon Sep 17 00:00:00 2001 From: Toby Lawrence Date: Sat, 23 Dec 2023 10:22:47 -0500 Subject: [PATCH 14/17] clean up docs for set_global_recorder --- metrics/src/recorder/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/metrics/src/recorder/mod.rs b/metrics/src/recorder/mod.rs index b9d6fbf6..458a1ddb 100644 --- a/metrics/src/recorder/mod.rs +++ b/metrics/src/recorder/mod.rs @@ -94,7 +94,7 @@ impl Drop for LocalRecorderGuard { /// Sets the global recorder. /// /// This function may only be called once in the lifetime of a program. Any metrics recorded -/// before the call to `set_recorder` occurs will be completely ignored. +/// 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. From dfa7d6249b16d30cd8466501711c6115952af344 Mon Sep 17 00:00:00 2001 From: Toby Lawrence Date: Sat, 23 Dec 2023 11:09:54 -0500 Subject: [PATCH 15/17] pin home transitive dep because people are dicks about MSRV --- metrics-exporter-tcp/Cargo.toml | 1 + metrics-observer/Cargo.toml | 1 + 2 files changed, 2 insertions(+) diff --git a/metrics-exporter-tcp/Cargo.toml b/metrics-exporter-tcp/Cargo.toml index 9a3afd36..45105e6c 100644 --- a/metrics-exporter-tcp/Cargo.toml +++ b/metrics-exporter-tcp/Cargo.toml @@ -27,6 +27,7 @@ tracing = { version = "0.1", default-features = false, features = ["attributes"] [build-dependencies] prost-build = "0.11" +home = "=0.5.5" [dev-dependencies] quanta = "0.12" diff --git a/metrics-observer/Cargo.toml b/metrics-observer/Cargo.toml index 2cb3fb69..6dad4911 100644 --- a/metrics-observer/Cargo.toml +++ b/metrics-observer/Cargo.toml @@ -29,3 +29,4 @@ chrono = { version = "0.4", default-features = false, features = ["clock"] } [build-dependencies] prost-build = "0.11" +home = "=0.5.5" From c4247eddd1cb0bba895ae755f3aee110830e3c6f Mon Sep 17 00:00:00 2001 From: Toby Lawrence Date: Sat, 23 Dec 2023 11:16:42 -0500 Subject: [PATCH 16/17] install clippy component --- .github/workflows/ci.yml | 2 ++ 1 file changed, 2 insertions(+) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 03840854..b3aafd30 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -86,5 +86,7 @@ jobs: run: sudo apt-get install protobuf-compiler - name: Install Rust Stable run: rustup default stable + - name: Install Clippy + run: rustup component add clippy - name: Run Clippy run: cargo clippy --all-features --workspace --exclude=metrics-benchmark From 416452bce907675d26b826e6430588a9a3c809ad Mon Sep 17 00:00:00 2001 From: Toby Lawrence Date: Sat, 23 Dec 2023 12:05:21 -0500 Subject: [PATCH 17/17] fix breaking clippy lints --- metrics-exporter-prometheus/src/builder.rs | 4 ++-- metrics/src/recorder/mod.rs | 8 +++----- 2 files changed, 5 insertions(+), 7 deletions(-) diff --git a/metrics-exporter-prometheus/src/builder.rs b/metrics-exporter-prometheus/src/builder.rs index f19fe907..442a492d 100644 --- a/metrics-exporter-prometheus/src/builder.rs +++ b/metrics-exporter-prometheus/src/builder.rs @@ -560,9 +560,9 @@ fn basic_auth(username: &str, password: Option<&str>) -> HeaderValue { let mut buf = b"Basic ".to_vec(); { let mut encoder = EncoderWriter::new(&mut buf, &BASE64_STANDARD); - let _ = write!(encoder, "{username}:"); + write!(encoder, "{username}:").expect("should not fail to encode username"); if let Some(password) = password { - let _ = write!(encoder, "{password}"); + write!(encoder, "{password}").expect("should not fail to encode password"); } } let mut header = HeaderValue::from_bytes(&buf).expect("base64 is always valid HeaderValue"); diff --git a/metrics/src/recorder/mod.rs b/metrics/src/recorder/mod.rs index 458a1ddb..8470d7cc 100644 --- a/metrics/src/recorder/mod.rs +++ b/metrics/src/recorder/mod.rs @@ -132,12 +132,10 @@ pub fn with_recorder(f: impl FnOnce(&dyn Recorder) -> T) -> T { // 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) } else { - if let Some(global_recorder) = GLOBAL_RECORDER.try_load() { - f(global_recorder) - } else { - f(&NOOP_RECORDER) - } + f(&NOOP_RECORDER) } }) }