Backport the InvalidOverflow error

docs/user/collected-metrics/metrics.md

@@ -20,6 +20,7 @@ The following metrics are added to the ping:
 | Name | Type | Description | Data reviews | Extras | Expiration |
 | --- | --- | --- | --- | --- | --- |
 | glean.error.invalid_label |[labeled_counter](https://mozilla.github.io/glean/book/user/metrics/labeled_counters.html) |Counts the number of times a metric was set with an invalid label. The labels are the `category.name` identifier of the metric. |[1](https://bugzilla.mozilla.org/show_bug.cgi?id=1499761#c5)||never |
+| glean.error.invalid_overflow |[labeled_counter](https://mozilla.github.io/glean/book/user/metrics/labeled_counters.html) |Counts the number of times a metric was set a value that overflew. The labels are the `category.name` identifier of the metric. |[1](https://bugzilla.mozilla.org/show_bug.cgi?id=1591912#c3)||never |
 | glean.error.invalid_state |[labeled_counter](https://mozilla.github.io/glean/book/user/metrics/labeled_counters.html) |Counts the number of times a timing metric was used incorrectly. The labels are the `category.name` identifier of the metric. |[1](https://bugzilla.mozilla.org/show_bug.cgi?id=1499761#c5)||never |
 | glean.error.invalid_value |[labeled_counter](https://mozilla.github.io/glean/book/user/metrics/labeled_counters.html) |Counts the number of times a metric was set to an invalid value. The labels are the `category.name` identifier of the metric. |[1](https://bugzilla.mozilla.org/show_bug.cgi?id=1499761#c5)||never |
 

docs/user/error-reporting.md

@@ -7,8 +7,10 @@ Additionally, error metrics are always sent in the [`metrics` ping](pings/metric
 
 The following categories of errors are recorded:
 
-- `invalid_value`: The metric value was invalid or out-of-range.
+- `invalid_value`: The metric value was invalid.
 - `invalid_label`: The label on a labeled metric was invalid.
+- `invalid_state`: The metric caught an invalid state while recording.
+- `invalid_overflow`: The metric value to be recorded overflows the metric-specific upper range.
 
 For example, if you had a string metric and passed it a string that was too long:
 

docs/user/metrics/timing_distribution.md

@@ -183,7 +183,7 @@ XCTAssertEqual(1, pages.pageLoad.testGetNumRecordedErrors(.invalidValue))
 
 * `invalid_value`: If recording a negative timespan.
 * `invalid_value`: If a non-existing/stopped timer is stopped again.
-* `invalid_value`: If recording a time longer than 10 minutes.
+* `invalid_overflow`: If recording a time longer than 10 minutes.
 
 ## Reference
 

glean-core/android/src/main/java/mozilla/telemetry/glean/testing/ErrorType.kt

@@ -18,5 +18,10 @@ enum class ErrorType {
     /**
      * For when timings are recorded incorrectly
      */
-    InvalidState
+    InvalidState,
+
+    /**
+     * For when the value to be recorded overflows the metric-specific upper range
+     */
+    InvalidOverflow
 }

glean-core/ios/Glean/Testing/ErrorType.swift

@@ -14,4 +14,7 @@ public enum ErrorType: Int32 {
 
     // For when timings are recorded incorrectly
     case invalidState = 2
+
+    // For when the value to be recorded overflows the metric-specific upper range
+    case invalidOverflow = 3
 }

glean-core/metrics.yaml

@@ -244,3 +244,20 @@ glean.error:
       - all_pings
     no_lint:
       - COMMON_PREFIX
+
+  invalid_overflow:
+    type: labeled_counter
+    description:
+      Counts the number of times a metric was set a value that overflew.
+      The labels are the `category.name` identifier of the metric.
+    bugs:
+      - https://bugzilla.mozilla.org/show_bug.cgi?id=1591912
+    data_reviews:
+      - https://bugzilla.mozilla.org/show_bug.cgi?id=1591912#c3
+    notification_emails:
+      - [email protected]
+    expires: never
+    send_in_pings:
+      - all_pings
+    no_lint:
+      - COMMON_PREFIX

glean-core/python/glean/testing/error_type.py

@@ -19,3 +19,6 @@ class ErrorType(Enum):
 
     # For when timings are recorded incorrectly
     INVALID_STATE = 2
+
+    # For when the value to be recorded overflows the metric-specific upper range
+    INVALID_OVERFLOW = 3

glean-core/src/error_recording.rs

@@ -23,6 +23,9 @@ use crate::Glean;
 use crate::Lifetime;
 
 /// The possible error types for metric recording.
+/// Note: the cases in this enum must be kept in sync with the ones
+/// in the platform-specific code (e.g. ErrorType.kt) and with the
+/// metrics in the registry files.
 #[derive(Debug)]
 pub enum ErrorType {
     /// For when the value to be recorded does not match the metric-specific restrictions
@@ -31,6 +34,8 @@ pub enum ErrorType {
     InvalidLabel,
     /// For when the metric caught an invalid state while recording
     InvalidState,
+    /// For when the value to be recorded overflows the metric-specific upper range
+    InvalidOverflow,
 }
 
 impl ErrorType {
@@ -40,6 +45,7 @@ impl ErrorType {
             ErrorType::InvalidValue => "invalid_value",
             ErrorType::InvalidLabel => "invalid_label",
             ErrorType::InvalidState => "invalid_state",
+            ErrorType::InvalidOverflow => "invalid_overflow",
         }
     }
 }
@@ -52,6 +58,7 @@ impl TryFrom<i32> for ErrorType {
             0 => Ok(ErrorType::InvalidValue),
             1 => Ok(ErrorType::InvalidLabel),
             2 => Ok(ErrorType::InvalidState),
+            4 => Ok(ErrorType::InvalidOverflow),
             e => Err(ErrorKind::Lifetime(e).into()),
         }
     }

glean-core/src/metrics/timing_distribution.rs

@@ -171,7 +171,7 @@ impl TimingDistributionMetric {
 
         if duration > MAX_SAMPLE_TIME {
             let msg = "Sample is longer than 10 minutes";
-            record_error(glean, &self.meta, ErrorType::InvalidValue, msg, None);
+            record_error(glean, &self.meta, ErrorType::InvalidOverflow, msg, None);
             duration = MAX_SAMPLE_TIME;
         }
 
@@ -226,10 +226,11 @@ impl TimingDistributionMetric {
     /// ## Notes
     ///
     /// Discards any negative value in `samples` and report an `ErrorType::InvalidValue`
-    /// for each of them.
+    /// for each of them. Reports an `ErrorType::InvalidOverflow` error for samples that
+    /// are longer than `MAX_SAMPLE_TIME`.
     pub fn accumulate_samples_signed(&mut self, glean: &Glean, samples: Vec<i64>) {
         let mut num_negative_samples = 0;
-        let mut num_too_log_samples = 0;
+        let mut num_too_long_samples = 0;
 
         glean.storage().record_with(glean, &self.meta, |old_value| {
             let mut hist = match old_value {
@@ -244,7 +245,7 @@ impl TimingDistributionMetric {
                     let sample = sample as u64;
                     let mut sample = self.time_unit.as_nanos(sample);
                     if sample > MAX_SAMPLE_TIME {
-                        num_too_log_samples += 1;
+                        num_too_long_samples += 1;
                         sample = MAX_SAMPLE_TIME;
                     }
 
@@ -265,17 +266,17 @@ impl TimingDistributionMetric {
             );
         }
 
-        if num_too_log_samples > 0 {
+        if num_too_long_samples > 0 {
             let msg = format!(
                 "Accumulated {} samples longer than 10 minutes",
-                num_too_log_samples
+                num_too_long_samples
             );
             record_error(
                 glean,
                 &self.meta,
-                ErrorType::InvalidValue,
+                ErrorType::InvalidOverflow,
                 msg,
-                num_too_log_samples,
+                num_too_long_samples,
             );
         }
     }

glean-core/tests/timing_distribution.rs

@@ -238,6 +238,53 @@ fn the_accumulate_samples_api_correctly_handles_negative_values() {
     );
 }
 
+#[test]
+fn the_accumulate_samples_api_correctly_handles_overflowing_values() {
+    let (glean, _t) = new_glean();
+
+    let mut metric = TimingDistributionMetric::new(
+        CommonMetricData {
+            name: "distribution".into(),
+            category: "telemetry".into(),
+            send_in_pings: vec!["store1".into()],
+            disabled: false,
+            lifetime: Lifetime::Ping,
+            ..Default::default()
+        },
+        TimeUnit::Nanosecond,
+    );
+
+    // The MAX_SAMPLE_TIME is the same from `metrics/timing_distribution.rs`.
+    const MAX_SAMPLE_TIME: u64 = 1000 * 1000 * 1000 * 60 * 10;
+    let overflowing_val = MAX_SAMPLE_TIME as i64 + 1;
+    // Accumulate the samples.
+    metric.accumulate_samples_signed(&glean, [overflowing_val, 1, 2, 3].to_vec());
+
+    let val = metric
+        .test_get_value(&glean, "store1")
+        .expect("Value should be stored");
+
+    // Overflowing values are truncated to MAX_SAMPLE_TIME and recorded.
+    assert_eq!(val.sum(), MAX_SAMPLE_TIME + 6);
+    assert_eq!(val.count(), 4);
+
+    // We should get a sample in each of the first 3 buckets.
+    assert_eq!(1, val.values()[&1]);
+    assert_eq!(1, val.values()[&2]);
+    assert_eq!(1, val.values()[&3]);
+
+    // 1 error should be reported.
+    assert_eq!(
+        Ok(1),
+        test_get_num_recorded_errors(
+            &glean,
+            metric.meta(),
+            ErrorType::InvalidOverflow,
+            Some("store1")
+        )
+    );
+}
+
 #[test]
 fn large_nanoseconds_values() {
     let (glean, _t) = new_glean();