diff --git a/toolkit/components/glean/api/src/ipc.rs b/toolkit/components/glean/api/src/ipc.rs index 7fcab3aba175..c2dfbc07e71c 100644 --- a/toolkit/components/glean/api/src/ipc.rs +++ b/toolkit/components/glean/api/src/ipc.rs @@ -24,6 +24,7 @@ pub struct IPCPayload { pub counters: HashMap, pub events: HashMap>)>>, pub memory_samples: HashMap>, + pub string_lists: HashMap>, } /// Uniquely identifies a single metric within its metric type. diff --git a/toolkit/components/glean/api/src/private/string_list.rs b/toolkit/components/glean/api/src/private/string_list.rs index 4f7e8d4d3e04..6ff16c24cd60 100644 --- a/toolkit/components/glean/api/src/private/string_list.rs +++ b/toolkit/components/glean/api/src/private/string_list.rs @@ -2,18 +2,34 @@ // 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 super::CommonMetricData; +use crate::dispatcher; +use crate::ipc::{need_ipc, with_ipc_payload, MetricId}; + /// A string list metric. /// /// This allows appending a string value with arbitrary content to a list. +#[derive(Debug)] +pub enum StringListMetric { + Parent(Arc), + Child(StringListMetricIpc), +} #[derive(Clone, Debug)] -pub struct StringListMetric(glean_core::metrics::StringListMetric); +pub struct StringListMetricImpl(glean_core::metrics::StringListMetric); +#[derive(Debug)] +pub struct StringListMetricIpc(MetricId); impl StringListMetric { /// Create a new string list metric. pub fn new(meta: CommonMetricData) -> Self { - Self(glean_core::metrics::StringListMetric::new(meta)) + if need_ipc() { + StringListMetric::Child(StringListMetricIpc(MetricId::new(meta))) + } else { + StringListMetric::Parent(Arc::new(StringListMetricImpl::new(meta))) + } } /// Add a new string to the list. @@ -27,7 +43,24 @@ impl StringListMetric { /// Truncates the value if it is longer than `MAX_STRING_LENGTH` bytes and logs an error. /// See [String list metric limits](https://mozilla.github.io/glean/book/user/metrics/string_list.html#limits). pub fn add>(&self, value: S) { - crate::with_glean(move |glean| self.0.add(glean, value)) + match self { + StringListMetric::Parent(p) => { + let metric = Arc::clone(&p); + let value = value.into(); + dispatcher::launch(move || metric.add(value)); + } + StringListMetric::Child(c) => { + with_ipc_payload(move |payload| { + if let Some(v) = payload.string_lists.get_mut(&c.0) { + v.push(value.into()); + } else { + let mut v = vec![]; + v.push(value.into()); + payload.string_lists.insert(c.0.clone(), v); + } + }); + } + } } /// Set to a specific list of strings. @@ -42,7 +75,19 @@ impl StringListMetric { /// Truncates the list if it is longer than `MAX_LIST_LENGTH` and logs an error. /// Truncates any value in the list if it is longer than `MAX_STRING_LENGTH` and logs an error. pub fn set(&self, value: Vec) { - crate::with_glean(move |glean| self.0.set(glean, value)) + match self { + StringListMetric::Parent(p) => { + let metric = Arc::clone(&p); + dispatcher::launch(move || metric.set(value)); + } + StringListMetric::Child(_c) => { + log::error!( + "Unable to set string list metric {:?} in non-main process. Ignoring.", + self + ); + // TODO: Record an error. + } + } } /// **Test-only API.** @@ -57,6 +102,33 @@ impl StringListMetric { /// ## Return value /// /// Returns the stored value or `None` if nothing stored. + pub fn test_get_value(&self, storage_name: &str) -> Option> { + match self { + StringListMetric::Parent(p) => { + dispatcher::block_on_queue(); + p.test_get_value(storage_name) + } + StringListMetric::Child(_c) => panic!( + "Cannot get test value for {:?} in non-parent process!", + self + ), + } + } +} + +impl StringListMetricImpl { + pub fn new(meta: CommonMetricData) -> Self { + Self(glean_core::metrics::StringListMetric::new(meta)) + } + + pub fn add>(&self, value: S) { + crate::with_glean(move |glean| self.0.add(glean, value)) + } + + pub fn set(&self, value: Vec) { + crate::with_glean(move |glean| self.0.set(glean, value)) + } + pub fn test_get_value(&self, storage_name: &str) -> Option> { crate::with_glean(move |glean| self.0.test_get_value(glean, storage_name)) } diff --git a/toolkit/components/glean/api/tests/string_list.rs b/toolkit/components/glean/api/tests/string_list.rs new file mode 100644 index 000000000000..181c6d44538e --- /dev/null +++ b/toolkit/components/glean/api/tests/string_list.rs @@ -0,0 +1,70 @@ +// This Source Code Form is subject to the terms of the Mozilla Public +// 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/. + +mod common; +use common::*; + +use fog::ipc; +use fog::private::{CommonMetricData, Lifetime, StringListMetric}; + +#[test] +fn sets_string_list_value() { + let _lock = lock_test(); + let _t = setup_glean(None); + let store_names: Vec = vec!["store1".into()]; + + let metric = StringListMetric::new(CommonMetricData { + name: "string_list_metric".into(), + category: "telemetry".into(), + send_in_pings: store_names.clone(), + disabled: false, + lifetime: Lifetime::Ping, + ..Default::default() + }); + + metric.set(vec!["test_string_value".to_string()]); + metric.add("another test value"); + + assert_eq!( + vec!["test_string_value", "another test value"], + metric.test_get_value("store1").unwrap() + ); +} + +#[test] +fn string_list_ipc() { + // StringListMetric supports IPC only for `add`, not `set`. + let _lock = lock_test(); + let _t = setup_glean(None); + + let store_names: Vec = vec!["store1".into()]; + let meta = CommonMetricData { + name: "string metric".into(), + category: "ipc".into(), + send_in_pings: store_names.clone(), + disabled: false, + lifetime: Lifetime::Ping, + ..Default::default() + }; + + { + let _raii = ipc::test_set_need_ipc(true); + let child_metric = StringListMetric::new(meta.clone()); + + // Recording APIs do not panic, even when they don't work. + child_metric.set(vec!["not gonna be set".to_string()]); + + child_metric.add("child_value"); + assert!(ipc::take_buf().unwrap().len() > 0); + + // Test APIs are allowed to panic, though. + let result = std::panic::catch_unwind(move || { + child_metric.test_get_value("store1"); + }); + assert!(result.is_err()); + } + + // TODO: implement replay. See bug 1646165. + // Then perform the replay and assert we have the values from both "processes". +}