Merge pull request #876 from mozilla/expose-more

glean-core/src/event_database/mod.rs

@@ -19,20 +19,35 @@ use crate::CommonMetricData;
 use crate::Glean;
 use crate::Result;
 
-/// Represents the data for a single event.
+/// Represents the recorded data for a single event.
 #[derive(Debug, Clone, Deserialize, Serialize, PartialEq, Eq)]
-pub struct RecordedEventData {
+pub struct RecordedEvent {
+    /// The timestamp of when the event was recorded.
+    ///
+    /// This allows to order events from a single process run.
     pub timestamp: u64,
+
+    /// The event's category.
+    ///
+    /// This is defined by users in the metrics file.
     pub category: String,
+
+    /// The event's name.
+    ///
+    /// This is defined by users in the metrics file.
     pub name: String,
+
+    /// A map of all extra data values.
+    ///
+    /// The set of allowed extra keys is defined by users in the metrics file.
     #[serde(skip_serializing_if = "Option::is_none")]
     pub extra: Option<HashMap<String, String>>,
 }
 
-impl RecordedEventData {
+impl RecordedEvent {
     /// Serialize an event to JSON, adjusting its timestamp relative to a base timestamp
-    pub fn serialize_relative(&self, timestamp_offset: u64) -> JsonValue {
-        json!(&RecordedEventData {
+    fn serialize_relative(&self, timestamp_offset: u64) -> JsonValue {
+        json!(&RecordedEvent {
             timestamp: self.timestamp - timestamp_offset,
             category: self.category.clone(),
             name: self.name.clone(),
@@ -58,7 +73,7 @@ pub struct EventDatabase {
     /// Path to directory of on-disk event files
     pub path: PathBuf,
     /// The in-memory list of events
-    event_stores: RwLock<HashMap<String, Vec<RecordedEventData>>>,
+    event_stores: RwLock<HashMap<String, Vec<RecordedEvent>>>,
     /// A lock to be held when doing operations on the filesystem
     file_lock: RwLock<()>,
 }
@@ -123,7 +138,7 @@ impl EventDatabase {
                     store_name,
                     file.lines()
                         .filter_map(|line| line.ok())
-                        .filter_map(|line| serde_json::from_str::<RecordedEventData>(&line).ok())
+                        .filter_map(|line| serde_json::from_str::<RecordedEvent>(&line).ok())
                         .collect(),
                 );
             }
@@ -171,9 +186,9 @@ impl EventDatabase {
         timestamp: u64,
         extra: Option<HashMap<String, String>>,
     ) {
-        // Create RecordedEventData object, and its JSON form for serialization
+        // Create RecordedEvent object, and its JSON form for serialization
         // on disk.
-        let event = RecordedEventData {
+        let event = RecordedEvent {
             timestamp,
             category: meta.category.to_string(),
             name: meta.name.to_string(),
@@ -319,8 +334,8 @@ impl EventDatabase {
         &'a self,
         meta: &'a CommonMetricData,
         store_name: &str,
-    ) -> Option<Vec<RecordedEventData>> {
-        let value: Vec<RecordedEventData> = self
+    ) -> Option<Vec<RecordedEvent>> {
+        let value: Vec<RecordedEvent> = self
             .event_stores
             .read()
             .unwrap() // safe unwrap, only error case is poisoning
@@ -366,7 +381,7 @@ mod test {
 
     #[test]
     fn stable_serialization() {
-        let event_empty = RecordedEventData {
+        let event_empty = RecordedEvent {
             timestamp: 2,
             category: "cat".to_string(),
             name: "name".to_string(),
@@ -375,7 +390,7 @@ mod test {
 
         let mut data = HashMap::new();
         data.insert("a key".to_string(), "a value".to_string());
-        let event_data = RecordedEventData {
+        let event_data = RecordedEvent {
             timestamp: 2,
             category: "cat".to_string(),
             name: "name".to_string(),
@@ -413,7 +428,7 @@ mod test {
 }
         "#;
 
-        let event_empty = RecordedEventData {
+        let event_empty = RecordedEvent {
             timestamp: 2,
             category: "cat".to_string(),
             name: "name".to_string(),
@@ -422,7 +437,7 @@ mod test {
 
         let mut data = HashMap::new();
         data.insert("a key".to_string(), "a value".to_string());
-        let event_data = RecordedEventData {
+        let event_data = RecordedEvent {
             timestamp: 2,
             category: "cat".to_string(),
             name: "name".to_string(),

glean-core/src/histogram/mod.rs

@@ -84,18 +84,6 @@ pub trait Bucketing {
     fn ranges(&self) -> &[u64];
 }
 
-/// Implement the bucketing algorithm on every object that has that algorithm using dynamic
-/// dispatch.
-impl Bucketing for Box<dyn Bucketing> {
-    fn sample_to_bucket_minimum(&self, sample: u64) -> u64 {
-        (**self).sample_to_bucket_minimum(sample)
-    }
-
-    fn ranges(&self) -> &[u64] {
-        (**self).ranges()
-    }
-}
-
 impl<B: Bucketing> Histogram<B> {
     /// Get the number of buckets in this histogram.
     pub fn bucket_count(&self) -> usize {
@@ -149,15 +137,3 @@ impl<B: Bucketing> Histogram<B> {
         res
     }
 }
-
-impl<B: Bucketing + 'static> Histogram<B> {
-    /// Box the contained bucketing algorithm to allow for dynamic dispatch.
-    pub fn boxed(self) -> Histogram<Box<dyn Bucketing>> {
-        Histogram {
-            values: self.values,
-            count: self.count,
-            sum: self.sum,
-            bucketing: Box::new(self.bucketing),
-        }
-    }
-}

glean-core/src/lib_unit_tests.rs

@@ -696,21 +696,28 @@ fn timing_distribution_truncation() {
             dist.set_stop_and_accumulate(&glean, timer_id, value);
         }
 
-        let hist = dist.test_get_value(&glean, "baseline").unwrap();
-
-        assert_eq!(4, hist.values().len());
-        for &key in hist.values().keys() {
-            assert!(key < max_sample_time * unit.as_nanos(1))
+        let snapshot = dist.test_get_value(&glean, "baseline").unwrap();
+
+        let mut keys = HashSet::new();
+        let mut recorded_values = 0;
+
+        for (&key, &value) in &snapshot.values {
+            // A snapshot potentially includes buckets with a 0 count.
+            // We can ignore them here.
+            if value > 0 {
+                assert!(key < max_sample_time * unit.as_nanos(1));
+                keys.insert(key);
+                recorded_values += 1;
+            }
         }
 
-        let keys = HashSet::<u64>::from_iter(hist.values().keys().cloned());
+        assert_eq!(4, recorded_values);
         assert_eq!(keys, *expected_keys);
 
-        let snapshot = hist.snapshot();
         // The number of samples was originally designed around 1ns to
         // 10minutes, with 8 steps per power of 2, which works out to 316 items.
         // This is to ensure that holds even when the time unit is changed.
-        assert!(snapshot.len() < 316);
+        assert!(snapshot.values.len() < 316);
     }
 }
 
@@ -748,14 +755,11 @@ fn timing_distribution_truncation_accumulate() {
             ],
         );
 
-        let hist = dist.test_get_value(&glean, "baseline").unwrap();
-
-        assert_eq!(4, hist.values().len());
+        let snapshot = dist.test_get_value(&glean, "baseline").unwrap();
 
-        let snapshot = hist.snapshot();
         // The number of samples was originally designed around 1ns to
         // 10minutes, with 8 steps per power of 2, which works out to 316 items.
         // This is to ensure that holds even when the time unit is changed.
-        assert!(snapshot.len() < 316);
+        assert!(snapshot.values.len() < 316);
     }
 }

glean-core/src/metrics/custom_distribution.rs

@@ -2,14 +2,9 @@
 // License, v. 2.0. If a copy of the MPL was not distributed with this
 // file, You can obtain one at https://mozilla.org/MPL/2.0/.
 
-use std::collections::HashMap;
-
-use serde::Serialize;
-
 use crate::error_recording::{record_error, ErrorType};
 use crate::histogram::{Bucketing, Histogram, HistogramType};
-use crate::metrics::Metric;
-use crate::metrics::MetricType;
+use crate::metrics::{DistributionData, Metric, MetricType};
 use crate::storage::StorageManager;
 use crate::CommonMetricData;
 use crate::Glean;
@@ -26,18 +21,11 @@ pub struct CustomDistributionMetric {
     histogram_type: HistogramType,
 }
 
-/// A serializable representation of a snapshotted histogram.
-#[derive(Debug, Serialize)]
-pub struct Snapshot {
-    values: HashMap<u64, u64>,
-    sum: u64,
-}
-
 /// Create a snapshot of the histogram.
 ///
 /// The snapshot can be serialized into the payload format.
-pub(crate) fn snapshot<B: Bucketing>(hist: &Histogram<B>) -> Snapshot {
-    Snapshot {
+pub(crate) fn snapshot<B: Bucketing>(hist: &Histogram<B>) -> DistributionData {
+    DistributionData {
         values: hist.snapshot_values(),
         sum: hist.sum(),
     }
@@ -161,19 +149,15 @@ impl CustomDistributionMetric {
     /// Get the currently stored histogram.
     ///
     /// This doesn't clear the stored value.
-    pub fn test_get_value(
-        &self,
-        glean: &Glean,
-        storage_name: &str,
-    ) -> Option<Histogram<Box<dyn Bucketing>>> {
+    pub fn test_get_value(&self, glean: &Glean, storage_name: &str) -> Option<DistributionData> {
         match StorageManager.snapshot_metric(
             glean.storage(),
             storage_name,
             &self.meta.identifier(glean),
         ) {
             // Boxing the value, in order to return either of the possible buckets
-            Some(Metric::CustomDistributionExponential(hist)) => Some(hist.boxed()),
-            Some(Metric::CustomDistributionLinear(hist)) => Some(hist.boxed()),
+            Some(Metric::CustomDistributionExponential(hist)) => Some(snapshot(&hist)),
+            Some(Metric::CustomDistributionLinear(hist)) => Some(snapshot(&hist)),
             _ => None,
         }
     }
@@ -189,6 +173,6 @@ impl CustomDistributionMetric {
         storage_name: &str,
     ) -> Option<String> {
         self.test_get_value(glean, storage_name)
-            .map(|hist| serde_json::to_string(&snapshot(&hist)).unwrap())
+            .map(|snapshot| serde_json::to_string(&snapshot).unwrap())
     }
 }

glean-core/src/metrics/event.rs

@@ -7,7 +7,7 @@ use std::collections::HashMap;
 use serde_json::{json, Value as JsonValue};
 
 use crate::error_recording::{record_error, ErrorType};
-use crate::event_database::RecordedEventData;
+use crate::event_database::RecordedEvent;
 use crate::metrics::MetricType;
 use crate::util::truncate_string_at_boundary_with_error;
 use crate::CommonMetricData;
@@ -116,11 +116,7 @@ impl EventMetric {
     /// Get the vector of currently stored events for this event metric.
     ///
     /// This doesn't clear the stored value.
-    pub fn test_get_value(
-        &self,
-        glean: &Glean,
-        store_name: &str,
-    ) -> Option<Vec<RecordedEventData>> {
+    pub fn test_get_value(&self, glean: &Glean, store_name: &str) -> Option<Vec<RecordedEvent>> {
         glean.event_storage().test_get_value(&self.meta, store_name)
     }
 

glean-core/src/metrics/memory_distribution.rs

@@ -2,15 +2,10 @@
 // License, v. 2.0. If a copy of the MPL was not distributed with this
 // file, You can obtain one at https://mozilla.org/MPL/2.0/.
 
-use std::collections::HashMap;
-
-use serde::Serialize;
-
 use crate::error_recording::{record_error, ErrorType};
 use crate::histogram::{Functional, Histogram};
 use crate::metrics::memory_unit::MemoryUnit;
-use crate::metrics::Metric;
-use crate::metrics::MetricType;
+use crate::metrics::{DistributionData, Metric, MetricType};
 use crate::storage::StorageManager;
 use crate::CommonMetricData;
 use crate::Glean;
@@ -34,18 +29,11 @@ pub struct MemoryDistributionMetric {
     memory_unit: MemoryUnit,
 }
 
-/// A serializable representation of a snapshotted histogram.
-#[derive(Debug, Serialize)]
-pub struct Snapshot {
-    values: HashMap<u64, u64>,
-    sum: u64,
-}
-
 /// Create a snapshot of the histogram.
 ///
 /// The snapshot can be serialized into the payload format.
-pub(crate) fn snapshot(hist: &Histogram<Functional>) -> Snapshot {
-    Snapshot {
+pub(crate) fn snapshot(hist: &Histogram<Functional>) -> DistributionData {
+    DistributionData {
         // **Caution**: This cannot use `Histogram::snapshot_values` and needs to use the more
         // specialized snapshot function.
         values: hist.snapshot(),
@@ -189,17 +177,13 @@ impl MemoryDistributionMetric {
     /// Get the currently stored value as an integer.
     ///
     /// This doesn't clear the stored value.
-    pub fn test_get_value(
-        &self,
-        glean: &Glean,
-        storage_name: &str,
-    ) -> Option<Histogram<Functional>> {
+    pub fn test_get_value(&self, glean: &Glean, storage_name: &str) -> Option<DistributionData> {
         match StorageManager.snapshot_metric(
             glean.storage(),
             storage_name,
             &self.meta.identifier(glean),
         ) {
-            Some(Metric::MemoryDistribution(hist)) => Some(hist),
+            Some(Metric::MemoryDistribution(hist)) => Some(snapshot(&hist)),
             _ => None,
         }
     }
@@ -215,6 +199,6 @@ impl MemoryDistributionMetric {
         storage_name: &str,
     ) -> Option<String> {
         self.test_get_value(glean, storage_name)
-            .map(|hist| serde_json::to_string(&snapshot(&hist)).unwrap())
+            .map(|snapshot| serde_json::to_string(&snapshot).unwrap())
     }
 }

glean-core/src/metrics/mod.rs

@@ -4,6 +4,8 @@
 
 //! The different metric types supported by the Glean SDK to handle data.
 
+use std::collections::HashMap;
+
 use chrono::{DateTime, FixedOffset};
 use serde::{Deserialize, Serialize};
 use serde_json::{json, Value as JsonValue};
@@ -26,6 +28,7 @@ mod timespan;
 mod timing_distribution;
 mod uuid;
 
+pub use crate::event_database::RecordedEvent;
 use crate::histogram::{Functional, Histogram, PrecomputedExponential, PrecomputedLinear};
 use crate::util::get_iso_time_string;
 use crate::CommonMetricData;
@@ -57,6 +60,18 @@ pub use self::timing_distribution::TimerId;
 pub use self::timing_distribution::TimingDistributionMetric;
 pub use self::uuid::UuidMetric;
 
+/// A snapshot of all buckets and the accumulated sum of a distribution.
+#[derive(Debug, Serialize)]
+pub struct DistributionData {
+    /// A map containig the bucket index mapped to the accumulated count.
+    ///
+    /// This can contain buckets with a count of `0`.
+    pub values: HashMap<u64, u64>,
+
+    /// The accumulated sum of all the samples in the distribution.
+    pub sum: u64,
+}
+
 /// The available metrics.
 ///
 /// This is the in-memory and persisted layout of a metric.