test(io-metrics): assert what the remaining smoke tests only called (#6238)

Eight tests in this crate called a recorder and asserted nothing. Five of them were worse than that: every `record_*` in `list_objects_metrics` returns early unless `get_stage_metrics_enabled()` is true, and that flag defaults to false, so those tests only ever exercised the early return — never the code their names describe.

They now run against a local `DebuggingRecorder` with the flag on, and each asserts the boundary it is named for: an empty page reports the scan count as its amplification instead of dividing by zero, a zero read quorum is recorded rather than skipped, index serving divides verification attempts by returned objects, and the `-1` whole-directory sentinel reaches the limit histogram unclamped.

`msgpack_json_fallback_counter_records_without_panicking` has no in-struct total to check, so it now asserts the emission: two direction/message pairs must land in two separate series, which a dropped label would collapse into one.

The two process-sampler tests discarded their snapshots. They now assert what cannot differ between callers — a process has one start time and one descriptor limit regardless of which entry point or which sampler observed it, and the status enum must match its numeric projection.

This clears io-metrics from the census (`scripts/find_assertless_tests.py`), taking the tree from 61 candidates to 53.

Refs backlog#1836

crates/io-metrics/src/internode_metrics.rs

@@ -916,7 +916,7 @@ fn cluster_peer_health_keys() -> Vec<String> {
 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<String, String> = 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]

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<crate::tests::MetricRow> {
+        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));
     }
 }

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);
     }
 }