Bug 1566001 - Add limits for the experiment's extra HashMap

docs/user/experiments-api.md

@@ -39,7 +39,8 @@ assertEquals(
 
 ## Limits
 
-* Fixed maximum experiment and branch id lengths: 30. Longer strings are truncated. For the original Kotlin implementation of the Glean SDK, this is measured in Unicode characters. For the Rust implementation, this is measured in the number of bytes when the string is encoded in UTF-8.
+* `experimentId`, `branch`, and the keys of the 'extra' field are fixed at a maximum length of 30. Length for the values of the `extra` field is fixed at 50. Longer strings used as ids, keys, or values are truncated to their respective maximum length. For the original Kotlin implementation of the Glean SDK, this is measured in Unicode characters. For the Rust implementation, this is measured in the number of bytes when the string is encoded in UTF-8.
+* `extra` map is limited to 20 entries. If passed a map which contains more elements than this, it is truncated to 20 elements.  **WARNING** Which items are truncated is nondeterministic due to the unordered nature of maps and what's left may not necessarily be the first elements added.
 
 ## Reference
 

glean-core/src/lib.rs

@@ -596,6 +596,61 @@ mod test {
         assert_eq!(expected_branch_id, parsed_json.branch);
     }
 
+    #[test]
+    fn limits_on_experiments_extras_are_applied_correctly() {
+        let t = tempfile::tempdir().unwrap();
+        let name = t.path().display().to_string();
+        let glean = Glean::with_options(&name, "org.mozilla.glean.tests", true).unwrap();
+
+        let experiment_id = "test-experiment_id".to_string();
+        let branch_id = "test-branch-id".to_string();
+        let mut extras = HashMap::new();
+
+        let too_long_key = "0123456789".repeat(5);
+        let too_long_value = "0123456789".repeat(6);
+
+        // Build and extras HashMap that's a little too long in every way
+        for n in 0..21 {
+            extras.insert(format!("{}-{}", n, too_long_key), too_long_value.clone());
+        }
+
+        // Mark the experiment as active.
+        glean.set_experiment_active(experiment_id.clone(), branch_id.clone(), Some(extras));
+
+        // Make sure it is active
+        assert!(
+            glean.test_is_experiment_active(experiment_id.clone()),
+            "An experiment with the truncated id should be available"
+        );
+
+        // Get the data
+        let experiment_data = glean.test_get_experiment_data_as_json(experiment_id.clone());
+        assert!(
+            !experiment_data.is_none(),
+            "Experiment data must be available"
+        );
+
+        // Parse the JSON and validate the lengths
+        let parsed_json: RecordedExperimentData =
+            ::serde_json::from_str(&experiment_data.unwrap()).unwrap();
+        assert_eq!(
+            20,
+            parsed_json.clone().extra.unwrap().len(),
+            "Experiments extra must be less than max length"
+        );
+
+        for (key, value) in parsed_json.extra.as_ref().unwrap().iter() {
+            assert!(
+                key.len() <= 30,
+                "Experiments extra key must be less than max length"
+            );
+            assert!(
+                value.len() <= 50,
+                "Experiments extra value must be less than max length"
+            );
+        }
+    }
+
     #[test]
     fn experiments_status_is_correctly_toggled() {
         let t = tempfile::tempdir().unwrap();

glean-core/src/metrics/experiment.rs

@@ -18,9 +18,16 @@ use crate::Lifetime;
 // An internal ping name, not to be touched by anything else
 const INTERNAL_STORAGE: &str = "glean_internal_info";
 
-/// The maximum length of the experiment id and the branch id. Identifiers
-/// longer than this number of characters are truncated.
+/// The maximum length of the experiment id, the branch id, and the keys of the
+/// `extra` map. Identifiers longer than this number of characters are truncated.
 const MAX_EXPERIMENTS_IDS_LEN: usize = 30;
+/// The maximum length of the experiment `extra` values.  Values longer than this
+/// limit will be truncated.
+const MAX_EXPERIMENT_VALUE_LEN: usize = 50;
+/// The maximum number of extras allowed in the `extra` hash map.  Any items added
+/// beyond this limit will be dropped. Note that truncation of a hash map is
+/// nondeterministic in which items are truncated.
+const MAX_EXPERIMENTS_EXTRAS_SIZE: usize = 20;
 
 /// The data for a single experiment.
 #[derive(Debug, Clone, Deserialize, Serialize)]
@@ -112,12 +119,47 @@ impl ExperimentMetric {
             branch
         };
 
-        // TODO (bug 1566001): add sane limits for the extra keys and
-        // values as well.
+        // Apply limits to extras
+        let truncated_extras = extra.and_then(|extra| {
+            if extra.len() > MAX_EXPERIMENTS_EXTRAS_SIZE {
+                log::warn!(
+                    "Extra hash map length {} exceeds maximum of {}",
+                    extra.len(),
+                    MAX_EXPERIMENTS_EXTRAS_SIZE
+                );
+            }
+
+            let mut temp_map = HashMap::new();
+            for (key, value) in extra.into_iter().take(MAX_EXPERIMENTS_EXTRAS_SIZE) {
+                let truncated_key = if key.len() > MAX_EXPERIMENTS_IDS_LEN {
+                    log::warn!(
+                        "Extra key length {} exceeds maximum of {}",
+                        key.len(),
+                        MAX_EXPERIMENTS_IDS_LEN
+                    );
+                    truncate_string_at_boundary(key, MAX_EXPERIMENTS_IDS_LEN)
+                } else {
+                    key
+                };
+                let truncated_value = if value.len() > MAX_EXPERIMENT_VALUE_LEN {
+                    log::warn!(
+                        "Extra value length {} exceeds maximum of {}",
+                        value.len(),
+                        MAX_EXPERIMENT_VALUE_LEN
+                    );
+                    truncate_string_at_boundary(value, MAX_EXPERIMENT_VALUE_LEN)
+                } else {
+                    value
+                };
+
+                temp_map.insert(truncated_key, truncated_value);
+            }
+            Some(temp_map)
+        });
 
         let value = Metric::Experiment(RecordedExperimentData {
             branch: truncated_branch,
-            extra,
+            extra: truncated_extras,
         });
         glean.storage().record(&self.meta, &value)
     }