Merge pull request #1923 from mozilla/remove-invalid_timezone_offset

CHANGELOG.md

@@ -2,6 +2,9 @@
 
 [Full changelog](https://github.com/mozilla/glean/compare/v42.3.2...main)
 
+* General
+  * Removed `invalid_timezone_offset` metric ([#1923](https://github.com/mozilla/glean/pull/1923))
+
 # v42.3.2 (2021-12-15)
 
 [Full changelog](https://github.com/mozilla/glean/compare/v42.3.1...v42.3.2)

docs/user/user/collected-metrics/metrics.md

@@ -162,7 +162,6 @@ In addition to those built-in metrics, the following metrics are added to the pi
 | glean.database.size |[memory_distribution](https://mozilla.github.io/glean/book/user/metrics/memory_distribution.html) |The size of the database file at startup.  |[Bug 1656589](https://bugzilla.mozilla.org/show_bug.cgi?id=1656589#c7)||never |1 |
 | glean.error.io |[counter](https://mozilla.github.io/glean/book/user/metrics/counter.html) |The number of times we encountered an IO error when writing a pending ping to disk.  |[Bug 1686233](https://bugzilla.mozilla.org/show_bug.cgi?id=1686233#c2)||never |1 |
 | glean.error.preinit_tasks_overflow |[counter](https://mozilla.github.io/glean/book/user/metrics/counter.html) |The number of tasks queued in the pre-initialization buffer. Only sent if the buffer overflows.  |[Bug 1609482](https://bugzilla.mozilla.org/show_bug.cgi?id=1609482#c3)||never |1 |
-| glean.time.invalid_timezone_offset |[counter](https://mozilla.github.io/glean/book/user/metrics/counter.html) |Counts the number of times we encountered an invalid timezone offset when trying to get the current time. A timezone offset is invalid if it is outside [-24h, +24h]. If invalid a UTC offset is used (+0h).  |[Bug 1611770](https://bugzilla.mozilla.org/show_bug.cgi?id=1611770#c9), [Bug 1717402](https://bugzilla.mozilla.org/show_bug.cgi?id=1717402#c4)||2021-12-31 |1 |
 | glean.upload.deleted_pings_after_quota_hit |[counter](https://mozilla.github.io/glean/book/user/metrics/counter.html) |The number of pings deleted after the quota for the size of the pending pings directory or number of files is hit. Since quota is only calculated for the pending pings directory, and deletion request ping live in a different directory, deletion request pings are never deleted.  |[Bug 1601550](https://bugzilla.mozilla.org/show_bug.cgi?id=1601550#c3)||never |1 |
 | glean.upload.discarded_exceeding_pings_size |[memory_distribution](https://mozilla.github.io/glean/book/user/metrics/memory_distribution.html) |The size of pings that exceeded the maximum ping size allowed for upload.  |[Bug 1597761](https://bugzilla.mozilla.org/show_bug.cgi?id=1597761#c10)||never |1 |
 | glean.upload.pending_pings |[counter](https://mozilla.github.io/glean/book/user/metrics/counter.html) |The total number of pending pings at startup. This does not include deletion-request pings.  |[Bug 1665041](https://bugzilla.mozilla.org/show_bug.cgi?id=1665041#c23)||never |1 |

glean-core/metrics.yaml

@@ -640,27 +640,3 @@ glean.validation:
       - glean-team@mozilla.com
     expires:
       never
-
-glean.time:
-  invalid_timezone_offset:
-    type: counter
-    description: |
-      Counts the number of times we encountered an invalid timezone offset
-      when trying to get the current time.
-      A timezone offset is invalid if it is outside [-24h, +24h].
-      If invalid a UTC offset is used (+0h).
-    lifetime: ping
-    send_in_pings:
-      - metrics
-    bugs:
-      - https://bugzilla.mozilla.org/show_bug.cgi?id=1611770
-      - https://bugzilla.mozilla.org/show_bug.cgi?id=1717402#c1
-    data_reviews:
-      - https://bugzilla.mozilla.org/show_bug.cgi?id=1611770#c9
-      - https://bugzilla.mozilla.org/show_bug.cgi?id=1717402#c4
-    data_sensitivity:
-      - technical
-    notification_emails:
-      - jrediger@mozilla.com
-      - glean-team@mozilla.com
-    expires: 2021-12-31

glean-core/src/internal_metrics.rs

@@ -20,14 +20,6 @@ pub struct AdditionalMetrics {
 
     /// A count of the pings submitted, by ping type.
     pub pings_submitted: LabeledMetric<CounterMetric>,
-
-    /// The number of times we encountered an invalid timezone offset
-    /// (outside of [-24, +24] hours).
-    ///
-    /// **Note**: This metric has an expiration date set.
-    /// However because it's statically defined here we can't specify that.
-    /// Needs to be removed after 2021-06-30.
-    pub invalid_timezone_offset: CounterMetric,
 }
 
 impl CoreMetrics {
@@ -101,15 +93,6 @@ impl AdditionalMetrics {
                 }),
                 None,
             ),
-
-            invalid_timezone_offset: CounterMetric::new(CommonMetricData {
-                name: "invalid_timezone_offset".into(),
-                category: "glean.time".into(),
-                send_in_pings: vec!["metrics".into()],
-                lifetime: Lifetime::Ping,
-                disabled: false,
-                dynamic_label: None,
-            }),
         }
     }
 }

glean-core/src/lib.rs

@@ -224,7 +224,7 @@ impl Glean {
             let _scanning_thread = upload_manager.scan_pending_pings_directories();
         }
 
-        let (start_time, start_time_is_corrected) = local_now_with_offset();
+        let start_time = local_now_with_offset();
         let this = Self {
             upload_enabled: cfg.upload_enabled,
             // In the subprocess, we want to avoid accessing the database entirely.
@@ -248,13 +248,6 @@ impl Glean {
             schedule_metrics_pings: false,
         };
 
-        // Can't use `local_now_with_offset_and_record` above, because we needed a valid `Glean` first.
-        if start_time_is_corrected {
-            this.additional_metrics
-                .invalid_timezone_offset
-                .add(&this, 1);
-        }
-
         Ok(this)
     }
 

glean-core/src/lib_unit_tests.rs

@@ -401,7 +401,7 @@ fn correct_order() {
         Counter(0),
         CustomDistributionExponential(Histogram::exponential(1, 500, 10)),
         CustomDistributionLinear(Histogram::linear(1, 500, 10)),
-        Datetime(local_now_with_offset().0, TimeUnit::Second),
+        Datetime(local_now_with_offset(), TimeUnit::Second),
         Experiment(RecordedExperimentData { branch: "branch".into(), extra: None, }),
         Quantity(0),
         String("glean".into()),
@@ -931,20 +931,3 @@ fn test_activity_api() {
     // Check that we set everything we needed for the 'inactuve' status.
     assert!(!glean.is_dirty_flag_set());
 }
-
-/// We explicitly test that NO invalid timezone offset was recorded.
-/// If it _does_ happen and fails on a developer machine or CI, we better know about it.
-#[test]
-fn handles_local_now_gracefully() {
-    let _ = env_logger::builder().is_test(true).try_init();
-
-    let dir = tempfile::tempdir().unwrap();
-    let (glean, _) = new_glean(Some(dir));
-
-    let metric = &glean.additional_metrics.invalid_timezone_offset;
-    assert_eq!(
-        None,
-        metric.test_get_value(&glean, "metrics"),
-        "Timezones should be valid"
-    );
-}

glean-core/src/metrics/datetime.rs

@@ -9,7 +9,7 @@ use crate::metrics::time_unit::TimeUnit;
 use crate::metrics::Metric;
 use crate::metrics::MetricType;
 use crate::storage::StorageManager;
-use crate::util::{get_iso_time_string, local_now_with_offset_and_record};
+use crate::util::{get_iso_time_string, local_now_with_offset};
 use crate::CommonMetricData;
 use crate::Glean;
 
@@ -117,7 +117,7 @@ impl DatetimeMetric {
             return;
         }
 
-        let value = value.unwrap_or_else(|| local_now_with_offset_and_record(glean));
+        let value = value.unwrap_or_else(local_now_with_offset);
         let value = Metric::Datetime(value, self.time_unit);
         glean.storage().record(glean, &self.meta, &value)
     }

glean-core/src/ping/mod.rs

@@ -15,7 +15,7 @@ use crate::common_metric_data::{CommonMetricData, Lifetime};
 use crate::metrics::{CounterMetric, DatetimeMetric, Metric, MetricType, PingType, TimeUnit};
 use crate::storage::StorageManager;
 use crate::upload::HeaderMap;
-use crate::util::{get_iso_time_string, local_now_with_offset_and_record};
+use crate::util::{get_iso_time_string, local_now_with_offset};
 use crate::{
     Glean, Result, DELETION_REQUEST_PINGS_DIRECTORY, INTERNAL_STORAGE, PENDING_PINGS_DIRECTORY,
 };
@@ -112,7 +112,7 @@ impl PingMaker {
         let start_time_data = start_time
             .get_value(glean, INTERNAL_STORAGE)
             .unwrap_or_else(|| glean.start_time());
-        let end_time_data = local_now_with_offset_and_record(glean);
+        let end_time_data = local_now_with_offset();
 
         // Update the start time with the current time.
         start_time.set(glean, Some(end_time_data));

glean-core/src/scheduler.rs

@@ -73,7 +73,7 @@ impl MetricsPingScheduler for GleanMetricsPingScheduler {
 /// **Must** be called before draining the preinit queue.
 /// (We're at the Language Bindings' mercy for that)
 pub fn schedule(glean: &Glean) {
-    let now = local_now_with_offset().0;
+    let now = local_now_with_offset();
 
     let (cancelled_lock, _condvar) = &**TASK_CONDVAR;
     if *cancelled_lock.lock().unwrap() {
@@ -233,7 +233,7 @@ fn start_scheduler(
                     submitter.submit_metrics_ping(&glean, Some(when.reason()), now);
                     when = When::Reschedule;
                 }
-                now = local_now_with_offset().0;
+                now = local_now_with_offset();
             }
         }).expect("Unable to spawn Metrics Ping Scheduler thread.")
 }
@@ -337,7 +337,7 @@ mod test {
             |_, when| assert_eq!(when, When::Reschedule),
         );
 
-        schedule_internal(&glean, submitter, scheduler, local_now_with_offset().0);
+        schedule_internal(&glean, submitter, scheduler, local_now_with_offset());
         assert_eq!(1, submitter_count.swap(0, Ordering::Relaxed));
         assert_eq!(1, scheduler_count.swap(0, Ordering::Relaxed));
     }

glean-core/src/util.rs

@@ -48,8 +48,7 @@ pub fn get_iso_time_string(datetime: DateTime<FixedOffset>, truncate_to: TimeUni
 ///
 /// This converts from the `Local` timezone into its fixed-offset equivalent.
 /// If a timezone outside of [-24h, +24h] is detected it corrects the timezone offset to UTC (+0).
-/// The return value will signal if the timezone offset was corrected.
-pub(crate) fn local_now_with_offset() -> (DateTime<FixedOffset>, bool) {
+pub(crate) fn local_now_with_offset() -> DateTime<FixedOffset> {
     #[cfg(target_os = "windows")]
     {
         // `Local::now` takes the user's timezone offset
@@ -84,29 +83,12 @@ pub(crate) fn local_now_with_offset() -> (DateTime<FixedOffset>, bool) {
             );
             let now: DateTime<Utc> = Utc::now();
             let utc_offset = FixedOffset::east(0);
-            return (now.with_timezone(&utc_offset), true);
+            return now.with_timezone(&utc_offset);
         }
     }
 
     let now: DateTime<Local> = Local::now();
-    (now.with_timezone(now.offset()), false)
-}
-
-/// Get the current date & time with a fixed-offset timezone.
-///
-/// This converts from the `Local` timezone into its fixed-offset equivalent.
-/// If a timezone outside of [-24h, +24h] is detected it corrects the timezone offset to UTC (+0).
-/// The corresponding error counter is incremented in this case.
-pub(crate) fn local_now_with_offset_and_record(glean: &Glean) -> DateTime<FixedOffset> {
-    let (now, is_corrected) = local_now_with_offset();
-    if is_corrected {
-        glean
-            .additional_metrics
-            .invalid_timezone_offset
-            .add(glean, 1);
-    }
-
-    now
+    now.with_timezone(now.offset())
 }
 
 /// Truncates a string, ensuring that it doesn't end in the middle of a codepoint.
@@ -297,11 +279,7 @@ mod test {
     #[test]
     fn local_now_gets_the_time() {
         let now = Local::now();
-        let (fixed_now, is_corrected) = local_now_with_offset();
-
-        // We explicitly test that NO invalid timezone offset was recorded.
-        // If it _does_ happen and fails on a developer machine or CI, we better know about it.
-        assert!(!is_corrected, "Timezone offset should be valid.");
+        let fixed_now = local_now_with_offset();
 
         // We can't compare across differing timezones, so we just compare the UTC timestamps.
         // The second timestamp should be just a few nanoseconds later.