From 22688afe3d321fb1e30da0c25afced6213b64ec6 Mon Sep 17 00:00:00 2001 From: zohnannor Date: Fri, 10 Nov 2023 13:23:23 +0400 Subject: [PATCH] Change Labels storage to be an IndexMap --- metrics-tracing-context/Cargo.toml | 1 + metrics-tracing-context/benches/visit.rs | 11 +++-- metrics-tracing-context/src/lib.rs | 25 +++++----- .../src/tracing_integration.rs | 41 ++++++++++------ metrics-tracing-context/tests/integration.rs | 49 +++++++++++++++++-- 5 files changed, 88 insertions(+), 39 deletions(-) diff --git a/metrics-tracing-context/Cargo.toml b/metrics-tracing-context/Cargo.toml index 0a692f7f..8160af84 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 = "2.1" 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 } diff --git a/metrics-tracing-context/benches/visit.rs b/metrics-tracing-context/benches/visit.rs index a3128af9..5f806700 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::{Label, 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()))) +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))) } const BATCH_SIZE: usize = 1000; diff --git a/metrics-tracing-context/src/lib.rs b/metrics-tracing-context/src/lib.rs index a5237acf..7fb9c362 100644 --- a/metrics-tracing-context/src/lib.rs +++ b/metrics-tracing-context/src/lib.rs @@ -65,7 +65,9 @@ //! ## Support for dynamism //! //! If you use [`Span::record`][tracing::Span::record] to add fields to a span after it has been -//! created, those fields will be captured and added to your metric key. +//! created, those fields will be captured and added to your metric key. Multiple records of the +//! same field would overwrite it, leaving the most recent value. This way you can change any of +//! the metrics keys coming from span fields. //! //! ## Span fields and ancestry //! @@ -93,20 +95,17 @@ #![deny(missing_docs)] #![cfg_attr(docsrs, feature(doc_cfg), deny(rustdoc::broken_intra_doc_links))] -use metrics::{ - Counter, Gauge, Histogram, Key, KeyName, Label, Metadata, Recorder, SharedString, Unit, -}; +use metrics::{Counter, Gauge, Histogram, Key, KeyName, Metadata, Recorder, SharedString, Unit}; use metrics_util::layers::Layer; pub mod label_filter; mod tracing_integration; pub use label_filter::LabelFilter; -use tracing_integration::WithContext; pub use tracing_integration::{Labels, MetricsLayer}; +use tracing_integration::{Map, WithContext}; -/// [`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, } @@ -172,22 +171,20 @@ where // 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 mut f = |new_labels: &Map| { + (!new_labels.is_empty()).then(|| { let (name, mut labels) = key.clone().into_parts(); let filtered_labels = new_labels - .iter() + .values() .filter(|label| { self.label_filter.should_include_label(&name, label) }) .cloned(); labels.extend(filtered_labels); - Some(Key::from_parts(name, labels)) - } else { - None - } + Key::from_parts(name, labels) + }) }; // Pull in the span's fields/labels if they exist. diff --git a/metrics-tracing-context/src/tracing_integration.rs b/metrics-tracing-context/src/tracing_integration.rs index 98d32321..ca1a58b2 100644 --- a/metrics-tracing-context/src/tracing_integration.rs +++ b/metrics-tracing-context/src/tracing_integration.rs @@ -1,28 +1,37 @@ //! The code that integrates with the `tracing` crate. +use indexmap::IndexMap; use lockfree_object_pool::{LinearObjectPool, LinearOwnedReusable}; -use metrics::{Key, Label}; +use metrics::{Key, Label, SharedString}; use once_cell::sync::OnceCell; use std::sync::Arc; -use std::{any::TypeId, marker::PhantomData}; +use std::{any::TypeId, cmp, 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()); + 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() { + self.0.insert(k.clone(), v.clone()); + } } } @@ -35,32 +44,32 @@ 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(), label); } 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(), label); } 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(), label); } 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(), label); } fn record_debug(&mut self, field: &Field, value: &dyn std::fmt::Debug) { - let value_string = format!("{:?}", value); + let value_string = format!("{value:?}"); let label = Label::new(field.name(), value_string); - self.0.push(label); + self.0.insert(field.name().into(), label); } } @@ -72,8 +81,8 @@ impl Labels { } } -impl AsRef<[Label]> for Labels { - fn as_ref(&self) -> &[Label] { +impl AsRef for Labels { + fn as_ref(&self) -> &Map { &self.0 } } @@ -87,7 +96,7 @@ impl WithContext { &self, dispatch: &Dispatch, id: &Id, - f: &mut dyn FnMut(&[Label]) -> Option, + f: &mut dyn FnMut(&Map) -> Option, ) -> Option { let mut ff = |labels: &Labels| f(labels.as_ref()); (self.with_labels)(dispatch, id, &mut ff) diff --git a/metrics-tracing-context/tests/integration.rs b/metrics-tracing-context/tests/integration.rs index a89e2171..3cae15be 100644 --- a/metrics-tracing-context/tests/integration.rs +++ b/metrics-tracing-context/tests/integration.rs @@ -18,6 +18,11 @@ static USER_EMAIL: &[Label] = &[ Label::from_static_parts("user", "ferris"), Label::from_static_parts("user.email", "ferris@rust-lang.org"), ]; +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.email", "ferris@rust-lang.org"), @@ -50,10 +55,9 @@ static SVC_NODE_USER_EMAIL: &[Label] = &[ Label::from_static_parts("user.email", "ferris@rust-lang.org"), ]; static COMBINED_LABELS: &[Label] = &[ - Label::from_static_parts("shared_field", "inner"), + Label::from_static_parts("shared_field", "outer"), 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"), ]; @@ -209,8 +213,7 @@ fn test_basic_functionality_then_record() { #[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", "42"), Label::from_static_parts("user.id", "123")]; + static USER_ID_123: &[Label] = &[Label::from_static_parts("user.id", "123")]; let (_guard, snapshotter) = setup(TracingContextLayer::all()); @@ -250,6 +253,42 @@ 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(); + + 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_macro_forms() { let (_guard, snapshotter) = setup(TracingContextLayer::all()); @@ -345,7 +384,7 @@ fn test_no_labels() { fn test_no_labels_record() { let (_guard, snapshotter) = setup(TracingContextLayer::all()); - let span = span!(Level::TRACE, "login", user.id = tracing_core::field::Empty,); + let span = span!(Level::TRACE, "login", user.id = tracing_core::field::Empty); let _guard = span.enter(); span.record("user.id", 42);