diff --git a/toolkit/components/glean/api/src/ffi/mod.rs b/toolkit/components/glean/api/src/ffi/mod.rs index 4e2b0a49bee2..3d5b5ab4d60f 100644 --- a/toolkit/components/glean/api/src/ffi/mod.rs +++ b/toolkit/components/glean/api/src/ffi/mod.rs @@ -169,7 +169,7 @@ pub extern "C" fn fog_datetime_test_get_value( value: &mut nsACString, ) { let val = test_get!(DATETIME_MAP, id, storage_name); - value.assign(&val); + value.assign(&val.to_rfc3339()); } #[no_mangle] diff --git a/toolkit/components/glean/api/src/private/datetime.rs b/toolkit/components/glean/api/src/private/datetime.rs index 03f801985863..d5e25bbf24d2 100644 --- a/toolkit/components/glean/api/src/private/datetime.rs +++ b/toolkit/components/glean/api/src/private/datetime.rs @@ -2,27 +2,25 @@ // 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::sync::Arc; +use inherent::inherent; use super::{CommonMetricData, MetricId}; use super::TimeUnit; -use crate::dispatcher; use crate::ipc::need_ipc; -use chrono::{DateTime, FixedOffset}; +use chrono::{FixedOffset, TimeZone}; +use glean_core::traits::Datetime; /// A datetime metric of a certain resolution. /// /// Datetimes are used to make record when something happened according to the /// client's clock. -#[derive(Debug)] +#[derive(Clone)] pub enum DatetimeMetric { - Parent(Arc), + Parent(glean::private::DatetimeMetric), Child(DatetimeMetricIpc), } -#[derive(Debug)] -pub struct DatetimeMetricImpl(glean_core::metrics::DatetimeMetric); -#[derive(Debug)] +#[derive(Debug, Clone)] pub struct DatetimeMetricIpc; impl DatetimeMetric { @@ -31,7 +29,7 @@ impl DatetimeMetric { if need_ipc() { DatetimeMetric::Child(DatetimeMetricIpc) } else { - DatetimeMetric::Parent(Arc::new(DatetimeMetricImpl::new(meta, time_unit))) + DatetimeMetric::Parent(glean::private::DatetimeMetric::new(meta, time_unit)) } } @@ -43,28 +41,6 @@ impl DatetimeMetric { } } - /// Set the datetime to the provided value, or the local now. - /// - /// ## Arguments - /// - /// - `value` - The date and time and timezone value to set. - /// If None we use the current local time. - pub fn set(&self, value: Option>) { - match self { - DatetimeMetric::Parent(p) => { - let metric = Arc::clone(&p); - dispatcher::launch(move || metric.set(value)); - } - DatetimeMetric::Child(_) => { - log::error!( - "Unable to set datetime metric {:?} in non-parent process. Ignoring.", - self - ); - // TODO: Record an error. - } - } - } - /// Sets the metric to a date/time including the timezone offset. /// /// # Arguments @@ -93,112 +69,113 @@ impl DatetimeMetric { ) { match self { DatetimeMetric::Parent(p) => { - let metric = Arc::clone(&p); - dispatcher::launch(move || { - metric.set_with_details( - year, - month, - day, - hour, - minute, - second, - nano, - offset_seconds, - ) - }); + let tz = FixedOffset::east_opt(offset_seconds); + if tz.is_none() { + log::error!( + "Unable to set datetime metric with invalid offset seconds {}", + offset_seconds + ); + // TODO: Record an error + return; + } + + let value = FixedOffset::east(offset_seconds) + .ymd_opt(year, month, day) + .and_hms_nano_opt(hour, minute, second, nano); + match value.single() { + Some(d) => p.set(Some(d)), + _ => { + log::error!("Unable to construct datetime") + // TODO: Record an error + } + } + } + DatetimeMetric::Child(_) => { + log::error!("Unable to set datetime metric in non-parent process. Ignoring."); + // TODO: Record an error. + } + } + } +} + +#[inherent(pub)] +impl Datetime for DatetimeMetric { + /// Sets the metric to a date/time which including the timezone offset. + /// + /// ## Arguments + /// + /// - `value` - The date and time and timezone value to set. + /// If None we use the current local time. + fn set(&self, value: Option) { + match self { + DatetimeMetric::Parent(p) => { + Datetime::set(&*p, value); } DatetimeMetric::Child(_) => { log::error!( - "Unable to set datetime metric {:?} in non-parent process. Ignoring.", - self + "Unable to set datetime metric DatetimeMetric in non-parent process. Ignoring." ); // TODO: Record an error. } } } - /// **Test-only API.** + /// **Exported for test purposes.** + /// + /// Gets the currently stored value as a Datetime. + /// + /// The precision of this value is truncated to the `time_unit` precision. /// - /// Get the currently stored value. /// This doesn't clear the stored value. /// - /// ## Note + /// # Arguments /// - /// This currently returns the value as an ISO 8601 time string. - /// See [bug 1636176](https://bugzilla.mozilla.org/show_bug.cgi?id=1636176). - /// - /// ## Arguments - /// - /// * `storage_name` - the storage name to look into. - /// - /// ## Return value - /// - /// Returns the stored value or `None` if nothing stored. - pub fn test_get_value<'a, S: Into>>(&self, storage_name: S) -> Option { + /// * `ping_name` - represents the optional name of the ping to retrieve the + /// metric for. Defaults to the first value in `send_in_pings`. + fn test_get_value<'a, S: Into>>( + &self, + ping_name: S, + ) -> Option { match self { - DatetimeMetric::Parent(p) => { - dispatcher::block_on_queue(); - p.test_get_value(storage_name) + DatetimeMetric::Parent(p) => p.test_get_value(ping_name), + DatetimeMetric::Child(_) => { + panic!("Cannot get test value for DatetimeMetric in non-parent process!") } - DatetimeMetric::Child(_) => panic!( - "Cannot get test value for {:?} in non-parent process!", - self - ), } } -} -impl DatetimeMetricImpl { - pub fn new(meta: CommonMetricData, time_unit: TimeUnit) -> Self { - Self(glean_core::metrics::DatetimeMetric::new(meta, time_unit)) - } - - pub fn set(&self, value: Option>) { - crate::with_glean(move |glean| self.0.set(glean, value)) - } - - #[allow(clippy::too_many_arguments)] - pub(crate) fn set_with_details( + /// **Exported for test purposes.** + /// + /// Gets the number of recorded errors for the given metric and error type. + /// + /// # Arguments + /// + /// * `error` - The type of error + /// * `ping_name` - represents the optional name of the ping to retrieve the + /// metric for. Defaults to the first value in `send_in_pings`. + /// + /// # Returns + /// + /// The number of errors reported. + fn test_get_num_recorded_errors<'a, S: Into>>( &self, - year: i32, - month: u32, - day: u32, - hour: u32, - minute: u32, - second: u32, - nano: u32, - offset_seconds: i32, - ) { - crate::with_glean(move |glean| { - self.0.set_with_details( - glean, - year, - month, - day, - hour, - minute, - second, - nano, - offset_seconds, - ); - }) - } - - fn test_get_value<'a, S: Into>>(&self, storage_name: S) -> Option { - // FIXME(bug 1677448): This hack goes away when the type is implemented in RLB. - let storage = storage_name.into().expect("storage name required."); - crate::with_glean(move |glean| self.0.test_get_value_as_string(glean, storage)) + error: glean::ErrorType, + ping_name: S, + ) -> i32 { + match self { + DatetimeMetric::Parent(p) => p.test_get_num_recorded_errors(error, ping_name), + DatetimeMetric::Child(_) => panic!("Cannot get the number of recorded errors for DatetimeMetric in non-parent process!"), + } } } #[cfg(test)] mod test { - use chrono::{FixedOffset, TimeZone}; + use chrono::{DateTime, FixedOffset, TimeZone}; use crate::{common_test::*, ipc, metrics}; #[test] - #[ignore] // TODO: Enable them back when bug 1677448 lands. fn sets_datetime_value() { let _lock = lock_test(); @@ -210,13 +187,12 @@ mod test { metric.set(Some(a_datetime)); assert_eq!( - "2020-05-07T11:58:00+05:00", + DateTime::parse_from_rfc3339("2020-05-07T11:58:00+05:00").unwrap(), metric.test_get_value("store1").unwrap() ); } #[test] - #[ignore] // TODO: Enable them back when bug 1677448 lands. fn sets_datetime_value_with_details() { let _lock = lock_test(); @@ -225,13 +201,12 @@ mod test { metric.set_with_details(2020, 05, 07, 11, 58, 0, 0, 5 * 3600); assert_eq!( - "2020-05-07T11:58:00+05:00", + DateTime::parse_from_rfc3339("2020-05-07T11:58:00+05:00").unwrap(), metric.test_get_value("store1").unwrap() ); } #[test] - #[ignore] // TODO: Enable them back when bug 1677448 lands. fn datetime_ipc() { // DatetimeMetric doesn't support IPC. let _lock = lock_test(); @@ -265,7 +240,7 @@ mod test { assert!(ipc::replay_from_buf(&ipc::take_buf().unwrap()).is_ok()); assert_eq!( - "2020-10-13T16:41:00+05:00", + DateTime::parse_from_rfc3339("2020-10-13T16:41:00+05:00").unwrap(), parent_metric.test_get_value("store1").unwrap() ); }