diff --git a/crates/io-metrics/src/internode_metrics.rs b/crates/io-metrics/src/internode_metrics.rs index 813bb6302..a12a8edfe 100644 --- a/crates/io-metrics/src/internode_metrics.rs +++ b/crates/io-metrics/src/internode_metrics.rs @@ -916,7 +916,7 @@ fn cluster_peer_health_keys() -> Vec { mod tests { use super::*; use metrics::with_local_recorder; - use metrics_util::debugging::DebuggingRecorder; + use metrics_util::debugging::{DebugValue, DebuggingRecorder}; use std::collections::{HashMap, HashSet}; #[test] @@ -1308,11 +1308,47 @@ mod tests { } #[test] - fn msgpack_json_fallback_counter_records_without_panicking() { - // Smoke test: the counter accepts both directions and a static message label. + fn msgpack_json_fallback_counter_separates_the_two_directions() { + // Previously a smoke test that asserted nothing; the counter carries no + // in-struct total, so the emission itself is what has to be checked + // (rustfs/backlog#1836). + let recorder = DebuggingRecorder::new(); + let snapshotter = recorder.snapshotter(); let metrics = InternodeMetrics::default(); - metrics.record_msgpack_json_fallback(INTERNODE_MSGPACK_DIRECTION_REQUEST, "FileInfo"); - metrics.record_msgpack_json_fallback(INTERNODE_MSGPACK_DIRECTION_RESPONSE, "RawFileInfo"); + + metrics::with_local_recorder(&recorder, || { + metrics.record_msgpack_json_fallback(INTERNODE_MSGPACK_DIRECTION_REQUEST, "FileInfo"); + metrics.record_msgpack_json_fallback(INTERNODE_MSGPACK_DIRECTION_RESPONSE, "RawFileInfo"); + }); + + let observed: Vec<(String, String, u64)> = snapshotter + .snapshot() + .into_vec() + .into_iter() + .filter(|(composite, _, _, _)| composite.key().name() == INTERNODE_MSGPACK_JSON_FALLBACK_TOTAL) + .map(|(composite, _, _, value)| { + let labels: HashMap = composite + .key() + .labels() + .map(|label| (label.key().to_string(), label.value().to_string())) + .collect(); + let count = match value { + DebugValue::Counter(count) => count, + other => panic!("fallback total must be a counter, got {other:?}"), + }; + ( + labels.get(DIRECTION_LABEL).cloned().unwrap_or_default(), + labels.get(MESSAGE_LABEL).cloned().unwrap_or_default(), + count, + ) + }) + .collect(); + + // Each direction/message pair is its own series, so a regression that + // dropped a label would collapse these into one row. + assert_eq!(observed.len(), 2, "each direction must land in its own series: {observed:?}"); + assert!(observed.contains(&(INTERNODE_MSGPACK_DIRECTION_REQUEST.to_string(), "FileInfo".to_string(), 1))); + assert!(observed.contains(&(INTERNODE_MSGPACK_DIRECTION_RESPONSE.to_string(), "RawFileInfo".to_string(), 1))); } #[test] diff --git a/crates/io-metrics/src/list_objects_metrics.rs b/crates/io-metrics/src/list_objects_metrics.rs index c5283152e..b5f233557 100644 --- a/crates/io-metrics/src/list_objects_metrics.rs +++ b/crates/io-metrics/src/list_objects_metrics.rs @@ -403,73 +403,132 @@ pub fn record_list_objects_local_read_dir(observation: ListObjectsLocalReadDirOb #[cfg(test)] mod tests { use super::*; + use crate::set_get_stage_metrics_enabled; + use crate::tests::{METRICS_FLAG_LOCK, counter_total, emitted_names, histogram_samples}; + use metrics_util::debugging::DebuggingRecorder; - #[test] - fn record_gather_observation_handles_empty_page() { - init_list_objects_metrics(); - record_list_objects_gather(ListObjectsGatherObservation { - source: LIST_OBJECTS_SOURCE_WALKER, - outcome: LIST_OBJECTS_GATHER_OUTCOME_INPUT_CLOSED, - limit: 1001, - scanned_entries: 42, - returned_entries: 0, - duration_ms: 3.5, - has_prefix: true, - has_delimiter: false, - has_marker: true, + /// Run `body` against a local recorder with stage metrics on, and return the + /// snapshot rows. + /// + /// Enabling the flag is the point: every `record_*` here returns early when + /// `get_stage_metrics_enabled()` is false, which defaults to false. The + /// previous versions of these tests never set it, so they exercised nothing + /// but the early return (rustfs/backlog#1836). + fn recorded(body: impl FnOnce()) -> Vec { + let _guard = METRICS_FLAG_LOCK.lock().unwrap_or_else(|e| e.into_inner()); + let recorder = DebuggingRecorder::new(); + let snapshotter = recorder.snapshotter(); + metrics::with_local_recorder(&recorder, || { + init_list_objects_metrics(); + set_get_stage_metrics_enabled(true); + body(); + set_get_stage_metrics_enabled(false); }); + snapshotter.snapshot().into_vec() } #[test] - fn record_merge_observation_accepts_zero_quorum() { - init_list_objects_metrics(); - record_list_objects_merge(LIST_OBJECTS_SOURCE_WALKER, 4, 0); + fn gather_scan_amplification_falls_back_to_the_scan_count_on_an_empty_page() { + let rows = recorded(|| { + record_list_objects_gather(ListObjectsGatherObservation { + source: LIST_OBJECTS_SOURCE_WALKER, + outcome: LIST_OBJECTS_GATHER_OUTCOME_INPUT_CLOSED, + limit: 1001, + scanned_entries: 42, + returned_entries: 0, + duration_ms: 3.5, + has_prefix: true, + has_delimiter: false, + has_marker: true, + }) + }); + + assert_eq!(counter_total(&rows, LIST_OBJECTS_GATHER_TOTAL), Some(1)); + // Zero returned entries must not divide: the amplification reports the + // scan count itself rather than an infinity or a NaN. + assert_eq!(histogram_samples(&rows, LIST_OBJECTS_GATHER_SCAN_AMPLIFICATION), vec![42.0]); + assert_eq!(histogram_samples(&rows, LIST_OBJECTS_GATHER_FILTERED_ENTRIES), vec![42.0]); + assert_eq!(histogram_samples(&rows, LIST_OBJECTS_GATHER_RETURNED_ENTRIES), vec![0.0]); + assert_eq!(histogram_samples(&rows, LIST_OBJECTS_GATHER_DURATION_MS), vec![3.5]); } #[test] - fn record_index_fallback_observation_accepts_reason() { - init_list_objects_metrics(); - record_list_objects_index_fallback("index_key_only", "unsupported_request"); + fn merge_records_a_zero_read_quorum_rather_than_skipping_it() { + let rows = recorded(|| record_list_objects_merge(LIST_OBJECTS_SOURCE_WALKER, 4, 0)); + + assert_eq!(histogram_samples(&rows, LIST_OBJECTS_MERGE_FAN_IN), vec![4.0]); + assert_eq!(histogram_samples(&rows, LIST_OBJECTS_MERGE_READ_QUORUM), vec![0.0]); } #[test] - fn record_index_attempt_and_served_observations_accept_counts() { - init_list_objects_metrics(); - record_list_objects_index_attempt("index_key_only", "walker_key_only", true, true, false); - record_list_objects_index_served(ListObjectsIndexPageObservation { - source: "index_key_only", - provider: "walker_key_only", - candidate_keys: 1000, - live_verify_attempts: 700, - live_verify_hits: 650, - live_verify_misses: 50, - returned_objects: 600, - returned_prefixes: 10, - is_truncated: true, + fn index_fallback_counts_once_per_reason() { + let rows = recorded(|| { + record_list_objects_index_fallback("index_key_only", "unsupported_request"); + record_list_objects_index_fallback("index_key_only", "unsupported_request"); }); - record_list_objects_index_live_verify_failure("index_key_only", "read_error"); + + assert_eq!(counter_total(&rows, LIST_OBJECTS_INDEX_FALLBACK_TOTAL), Some(2)); } #[test] - fn record_local_read_dir_observation_accepts_whole_directory_counts() { - init_list_objects_metrics(); - record_list_objects_local_read_dir(ListObjectsLocalReadDirObservation { - outcome: LIST_OBJECTS_LOCAL_READ_DIR_OUTCOME_OK, - requested_count: -1, - returned_entries: 4096, - duration_ms: 12.5, - is_root: true, - has_filter_prefix: false, - has_forward: false, + fn index_serving_reports_verification_amplification_against_returned_objects() { + let rows = recorded(|| { + record_list_objects_index_attempt("index_key_only", "walker_key_only", true, true, false); + record_list_objects_index_served(ListObjectsIndexPageObservation { + source: "index_key_only", + provider: "walker_key_only", + candidate_keys: 1000, + live_verify_attempts: 700, + live_verify_hits: 650, + live_verify_misses: 50, + returned_objects: 600, + returned_prefixes: 10, + is_truncated: true, + }); + record_list_objects_index_live_verify_failure("index_key_only", "read_error"); }); - record_list_objects_local_read_dir(ListObjectsLocalReadDirObservation { - outcome: LIST_OBJECTS_LOCAL_READ_DIR_OUTCOME_ERROR, - requested_count: -1, - returned_entries: 0, - duration_ms: 5000.0, - is_root: true, - has_filter_prefix: false, - has_forward: true, + + assert_eq!(counter_total(&rows, LIST_OBJECTS_INDEX_ATTEMPT_TOTAL), Some(1)); + assert_eq!(counter_total(&rows, LIST_OBJECTS_INDEX_SERVED_TOTAL), Some(1)); + assert_eq!(counter_total(&rows, LIST_OBJECTS_INDEX_LIVE_VERIFY_FAILURE_TOTAL), Some(1)); + assert_eq!(histogram_samples(&rows, LIST_OBJECTS_INDEX_CANDIDATE_KEYS), vec![1000.0]); + assert_eq!(histogram_samples(&rows, LIST_OBJECTS_INDEX_LIVE_VERIFY_HITS), vec![650.0]); + assert_eq!(histogram_samples(&rows, LIST_OBJECTS_INDEX_LIVE_VERIFY_MISSES), vec![50.0]); + assert_eq!( + histogram_samples(&rows, LIST_OBJECTS_INDEX_VERIFICATION_IO_AMPLIFICATION), + vec![700.0 / 600.0] + ); + } + + #[test] + fn local_read_dir_passes_the_whole_directory_sentinel_through_as_the_limit() { + let rows = recorded(|| { + record_list_objects_local_read_dir(ListObjectsLocalReadDirObservation { + outcome: LIST_OBJECTS_LOCAL_READ_DIR_OUTCOME_OK, + requested_count: -1, + returned_entries: 4096, + duration_ms: 12.5, + is_root: true, + has_filter_prefix: false, + has_forward: false, + }); + record_list_objects_local_read_dir(ListObjectsLocalReadDirObservation { + outcome: LIST_OBJECTS_LOCAL_READ_DIR_OUTCOME_ERROR, + requested_count: -1, + returned_entries: 0, + duration_ms: 5000.0, + is_root: true, + has_filter_prefix: false, + has_forward: true, + }); }); + + assert_eq!(counter_total(&rows, LIST_OBJECTS_LOCAL_READ_DIR_TOTAL), Some(2)); + // `-1` is the "read the whole directory" sentinel and must reach the + // limit histogram unchanged rather than being clamped to zero. + assert_eq!(histogram_samples(&rows, LIST_OBJECTS_LOCAL_READ_DIR_LIMIT), vec![-1.0, -1.0]); + assert_eq!(histogram_samples(&rows, LIST_OBJECTS_LOCAL_READ_DIR_ENTRIES), vec![0.0, 4096.0]); + assert_eq!(histogram_samples(&rows, LIST_OBJECTS_LOCAL_READ_DIR_DURATION_MS), vec![12.5, 5000.0]); + assert!(emitted_names(&rows).contains(LIST_OBJECTS_LOCAL_READ_DIR_TOTAL)); } } diff --git a/crates/io-metrics/src/sampler/process.rs b/crates/io-metrics/src/sampler/process.rs index d1fde2127..6adff481f 100644 --- a/crates/io-metrics/src/sampler/process.rs +++ b/crates/io-metrics/src/sampler/process.rs @@ -188,18 +188,40 @@ mod tests { } #[test] - fn process_snapshots_are_collectable() { - let _ = snapshot_process_resource(); - let _ = snapshot_process_system(); - let _ = snapshot_process_resource_and_system(); + fn combined_snapshot_agrees_with_the_individual_ones_on_per_process_facts() { + // Previously three discarded calls that asserted nothing. The values that + // move (cpu, memory) cannot be compared across calls, but the facts that + // identify the process must not differ by which entry point produced them + // (rustfs/backlog#1836). + let system = snapshot_process_system(); + let (_, combined_system) = snapshot_process_resource_and_system(); + + assert_eq!( + system.start_time_seconds, combined_system.start_time_seconds, + "both entry points describe this process, so its start time cannot differ" + ); + assert_eq!( + system.file_descriptor_limit_total, combined_system.file_descriptor_limit_total, + "the descriptor limit is a property of the process, not of the call" + ); + assert_eq!( + system.status_value, combined_system.status_value, + "the status enum and its numeric projection must stay in step" + ); + assert_eq!(combined_system.status_value, combined_system.status as i64); } #[test] - fn independent_samplers_are_collectable() { + fn independent_samplers_observe_the_same_process() { let mut sampler_a = ProcessSampler::new(); let mut sampler_b = ProcessSampler::new(); - let _ = snapshot_process_resource_and_system_with(&mut sampler_a); - let _ = snapshot_process_resource_and_system_with(&mut sampler_b); + let (_, system_a) = snapshot_process_resource_and_system_with(&mut sampler_a); + let (_, system_b) = snapshot_process_resource_and_system_with(&mut sampler_b); + + // Two samplers hold separate sysinfo state; they must still agree on the + // process they are both looking at rather than each inventing a value. + assert_eq!(system_a.start_time_seconds, system_b.start_time_seconds); + assert_eq!(system_a.file_descriptor_limit_total, system_b.file_descriptor_limit_total); } }