From 1eb393cab1042d90a6bc4390386d4a2899ed8fb9 Mon Sep 17 00:00:00 2001 From: houseme Date: Wed, 8 Jul 2026 19:23:23 +0800 Subject: [PATCH] fix(obs): align metrics schema and collector contracts (#4476) fix(obs): align schema and collector contracts Refs rustfs/backlog#1005 - align obs schema descriptors with emitted labels and metric types - fix bucket traffic help text and TTFB bucket descriptor semantics - add regression tests for drive, network host, bucket, and node bucket contracts Co-authored-by: heihutu --- .../src/metrics/collectors/system_drive.rs | 31 +++++++++++++++++-- .../metrics/collectors/system_network_host.rs | 27 +++++++++++----- crates/obs/src/metrics/schema/bucket.rs | 31 ++++++++++++++++--- crates/obs/src/metrics/schema/compression.rs | 10 +++--- crates/obs/src/metrics/schema/node_bucket.rs | 20 +++++++++--- crates/obs/src/metrics/schema/system_drive.rs | 4 +-- .../src/metrics/schema/system_network_host.rs | 31 ++++++++++++++++--- 7 files changed, 123 insertions(+), 31 deletions(-) diff --git a/crates/obs/src/metrics/collectors/system_drive.rs b/crates/obs/src/metrics/collectors/system_drive.rs index 87f7a3af4..521ee3f59 100644 --- a/crates/obs/src/metrics/collectors/system_drive.rs +++ b/crates/obs/src/metrics/collectors/system_drive.rs @@ -102,8 +102,8 @@ pub fn collect_drive_detailed_metrics(stats: &[DriveDetailedStats]) -> Vec Vec = metric.labels.iter().map(|(key, _)| *key).collect(); + let expected: BTreeSet<&str> = expected_keys.iter().copied().collect(); + assert_eq!(actual, expected); + } #[test] fn test_collect_drive_detailed_metrics() { @@ -277,6 +294,14 @@ mod tests { let total_bytes = metrics.iter().find(|m| m.name == total_bytes_name); assert!(total_bytes.is_some()); assert_eq!(total_bytes.map(|m| m.value), Some(1024.0 * 1024.0 * 1024.0 * 100.0)); + assert_metric_label_keys( + &metrics, + &DRIVE_TOTAL_BYTES_MD, + 1024.0 * 1024.0 * 1024.0 * 100.0, + &[SERVER_LABEL, DRIVE_LABEL], + ); + assert_metric_label_keys(&metrics, &DRIVE_API_LATENCY_MD, 1500.0, &[SERVER_LABEL, DRIVE_LABEL]); + assert_metric_label_keys(&metrics, &DRIVE_CAPACITY_OBSERVATION_STATE_MD, 1.0, &[SERVER_LABEL, DRIVE_LABEL, "state"]); } #[test] diff --git a/crates/obs/src/metrics/collectors/system_network_host.rs b/crates/obs/src/metrics/collectors/system_network_host.rs index 1bdc72d30..9dccd9d20 100644 --- a/crates/obs/src/metrics/collectors/system_network_host.rs +++ b/crates/obs/src/metrics/collectors/system_network_host.rs @@ -15,7 +15,9 @@ //! Network I/O metrics collector for host-wide interface counters. use crate::metrics::report::PrometheusMetric; -use crate::metrics::schema::system_network_host::{HOST_NETWORK_IO_MD, HOST_NETWORK_IO_PER_INTERFACE_MD}; +use crate::metrics::schema::system_network_host::{ + DIRECTION_LABEL, HOST_NETWORK_IO_MD, HOST_NETWORK_IO_PER_INTERFACE_MD, INTERFACE_LABEL, +}; use std::borrow::Cow; /// Network I/O statistics. @@ -45,8 +47,10 @@ pub fn collect_host_network_metrics( let mut received_metric = PrometheusMetric::from_descriptor(&HOST_NETWORK_IO_MD, stats.total_received as f64); let mut transmitted_metric = PrometheusMetric::from_descriptor(&HOST_NETWORK_IO_MD, stats.total_transmitted as f64); - received_metric.labels.push(("direction", Cow::Borrowed("received"))); - transmitted_metric.labels.push(("direction", Cow::Borrowed("transmitted"))); + received_metric.labels.push((DIRECTION_LABEL, Cow::Borrowed("received"))); + transmitted_metric + .labels + .push((DIRECTION_LABEL, Cow::Borrowed("transmitted"))); if let Some(l) = labels { received_metric.labels.extend(l.iter().map(|(k, v)| (*k, v.clone()))); @@ -60,11 +64,13 @@ pub fn collect_host_network_metrics( let mut iface_received = PrometheusMetric::from_descriptor(&HOST_NETWORK_IO_PER_INTERFACE_MD, *received as f64); let mut iface_transmitted = PrometheusMetric::from_descriptor(&HOST_NETWORK_IO_PER_INTERFACE_MD, *transmitted as f64); - iface_received.labels.push(("interface", Cow::Owned(interface.clone()))); - iface_received.labels.push(("direction", Cow::Borrowed("received"))); + iface_received.labels.push((INTERFACE_LABEL, Cow::Owned(interface.clone()))); + iface_received.labels.push((DIRECTION_LABEL, Cow::Borrowed("received"))); - iface_transmitted.labels.push(("interface", Cow::Owned(interface.clone()))); - iface_transmitted.labels.push(("direction", Cow::Borrowed("transmitted"))); + iface_transmitted + .labels + .push((INTERFACE_LABEL, Cow::Owned(interface.clone()))); + iface_transmitted.labels.push((DIRECTION_LABEL, Cow::Borrowed("transmitted"))); if let Some(l) = labels { iface_received.labels.extend(l.iter().map(|(k, v)| (*k, v.clone()))); @@ -81,6 +87,7 @@ pub fn collect_host_network_metrics( #[cfg(test)] mod tests { use super::*; + use std::collections::BTreeSet; #[test] fn host_network_metrics_use_dedicated_network_host_prefix() { @@ -98,5 +105,11 @@ mod tests { .iter() .all(|metric| metric.name.starts_with("rustfs_system_network_host_")) ); + + let total_keys: BTreeSet<&str> = metrics[0].labels.iter().map(|(key, _)| *key).collect(); + assert_eq!(total_keys, BTreeSet::from([DIRECTION_LABEL])); + + let per_interface_keys: BTreeSet<&str> = metrics[2].labels.iter().map(|(key, _)| *key).collect(); + assert_eq!(per_interface_keys, BTreeSet::from([DIRECTION_LABEL, INTERFACE_LABEL])); } } diff --git a/crates/obs/src/metrics/schema/bucket.rs b/crates/obs/src/metrics/schema/bucket.rs index f007712e6..e4af34615 100644 --- a/crates/obs/src/metrics/schema/bucket.rs +++ b/crates/obs/src/metrics/schema/bucket.rs @@ -14,13 +14,13 @@ #![allow(dead_code)] -use crate::{MetricDescriptor, MetricName, new_counter_md, new_gauge_md, new_histogram_md, subsystems}; +use crate::{MetricDescriptor, MetricName, new_counter_md, new_gauge_md, subsystems}; use std::sync::LazyLock; pub static BUCKET_API_TRAFFIC_SENT_BYTES_MD: LazyLock = LazyLock::new(|| { new_counter_md( MetricName::ApiTrafficSentBytes, - "Total number of bytes received for a bucket", + "Total number of bytes sent for a bucket", &["bucket", "type"], subsystems::BUCKET_API, ) @@ -29,7 +29,7 @@ pub static BUCKET_API_TRAFFIC_SENT_BYTES_MD: LazyLock = LazyLo pub static BUCKET_API_TRAFFIC_RECV_BYTES_MD: LazyLock = LazyLock::new(|| { new_counter_md( MetricName::ApiTrafficRecvBytes, - "Total number of bytes sent for a bucket", + "Total number of bytes received for a bucket", &["bucket", "type"], subsystems::BUCKET_API, ) @@ -81,10 +81,31 @@ pub static BUCKET_API_REQUESTS_5XX_ERRORS_MD: LazyLock = LazyL }); pub static BUCKET_API_REQUESTS_TTFB_SECONDS_DISTRIBUTION_MD: LazyLock = LazyLock::new(|| { - new_histogram_md( + new_counter_md( MetricName::ApiRequestsTTFBSecondsDistribution, "Distribution of time to first byte across API calls for a bucket", - &["bucket", "name", "le", "type"], + &["bucket", "name", "type", "le"], subsystems::BUCKET_API, ) }); + +#[cfg(test)] +mod tests { + use super::*; + use crate::MetricType; + + #[test] + fn bucket_traffic_help_matches_direction() { + assert_eq!(BUCKET_API_TRAFFIC_SENT_BYTES_MD.help, "Total number of bytes sent for a bucket"); + assert_eq!(BUCKET_API_TRAFFIC_RECV_BYTES_MD.help, "Total number of bytes received for a bucket"); + } + + #[test] + fn bucket_ttfb_distribution_uses_counter_bucket_contract() { + assert_eq!(BUCKET_API_REQUESTS_TTFB_SECONDS_DISTRIBUTION_MD.metric_type, MetricType::Counter); + assert_eq!( + BUCKET_API_REQUESTS_TTFB_SECONDS_DISTRIBUTION_MD.variable_labels, + vec!["bucket".to_string(), "name".to_string(), "type".to_string(), "le".to_string(),] + ); + } +} diff --git a/crates/obs/src/metrics/schema/compression.rs b/crates/obs/src/metrics/schema/compression.rs index 5e031e0f0..86c76364b 100644 --- a/crates/obs/src/metrics/schema/compression.rs +++ b/crates/obs/src/metrics/schema/compression.rs @@ -19,7 +19,7 @@ pub static COMPRESSION_BYTES_ORIGINAL_TOTAL: LazyLock = LazyLo new_counter_md( MetricName::OriginedBytesTotal, "Total bytes before compression (original)", - &["compression"], + &[], subsystems::COMPRESSION, ) }); @@ -28,7 +28,7 @@ pub static COMPRESSION_BYTES_COMPRESSED_TOTAL: LazyLock = Lazy new_counter_md( MetricName::CompressedBytesTotal, "Total bytes after compression", - &["compression"], + &[], subsystems::COMPRESSION, ) }); @@ -37,7 +37,7 @@ pub static COMPRESSION_BYTES_SAVED_TOTAL: LazyLock = LazyLock: new_counter_md( MetricName::SavedBytesTotal, "Total bytes saved by compression", - &["compression"], + &[], subsystems::COMPRESSION, ) }); @@ -46,7 +46,7 @@ pub static COMPRESSION_RATIO: LazyLock = LazyLock::new(|| { new_gauge_md( MetricName::CompressionRatio, "Compression ratio (0.0 - 1.0)", - &["compression"], + &[], subsystems::COMPRESSION, ) }); @@ -55,7 +55,7 @@ pub static COMPRESSION_OPERATIONS_TOTAL: LazyLock = LazyLock:: new_counter_md( MetricName::CompressionOperationsTotal, "Total number of compression operations performed", - &["compression"], + &[], subsystems::COMPRESSION, ) }); diff --git a/crates/obs/src/metrics/schema/node_bucket.rs b/crates/obs/src/metrics/schema/node_bucket.rs index fa006eb43..f8a36acd3 100644 --- a/crates/obs/src/metrics/schema/node_bucket.rs +++ b/crates/obs/src/metrics/schema/node_bucket.rs @@ -14,7 +14,7 @@ #![allow(dead_code)] -use crate::{MetricDescriptor, MetricName, new_gauge_md, subsystems}; +use crate::{MetricDescriptor, MetricName, MetricSubsystem, new_gauge_md}; use std::sync::LazyLock; const BUCKET_LABEL: &str = "bucket"; @@ -25,7 +25,7 @@ pub static BUCKET_USAGE_BYTES_MD: LazyLock = LazyLock::new(|| MetricName::Custom("usage_bytes".to_string()), "Total bytes used by the bucket", &[BUCKET_LABEL], - subsystems::BUCKET_API, + MetricSubsystem::new("/bucket/usage"), ) }); @@ -35,7 +35,7 @@ pub static BUCKET_OBJECTS_TOTAL_MD: LazyLock = LazyLock::new(| MetricName::Custom("objects_total".to_string()), "Total number of objects in the bucket", &[BUCKET_LABEL], - subsystems::BUCKET_API, + MetricSubsystem::new("/bucket/usage"), ) }); @@ -45,6 +45,18 @@ pub static BUCKET_QUOTA_BYTES_MD: LazyLock = LazyLock::new(|| MetricName::Custom("quota_bytes".to_string()), "Quota limit in bytes for the bucket", &[BUCKET_LABEL], - subsystems::BUCKET_API, + MetricSubsystem::new("/bucket/usage"), ) }); + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn bucket_usage_descriptors_use_bucket_usage_subsystem() { + assert_eq!(BUCKET_USAGE_BYTES_MD.subsystem.path(), "/bucket/usage"); + assert_eq!(BUCKET_OBJECTS_TOTAL_MD.subsystem.path(), "/bucket/usage"); + assert_eq!(BUCKET_QUOTA_BYTES_MD.subsystem.path(), "/bucket/usage"); + } +} diff --git a/crates/obs/src/metrics/schema/system_drive.rs b/crates/obs/src/metrics/schema/system_drive.rs index 2f0ddea33..9fbac5f5e 100644 --- a/crates/obs/src/metrics/schema/system_drive.rs +++ b/crates/obs/src/metrics/schema/system_drive.rs @@ -31,7 +31,7 @@ pub const DRIVE_INDEX_LABEL: &str = "drive_index"; pub const API_LABEL: &str = "api"; /// All drive-related labels -pub const ALL_DRIVE_LABELS: [&str; 4] = [DRIVE_LABEL, POOL_INDEX_LABEL, SET_INDEX_LABEL, DRIVE_INDEX_LABEL]; +pub const ALL_DRIVE_LABELS: [&str; 2] = [SERVER_LABEL, DRIVE_LABEL]; pub static DRIVE_USED_BYTES_MD: LazyLock = LazyLock::new(|| { new_gauge_md( @@ -145,7 +145,7 @@ pub static DRIVE_API_LATENCY_MD: LazyLock = LazyLock::new(|| { new_gauge_md( MetricName::DriveAPILatencyMicros, "Average last minute latency in µs for drive API storage operations", - &[&ALL_DRIVE_LABELS[..], &[API_LABEL]].concat(), + &ALL_DRIVE_LABELS[..], subsystems::SYSTEM_DRIVE, ) }); diff --git a/crates/obs/src/metrics/schema/system_network_host.rs b/crates/obs/src/metrics/schema/system_network_host.rs index cb0881b7b..b7d26572c 100644 --- a/crates/obs/src/metrics/schema/system_network_host.rs +++ b/crates/obs/src/metrics/schema/system_network_host.rs @@ -14,25 +14,46 @@ #![allow(dead_code)] -use crate::{MetricDescriptor, MetricName, new_gauge_md, subsystems}; +use crate::{MetricDescriptor, MetricName, new_counter_md, subsystems}; use std::sync::LazyLock; +pub const DIRECTION_LABEL: &str = "direction"; +pub const INTERFACE_LABEL: &str = "interface"; + /// Host network I/O bytes collected from system network interfaces. pub static HOST_NETWORK_IO_MD: LazyLock = LazyLock::new(|| { - new_gauge_md( + new_counter_md( MetricName::HostNetworkIO, "Network bytes transferred across system network interfaces", - &[], + &[DIRECTION_LABEL], subsystems::SYSTEM_NETWORK_HOST, ) }); /// Host network I/O bytes collected from system network interfaces, grouped per interface. pub static HOST_NETWORK_IO_PER_INTERFACE_MD: LazyLock = LazyLock::new(|| { - new_gauge_md( + new_counter_md( MetricName::HostNetworkIOPerInterface, "Network bytes transferred across system network interfaces (per interface)", - &[], + &[INTERFACE_LABEL, DIRECTION_LABEL], subsystems::SYSTEM_NETWORK_HOST, ) }); + +#[cfg(test)] +mod tests { + use super::*; + use crate::MetricType; + + #[test] + fn host_network_descriptors_export_counter_labels() { + assert_eq!(HOST_NETWORK_IO_MD.metric_type, MetricType::Counter); + assert_eq!(HOST_NETWORK_IO_MD.variable_labels, vec![DIRECTION_LABEL.to_string()]); + + assert_eq!(HOST_NETWORK_IO_PER_INTERFACE_MD.metric_type, MetricType::Counter); + assert_eq!( + HOST_NETWORK_IO_PER_INTERFACE_MD.variable_labels, + vec![INTERFACE_LABEL.to_string(), DIRECTION_LABEL.to_string()] + ); + } +}