diff --git a/.gitignore b/.gitignore index 762f710..35e886f 100644 --- a/.gitignore +++ b/.gitignore @@ -1,4 +1,4 @@ -/target +**/target /.idea Cargo.lock **/*.jsonl diff --git a/CHANGELOG.md b/CHANGELOG.md new file mode 100644 index 0000000..01c95aa --- /dev/null +++ b/CHANGELOG.md @@ -0,0 +1,19 @@ +# Changelog + +## Unreleased +- Mark `MetricsHandle` as `Send` and `Sync` with an explicit safety contract: it is only safe if backends are either thread-safe (internally synchronized) or are used in a strictly single-threaded manner. This allows allocator counters to live in a `static LazyLock` and fixes the `*mut u8`-derived `Sync` error without changing runtime behavior. +- Clarify allocator instrumentation behavior: allocation counters cache the active handle on first use, so `set_metrics` must run before allocator instrumentation is enabled if you expect these counters to emit. +- Move the release configuration to `[workspace.metadata.release]` to eliminate Cargo's unused manifest key warning while preserving the same metadata for release tooling. +- Remove an extra blank line after the `Histogram` doc comment to satisfy `clippy::empty_line_after_doc_comments` (no behavior change). +- Skip span timing work when the no-op metrics handle is active, returning a no-op span that avoids `Instant::now()`/`rdtsc` and record calls. This reduces overhead when metrics are disabled while keeping behavior unchanged when a real backend is set. +- Avoid u128 nanosecond conversion on non-`rdtsc` span drops by computing nanos from seconds + subsecond nanos with wrapping arithmetic. This keeps the u64 return value behavior while trimming a small amount of conversion overhead. +- Cache the active metrics handle inside `Counter`/`Histogram` so hot-path operations avoid an atomic load. This tightens the initialization contract: `set_metrics` must run before any counters/histograms (including macro statics) are created. +- Replace mutable global metrics access with an immutable handle, moving metrics vtable calls to `&self` and using relaxed loads plus an explicit SeqCst store for the global handle in `set_metrics`. This removes `static mut` access and `get_mut` usage, reducing unsafe mutable aliasing while keeping hot-path reads fast when initialization happens before worker threads. +- Switch counters/histograms and proc-macro statics to `LazyCell` with `LazyCell::force`, dropping `UnsafeCell` and `LazyLock` implementations. This narrows the API surface to a single lazy init path and avoids raw mutable access inside statics while preserving lazy initialization semantics. +- Keep exporter publish signatures using the `Counters`/`Histograms` aliases (including top-level/UDP exporters) for consistency, even though they are just `HashMap` type aliases. +- Add a default-on `span` feature: when disabled, `Span` becomes a no-op type and timing code is compiled out; when enabled, span timing behaves as before. + +This refactor makes `MetricsHandle` immutable because it is just a vtable pointer and backend synchronization lives behind it, so callers do not need mutable access to the handle. With an immutable global handle, we can remove `static mut` and `&mut` aliasing, and then use `LazyCell::force` to get shared references to counters and histograms without `UnsafeCell`, making the API cleaner and safer. + +Breaking: `CounterOps`/`HistogramOps` are no longer implemented for `LazyLock>`. Code using `LazyLock` should switch to `LazyCell` or use a plain `Counter`/`Histogram` value. +Behavioral note: `set_metrics` must run before counters/histograms are created (including macro statics) and before worker threads start; metrics cache the handle and relaxed loads may otherwise observe the old backend. diff --git a/Cargo.toml b/Cargo.toml index fef5433..9d18793 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -18,7 +18,7 @@ members = [ ] resolver = "3" -[release] +[workspace.metadata.release] workspace = true [workspace.dependencies] diff --git a/metricus/Cargo.toml b/metricus/Cargo.toml index 8ff5322..19e046b 100644 --- a/metricus/Cargo.toml +++ b/metricus/Cargo.toml @@ -12,7 +12,8 @@ categories.workspace = true rust-version.workspace = true [features] -default = [] +default = ["span"] +span = [] rdtsc = ["dep:quanta"] [dependencies] @@ -33,4 +34,3 @@ harness = false name = "dispatch" path = "benches/dispatch.rs" harness = false - diff --git a/metricus/src/counter.rs b/metricus/src/counter.rs index b30fe89..d55f468 100644 --- a/metricus/src/counter.rs +++ b/metricus/src/counter.rs @@ -1,9 +1,8 @@ //! A `Counter` proxy struct for managing a metrics counter. -use crate::access::get_metrics_mut; -use crate::{Id, Tags}; -use std::cell::{LazyCell, UnsafeCell}; -use std::sync::LazyLock; +use crate::access::get_metrics; +use crate::{Id, MetricsHandle, Tags}; +use std::cell::LazyCell; /// Provides methods to create a new counter, increment it, and /// increment it by a specified amount. It automatically deletes the counter @@ -36,9 +35,15 @@ use std::sync::LazyLock; /// /// my_function_with_tags(); /// ```` -#[derive(Debug)] pub struct Counter { id: Id, + handle: &'static MetricsHandle, +} + +impl std::fmt::Debug for Counter { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.debug_struct("Counter").field("id", &self.id).finish() + } } impl Counter { @@ -61,8 +66,12 @@ impl Counter { /// let counter = Counter::new("user_count", empty_tags()); /// ``` pub fn new(name: &str, tags: Tags) -> Self { - let counter_id = get_metrics_mut().new_counter(name, tags); - Self { id: counter_id } + let metrics = get_metrics(); + let counter_id = metrics.new_counter(name, tags); + Self { + id: counter_id, + handle: metrics, + } } /// Create a counter object without registering it. @@ -78,13 +87,14 @@ impl Counter { /// let counter = Counter::new_with_id(1); /// ``` pub fn new_with_id(id: Id) -> Self { - Self { id } + let metrics = get_metrics(); + Self { id, handle: metrics } } } impl Drop for Counter { fn drop(&mut self) { - get_metrics_mut().delete_counter(self.id); + self.handle.delete_counter(self.id); } } @@ -116,31 +126,25 @@ pub trait CounterOps { } impl CounterOps for Counter { + #[inline] fn increment(&self) { - get_metrics_mut().increment_counter(self.id); - } - - fn increment_by(&self, delta: u64) { - get_metrics_mut().increment_counter_by(self.id, delta); - } -} - -impl CounterOps for LazyCell> { - fn increment(&self) { - unsafe { &mut *self.get() }.increment() + self.handle.increment_counter(self.id); } + #[inline] fn increment_by(&self, delta: u64) { - unsafe { &mut *self.get() }.increment_by(delta) + self.handle.increment_counter_by(self.id, delta); } } -impl CounterOps for LazyLock> { +impl Counter> CounterOps for LazyCell { + #[inline] fn increment(&self) { - unsafe { &mut *self.get() }.increment() + LazyCell::force(self).increment() } + #[inline] fn increment_by(&self, delta: u64) { - unsafe { &mut *self.get() }.increment_by(delta) + LazyCell::force(self).increment_by(delta) } } diff --git a/metricus/src/histogram.rs b/metricus/src/histogram.rs index 6fe4582..8ad7b8c 100644 --- a/metricus/src/histogram.rs +++ b/metricus/src/histogram.rs @@ -1,11 +1,13 @@ //! A `Histogram` proxy struct for managing a metrics histogram. -use crate::access::get_metrics_mut; -use crate::{Id, Tags}; -#[cfg(feature = "rdtsc")] +use crate::access::get_metrics; +use crate::{Id, MetricsHandle, Tags}; +#[cfg(all(feature = "span", feature = "rdtsc"))] use quanta::Clock; -use std::cell::{LazyCell, UnsafeCell}; -#[cfg(not(feature = "rdtsc"))] +use std::cell::LazyCell; +#[cfg(not(feature = "span"))] +use std::marker::PhantomData; +#[cfg(all(feature = "span", not(feature = "rdtsc")))] use std::time::Instant; /// Facilitates the creation of a new histogram, recording of values, and @@ -40,14 +42,25 @@ use std::time::Instant; /// /// my_function_with_tags(); /// ```` - -#[derive(Debug)] pub struct Histogram { id: Id, - #[cfg(feature = "rdtsc")] + handle: &'static MetricsHandle, + #[cfg(all(feature = "span", feature = "rdtsc"))] clock: Clock, } +impl std::fmt::Debug for Histogram { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + let mut debug = f.debug_struct("Histogram"); + debug.field("id", &self.id); + #[cfg(all(feature = "span", feature = "rdtsc"))] + { + debug.field("clock", &self.clock); + } + debug.finish() + } +} + impl Histogram { /// Creates a new histogram with the specified name and tags. /// Units of measurement are not defined by the histogram itself but should be implied @@ -70,10 +83,12 @@ impl Histogram { /// let histogram = Histogram::new("login_duration", empty_tags()); /// ``` pub fn new(name: &str, tags: Tags) -> Self { - let histogram_id = get_metrics_mut().new_histogram(name, tags); + let metrics = get_metrics(); + let histogram_id = metrics.new_histogram(name, tags); Self { id: histogram_id, - #[cfg(feature = "rdtsc")] + handle: metrics, + #[cfg(all(feature = "span", feature = "rdtsc"))] clock: Clock::new(), } } @@ -124,64 +139,111 @@ pub trait HistogramOps { } impl HistogramOps for Histogram { + #[inline] fn record(&self, value: u64) { - get_metrics_mut().record(self.id, value); + self.handle.record(self.id, value); } + #[inline] + #[cfg(feature = "span")] fn span(&self) -> Span<'_> { + if std::ptr::eq(self.handle, &crate::NO_OP_METRICS_HANDLE) { + return Span { state: SpanState::NoOp }; + } Span { - histogram: self, - #[cfg(feature = "rdtsc")] - start_raw: self.clock.raw(), - #[cfg(not(feature = "rdtsc"))] - start_instant: Instant::now(), + state: SpanState::Active { + histogram: self, + #[cfg(feature = "rdtsc")] + start_raw: self.clock.raw(), + #[cfg(not(feature = "rdtsc"))] + start_instant: Instant::now(), + }, } } + #[inline] + #[cfg(not(feature = "span"))] + fn span(&self) -> Span<'_> { + Span { _marker: PhantomData } + } + + #[inline] fn with_span R, R>(&self, f: F) -> R { let _span = self.span(); f() } } -impl HistogramOps for LazyCell> { +impl Histogram> HistogramOps for LazyCell { + #[inline] fn record(&self, value: u64) { - unsafe { &mut *self.get() }.record(value) + LazyCell::force(self).record(value) } + #[inline] fn span(&self) -> Span<'_> { - unsafe { &mut *self.get() }.span() + LazyCell::force(self).span() } - fn with_span R, R>(&self, f: F) -> R { - unsafe { &mut *self.get() }.with_span(f) + #[inline] + fn with_span R, R>(&self, f: G) -> R { + LazyCell::force(self).with_span(f) } } impl Drop for Histogram { fn drop(&mut self) { - get_metrics_mut().delete_histogram(self.id); + self.handle.delete_histogram(self.id); } } /// Used for measuring how long given operation takes. The duration is recorded in nanoseconds. +#[cfg(feature = "span")] pub struct Span<'a> { - histogram: &'a Histogram, - #[cfg(feature = "rdtsc")] - start_raw: u64, - #[cfg(not(feature = "rdtsc"))] - start_instant: Instant, + state: SpanState<'a>, +} + +/// No-op span used when the `span` feature is disabled. +#[cfg(not(feature = "span"))] +pub struct Span<'a> { + _marker: PhantomData<&'a ()>, +} + +#[cfg(feature = "span")] +enum SpanState<'a> { + Active { + histogram: &'a Histogram, + #[cfg(feature = "rdtsc")] + start_raw: u64, + #[cfg(not(feature = "rdtsc"))] + start_instant: Instant, + }, + NoOp, } +#[cfg(feature = "span")] impl Drop for Span<'_> { fn drop(&mut self) { #[cfg(feature = "rdtsc")] { - let end_raw = self.histogram.clock.raw(); - let elapsed = self.histogram.clock.delta_as_nanos(self.start_raw, end_raw); - self.histogram.record(elapsed); + if let SpanState::Active { histogram, start_raw } = &self.state { + let end_raw = histogram.clock.raw(); + let elapsed = histogram.clock.delta_as_nanos(*start_raw, end_raw); + histogram.record(elapsed); + } } #[cfg(not(feature = "rdtsc"))] - self.histogram.record(self.start_instant.elapsed().as_nanos() as u64); + if let SpanState::Active { + histogram, + start_instant, + } = &self.state + { + let elapsed = start_instant.elapsed(); + let nanos = elapsed + .as_secs() + .wrapping_mul(1_000_000_000) + .wrapping_add(u64::from(elapsed.subsec_nanos())); + histogram.record(nanos); + } } } diff --git a/metricus/src/lib.rs b/metricus/src/lib.rs index 804b8c2..52258db 100644 --- a/metricus/src/lib.rs +++ b/metricus/src/lib.rs @@ -25,6 +25,8 @@ pub const fn empty_tags() -> Tags<'static> { } /// Common interface for metrics backend. Each new backend must implement this trait. +/// Implementations must be safe for concurrent use or be used in a strictly single-threaded +/// manner, because the global metrics handle can be shared across threads. pub trait Metrics { fn name(&self) -> &'static str; @@ -204,15 +206,16 @@ struct MetricsHolder { } /// Initially set to no-op backend. -static mut METRICS: MetricsHolder = MetricsHolder { +static METRICS: MetricsHolder = MetricsHolder { handle: AtomicRef::new(&NO_OP_METRICS_HANDLE), }; -/// Set a new metrics backend. This should be called as early as possible. Otherwise, -/// all metrics calls will delegate to the `NoOpMetrics`. +/// Set a new metrics backend. This must be called before any counters or histograms are +/// created (including through macros), because metric objects cache the active handle. +/// It should also be called before any worker threads start so hot-path loads can use +/// relaxed ordering. Otherwise, all metrics calls will delegate to the `NoOpMetrics`. pub fn set_metrics(metrics: impl Metrics) { - #[allow(static_mut_refs)] - unsafe { &mut METRICS } + METRICS .handle .set(Box::leak(Box::new(metrics.into_handle())), Ordering::SeqCst); } @@ -239,39 +242,44 @@ pub struct MetricsHandle { name: &'static str, } +// Safety: the handle is immutable and backend implementations are responsible for +// any internal synchronization if they are used across threads. +unsafe impl Send for MetricsHandle {} +unsafe impl Sync for MetricsHandle {} + impl MetricsHandle { #[inline] - fn new_counter(&mut self, name: &str, tags: Tags) -> Id { + fn new_counter(&self, name: &str, tags: Tags) -> Id { (self.vtable.new_counter)(self.ptr, name, tags) } #[inline] - fn delete_counter(&mut self, id: Id) { + fn delete_counter(&self, id: Id) { (self.vtable.delete_counter)(self.ptr, id) } #[inline] - fn increment_counter_by(&mut self, id: Id, delta: u64) { + fn increment_counter_by(&self, id: Id, delta: u64) { (self.vtable.increment_counter_by)(self.ptr, id, delta) } #[inline] - fn increment_counter(&mut self, id: Id) { + fn increment_counter(&self, id: Id) { (self.vtable.increment_counter)(self.ptr, id) } #[inline] - fn new_histogram(&mut self, name: &str, tags: Tags) -> Id { + fn new_histogram(&self, name: &str, tags: Tags) -> Id { (self.vtable.new_histogram)(self.ptr, name, tags) } #[inline] - fn delete_histogram(&mut self, id: Id) { + fn delete_histogram(&self, id: Id) { (self.vtable.delete_histogram)(self.ptr, id) } #[inline] - fn record(&mut self, id: Id, value: u64) { + fn record(&self, id: Id, value: u64) { (self.vtable.record)(self.ptr, id, value) } } @@ -288,18 +296,13 @@ impl AtomicRef { } #[inline] - pub fn get(&self, order: Ordering) -> &T { - unsafe { &*self.ptr.load(order) } + pub fn get(&self, ordering: Ordering) -> &T { + unsafe { &*self.ptr.load(ordering) } } #[inline] - pub fn get_mut(&mut self, order: Ordering) -> &mut T { - unsafe { &mut *self.ptr.load(order) } - } - - #[inline] - pub fn set(&self, new_ref: &T, order: Ordering) { - self.ptr.store(new_ref as *const T as *mut T, order); + pub fn set(&self, new_ref: &T, ordering: Ordering) { + self.ptr.store(new_ref as *const T as *mut T, ordering); } } @@ -307,13 +310,8 @@ mod access { use crate::{METRICS, MetricsHandle}; use std::sync::atomic::Ordering; - #[allow(static_mut_refs)] - pub fn get_metrics_mut() -> &'static mut MetricsHandle { - unsafe { &mut METRICS }.handle.get_mut(Ordering::Acquire) - } - - #[allow(static_mut_refs)] + #[inline(always)] pub fn get_metrics() -> &'static MetricsHandle { - unsafe { &METRICS }.handle.get(Ordering::Acquire) + METRICS.handle.get(Ordering::Relaxed) } } diff --git a/metricus_agent/src/exporter.rs b/metricus_agent/src/exporter.rs index 0db698f..20b414c 100644 --- a/metricus_agent/src/exporter.rs +++ b/metricus_agent/src/exporter.rs @@ -1,4 +1,4 @@ -use crate::aggregator::{Counter, Counters, Encoder, Histogram, Histograms}; +use crate::aggregator::{Counters, Encoder, Histograms}; use crate::config::{ExporterSource, FileConfig, UdpConfig, UnixSocketConfig}; use log::warn; use metricus::Id; @@ -35,7 +35,7 @@ impl TryFrom for Exporter { } impl Exporter { - pub fn publish_counters(&mut self, counters: &HashMap, timestamp: u64) -> std::io::Result<()> { + pub fn publish_counters(&mut self, counters: &Counters, timestamp: u64) -> std::io::Result<()> { match self { Exporter::NoOp => Ok(()), Exporter::Udp(exporter) => exporter.publish_counters(counters, timestamp), @@ -45,7 +45,7 @@ impl Exporter { } } - pub fn publish_histograms(&mut self, histograms: &HashMap, timestamp: u64) -> std::io::Result<()> { + pub fn publish_histograms(&mut self, histograms: &Histograms, timestamp: u64) -> std::io::Result<()> { match self { Exporter::NoOp => Ok(()), Exporter::Udp(exporter) => exporter.publish_histograms(histograms, timestamp), @@ -209,7 +209,7 @@ impl TryFrom for StreamExporter { } impl StreamExporter { - fn publish_counters(&mut self, counters: &HashMap, timestamp: u64) -> std::io::Result<()> { + fn publish_counters(&mut self, counters: &Counters, timestamp: u64) -> std::io::Result<()> { for counter in counters.values() { self.encoder.encode_counter(counter, timestamp, &mut self.writer)?; } @@ -217,7 +217,7 @@ impl StreamExporter { Ok(()) } - fn publish_histograms(&mut self, histograms: &HashMap, timestamp: u64) -> std::io::Result<()> { + fn publish_histograms(&mut self, histograms: &Histograms, timestamp: u64) -> std::io::Result<()> { for histogram in histograms.values() { self.encoder.encode_histogram(histogram, timestamp, &mut self.writer)?; } diff --git a/metricus_allocator/README.md b/metricus_allocator/README.md index 3bc2987..cfdbd05 100644 --- a/metricus_allocator/README.md +++ b/metricus_allocator/README.md @@ -1,3 +1,8 @@ # metricus-allocator -Contains allocator. \ No newline at end of file +Contains allocator. + +## Usage notes + +- Call `metricus::set_metrics` before enabling allocator instrumentation if you expect allocation counters to emit. +- Call `enable_allocator_instrumentation` for each thread that should report allocation metrics. diff --git a/metricus_allocator/src/lib.rs b/metricus_allocator/src/lib.rs index 3b4cec4..47fcfc9 100644 --- a/metricus_allocator/src/lib.rs +++ b/metricus_allocator/src/lib.rs @@ -134,8 +134,9 @@ pub fn enable_allocator_instrumentation() { static COUNTERS: LazyLock = LazyLock::new(|| Counters { // `counter_with_id` creates a counter object without registering it. - // This is used for allocation and de-allocation counters, which are special cases that are initialised before the metrics backend is created. - // In that case, the `Counter` is created with the `NoOpBackend`, so we defer the registration of the counters until the actual backend is ready. + // These allocation counters are created lazily on first use and cache the active metrics handle. + // If they are initialized before `set_metrics`, they will remain bound to the no-op backend. + // Ensure the backend is set before enabling allocator instrumentation if you want these to emit. alloc_count: Counter::new_with_id(ALLOC_COUNTER_ID), alloc_bytes: Counter::new_with_id(ALLOC_BYTES_COUNTER_ID), dealloc_count: Counter::new_with_id(DEALLOC_COUNTER_ID), diff --git a/metricus_macros/src/lib.rs b/metricus_macros/src/lib.rs index d37284b..1076014 100644 --- a/metricus_macros/src/lib.rs +++ b/metricus_macros/src/lib.rs @@ -129,7 +129,7 @@ pub fn counter(attr: TokenStream, item: TokenStream) -> TokenStream { #(#attrs)* #fn_vis #fn_async #fn_unsafe fn #fn_name #fn_generics (#fn_args) #fn_output #fn_where_clause { - static mut COUNTER: core::cell::LazyCell> = core::cell::LazyCell::new(|| core::cell::UnsafeCell::new(metricus::Counter::new(#measurement, &[ #(#tags),* ]))); + static mut COUNTER: core::cell::LazyCell = core::cell::LazyCell::new(|| metricus::Counter::new(#measurement, &[ #(#tags),* ])); #[allow(static_mut_refs)] unsafe { metricus::CounterOps::increment(&COUNTER); } @@ -235,7 +235,7 @@ pub fn counter_with_id(attr: TokenStream, item: TokenStream) -> TokenStream { #(#attrs)* #fn_vis #fn_async #fn_unsafe fn #fn_name #fn_generics (#fn_args) #fn_output #fn_where_clause { - static mut COUNTER: core::cell::LazyCell> = core::cell::LazyCell::new(|| core::cell::UnsafeCell::new(metricus::Counter::new_with_id(#counter_id))); + static mut COUNTER: core::cell::LazyCell = core::cell::LazyCell::new(|| metricus::Counter::new_with_id(#counter_id)); #[allow(static_mut_refs)] unsafe { metricus::CounterOps::increment(&COUNTER); } @@ -362,7 +362,7 @@ pub fn span(attr: TokenStream, item: TokenStream) -> TokenStream { #(#attrs)* #fn_vis #fn_async #fn_unsafe fn #fn_name #fn_generics (#fn_args) #fn_output #fn_where_clause { - static mut HISTOGRAM: core::cell::LazyCell> = core::cell::LazyCell::new(|| core::cell::UnsafeCell::new(metricus::Histogram::new(#measurement, &[ #(#tags),* ]))); + static mut HISTOGRAM: core::cell::LazyCell = core::cell::LazyCell::new(|| metricus::Histogram::new(#measurement, &[ #(#tags),* ])); #[allow(static_mut_refs)] let _span = unsafe { metricus::HistogramOps::span(&HISTOGRAM) };