diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 2fde46c8..2c5dd6a0 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -24,9 +24,11 @@ jobs: - name: Install Rust Stable run: rustup default stable - name: Install cargo-hack - run: cargo install cargo-hack + uses: taiki-e/install-action@v2 + with: + tool: cargo-hack - name: Check Feature Matrix - run: cargo hack build --all --all-targets --feature-powerset + run: cargo hack check --all --all-targets --feature-powerset --release test: name: Test ${{ matrix.rust_version }} runs-on: ubuntu-latest @@ -64,3 +66,17 @@ jobs: run: rustup default stable - name: Run Benchmarks run: cargo bench --all-features --workspace --exclude=metrics-observer + clippy: + name: Clippy ${{ matrix.rust_version }} + 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: Run Clippy + run: cargo clippy --all-features --workspace --exclude=metrics-observer diff --git a/metrics-exporter-prometheus/CHANGELOG.md b/metrics-exporter-prometheus/CHANGELOG.md index 1ebd8c1b..600265f2 100644 --- a/metrics-exporter-prometheus/CHANGELOG.md +++ b/metrics-exporter-prometheus/CHANGELOG.md @@ -8,6 +8,16 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] - ReleaseDate +### Added + +- Support for using HTTPS in Push Gateway mode. ([#392](https://github.com/metrics-rs/metrics/pull/392)) + +## [0.12.2] - 2023-12-13 + +### Fixed + +- Fixed overflow/underflow panic with time moving backwards ([#423](https://github.com/metrics-rs/metrics/pull/423)) + ## [0.12.1] - 2023-05-09 ### Added diff --git a/metrics-exporter-prometheus/Cargo.toml b/metrics-exporter-prometheus/Cargo.toml index 308a11be..28df36bd 100644 --- a/metrics-exporter-prometheus/Cargo.toml +++ b/metrics-exporter-prometheus/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "metrics-exporter-prometheus" -version = "0.12.1" +version = "0.12.2" authors = ["Toby Lawrence "] edition = "2018" rust-version = "1.61.0" diff --git a/metrics-exporter-prometheus/src/builder.rs b/metrics-exporter-prometheus/src/builder.rs index 8c41ce15..70f5f762 100644 --- a/metrics-exporter-prometheus/src/builder.rs +++ b/metrics-exporter-prometheus/src/builder.rs @@ -29,6 +29,7 @@ use hyper::{ http::HeaderValue, Method, Request, Uri, }; +#[cfg(feature = "push-gateway")] use hyper_tls::HttpsConnector; use indexmap::IndexMap; @@ -403,6 +404,7 @@ impl PrometheusBuilder { /// /// If there is an error while building the recorder and exporter, an error variant will be /// returned describing the error. + #[warn(clippy::too_many_lines)] #[cfg(any(feature = "http-listener", feature = "push-gateway"))] #[cfg_attr(docsrs, doc(cfg(any(feature = "http-listener", feature = "push-gateway"))))] #[cfg_attr(not(feature = "http-listener"), allow(unused_mut))] @@ -500,7 +502,7 @@ impl PrometheusBuilder { .map(|mut b| b.copy_to_bytes(b.remaining())) .map(|b| b[..].to_vec()) .and_then(|s| String::from_utf8(s).map_err(|_| ())) - .unwrap_or_else(|_| { + .unwrap_or_else(|()| { String::from("") }); error!( diff --git a/metrics-exporter-prometheus/src/distribution.rs b/metrics-exporter-prometheus/src/distribution.rs index 5dcc7984..24475d82 100644 --- a/metrics-exporter-prometheus/src/distribution.rs +++ b/metrics-exporter-prometheus/src/distribution.rs @@ -27,6 +27,7 @@ pub enum Distribution { impl Distribution { /// Creates a histogram distribution. + #[warn(clippy::missing_panics_doc)] pub fn new_histogram(buckets: &[f64]) -> Distribution { let hist = Histogram::new(buckets).expect("buckets should never be empty"); Distribution::Histogram(hist) @@ -83,7 +84,7 @@ impl DistributionBuilder { /// Returns a distribution for the given metric key. pub fn get_distribution(&self, name: &str) -> Distribution { if let Some(ref overrides) = self.bucket_overrides { - for (matcher, buckets) in overrides.iter() { + for (matcher, buckets) in overrides { if matcher.matches(name) { return Distribution::new_histogram(buckets); } @@ -104,7 +105,7 @@ impl DistributionBuilder { } if let Some(ref overrides) = self.bucket_overrides { - for (matcher, _) in overrides.iter() { + for (matcher, _) in overrides { if matcher.matches(name) { return "histogram"; } @@ -218,15 +219,6 @@ impl RollingSummary { self.buckets.truncate(self.max_buckets - 1); self.buckets.insert(0, Bucket { begin, summary }); - } else { - begin = reftime - self.bucket_duration; - while now < begin { - begin -= self.bucket_duration; - } - - self.buckets.truncate(self.max_buckets - 1); - self.buckets.push(Bucket { begin, summary }); - self.buckets.sort_unstable_by(|a, b| b.begin.cmp(&a.begin)); } } @@ -358,58 +350,33 @@ mod tests { } #[test] - fn add_to_tail() { + fn add_value_ts_before_first_bucket() { let (clock, mock) = Clock::mock(); - mock.increment(Duration::from_secs(3600)); - - let mut summary = RollingSummary::default(); - summary.add(42.0, clock.now()); - let mut expected = Vec::new(); - expected.push(clock.now()); - mock.decrement(Duration::from_secs(20)); - summary.add(42.0, clock.now()); - expected.push(clock.now()); + mock.increment(Duration::from_secs(4)); - let actual: Vec = summary.buckets().iter().map(|b| b.begin).collect(); - assert_eq!(expected, actual); - } + let bucket_count = NonZeroU32::new(2).unwrap(); + let bucket_width = Duration::from_secs(5); - #[test] - fn add_to_tail_with_gap() { - let (clock, mock) = Clock::mock(); - mock.increment(Duration::from_secs(3600)); + let mut summary = RollingSummary::new(bucket_count, bucket_width); + assert_eq!(0, summary.buckets().len()); + assert_eq!(0, summary.count()); - let mut summary = RollingSummary::default(); - summary.add(42.0, clock.now()); - let mut expected = Vec::new(); - expected.push(clock.now()); - mock.decrement(Duration::from_secs(40)); + // Add a single value to create our first bucket. summary.add(42.0, clock.now()); - expected.push(clock.now()); - let actual: Vec = summary.buckets().iter().map(|b| b.begin).collect(); - assert_eq!(expected, actual); - } + // Make sure the value got added. + assert_eq!(1, summary.buckets().len()); + assert_eq!(1, summary.count()); + assert!(!summary.is_empty()); - #[test] - fn add_to_middle_gap() { - let (clock, mock) = Clock::mock(); - mock.increment(Duration::from_secs(3600)); + // Our first bucket is now marked as begin=4/width=5, so make sure that if we add a version + // with now=3, the count goes up but it's not actually added. + mock.decrement(Duration::from_secs(1)); - let mut expected = Vec::new(); - expected.resize(3, Instant::now()); + summary.add(43.0, clock.now()); - let mut summary = RollingSummary::default(); - summary.add(42.0, clock.now()); - expected[0] = clock.now(); - mock.decrement(Duration::from_secs(40)); - summary.add(42.0, clock.now()); - expected[2] = clock.now(); - mock.increment(Duration::from_secs(20)); - summary.add(42.0, clock.now()); - expected[1] = clock.now(); - - let actual: Vec = summary.buckets().iter().map(|b| b.begin).collect(); - assert_eq!(expected, actual); + assert_eq!(1, summary.buckets().len()); + assert_eq!(2, summary.count()); + assert!(!summary.is_empty()); } } diff --git a/metrics-exporter-prometheus/src/recorder.rs b/metrics-exporter-prometheus/src/recorder.rs index 699f0d75..5453c06e 100644 --- a/metrics-exporter-prometheus/src/recorder.rs +++ b/metrics-exporter-prometheus/src/recorder.rs @@ -86,7 +86,7 @@ impl Inner { let mut wg = self.distributions.write().unwrap_or_else(PoisonError::into_inner); let entry = wg .entry(name.clone()) - .or_insert_with(IndexMap::new) + .or_default() .entry(labels) .or_insert_with(|| self.distribution_builder.get_distribution(name.as_str())); diff --git a/metrics-observer/CHANGELOG.md b/metrics-observer/CHANGELOG.md index d1349e4c..2d3f581e 100644 --- a/metrics-observer/CHANGELOG.md +++ b/metrics-observer/CHANGELOG.md @@ -8,11 +8,17 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] - ReleaseDate +### Fixed + +- All addresses returned when trying to connect to the specified exporter endpoint will be tried, in + order, instead of only trying the first and then giving up. + ([#429](https://github.com/metrics-rs/metrics/pull/429)) + ## [0.2.0] - 2023-04-16 ### Added -- Update hdrhistogram dependency to 7.2 +- Update `hdrhistogram`` dependency to 7.2 ### Changed diff --git a/metrics-observer/src/metrics.rs b/metrics-observer/src/metrics.rs index 83c2a45f..af5c8e72 100644 --- a/metrics-observer/src/metrics.rs +++ b/metrics-observer/src/metrics.rs @@ -114,30 +114,22 @@ impl Runner { let mut state = self.client_state.lock().unwrap(); *state = ClientState::Disconnected(None); } - - // Try to connect to our target and transition into Connected. - let addr = match self.addr.to_socket_addrs() { - Ok(mut addrs) => match addrs.next() { - Some(addr) => addr, - None => { - let mut state = self.client_state.lock().unwrap(); - *state = ClientState::Disconnected(Some( - "failed to resolve specified host".to_string(), - )); - break; - } - }, - Err(_) => { - let mut state = self.client_state.lock().unwrap(); - *state = ClientState::Disconnected(Some( - "failed to resolve specified host".to_string(), - )); - break; - } + // Resolve the target address. + let Ok(mut addrs) = self.addr.to_socket_addrs() else { + let mut state = self.client_state.lock().unwrap(); + *state = ClientState::Disconnected(Some( + "failed to resolve specified host".to_string(), + )); + break; }; - match TcpStream::connect_timeout(&addr, Duration::from_secs(3)) { - Ok(stream) => RunnerState::Connected(stream), - Err(_) => RunnerState::ErrorBackoff( + // Some of the resolved addresses may be unreachable (e.g. IPv6). + // Pick the first one that works. + let maybe_stream = addrs.find_map(|addr| { + TcpStream::connect_timeout(&addr, Duration::from_secs(3)).ok() + }); + match maybe_stream { + Some(stream) => RunnerState::Connected(stream), + None => RunnerState::ErrorBackoff( "error while connecting", Duration::from_secs(3), ), diff --git a/metrics-tracing-context/CHANGELOG.md b/metrics-tracing-context/CHANGELOG.md index 389f8da2..63ed2d4d 100644 --- a/metrics-tracing-context/CHANGELOG.md +++ b/metrics-tracing-context/CHANGELOG.md @@ -8,6 +8,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] - ReleaseDate +### Added + +- Support for dynamism using `tracing::Span::record` to add label values. ([#408](https://github.com/metrics-rs/metrics/pull/408)) + ## [0.14.0] - 2023-04-16 ### Changed diff --git a/metrics-tracing-context/Cargo.toml b/metrics-tracing-context/Cargo.toml index 0a692f7f..96287598 100644 --- a/metrics-tracing-context/Cargo.toml +++ b/metrics-tracing-context/Cargo.toml @@ -32,6 +32,7 @@ itoa = { version = "1", default-features = false } metrics = { version = "^0.21", path = "../metrics" } metrics-util = { version = "^0.15", path = "../metrics-util" } lockfree-object-pool = { version = "0.1.3", default-features = false } +indexmap = { version = "2.1", default-features = false, features = ["std"] } once_cell = { version = "1", default-features = false, features = ["std"] } tracing = { version = "0.1.29", default-features = false } tracing-core = { version = "0.1.21", default-features = false } @@ -42,3 +43,4 @@ criterion = { version = "=0.3.3", default-features = false } parking_lot = { version = "0.12.1", default-features = false } tracing = { version = "0.1.29", default-features = false, features = ["std"] } tracing-subscriber = { version = "0.3.1", default-features = false, features = ["registry"] } +itertools = { version = "0.12.0", default-features = false, features = ["use_std"] } diff --git a/metrics-tracing-context/benches/visit.rs b/metrics-tracing-context/benches/visit.rs index a3128af9..9b6db089 100644 --- a/metrics-tracing-context/benches/visit.rs +++ b/metrics-tracing-context/benches/visit.rs @@ -1,8 +1,9 @@ use std::sync::Arc; use criterion::{criterion_group, criterion_main, BatchSize, Criterion}; +use indexmap::IndexMap; use lockfree_object_pool::LinearObjectPool; -use metrics::Label; +use metrics::SharedString; use metrics_tracing_context::Labels; use once_cell::sync::OnceCell; use tracing::Metadata; @@ -13,9 +14,11 @@ use tracing_core::{ Callsite, Interest, }; -fn get_pool() -> &'static Arc>> { - static POOL: OnceCell>>> = OnceCell::new(); - POOL.get_or_init(|| Arc::new(LinearObjectPool::new(|| Vec::new(), |vec| vec.clear()))) +type Map = IndexMap; + +fn get_pool() -> &'static Arc> { + static POOL: OnceCell>> = OnceCell::new(); + POOL.get_or_init(|| Arc::new(LinearObjectPool::new(Map::new, Map::clear))) } const BATCH_SIZE: usize = 1000; diff --git a/metrics-tracing-context/src/lib.rs b/metrics-tracing-context/src/lib.rs index 71d219d4..656e6449 100644 --- a/metrics-tracing-context/src/lib.rs +++ b/metrics-tracing-context/src/lib.rs @@ -55,19 +55,25 @@ //! //! # Implementation //! -//! The integration layer works by capturing all fields present when a span is created and storing -//! them as an extension to the span. If a metric is emitted while a span is entered, we check that -//! span to see if it has any fields in the extension data, and if it does, we add those fields as -//! labels to the metric key. -//! -//! There are two important behaviors to be aware of: -//! - we only capture the fields present when the span is created -//! - we store all fields that a span has, including the fields of its parent span(s) -//! -//! ## Lack of dynamism -//! -//! This means that if you use [`Span::record`][tracing::Span::record] to add fields to a span after -//! it has been created, those fields will not be captured and added to your metric key. +//! The integration layer works by capturing all fields that are present when a span is created, +//! as well as fields recorded after the fact, and storing them as an extension to the span. If +//! a metric is emitted while a span is entered, any fields captured for that span will be added +//! to the metric as additional labels. +//! +//! Be aware that we recursively capture the fields of a span, including fields from +//! parent spans, and use them when generating metric labels. This means that if a metric is being +//! emitted in span B, which is a child of span A, and span A has field X, and span B has field Y, +//! then the metric labels will include both field X and Y. This applies regardless of how many +//! nested spans are currently entered. +//! +//! ## Duplicate span fields +//! +//! When span fields are captured, they are deduplicated such that only the most recent value is kept. +//! For merging parent span fields into the current span fields, the fields from the current span have +//! the highest priority. Additionally, when using [`Span::record`][tracing::Span::record] to add fields +//! to a span after it has been created, the same behavior applies. This means that recording a field +//! multiple times only keeps the most recently recorded value, including if a field was already present +//! from a parent span and is then recorded dynamically in the current span. //! //! ## Span fields and ancestry //! @@ -104,11 +110,10 @@ pub mod label_filter; mod tracing_integration; pub use label_filter::LabelFilter; -use tracing_integration::WithContext; +use tracing_integration::Map; pub use tracing_integration::{Labels, MetricsLayer}; -/// [`TracingContextLayer`] provides an implementation of a [`Layer`][metrics_util::layers::Layer] -/// for [`TracingContext`]. +/// [`TracingContextLayer`] provides an implementation of a [`Layer`] for [`TracingContext`]. pub struct TracingContextLayer { label_filter: F, } @@ -170,34 +175,33 @@ where // doing the iteration would likely exceed the cost of simply constructing the new key. tracing::dispatcher::get_default(|dispatch| { let current = dispatch.current_span(); - if let Some(id) = current.id() { - // We're currently within a live tracing span, so see if we have an available - // metrics context to grab any fields/labels out of. - if let Some(ctx) = dispatch.downcast_ref::() { - let mut f = |new_labels: &[Label]| { - if !new_labels.is_empty() { - let (name, mut labels) = key.clone().into_parts(); - - let filtered_labels = new_labels - .iter() - .filter(|label| { - self.label_filter.should_include_label(&name, label) - }) - .cloned(); - labels.extend(filtered_labels); - - Some(Key::from_parts(name, labels)) - } else { - None - } - }; - - // Pull in the span's fields/labels if they exist. - return ctx.with_labels(dispatch, id, &mut f); - } - } - - None + let id = current.id()?; + let ctx = dispatch.downcast_ref::()?; + + let mut f = |mut span_labels: Map| { + (!span_labels.is_empty()).then(|| { + let (name, labels) = key.clone().into_parts(); + + // Filter only span labels + span_labels.retain(|key: &SharedString, value: &mut SharedString| { + let label = Label::new(key.clone(), value.clone()); + self.label_filter.should_include_label(&name, &label) + }); + + // Overwrites labels from spans + span_labels.extend(labels.into_iter().map(Label::into_parts)); + + let labels = span_labels + .into_iter() + .map(|(key, value)| Label::new(key, value)) + .collect::>(); + + Key::from_parts(name, labels) + }) + }; + + // Pull in the span's fields/labels if they exist. + ctx.with_labels(dispatch, id, &mut f) }) } } diff --git a/metrics-tracing-context/src/tracing_integration.rs b/metrics-tracing-context/src/tracing_integration.rs index 7507b34e..d6e235f4 100644 --- a/metrics-tracing-context/src/tracing_integration.rs +++ b/metrics-tracing-context/src/tracing_integration.rs @@ -1,28 +1,49 @@ //! The code that integrates with the `tracing` crate. +use indexmap::IndexMap; use lockfree_object_pool::{LinearObjectPool, LinearOwnedReusable}; -use metrics::{Key, Label}; +use metrics::{Key, SharedString}; use once_cell::sync::OnceCell; +use std::cmp; use std::sync::Arc; -use std::{any::TypeId, marker::PhantomData}; use tracing_core::span::{Attributes, Id, Record}; use tracing_core::{field::Visit, Dispatch, Field, Subscriber}; use tracing_subscriber::{layer::Context, registry::LookupSpan, Layer}; -fn get_pool() -> &'static Arc>> { - static POOL: OnceCell>>> = OnceCell::new(); - POOL.get_or_init(|| Arc::new(LinearObjectPool::new(Vec::new, Vec::clear))) +pub(crate) type Map = IndexMap; + +fn get_pool() -> &'static Arc> { + static POOL: OnceCell>> = OnceCell::new(); + POOL.get_or_init(|| Arc::new(LinearObjectPool::new(Map::new, Map::clear))) } + /// Span fields mapped as metrics labels. /// /// Hidden from documentation as there is no need for end users to ever touch this type, but it must /// be public in order to be pulled in by external benchmark code. #[doc(hidden)] -pub struct Labels(pub LinearOwnedReusable>); +pub struct Labels(pub LinearOwnedReusable); impl Labels { - pub(crate) fn extend_from_labels(&mut self, other: &Labels) { - self.0.extend_from_slice(other.as_ref()); + fn extend(&mut self, other: &Labels, f: impl Fn(&mut Map, &SharedString, &SharedString)) { + let new_len = cmp::max(self.as_ref().len(), other.as_ref().len()); + let additional = new_len - self.as_ref().len(); + self.0.reserve(additional); + for (k, v) in other.as_ref() { + f(&mut self.0, k, v); + } + } + + fn extend_from_labels(&mut self, other: &Labels) { + self.extend(other, |map, k, v| { + map.entry(k.clone()).or_insert_with(|| v.clone()); + }); + } + + fn extend_from_labels_overwrite(&mut self, other: &Labels) { + self.extend(other, |map, k, v| { + map.insert(k.clone(), v.clone()); + }); } } @@ -34,108 +55,86 @@ impl Default for Labels { impl Visit for Labels { fn record_str(&mut self, field: &Field, value: &str) { - let label = Label::new(field.name(), value.to_string()); - self.0.push(label); + self.0.insert(field.name().into(), value.to_owned().into()); } fn record_bool(&mut self, field: &Field, value: bool) { - let label = Label::from_static_parts(field.name(), if value { "true" } else { "false" }); - self.0.push(label); + self.0.insert(field.name().into(), if value { "true" } else { "false" }.into()); } fn record_i64(&mut self, field: &Field, value: i64) { let mut buf = itoa::Buffer::new(); let s = buf.format(value); - let label = Label::new(field.name(), s.to_string()); - self.0.push(label); + self.0.insert(field.name().into(), s.to_owned().into()); } fn record_u64(&mut self, field: &Field, value: u64) { let mut buf = itoa::Buffer::new(); let s = buf.format(value); - let label = Label::new(field.name(), s.to_string()); - self.0.push(label); + self.0.insert(field.name().into(), s.to_owned().into()); } fn record_debug(&mut self, field: &Field, value: &dyn std::fmt::Debug) { - let value_string = format!("{:?}", value); - let label = Label::new(field.name(), value_string); - self.0.push(label); + self.0.insert(field.name().into(), format!("{value:?}").into()); } } impl Labels { - fn from_attributes(attrs: &Attributes<'_>) -> Labels { + fn from_record(record: &Record) -> Labels { let mut labels = Labels::default(); - let record = Record::new(attrs.values()); record.record(&mut labels); labels } } -impl AsRef<[Label]> for Labels { - fn as_ref(&self) -> &[Label] { +impl AsRef for Labels { + fn as_ref(&self) -> &Map { &self.0 } } -pub struct WithContext { - with_labels: fn(&Dispatch, &Id, f: &mut dyn FnMut(&Labels) -> Option) -> Option, -} - -impl WithContext { - pub fn with_labels( - &self, - dispatch: &Dispatch, - id: &Id, - f: &mut dyn FnMut(&[Label]) -> Option, - ) -> Option { - let mut ff = |labels: &Labels| f(labels.as_ref()); - (self.with_labels)(dispatch, id, &mut ff) - } -} - /// [`MetricsLayer`] is a [`tracing_subscriber::Layer`] that captures the span /// fields and allows them to be later on used as metrics labels. -pub struct MetricsLayer { - ctx: WithContext, - _subscriber: PhantomData, +#[derive(Default)] +pub struct MetricsLayer { + with_labels: + Option Option) -> Option>, } -impl MetricsLayer -where - S: Subscriber + for<'span> LookupSpan<'span>, -{ - /// Create a new `MetricsLayer`. +impl MetricsLayer { + /// Create a new [`MetricsLayer`]. pub fn new() -> Self { - let ctx = WithContext { with_labels: Self::with_labels }; - - Self { ctx, _subscriber: PhantomData } + Self::default() } - fn with_labels( + pub(crate) fn with_labels( + &self, dispatch: &Dispatch, id: &Id, - f: &mut dyn FnMut(&Labels) -> Option, + f: &mut dyn FnMut(Map) -> Option, ) -> Option { - let subscriber = dispatch - .downcast_ref::() - .expect("subscriber should downcast to expected type; this is a bug!"); - let span = subscriber.span(id).expect("registry should have a span for the current ID"); - - let result = - if let Some(labels) = span.extensions().get::() { f(labels) } else { None }; - result + let mut ff = |labels: &Labels| f(labels.0.clone()); + (self.with_labels?)(dispatch, id, &mut ff) } } -impl Layer for MetricsLayer +impl Layer for MetricsLayer where S: Subscriber + for<'a> LookupSpan<'a>, { + fn on_layer(&mut self, _: &mut S) { + self.with_labels = Some(|dispatch, id, f| { + let subscriber = dispatch.downcast_ref::()?; + let span = subscriber.span(id)?; + + let ext = span.extensions(); + f(ext.get::()?) + }); + } + fn on_new_span(&self, attrs: &Attributes<'_>, id: &Id, cx: Context<'_, S>) { let span = cx.span(id).expect("span must already exist!"); - let mut labels = Labels::from_attributes(attrs); + let mut labels = Labels::from_record(&Record::new(attrs.values())); if let Some(parent) = span.parent() { if let Some(parent_labels) = parent.extensions().get::() { @@ -146,20 +145,15 @@ where span.extensions_mut().insert(labels); } - unsafe fn downcast_raw(&self, id: TypeId) -> Option<*const ()> { - match id { - id if id == TypeId::of::() => Some(self as *const _ as *const ()), - id if id == TypeId::of::() => Some(&self.ctx as *const _ as *const ()), - _ => None, - } - } -} + fn on_record(&self, id: &Id, values: &Record<'_>, cx: Context<'_, S>) { + let span = cx.span(id).expect("span must already exist!"); + let labels = Labels::from_record(values); -impl Default for MetricsLayer -where - S: Subscriber + for<'span> LookupSpan<'span>, -{ - fn default() -> Self { - MetricsLayer::new() + let ext = &mut span.extensions_mut(); + if let Some(existing) = ext.get_mut::() { + existing.extend_from_labels_overwrite(&labels); + } else { + ext.insert(labels); + } } } diff --git a/metrics-tracing-context/tests/integration.rs b/metrics-tracing-context/tests/integration.rs index 0229393e..526b0c63 100644 --- a/metrics-tracing-context/tests/integration.rs +++ b/metrics-tracing-context/tests/integration.rs @@ -1,3 +1,4 @@ +use itertools::Itertools; use metrics::{counter, Key, KeyName, Label}; use metrics_tracing_context::{LabelFilter, MetricsLayer, TracingContextLayer}; use metrics_util::debugging::{DebugValue, DebuggingRecorder, Snapshotter}; @@ -8,54 +9,65 @@ use tracing::{span, Level}; use tracing_subscriber::{layer::SubscriberExt, Registry}; static TEST_MUTEX: Mutex<()> = const_mutex(()); -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] = &[ +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] = &[ Label::from_static_parts("user", "ferris"), Label::from_static_parts("user.email", "ferris@rust-lang.org"), ]; -static EMAIL_USER: &'static [Label] = &[ +static USER_EMAIL_ATTEMPT: &[Label] = &[ + Label::from_static_parts("user", "ferris"), Label::from_static_parts("user.email", "ferris@rust-lang.org"), + Label::from_static_parts("attempt", "42"), +]; +static USER_ID: &[Label] = &[Label::from_static_parts("user.id", "42")]; +static EMAIL_USER: &[Label] = &[ Label::from_static_parts("user", "ferris"), + Label::from_static_parts("user.email", "ferris@rust-lang.org"), ]; -static SVC_ENV: &'static [Label] = &[ +static SVC_ENV: &[Label] = &[ Label::from_static_parts("service", "login_service"), Label::from_static_parts("env", "test"), ]; -static SVC_USER_EMAIL: &'static [Label] = &[ - Label::from_static_parts("service", "login_service"), +static SVC_USER_EMAIL: &[Label] = &[ Label::from_static_parts("user", "ferris"), Label::from_static_parts("user.email", "ferris@rust-lang.org"), + Label::from_static_parts("service", "login_service"), ]; -static NODE_USER_EMAIL: &'static [Label] = &[ - Label::from_static_parts("node_name", "localhost"), +static SVC_USER_EMAIL_ID: &[Label] = &[ Label::from_static_parts("user", "ferris"), Label::from_static_parts("user.email", "ferris@rust-lang.org"), -]; -static SVC_NODE_USER_EMAIL: &'static [Label] = &[ + Label::from_static_parts("user.id", "42"), Label::from_static_parts("service", "login_service"), +]; +static NODE_USER_EMAIL: &[Label] = &[ + Label::from_static_parts("user", "ferris"), + Label::from_static_parts("user.email", "ferris@rust-lang.org"), Label::from_static_parts("node_name", "localhost"), +]; +static SVC_NODE_USER_EMAIL: &[Label] = &[ Label::from_static_parts("user", "ferris"), Label::from_static_parts("user.email", "ferris@rust-lang.org"), + Label::from_static_parts("service", "login_service"), + Label::from_static_parts("node_name", "localhost"), ]; -static COMBINED_LABELS: &'static [Label] = &[ +static COMBINED_LABELS: &[Label] = &[ Label::from_static_parts("shared_field", "inner"), Label::from_static_parts("inner_specific", "foo"), Label::from_static_parts("inner_specific_dynamic", "foo_dynamic"), - Label::from_static_parts("shared_field", "outer"), Label::from_static_parts("outer_specific", "bar"), Label::from_static_parts("outer_specific_dynamic", "bar_dynamic"), ]; -static SAME_CALLSITE_PATH_1: &'static [Label] = &[ +static SAME_CALLSITE_PATH_1: &[Label] = &[ Label::from_static_parts("shared_field", "path1"), Label::from_static_parts("path1_specific", "foo"), Label::from_static_parts("path1_specific_dynamic", "foo_dynamic"), ]; -static SAME_CALLSITE_PATH_2: &'static [Label] = &[ +static SAME_CALLSITE_PATH_2: &[Label] = &[ Label::from_static_parts("shared_field", "path2"), Label::from_static_parts("path2_specific", "bar"), Label::from_static_parts("path2_specific_dynamic", "bar_dynamic"), @@ -110,7 +122,200 @@ fn test_basic_functionality() { None, DebugValue::Counter(1), )] - ) + ); +} + +#[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(); + + span.record("user.id", 42); + counter!("login_attempts", "service" => "login_service").increment(1); + + let snapshot = snapshotter.snapshot().into_vec(); + + assert_eq!( + snapshot, + vec![( + CompositeKey::new( + MetricKind::Counter, + Key::from_static_parts(LOGIN_ATTEMPTS, SVC_USER_EMAIL_ID) + ), + None, + None, + DebugValue::Counter(1), + )] + ); +} + +#[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), + )); + + assert_eq!(snapshot, snapshots); + } + span.record("user.id", 42); + { + 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_ID), + ), + None, + None, + DebugValue::Counter(1), + )); + + assert_eq!(snapshot, snapshots); + } +} + +#[test] +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(); + + span.record("user.id", 42); + counter!("login_attempts").increment(1); + + span.record("user.id", 123); + counter!("login_attempts").increment(1); + + let snapshot = snapshotter.snapshot().into_vec(); + + assert_eq!( + snapshot, + vec![ + ( + CompositeKey::new( + MetricKind::Counter, + Key::from_static_parts(LOGIN_ATTEMPTS, USER_ID_42) + ), + None, + None, + DebugValue::Counter(1), + ), + ( + CompositeKey::new( + MetricKind::Counter, + Key::from_static_parts(LOGIN_ATTEMPTS, USER_ID_123) + ), + None, + None, + DebugValue::Counter(1), + ) + ] + ); +} + +#[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(); + + for attempt in 1..=42 { + span.record("attempt", attempt); + } + counter!("login_attempts").increment(1); + + let snapshot = snapshotter.snapshot().into_vec(); + + assert_eq!( + snapshot, + vec![( + CompositeKey::new( + MetricKind::Counter, + Key::from_static_parts(LOGIN_ATTEMPTS, USER_EMAIL_ATTEMPT) + ), + None, + None, + DebugValue::Counter(1), + )] + ); +} + +#[test] +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(); + + span.record("user.id", 666); + counter!("login_attempts", "user.id" => "42").increment(1); + + let snapshot = snapshotter.snapshot().into_vec(); + + assert_eq!( + snapshot, + vec![( + CompositeKey::new( + MetricKind::Counter, + Key::from_static_parts(LOGIN_ATTEMPTS, USER_ID_42) + ), + None, + None, + DebugValue::Counter(1), + )] + ); } #[test] @@ -130,8 +335,11 @@ fn test_macro_forms() { 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()) + counter!( + "login_attempts_static_and_dynamic_labels", + "service" => "login_service", + "node_name" => node_name, + ) .increment(1); let snapshot = snapshotter.snapshot().into_vec(); @@ -176,7 +384,7 @@ fn test_macro_forms() { DebugValue::Counter(1), ), ] - ) + ); } #[test] @@ -198,7 +406,30 @@ fn test_no_labels() { None, DebugValue::Counter(1), )] - ) + ); +} + +#[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(); + + span.record("user.id", 42); + counter!("login_attempts").increment(1); + + let snapshot = snapshotter.snapshot().into_vec(); + + assert_eq!( + snapshot, + vec![( + CompositeKey::new(MetricKind::Counter, Key::from_static_parts(LOGIN_ATTEMPTS, USER_ID)), + None, + None, + DebugValue::Counter(1), + )] + ); } #[test] @@ -262,7 +493,7 @@ fn test_multiple_paths_to_the_same_callsite() { DebugValue::Counter(1), ) ] - ) + ); } #[test] @@ -347,12 +578,12 @@ fn test_label_filtering() { None, DebugValue::Counter(1), )] - ) + ); } #[test] fn test_label_allowlist() { - let (_guard, snapshotter) = setup(TracingContextLayer::only_allow(&["env", "service"])); + let (_guard, snapshotter) = setup(TracingContextLayer::only_allow(["env", "service"])); let user = "ferris"; let email = "ferris@rust-lang.org"; @@ -378,5 +609,187 @@ fn test_label_allowlist() { None, DebugValue::Counter(1), )] - ) + ); +} + +#[test] +fn test_all_permutations() { + let perms = (0..9).map(|_| [false, true]).multi_cartesian_product(); + + for v in perms { + let [metric_has_labels, in_span, span_has_fields, span_field_same_as_metric, span_has_parent, parent_field_same_as_span, span_field_is_empty, record_field, increment_before_recording] = + v[..] + else { + unreachable!("{:?}, {}", v, v.len()); + }; + + test( + metric_has_labels, + in_span, + span_has_fields, + span_field_same_as_metric, + span_has_parent, + parent_field_same_as_span, + span_field_is_empty, + record_field, + increment_before_recording, + ); + } +} + +#[allow(clippy::fn_params_excessive_bools, clippy::too_many_arguments, clippy::too_many_lines)] +fn test( + metric_has_labels: bool, + in_span: bool, + span_has_fields: bool, + span_field_same_as_metric: bool, + span_has_parent: bool, + parent_field_same_as_span: bool, + span_field_is_empty: bool, + record_field: bool, + increment_before_recording: bool, +) { + let (_guard, snapshotter) = setup(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 _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 = in_span.then(|| span.enter()); + + let increment = || { + if metric_has_labels { + counter!("login_attempts", "user.email" => "ferris@rust-lang.org").increment(1); + } else { + counter!("login_attempts").increment(1); + } + }; + + if increment_before_recording { + increment(); + } + + if record_field { + span.record("user.id", 42); + } + + increment(); + + let snapshot = snapshotter.snapshot().into_vec(); + + let mut expected = vec![]; + + if in_span + && span_has_fields + && !span_field_same_as_metric + && record_field + && increment_before_recording + { + expected.push(( + CompositeKey::new( + MetricKind::Counter, + Key::from_parts( + LOGIN_ATTEMPTS, + IntoIterator::into_iter([ + (span_has_parent || !span_field_is_empty).then(|| { + Label::new("user.id", if span_field_is_empty { "999" } else { "666" }) + }), + metric_has_labels.then(|| Label::new("user.email", "ferris@rust-lang.org")), + ]) + .flatten() + .collect::>(), + ), + ), + None, + None, + DebugValue::Counter(1), + )); + } + + let in_span_with_metric_field = + in_span && span_has_fields && span_field_same_as_metric && !span_field_is_empty; + let has_other_labels = !(!span_has_parent + && (!in_span + || (span_field_same_as_metric || !span_has_fields) + || (!record_field && span_field_is_empty))) + && !(span_field_same_as_metric && parent_field_same_as_span) + && !in_span_with_metric_field; + + expected.push(( + CompositeKey::new( + MetricKind::Counter, + Key::from_parts( + LOGIN_ATTEMPTS, + IntoIterator::into_iter([ + (metric_has_labels && !has_other_labels) + .then(|| Label::new("user.email", "ferris@rust-lang.org")), + (!metric_has_labels + && (in_span_with_metric_field + || span_field_same_as_metric + && span_has_parent + && parent_field_same_as_span)) + .then(|| { + if in_span_with_metric_field { + Label::new("user.email", "user@domain.com") + } else { + Label::new("user.email", "changed@domain.com") + } + }), + if in_span && span_has_fields && !span_field_same_as_metric && record_field { + Some(Label::new("user.id", "42")) + } else if in_span + && span_has_fields + && !span_field_same_as_metric + && !span_field_is_empty + && !record_field + { + Some(Label::new("user.id", "666")) + } else if (!in_span || !span_has_fields || span_field_same_as_metric) + && (!span_field_same_as_metric || !parent_field_same_as_span) + && span_has_parent + || span_has_parent + && span_field_is_empty + && !record_field + && !span_field_same_as_metric + { + Some(Label::new("user.id", "999")) + } else { + None + }, + (metric_has_labels && has_other_labels) + .then(|| Label::new("user.email", "ferris@rust-lang.org")), + ]) + .flatten() + .collect::>(), + ), + ), + None, + None, + DebugValue::Counter( + if !increment_before_recording + || in_span && span_has_fields && !span_field_same_as_metric && record_field + { + 1 + } else { + 2 + }, + ), + )); + + assert_eq!(snapshot, expected); } diff --git a/metrics-util/CHANGELOG.md b/metrics-util/CHANGELOG.md index f8817194..a666c911 100644 --- a/metrics-util/CHANGELOG.md +++ b/metrics-util/CHANGELOG.md @@ -9,6 +9,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] - ReleaseDate +### Fixed + +- Fixed the `Debug` implementation for `bucket::Block` which represented both an unsafe and + logically incorrect usage of `crossbeam-epoch.` + ## [0.15.1] - 2023-07-02 ### Added diff --git a/metrics-util/src/bucket.rs b/metrics-util/src/bucket.rs index 355323ad..3174522d 100644 --- a/metrics-util/src/bucket.rs +++ b/metrics-util/src/bucket.rs @@ -1,4 +1,4 @@ -use crossbeam_epoch::{pin as epoch_pin, unprotected, Atomic, Guard, Owned, Shared}; +use crossbeam_epoch::{pin as epoch_pin, Atomic, Guard, Owned, Shared}; use crossbeam_utils::Backoff; use std::{ cell::UnsafeCell, @@ -153,7 +153,8 @@ impl Drop for Block { impl std::fmt::Debug for Block { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - let has_next = unsafe { !self.next.load(Ordering::Acquire, unprotected()).is_null() }; + let guard = &epoch_pin(); + let has_next = !self.next.load(Ordering::Acquire, guard).is_null(); f.debug_struct("Block") .field("type", &std::any::type_name::()) .field("block_size", &BLOCK_SIZE) diff --git a/metrics-util/src/registry/mod.rs b/metrics-util/src/registry/mod.rs index 548cd460..68a0e5f1 100644 --- a/metrics-util/src/registry/mod.rs +++ b/metrics-util/src/registry/mod.rs @@ -32,17 +32,14 @@ type RegistryHashMap = HashMap>; /// ## Using `Registry` as the basis of an exporter /// /// As a reusable building blocking for building exporter implementations, users should look at -/// [`Key`] and [`AtomicStorage`][crate::registry::AtomicStorage] to use for their key and storage, -/// respectively. +/// [`Key`] and [`AtomicStorage`] to use for their key and storage, respectively. /// /// These two implementations provide behavior that is suitable for most exporters, providing /// seamless integration with the existing key type used by the core /// [`Recorder`][metrics::Recorder] trait, as well as atomic storage for metrics. /// -/// In some cases, users may prefer -/// [`GenerationalAtomicStorage`][crate::registry::GenerationalAtomicStorage] when know if a metric -/// has been touched, even if its value has not changed since the last time it was observed, is -/// necessary. +/// In some cases, users may prefer [`GenerationalAtomicStorage`] when know if a metric has been +/// touched, even if its value has not changed since the last time it was observed, is necessary. /// /// ## Performance /// diff --git a/metrics/CHANGELOG.md b/metrics/CHANGELOG.md index d07576f7..bc47838a 100644 --- a/metrics/CHANGELOG.md +++ b/metrics/CHANGELOG.md @@ -8,6 +8,19 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] - ReleaseDate +### Added + +- Support for using `Arc` with `Cow<'a, T>`. + ([#402](https://github.com/metrics-rs/metrics/pull/402)) + + This will primarily allow using `Arc` for metric names and labels, where previously only + `&'static str` or `String` were allowed. There's still work to be done to also support labels in + this regard. + +### Changed + +- Make `Unit` methods return `&'static str` (instead of `&str`) where possible. ([#392](https://github.com/metrics-rs/metrics/pull/393)) + ## [0.21.1] - 2023-07-02 ### Added diff --git a/metrics/RELEASES.md b/metrics/RELEASES.md index 403366d2..cdf7bc12 100644 --- a/metrics/RELEASES.md +++ b/metrics/RELEASES.md @@ -1,4 +1,5 @@ # Releases + Unlike the [CHANGELOG](CHANGELOG.md), this file tracks more complicated changes that required long-form description and would be too verbose for the changelog alone. @@ -6,7 +7,126 @@ long-form description and would be too verbose for the changelog alone. ## [Unreleased] - ReleaseDate -- No notable changes. +### Metric metadata + +Metrics now support a limited set of metadata field, which can be added to provide for context about +the metric in terms of where it originates from as well as its verbosity. + +In the grand vision of the `metrics` crate, where everyone uses the crate to emit metrics and then +users get those metrics in their application for free... the biggest problem is that users had no +way to actually filter out metrics they didn't want. Metric metadata aims to provide a solution to +this problem. + +A new type `Metadata<'a>` has been added to track all of this information, which includes **module +path**, a **target**, and a **level**. These fields map almost directly to +[`tracing`](https://docs.rs/tracing) -- the inspiration for adding this metadata support -- and +provide the ability to: + +- group/filter metrics by where they're defined (module path) +- group/filter metrics by where they're emitted from (target) +- group/filter metrics by their verbosity (level) + +`Metadata<'a>` is passed into the `Recorder` API when registering a metric so that exporters can +capture it and utilize it. + +#### Examples + +As an example, users may wish to filter out metrics defined by a particular crate because they don't +care about them at all. While they might have previously been able to get lucky and simply filter +the metrics by a common prefix, this still allows for changes to the metric names to breaking the +filter configuration. If we could instead filter by module path, where we can simply use the crate +name itself, then we'd catch all metrics for that crate regardless of their name and regardless of +the crate version. + +Similarly, as another example, users may wish to only emit common metrics related to operation of +their application/service in order to consume less resources, pay less money for the ingest/storage +of the metrics, and so on. During an outage, or when debugging an issue, though, they may wish to +increase the verbosity of metrics they emit in order to capture more granular detail. Being able to +filter by level now provides a mechanism to do so. + +#### Usage + +First, it needs to be said that nothing in the core `metrics` crates actually utilizes this +metadata yet. We'll add support in the future to existing layers, such as the +[filter][filter_layer_docs] layer, in order to take advantage of this support. + +With that said, actually setting this metadata is very easy! As a refresher, you'd normally emit +metrics with something like this: + +```rust +metrics::increment_counter!("my_counter"); +``` + +Now, you can specify the additional metadata attributes as fields at the beginning of the macro +call. This applies to all of the "emission" macros for counters, gauges, and histograms: + +```rust +metrics::increment_counter!(target: "myapp", "my_counter"); + +metrics::increment_gauge!(level: metrics::Level::DEBUG, "my_gauge", 42.2); + +metrics::histogram!(target: "myapp", level: metrics::Level::DEBUG, "my_histogram", 180.1); +``` + +These metrics will have the relevant metadata field set, and all of them will get the module path +provided automatically, as well. + +### Macros overhaul + +In this release, we've reworked the macros to both simplify their implementation and to hopefully +provide a more ergonomic experience for users. + +At a high level, we've: + +- removed all the various macros that were tied to specific _operations_ (e.g. `increment_counter!` + for incrementing a counter) and replaced them with one macro per metric type +- removed the standalone registration macros (e.g. `register_counter!`) +- exposed the operations as methods on the metric handles themselves +- switched from using procedural macros to declarative macros + +#### Fewer macros, more ergonomic usage + +Users no longer need to remember the specific macro to use for a given metric operation, such as +`increment_gauge!` or `decrement_gauge!`. Instead, if the user knows they're working with a gauge, +they simply call `gauge!(...)` to get the handle, and chain on a method call to perform the +operation, such as `gauge!(...).increment(42.2)`. + +Additionally, because we've condensed the registration macros into the new, simplified macros, the +same macro is used whether registering the metric to get a handle, or simply performing an operation +on the metric all at once. + +Let's look at a few examples: + +```rust +// This is the old style of registering a metric and then performing an operation on it. +// +// We're working with a counter here. +let counter_handle = metrics::register_counter!("my_counter"); +counter_handle.increment(1); + +metrics::increment_counter!("my_counter"); + +// Now let's use the new, simplified macros instead: +let counter_handle = metrics::counter!("my_counter"); +counter_handle.increment(1); + +metrics::counter!("my_counter").increment(1); +``` + +As you can see, users no longer need to know about as many macros, and their usage is consistent +whether working with a metric handle that is held long-term, or chaining the method call inline with +the macro call. As a benefit, this also means that IDE completion will be better in some situations, +as support for autocompletion of method calls is generally well supported, while macro +autocompletion is effectively nonexistent. + +#### Declarative macros + +As part of this rework, the macros have also been rewritten declaratively. While the macro code +itself is slightly more complicated/verbose, it has a major benefit that the `metrics-macros` crate +was able to be removed. This is one less dependency that has to be compiled, which should hopefully +help with build times, even if only slightly. + +[filter_layer_docs]: https://docs.rs/metrics-util/latest/metrics_util/layers/struct.FilterLayer.html ## [0.21.1] - 2023-07-02 @@ -35,6 +155,7 @@ long-form description and would be too verbose for the changelog alone. ## [0.18.0] - 2022-01-14 ### Switch to metric handles + `metrics` has now switched to "metric handles." This requires a small amount of backstory. #### Evolution of data storage in `metrics` diff --git a/metrics/src/common.rs b/metrics/src/common.rs index fdf9a948..6cabedc0 100644 --- a/metrics/src/common.rs +++ b/metrics/src/common.rs @@ -6,12 +6,15 @@ use crate::cow::Cow; /// An allocation-optimized string. /// -/// We specify `SharedString` to attempt to get the best of both worlds: flexibility to provide a -/// static or dynamic (owned) string, while retaining the performance benefits of being able to -/// take ownership of owned strings and borrows of completely static strings. +/// `SharedString` uses a custom copy-on-write implementation that is optimized for metric keys, +/// providing ergonomic sharing of single instances, or slices, of strings and labels. This +/// copy-on-write implementation is optimized to allow for constant-time construction (using static +/// values), as well as accepting owned values and values shared through [`Arc`](std::sync::Arc). /// -/// `SharedString` can be converted to from either `&'static str` or `String`, with a method, -/// `const_str`, from constructing `SharedString` from `&'static str` in a `const` fashion. +/// End users generally will not need to interact with this type directly, as the top-level macros +/// (`counter!`, etc), as well as the various conversion implementations +/// ([`From`](std::convert::From)), generally allow users to pass whichever variant of a value +/// (static, owned, shared) is best for them. pub type SharedString = Cow<'static, str>; /// Key-specific hashing algorithm. diff --git a/metrics/src/cow.rs b/metrics/src/cow.rs index 0fcb4f96..7087fcb5 100644 --- a/metrics/src/cow.rs +++ b/metrics/src/cow.rs @@ -5,32 +5,178 @@ use std::{ hash::{Hash, Hasher}, marker::PhantomData, mem::ManuallyDrop, + ops::Deref, ptr::{slice_from_raw_parts, NonNull}, + sync::Arc, }; -use crate::label::Label; +#[derive(Clone, Copy)] +enum Kind { + Owned, + Borrowed, + Shared, +} -/// A clone-on-write smart pointer with an optimized memory layout. +/// A clone-on-write smart pointer with an optimized memory layout, based on `beef`. +/// +/// # Strings, strings everywhere +/// +/// In `metrics`, strings are arguably the most common data type used despite the fact that metrics +/// are measuring numerical values. Both the name of a metric, and its labels, are strings: emitting +/// a metric may involve one string, or 10 strings. Many of these strings tend to be used over and +/// over during the life of the process, as well. +/// +/// In order to achieve and maintain a high level of performance, we use a "clone-on-write" smart +/// pointer to handle passing these strings around. Doing so allows us to potentially avoid having +/// to allocate entire copies of a string, instead using a lightweight smart pointer that can live +/// on the stack. +/// +/// # Why not `std::borrow::Cow`? +/// +/// The standard library already provides a clone-on-write smart pointer, `std::borrow::Cow`, which +/// works well in many cases. However, `metrics` strives to provide minimal overhead where possible, +/// and so `std::borrow::Cow` falls down in one particular way: it uses an enum representation which +/// consumes an additional word of storage. +/// +/// As an example, let's look at strings. A string in `std::borrow::Cow` implies that `T` is `str`, +/// and the owned version of `str` is simply `String`. Thus, for `std::borrow::Cow`, the in-memory +/// layout looks like this: +/// +/// ```text +/// Padding +/// | +/// v +/// +--------------+-------------+--------------+--------------+ +/// stdlib Cow::Borrowed: | Enum Tag | Pointer | Length | XXXXXXXX | +/// +--------------+-------------+--------------+--------------+ +/// +--------------+-------------+--------------+--------------+ +/// stdlib Cow::Owned: | Enum Tag | Pointer | Length | Capacity | +/// +--------------+-------------+--------------+--------------+ +/// ``` +/// +/// As you can see, you pay a memory size penalty to be able to wrap an owned string. This +/// additional word adds minimal overhead, but we can easily avoid it with some clever logic around +/// the values of the length and capacity fields. +/// +/// There is an existing crate that does just that: `beef`. Instead of using an enum, it is simply a +/// struct that encodes the discriminant values in the length and capacity fields directly. If we're +/// wrapping a borrowed value, we can infer that the "capacity" will always be zero, as we only need +/// to track the capacity when we're wrapping an owned value, in order to be able to recreate the +/// underlying storage when consuming the smart pointer, or dropping it. Instead of the above +/// layout, `beef` looks like this: +/// +/// ```text +/// +-------------+--------------+----------------+ +/// `beef` Cow (borrowed): | Pointer | Length (N) | Capacity (0) | +/// +-------------+--------------+----------------+ +/// +-------------+--------------+----------------+ +/// `beef` Cow (owned): | Pointer | Length (N) | Capacity (M) | +/// +-------------+--------------+----------------+ +/// ``` +/// +/// # Why not `beef`? +/// +/// Up until this point, it might not be clear why we didn't just use `beef`. In truth, our design +/// is fundamentally based on `beef`. Crucially, however, `beef` did not/still does not support +/// `const` construction for generic slices. Remember how we mentioned labels? The labels of a +/// metric `are `[Label]` under-the-hood, and so without a way to construct them in a `const` +/// fashion, our previous work to allow entirely static keys would not be possible. +/// +/// Thus, we forked `beef` and copied into directly into `metrics` so that we could write a +/// specialized `const` constructor for `[Label]`. +/// +/// This is why we have our own `Cow` bundled into `metrics` directly, which is based on `beef`. In +/// doing so, we can experiment with more interesting optimizations, and, as mentioned above, we can +/// add const methods to support all of the types involved in statically building metrics keys. +/// +/// # What we do that `beef` doesn't do +/// +/// It was already enough to use our own implementation for the specialized `const` capabilities, +/// but we've taken things even further in a key way: support for `Arc`-wrapped values. +/// +/// ## `Arc`-wrapped values +/// +/// For many strings, there is still a desire to share them cheaply even when they are constructed +/// at run-time. Remember, cloning a `Cow` of an owned value means cloning the value itself, so we +/// need another level of indirection to allow the cheap sharing, which is where `Arc` can +/// provide value. +/// +/// Users can construct a `Arc`, where `T` is lined up with the `T` of `metrics::Cow`, and use +/// that as the initial value instead. When `Cow` is cloned, we end up cloning the underlying +/// `Arc` instead, avoiding a new allocation. `Arc` still handles all of the normal logic +/// necessary to know when the wrapped value must be dropped, and how many live references to the +/// value that there are, and so on. +/// +/// We handle this by relying on an invariant of `Vec`: it never allocates more than `isize::MAX` +/// [1]. This lets us derive the following truth table of the valid combinations of length/capacity: +/// +/// ```text +/// Length (N) Capacity (M) +/// +---------------+----------------+ +/// Borrowed (&T): | N | 0 | +/// +---------------+----------------+ +/// Owned (T::ToOwned): | N | M < usize::MAX | +/// +---------------+----------------+ +/// Shared (Arc): | N | usize::MAX | +/// +---------------+----------------+ +/// ``` +/// +/// As we only implement `Cow` for types where their owned variants are either explicitly or +/// implicitly backed by `Vec<_>`, we know that our capacity will never be `usize::MAX`, as it is +/// limited to `isize::MAX`, and thus we can safely encode our "shared" state within the capacity +/// field. +/// +/// # Notes +/// +/// [1] - technically, `Vec` can have a capacity greater than `isize::MAX` when storing +/// zero-sized types, but we don't do that here, so we always enforce that an owned version's +/// capacity cannot be `usize::MAX` when constructing `Cow`. pub struct Cow<'a, T: Cowable + ?Sized + 'a> { - /// Pointer to data. ptr: NonNull, - - /// Pointer metadata: length and capacity. - meta: Metadata, - - /// Lifetime marker. - marker: PhantomData<&'a T>, + metadata: Metadata, + _lifetime: PhantomData<&'a T>, } impl Cow<'_, T> where T: Cowable + ?Sized, { - #[inline] - pub fn owned(val: T::Owned) -> Self { - let (ptr, meta) = T::owned_into_parts(val); + fn from_parts(ptr: NonNull, metadata: Metadata) -> Self { + Self { ptr, metadata, _lifetime: PhantomData } + } + + /// Creates a pointer to an owned value, consuming it. + pub fn from_owned(owned: T::Owned) -> Self { + let (ptr, metadata) = T::owned_into_parts(owned); - Cow { ptr, meta, marker: PhantomData } + // This check is partially to guard against the semantics of `Vec` changing in the + // future, and partially to ensure that we don't somehow implement `Cowable` for a type + // where its owned version is backed by a vector of ZSTs, where the capacity could + // _legitimately_ be `usize::MAX`. + if metadata.capacity() == usize::MAX { + panic!("Invalid capacity of `usize::MAX` for owned value."); + } + + Self::from_parts(ptr, metadata) + } + + /// Creates a pointer to a shared value. + pub fn from_shared(arc: Arc) -> Self { + let (ptr, metadata) = T::shared_into_parts(arc); + Self::from_parts(ptr, metadata) + } + + /// Extracts the owned data. + /// + /// Clones the data if it is not already owned. + pub fn into_owned(self) -> ::Owned { + // We need to ensure that our own `Drop` impl does _not_ run because we're simply + // transferring ownership of the value back to the caller. For borrowed values, this is + // naturally a no-op because there's nothing to drop, but for owned values, like `String` or + // `Arc`, we wouldn't want to double drop. + let cow = ManuallyDrop::new(self); + + T::owned_from_parts(cow.ptr, &cow.metadata) } } @@ -38,71 +184,69 @@ impl<'a, T> Cow<'a, T> where T: Cowable + ?Sized, { - #[inline] - pub fn borrowed(val: &'a T) -> Self { - let (ptr, meta) = T::ref_into_parts(val); + /// Creates a pointer to a borrowed value. + pub fn from_borrowed(borrowed: &'a T) -> Self { + let (ptr, metadata) = T::borrowed_into_parts(borrowed); - Cow { ptr, meta, marker: PhantomData } + Self::from_parts(ptr, metadata) } +} - #[inline] - pub fn into_owned(self) -> T::Owned { - let cow = ManuallyDrop::new(self); - - if cow.is_borrowed() { - unsafe { T::clone_from_parts(cow.ptr, &cow.meta) } - } else { - unsafe { T::owned_from_parts(cow.ptr, &cow.meta) } - } - } +impl<'a, T> Cow<'a, [T]> +where + T: Clone, +{ + pub const fn const_slice(val: &'a [T]) -> Cow<'a, [T]> { + // SAFETY: We can never create a null pointer by casting a reference to a pointer. + let ptr = unsafe { NonNull::new_unchecked(val.as_ptr() as *mut _) }; + let metadata = Metadata::borrowed(val.len()); - #[inline] - pub fn is_borrowed(&self) -> bool { - self.meta.capacity() == 0 + Self { ptr, metadata, _lifetime: PhantomData } } +} - #[inline] - pub fn is_owned(&self) -> bool { - self.meta.capacity() != 0 - } +impl<'a> Cow<'a, str> { + pub const fn const_str(val: &'a str) -> Self { + // SAFETY: We can never create a null pointer by casting a reference to a pointer. + let ptr = unsafe { NonNull::new_unchecked(val.as_ptr() as *mut _) }; + let metadata = Metadata::borrowed(val.len()); - #[inline] - fn borrow(&self) -> &T { - unsafe { &*T::ref_from_parts(self.ptr, &self.meta) } + Self { ptr, metadata, _lifetime: PhantomData } } } -// Implementations of constant functions for creating `Cow` via static strings, static string -// slices, and static label slices. -impl<'a> Cow<'a, str> { - pub const fn const_str(val: &'a str) -> Self { - Cow { - // We are casting *const T to *mut T, however for all borrowed values - // this raw pointer is only ever dereferenced back to &T. - ptr: unsafe { NonNull::new_unchecked(val.as_ptr() as *mut u8) }, - meta: Metadata::from_ref(val.len()), - marker: PhantomData, - } +impl Deref for Cow<'_, T> +where + T: Cowable + ?Sized, +{ + type Target = T; + + fn deref(&self) -> &Self::Target { + let borrowed_ptr = T::borrowed_from_parts(self.ptr, &self.metadata); + + // SAFETY: We only ever hold a pointer to a borrowed value of at least the lifetime of + // `Self`, or an owned value which we have ownership of (albeit indirectly when using + // `Arc`), so our pointer is always valid and live for derefencing. + unsafe { borrowed_ptr.as_ref().unwrap() } } } -impl<'a> Cow<'a, [Cow<'static, str>]> { - pub const fn const_slice(val: &'a [Cow<'static, str>]) -> Self { - Cow { - ptr: unsafe { NonNull::new_unchecked(val.as_ptr() as *mut Cow<'static, str>) }, - meta: Metadata::from_ref(val.len()), - marker: PhantomData, - } +impl Clone for Cow<'_, T> +where + T: Cowable + ?Sized, +{ + fn clone(&self) -> Self { + let (ptr, metadata) = T::clone_from_parts(self.ptr, &self.metadata); + Self { ptr, metadata, _lifetime: PhantomData } } } -impl<'a> Cow<'a, [Label]> { - pub const fn const_slice(val: &'a [Label]) -> Self { - Cow { - ptr: unsafe { NonNull::new_unchecked(val.as_ptr() as *mut Label) }, - meta: Metadata::from_ref(val.len()), - marker: PhantomData, - } +impl Drop for Cow<'_, T> +where + T: Cowable + ?Sized, +{ + fn drop(&mut self) { + T::drop_from_parts(self.ptr, &self.metadata); } } @@ -112,7 +256,7 @@ where { #[inline] fn hash(&self, state: &mut H) { - self.borrow().hash(state) + self.deref().hash(state) } } @@ -123,7 +267,7 @@ where { #[inline] fn default() -> Self { - Cow::borrowed(Default::default()) + Cow::from_borrowed(Default::default()) } } @@ -136,7 +280,7 @@ where { #[inline] fn partial_cmp(&self, other: &Cow<'_, B>) -> Option { - PartialOrd::partial_cmp(self.borrow(), other.borrow()) + PartialOrd::partial_cmp(self.deref(), other.deref()) } } @@ -146,7 +290,7 @@ where { #[inline] fn cmp(&self, other: &Self) -> Ordering { - Ord::cmp(self.borrow(), other.borrow()) + Ord::cmp(self.deref(), other.deref()) } } @@ -156,77 +300,44 @@ where { #[inline] fn from(val: &'a T) -> Self { - Cow::borrowed(val) - } -} - -impl From> for Cow<'_, str> { - #[inline] - fn from(s: std::borrow::Cow<'static, str>) -> Self { - match s { - std::borrow::Cow::Borrowed(bs) => Cow::borrowed(bs), - std::borrow::Cow::Owned(os) => Cow::owned(os), - } - } -} - -impl From for Cow<'_, str> { - #[inline] - fn from(s: String) -> Self { - Cow::owned(s) - } -} - -impl From> for Cow<'_, [Label]> { - #[inline] - fn from(v: Vec