From 5088a6cde4982e61e18abb5e6a6274af3f279a26 Mon Sep 17 00:00:00 2001 From: houseme Date: Sun, 12 Jul 2026 01:46:13 +0800 Subject: [PATCH] perf(obs): trim per-collection-cycle waste in report_metrics (#4748) `report_metrics` runs on every metrics collection cycle and did three things it did not need to, for every metric, every cycle: - interned `metric.name`/`metric.help` through a `Mutex` even when the `Cow` was already `Borrowed(&'static str)` (the common case for statically named metrics); - re-ran `describe_*!` (which re-locks the recorder's metadata map) although the metadata never changes; - allocated a fresh `Vec<(String, String)>`, cloning every label key and value, even though `metric.labels` is already `[(&'static str, Cow<'static, str>)]`. Now: names/help resolve to `&'static str` without touching the intern cache when already borrowed; each metric is described once (tracked in a `HashSet`); and the recorder is fed `&metric.labels` directly, removing the per-cycle label clone. No metric names, help, label keys/values, or emitted values change. Verified by building rustfs-obs and running the report unit tests (the `metrics` macro accepts the borrowed label slice directly). Addresses rustfs/backlog#1185 (P3, report path). Co-authored-by: heihutu --- crates/obs/src/metrics/report.rs | 64 ++++++++++++++++++++------------ 1 file changed, 41 insertions(+), 23 deletions(-) diff --git a/crates/obs/src/metrics/report.rs b/crates/obs/src/metrics/report.rs index 18c076259..b03630272 100644 --- a/crates/obs/src/metrics/report.rs +++ b/crates/obs/src/metrics/report.rs @@ -15,11 +15,14 @@ use crate::metrics::schema::{MetricDescriptor, MetricType}; use metrics::{counter, describe_counter, describe_gauge, describe_histogram, gauge}; use std::borrow::Cow; -use std::collections::HashMap; +use std::collections::{HashMap, HashSet}; use std::sync::{Mutex, OnceLock}; static NAME_CACHE: OnceLock>> = OnceLock::new(); static HELP_CACHE: OnceLock>> = OnceLock::new(); +/// Metric names already handed to `describe_*!`. The metadata never changes, so +/// each name is described once instead of on every collection cycle. +static DESCRIBED: OnceLock>> = OnceLock::new(); fn intern_string(cache: &OnceLock>>, value: &str) -> &'static str { let cache = cache.get_or_init(Default::default); @@ -34,8 +37,36 @@ fn intern_string(cache: &OnceLock>>, value: } } -fn into_static_str(cache: &OnceLock>>, value: &str) -> &'static str { - intern_string(cache, value) +/// Resolve a metric name/help `Cow` to a `&'static str`, skipping the intern +/// lock+leak when it is already borrowed (the common case for statically-named +/// metrics); only owned strings pay the intern cost. +// The `&Cow` is intentional: we must inspect the borrowed-vs-owned variant to +// return the existing `&'static str` without interning, so `&str` will not do. +#[allow(clippy::ptr_arg)] +fn static_str(cache: &OnceLock>>, value: &Cow<'static, str>) -> &'static str { + match value { + Cow::Borrowed(s) => s, + Cow::Owned(s) => intern_string(cache, s), + } +} + +/// Hand a metric name to `describe_*!` only the first time it is seen. The +/// metadata (help text) never changes, so describing on every collection cycle +/// only re-locks the recorder's metadata map. The help is resolved lazily, so an +/// already-described metric costs one `HashSet` probe and nothing else. +// `&Cow` mirrors `static_str`, which needs the borrowed-vs-owned variant. +#[allow(clippy::ptr_arg)] +fn describe_metric_once(name: &'static str, metric_type: MetricType, help: &Cow<'static, str>) { + let described = DESCRIBED.get_or_init(Default::default); + let mut described = described.lock().unwrap_or_else(|poisoned| poisoned.into_inner()); + if described.insert(name) { + let help = static_str(&HELP_CACHE, help); + match metric_type { + MetricType::Counter => describe_counter!(name, help), + MetricType::Gauge => describe_gauge!(name, help), + MetricType::Histogram => describe_histogram!(name, help), + } + } } fn counter_value_from_f64(value: f64) -> Option { @@ -52,32 +83,19 @@ fn counter_value_from_f64(value: f64) -> Option { pub fn report_metrics(metrics: &[PrometheusMetric]) { for metric in metrics { - let name = into_static_str(&NAME_CACHE, &metric.name); - let help = into_static_str(&HELP_CACHE, &metric.help); - - match metric.metric_type { - MetricType::Counter => describe_counter!(name, help), - MetricType::Gauge => describe_gauge!(name, help), - MetricType::Histogram => describe_histogram!(name, help), - } - - let labels: Vec<(String, String)> = metric.labels.iter().map(|(k, v)| (k.to_string(), v.to_string())).collect(); + let name = static_str(&NAME_CACHE, &metric.name); + describe_metric_once(name, metric.metric_type, &metric.help); + // `metric.labels` is already `[(&'static str, Cow<'static, str>)]`, so emit + // straight from it — no per-cycle `Vec<(String, String)>` clone. match metric.metric_type { MetricType::Counter => { if let Some(value) = counter_value_from_f64(metric.value) { - let counter = counter!(name, &labels); - counter.absolute(value); + counter!(name, &metric.labels).absolute(value); } } - MetricType::Gauge => { - let gauge = gauge!(name, &labels); - gauge.set(metric.value); - } - MetricType::Histogram => { - let histogram = metrics::histogram!(name, &labels); - histogram.record(metric.value); - } + MetricType::Gauge => gauge!(name, &metric.labels).set(metric.value), + MetricType::Histogram => metrics::histogram!(name, &metric.labels).record(metric.value), } } }