refactor: [#1403] extract Measurement strcut

To remove duplicate data. LabelSet is the HashMap key and it was also
included in the HashMap value.

Author: josecelano

packages/metrics/src/metric/mod.rs

@@ -7,9 +7,9 @@ use torrust_tracker_primitives::DurationSinceUnixEpoch;
 use super::counter::Counter;
 use super::label::LabelSet;
 use super::prometheus::PrometheusSerializable;
-use super::sample::Sample;
 use super::sample_collection::SampleCollection;
 use crate::gauge::Gauge;
+use crate::sample::Measurement;
 
 pub type MetricName = name::MetricName;
 
@@ -36,7 +36,7 @@ impl<T> Metric<T> {
     }
 
     #[must_use]
-    pub fn get_sample(&self, label_set: &LabelSet) -> Option<&Sample<T>> {
+    pub fn get_sample_data(&self, label_set: &LabelSet) -> Option<&Measurement<T>> {
         self.sample_collection.get(label_set)
     }
 
@@ -68,11 +68,11 @@ impl<T: PrometheusSerializable> PrometheusSerializable for Metric<T> {
         let samples: Vec<String> = self
             .sample_collection
             .iter()
-            .map(|(_label_set, sample)| {
+            .map(|(label_set, sample)| {
                 format!(
                     "{}{} {}",
                     self.name.to_prometheus(),
-                    sample.labels().to_prometheus(),
+                    label_set.to_prometheus(),
                     sample.value().to_prometheus()
                 )
             })
@@ -87,6 +87,7 @@ mod tests {
         use super::super::*;
         use crate::gauge::Gauge;
         use crate::label::{LabelName, LabelValue};
+        use crate::sample::Sample;
 
         #[test]
         fn it_should_be_empty_when_it_does_not_have_any_sample() {
@@ -132,6 +133,7 @@ mod tests {
         use super::super::*;
         use crate::counter::Counter;
         use crate::label::{LabelName, LabelValue};
+        use crate::sample::Sample;
 
         #[test]
         fn it_should_be_created_from_its_name_and_a_collection_of_samples() {
@@ -154,7 +156,7 @@ mod tests {
 
             let metric = Metric::<Counter>::new(name.clone(), samples);
 
-            assert_eq!(metric.get_sample(&label_set).unwrap().value().value(), 1);
+            assert_eq!(metric.get_sample_data(&label_set).unwrap().value().value(), 1);
         }
     }
 
@@ -164,6 +166,7 @@ mod tests {
         use super::super::*;
         use crate::gauge::Gauge;
         use crate::label::{LabelName, LabelValue};
+        use crate::sample::Sample;
 
         #[test]
         fn it_should_be_created_from_its_name_and_a_collection_of_samples() {
@@ -186,7 +189,7 @@ mod tests {
 
             let metric = Metric::<Gauge>::new(name.clone(), samples);
 
-            assert_relative_eq!(metric.get_sample(&label_set).unwrap().value().value(), 1.0);
+            assert_relative_eq!(metric.get_sample_data(&label_set).unwrap().value().value(), 1.0);
         }
     }
 }

packages/metrics/src/metric_collection.rs

@@ -278,7 +278,7 @@ impl MetricKindCollection<Counter> {
     pub fn get_value(&self, name: &MetricName, label_set: &LabelSet) -> Counter {
         self.metrics
             .get(name)
-            .and_then(|metric| metric.get_sample(label_set))
+            .and_then(|metric| metric.get_sample_data(label_set))
             .map_or(Counter::default(), |sample| sample.value().clone())
     }
 }
@@ -303,7 +303,7 @@ impl MetricKindCollection<Gauge> {
     pub fn get_value(&self, name: &MetricName, label_set: &LabelSet) -> Gauge {
         self.metrics
             .get(name)
-            .and_then(|metric| metric.get_sample(label_set))
+            .and_then(|metric| metric.get_sample_data(label_set))
             .map_or(Gauge::default(), |sample| sample.value().clone())
     }
 }

packages/metrics/src/sample.rs

@@ -9,10 +9,8 @@ use super::prometheus::PrometheusSerializable;
 
 #[derive(Debug, Clone, PartialEq, Serialize, Deserialize)]
 pub struct Sample<T> {
-    value: T,
-
-    #[serde(serialize_with = "serialize_duration", deserialize_with = "deserialize_duration")]
-    update_at: DurationSinceUnixEpoch,
+    #[serde(flatten)]
+    measurement: Measurement<T>,
 
     #[serde(rename = "labels")]
     label_set: LabelSet,
@@ -21,17 +19,68 @@ pub struct Sample<T> {
 impl<T> Sample<T> {
     #[must_use]
     pub fn new(value: T, update_at: DurationSinceUnixEpoch, label_set: LabelSet) -> Self {
+        let data = Measurement { value, update_at };
+
         Self {
-            value,
-            update_at,
+            measurement: data,
             label_set,
         }
     }
 
+    #[must_use]
+    pub fn measurement(&self) -> &Measurement<T> {
+        &self.measurement
+    }
+
+    #[must_use]
+    pub fn value(&self) -> &T {
+        &self.measurement.value
+    }
+
+    #[must_use]
+    pub fn update_at(&self) -> DurationSinceUnixEpoch {
+        self.measurement.update_at
+    }
+
     #[must_use]
     pub fn labels(&self) -> &LabelSet {
         &self.label_set
     }
+}
+
+impl<T: PrometheusSerializable> PrometheusSerializable for Sample<T> {
+    fn to_prometheus(&self) -> String {
+        format!("{} {}", self.label_set.to_prometheus(), self.measurement.to_prometheus())
+    }
+}
+
+impl Sample<Counter> {
+    pub fn increment(&mut self, time: DurationSinceUnixEpoch) {
+        self.measurement.increment(time);
+    }
+}
+
+impl Sample<Gauge> {
+    pub fn set(&mut self, value: f64, time: DurationSinceUnixEpoch) {
+        self.measurement.set(value, time);
+    }
+}
+
+#[derive(Debug, Clone, PartialEq, Serialize, Deserialize)]
+pub struct Measurement<T> {
+    /// The value of the sample.
+    value: T,
+
+    /// The time when the sample was last updated.
+    #[serde(serialize_with = "serialize_duration", deserialize_with = "deserialize_duration")]
+    update_at: DurationSinceUnixEpoch,
+}
+
+impl<T> Measurement<T> {
+    #[must_use]
+    pub fn new(value: T, update_at: DurationSinceUnixEpoch) -> Self {
+        Self { value, update_at }
+    }
 
     #[must_use]
     pub fn value(&self) -> &T {
@@ -48,20 +97,26 @@ impl<T> Sample<T> {
     }
 }
 
-impl<T: PrometheusSerializable> PrometheusSerializable for Sample<T> {
+impl<T> From<Sample<T>> for (LabelSet, Measurement<T>) {
+    fn from(sample: Sample<T>) -> Self {
+        (sample.label_set, sample.measurement)
+    }
+}
+
+impl<T: PrometheusSerializable> PrometheusSerializable for Measurement<T> {
     fn to_prometheus(&self) -> String {
-        format!("{} {}", self.label_set.to_prometheus(), self.value.to_prometheus())
+        self.value.to_prometheus()
     }
 }
 
-impl Sample<Counter> {
+impl Measurement<Counter> {
     pub fn increment(&mut self, time: DurationSinceUnixEpoch) {
         self.value.increment(1);
         self.set_update_at(time);
     }
 }
 
-impl Sample<Gauge> {
+impl Measurement<Gauge> {
     pub fn set(&mut self, value: f64, time: DurationSinceUnixEpoch) {
         self.value.set(value);
         self.set_update_at(time);
@@ -260,17 +315,13 @@ mod tests {
 
         #[test]
         fn test_serialization_round_trip() {
-            let original = Sample {
-                value: 42,
-                update_at: updated_at_time(),
-                label_set: LabelSet::from(vec![("test", "serialization")]),
-            };
+            let original = Sample::new(42, updated_at_time(), LabelSet::from(vec![("test", "serialization")]));
 
             let json = serde_json::to_string(&original).unwrap();
             let deserialized: Sample<i32> = serde_json::from_str(&json).unwrap();
 
-            assert_eq!(original.value, deserialized.value);
-            assert_eq!(original.update_at, deserialized.update_at);
+            assert_eq!(original.measurement.value, deserialized.measurement.value);
+            assert_eq!(original.measurement.update_at, deserialized.measurement.update_at);
             assert_eq!(original.label_set, deserialized.label_set);
         }
 

packages/metrics/src/sample_collection.rs

@@ -1,5 +1,6 @@
 use std::collections::hash_map::Iter;
 use std::collections::{HashMap, HashSet};
+use std::fmt::Write as _;
 
 use serde::{de, Deserialize, Deserializer, Serialize, Serializer};
 use torrust_tracker_primitives::DurationSinceUnixEpoch;
@@ -9,28 +10,26 @@ use super::gauge::Gauge;
 use super::label::LabelSet;
 use super::prometheus::PrometheusSerializable;
 use super::sample::Sample;
+use crate::sample::Measurement;
 
 #[derive(Debug, Clone, Default, PartialEq)]
 pub struct SampleCollection<T> {
-    samples: HashMap<LabelSet, Sample<T>>,
+    samples: HashMap<LabelSet, Measurement<T>>,
 }
 
 impl<T> SampleCollection<T> {
-    // IMPORTANT: It should never allow mutation of the samples because it would
-    // break the invariants. If the sample's `LabelSet` is changed, it can
-    // create duplicate `LabelSet`s even if the `LabelSet` in the `HashMap` key
-    // is unique.
-
     /// # Panics
     ///
     /// Panics if there are duplicate `LabelSets` in the provided samples.
     #[must_use]
     pub fn new(samples: Vec<Sample<T>>) -> Self {
-        let mut map = HashMap::with_capacity(samples.len());
+        let mut map: HashMap<LabelSet, Measurement<T>> = HashMap::with_capacity(samples.len());
 
         for sample in samples {
+            let (label_set, sample_data): (LabelSet, Measurement<T>) = sample.into();
+
             assert!(
-                map.insert(sample.labels().clone(), sample).is_none(),
+                map.insert(label_set, sample_data).is_none(),
                 "Duplicate LabelSet found in SampleCollection"
             );
         }
@@ -39,7 +38,7 @@ impl<T> SampleCollection<T> {
     }
 
     #[must_use]
-    pub fn get(&self, label: &LabelSet) -> Option<&Sample<T>> {
+    pub fn get(&self, label: &LabelSet) -> Option<&Measurement<T>> {
         self.samples.get(label)
     }
 
@@ -55,7 +54,7 @@ impl<T> SampleCollection<T> {
 
     #[must_use]
     #[allow(clippy::iter_without_into_iter)]
-    pub fn iter(&self) -> Iter<'_, LabelSet, Sample<T>> {
+    pub fn iter(&self) -> Iter<'_, LabelSet, Measurement<T>> {
         self.samples.iter()
     }
 }
@@ -65,7 +64,7 @@ impl SampleCollection<Counter> {
         let sample = self
             .samples
             .entry(label_set.clone())
-            .or_insert_with(|| Sample::new(Counter::default(), time, label_set.clone()));
+            .or_insert_with(|| Measurement::new(Counter::default(), time));
 
         sample.increment(time);
     }
@@ -76,7 +75,7 @@ impl SampleCollection<Gauge> {
         let sample = self
             .samples
             .entry(label_set.clone())
-            .or_insert_with(|| Sample::new(Gauge::default(), time, label_set.clone()));
+            .or_insert_with(|| Measurement::new(Gauge::default(), time));
 
         sample.set(value, time);
     }
@@ -87,7 +86,12 @@ impl<T: Serialize> Serialize for SampleCollection<T> {
     where
         S: Serializer,
     {
-        let samples: Vec<&Sample<T>> = self.samples.values().collect();
+        let mut samples: Vec<Sample<&T>> = vec![];
+
+        for (label_set, sample_data) in &self.samples {
+            samples.push(Sample::new(sample_data.value(), sample_data.update_at(), label_set.clone()));
+        }
+
         samples.serialize(serializer)
     }
 }
@@ -121,8 +125,8 @@ impl<T: PrometheusSerializable> PrometheusSerializable for SampleCollection<T> {
     fn to_prometheus(&self) -> String {
         let mut output = String::new();
 
-        for sample in self.samples.values() {
-            output.push_str(&sample.to_prometheus());
+        for (label_set, sample_data) in &self.samples {
+            let _ = write!(output, "{} {}", label_set.to_prometheus(), sample_data.to_prometheus());
         }
 
         output
@@ -165,7 +169,7 @@ mod tests {
 
         let retrieved = collection.get(&label_set);
 
-        assert_eq!(retrieved.unwrap(), &sample);
+        assert_eq!(retrieved.unwrap(), sample.measurement());
     }
 
     #[test]
@@ -179,10 +183,10 @@ mod tests {
         let collection = SampleCollection::new(vec![sample_1.clone(), sample_2.clone()]);
 
         let retrieved = collection.get(&label_set_1);
-        assert_eq!(retrieved.unwrap(), &sample_1);
+        assert_eq!(retrieved.unwrap(), sample_1.measurement());
 
         let retrieved = collection.get(&label_set_2);
-        assert_eq!(retrieved.unwrap(), &sample_2);
+        assert_eq!(retrieved.unwrap(), sample_2.measurement());
     }
 
     #[test]