diff --git a/.dictionary b/.dictionary index 8f2df5e12..847334f03 100644 --- a/.dictionary +++ b/.dictionary @@ -207,6 +207,7 @@ rethrow rfloor rkv rkv's +rollouts runtime runtimes rustc diff --git a/CHANGELOG.md b/CHANGELOG.md index a2e234fd6..4ae61129f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,8 @@ * Upgrade to `glean_parser` v6.5.0, with support for `Cow` in Rust code ([#2300](https://github.com/mozilla/glean/issues/2300)) * API REMOVED: The deprecated-since-v38 `event` metric `record(map)` API has been removed ([bug 1802550](https://bugzilla.mozilla.org/show_bug.cgi?id=1802550)) * BEHAVIOUR CHANGE: "events" pings will no longer be sent if they have metrics but no events ([bug 1803513](https://bugzilla.mozilla.org/show_bug.cgi?id=1803513)) + * *_Experimental:_* Add functionality necessary to remotely configure the metric `disabled` property ([bug 1798919](https://bugzilla.mozilla.org/show_bug.cgi?id=1798919)) + * This change has no effect when the API is not used and is transparent to consumers. The API is currently experimental because it is not stable and may change. * Rust * Static labels for labeled metrics are now `Cow<'static, str>` to reduce heap allocations ([#2272](https://github.com/mozilla/glean/pull/2272)) diff --git a/Cargo.lock b/Cargo.lock index fc3e7c18c..f9b793326 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -367,7 +367,7 @@ dependencies = [ [[package]] name = "glean-build" -version = "6.3.0" +version = "6.4.0" dependencies = [ "xshell-venv", ] diff --git a/docs/user/SUMMARY.md b/docs/user/SUMMARY.md index 182dacba4..b7279f84f 100644 --- a/docs/user/SUMMARY.md +++ b/docs/user/SUMMARY.md @@ -17,6 +17,7 @@ - [Validating metrics](user/metrics/validation-checklist.md) - [Error reporting](user/metrics/error-reporting.md) - [Metrics collected by the Glean SDKs](user/collected-metrics/metrics.md) + - [Enable and Disable metrics using Nimbus](user/metrics/metrics-remote-settings.md) - [Pings](user/pings/index.md) - [Adding new custom pings](user/pings/custom.md) - [Testing custom pings](user/pings/testing-custom-pings.md) diff --git a/docs/user/user/metrics/metrics-remote-settings.md b/docs/user/user/metrics/metrics-remote-settings.md new file mode 100644 index 000000000..0953ec169 --- /dev/null +++ b/docs/user/user/metrics/metrics-remote-settings.md @@ -0,0 +1,35 @@ +# Remote Configuration of Metrics + +## Overview + +> **Important:** This functionality is experimental and is not ready for production use without coordination with the Glean Team. + +Glean metrics have the ability to be disabled and enabled at runtime, effectively overriding the `disabled` property of the metric defined for it in the `metrics.yaml` file. This functionality is currently able to be controlled through [Nimbus experiments and rollouts](https://experimenter.info). + +Having metrics which can be remotely turned on and off via remote settings allows us to precisely control the sample of the population sending us telemetry. +Through this, event metrics can be instrumented in areas of high activity and only a subset of the population can be sampled to reduce the amount of traffic and data storage needed while still maintaining enough of a signal from the telemetry to make data-informed decisions based on it. + + +## How to Enable/Disable Metrics + +The instructions and requirements for running Nimbus experiments and rollouts can be found at https://experimenter.info. The purpose of the instructions found here in the Glean Book are meant to supplement the Nimbus information and aid in running experiments that interact with Glean metrics. + +When creating an experiment definition in the Experimenter UI, during the Branches Configuration stage: + +- Be sure to select `Glean` as the feature from the dropdown. If you do not see `Glean` as an option, then it has not been enabled for your application yet. +- Ensure that the feature is enabled is toggled to "On" in the Branch Configuration page for each branch. +- To disable remote configuration of metrics for a branch, enter empty braces into the "value" field: `{}`. +- To enable a remote configuration for a branch, enter JSON into the "Value" field in the following format: + - ```JSON + { + "metricsDisabled": { + "category.name": true, + "category.different_name": false, + ... + } + } + ``` + - Do not change `"metricsDisabled"`, this is the required object key for Nimbus to recognize the Glean feature. + - Do change `"category.name"` to match the category and name of the metric to disable/enable. + - This is a list you can add as many entries in the format as needed. + - Since this controls the `disabled` property of the metrics, `true` tells Glean to *_disable_* the metric, while `false` would tell Glean to *_enable_* the metric. diff --git a/glean-core/rlb/src/lib.rs b/glean-core/rlb/src/lib.rs index 0e6134e1a..e90c0af20 100644 --- a/glean-core/rlb/src/lib.rs +++ b/glean-core/rlb/src/lib.rs @@ -170,6 +170,13 @@ pub fn set_experiment_inactive(experiment_id: String) { glean_core::glean_set_experiment_inactive(experiment_id) } +/// Set the remote configuration values for the metrics' disabled property +/// +/// See [`glean_core::Glean::set_metrics_disabled_config`]. +pub fn glean_set_metrics_disabled_config(json: String) { + glean_core::glean_set_metrics_disabled_config(json) +} + /// Performs the collection/cleanup operations required by becoming active. /// /// This functions generates a baseline ping with reason `active` diff --git a/glean-core/src/common_metric_data.rs b/glean-core/src/common_metric_data.rs index 391bb286b..420badfc4 100644 --- a/glean-core/src/common_metric_data.rs +++ b/glean-core/src/common_metric_data.rs @@ -3,6 +3,7 @@ // file, You can obtain one at https://mozilla.org/MPL/2.0/. use std::convert::TryFrom; +use std::sync::atomic::{AtomicU8, Ordering}; use crate::error::{Error, ErrorKind}; use crate::metrics::labeled::validate_dynamic_label; @@ -78,18 +79,45 @@ pub struct CommonMetricData { pub dynamic_label: Option, } -impl CommonMetricData { +#[derive(Default, Debug, Deserialize, Serialize)] +pub struct CommonMetricDataInternal { + pub inner: CommonMetricData, + pub disabled: AtomicU8, +} + +impl Clone for CommonMetricDataInternal { + fn clone(&self) -> Self { + Self { + inner: self.inner.clone(), + disabled: AtomicU8::new(self.disabled.load(Ordering::Relaxed)), + } + } +} + +impl From for CommonMetricDataInternal { + fn from(input_data: CommonMetricData) -> Self { + Self { + inner: input_data.clone(), + disabled: AtomicU8::new(u8::from(input_data.disabled)), + } + } +} + +impl CommonMetricDataInternal { /// Creates a new metadata object. pub fn new, B: Into, C: Into>( category: A, name: B, ping_name: C, - ) -> CommonMetricData { - CommonMetricData { - name: name.into(), - category: category.into(), - send_in_pings: vec![ping_name.into()], - ..Default::default() + ) -> CommonMetricDataInternal { + CommonMetricDataInternal { + inner: CommonMetricData { + name: name.into(), + category: category.into(), + send_in_pings: vec![ping_name.into()], + ..Default::default() + }, + disabled: AtomicU8::new(0), } } @@ -98,10 +126,10 @@ impl CommonMetricData { /// If `category` is empty, it's ommitted. /// Otherwise, it's the combination of the metric's `category` and `name`. pub(crate) fn base_identifier(&self) -> String { - if self.category.is_empty() { - self.name.clone() + if self.inner.category.is_empty() { + self.inner.name.clone() } else { - format!("{}.{}", self.category, self.name) + format!("{}.{}", self.inner.category, self.inner.name) } } @@ -112,20 +140,15 @@ impl CommonMetricData { pub(crate) fn identifier(&self, glean: &Glean) -> String { let base_identifier = self.base_identifier(); - if let Some(label) = &self.dynamic_label { + if let Some(label) = &self.inner.dynamic_label { validate_dynamic_label(glean, self, &base_identifier, label) } else { base_identifier } } - /// Whether this metric should be recorded. - pub fn should_record(&self) -> bool { - !self.disabled - } - /// The list of storages this metric should be recorded into. pub fn storage_names(&self) -> &[String] { - &self.send_in_pings + &self.inner.send_in_pings } } diff --git a/glean-core/src/core/mod.rs b/glean-core/src/core/mod.rs index b43f6d630..52b109671 100644 --- a/glean-core/src/core/mod.rs +++ b/glean-core/src/core/mod.rs @@ -1,6 +1,7 @@ use std::collections::HashMap; use std::path::{Path, PathBuf}; -use std::sync::Mutex; +use std::sync::atomic::{AtomicU8, Ordering}; +use std::sync::{Arc, Mutex}; use chrono::{DateTime, FixedOffset}; use once_cell::sync::OnceCell; @@ -10,7 +11,9 @@ use crate::debug::DebugOptions; use crate::event_database::EventDatabase; use crate::internal_metrics::{AdditionalMetrics, CoreMetrics, DatabaseMetrics}; use crate::internal_pings::InternalPings; -use crate::metrics::{self, ExperimentMetric, Metric, MetricType, PingType, RecordedExperiment}; +use crate::metrics::{ + self, ExperimentMetric, Metric, MetricType, MetricsDisabledConfig, PingType, RecordedExperiment, +}; use crate::ping::PingMaker; use crate::storage::{StorageManager, INTERNAL_STORAGE}; use crate::upload::{PingUploadManager, PingUploadTask, UploadResult, UploadTaskAction}; @@ -148,6 +151,8 @@ pub struct Glean { debug: DebugOptions, pub(crate) app_build: String, pub(crate) schedule_metrics_pings: bool, + pub(crate) remote_settings_epoch: AtomicU8, + pub(crate) remote_settings_metrics_config: Arc>, } impl Glean { @@ -200,6 +205,8 @@ impl Glean { app_build: cfg.app_build.to_string(), // Subprocess doesn't use "metrics" pings so has no need for a scheduler. schedule_metrics_pings: false, + remote_settings_epoch: AtomicU8::new(0), + remote_settings_metrics_config: Arc::new(Mutex::new(MetricsDisabledConfig::new())), }; // Ensuring these pings are registered. @@ -687,6 +694,22 @@ impl Glean { metric.test_get_value(self) } + /// Set configuration for metrics' disabled property, typically from a remote_settings experiment + /// or rollout + /// + /// # Arguments + /// + /// * `json` - The stringified JSON representation of a `MetricsDisabledConfig` object + pub fn set_metrics_disabled_config(&self, cfg: MetricsDisabledConfig) { + // Set the current MetricsDisabledConfig, keeping the lock until the epoch is + // updated to prevent against reading a "new" config but an "old" epoch + let mut lock = self.remote_settings_metrics_config.lock().unwrap(); + *lock = cfg; + + // Update remote_settings epoch + self.remote_settings_epoch.fetch_add(1, Ordering::SeqCst); + } + /// Persists [`Lifetime::Ping`] data that might be in memory in case /// [`delay_ping_lifetime_io`](InternalConfiguration::delay_ping_lifetime_io) is set /// or was set at a previous time. @@ -829,7 +852,7 @@ impl Glean { self.storage(), INTERNAL_STORAGE, &dirty_bit_metric.meta().identifier(self), - dirty_bit_metric.meta().lifetime, + dirty_bit_metric.meta().inner.lifetime, ) { Some(Metric::Boolean(b)) => b, _ => false, diff --git a/glean-core/src/database/mod.rs b/glean-core/src/database/mod.rs index 2d1182a18..df7adf136 100644 --- a/glean-core/src/database/mod.rs +++ b/glean-core/src/database/mod.rs @@ -143,8 +143,8 @@ pub fn migrate(path: &Path, dst_env: &Rkv) { log::debug!("Migration ended. Safe-mode database in {}", path.display()); } +use crate::common_metric_data::CommonMetricDataInternal; use crate::metrics::Metric; -use crate::CommonMetricData; use crate::Glean; use crate::Lifetime; use crate::Result; @@ -449,7 +449,7 @@ impl Database { } /// Records a metric in the underlying storage system. - pub fn record(&self, glean: &Glean, data: &CommonMetricData, value: &Metric) { + pub fn record(&self, glean: &Glean, data: &CommonMetricDataInternal, value: &Metric) { // If upload is disabled we don't want to record. if !glean.is_upload_enabled() { return; @@ -458,7 +458,7 @@ impl Database { let name = data.identifier(glean); for ping_name in data.storage_names() { - if let Err(e) = self.record_per_lifetime(data.lifetime, ping_name, &name, value) { + if let Err(e) = self.record_per_lifetime(data.inner.lifetime, ping_name, &name, value) { log::error!("Failed to record metric into {}: {:?}", ping_name, e); } } @@ -508,7 +508,7 @@ impl Database { /// Records the provided value, with the given lifetime, /// after applying a transformation function. - pub fn record_with(&self, glean: &Glean, data: &CommonMetricData, mut transform: F) + pub fn record_with(&self, glean: &Glean, data: &CommonMetricDataInternal, mut transform: F) where F: FnMut(Option) -> Metric, { @@ -520,7 +520,7 @@ impl Database { let name = data.identifier(glean); for ping_name in data.storage_names() { if let Err(e) = - self.record_per_lifetime_with(data.lifetime, ping_name, &name, &mut transform) + self.record_per_lifetime_with(data.inner.lifetime, ping_name, &name, &mut transform) { log::error!("Failed to record metric into {}: {:?}", ping_name, e); } @@ -767,7 +767,6 @@ impl Database { mod test { use super::*; use crate::tests::new_glean; - use crate::CommonMetricData; use std::collections::HashMap; use std::path::Path; use tempfile::tempdir; @@ -1351,7 +1350,7 @@ mod test { // Init the database in a temporary directory. let test_storage = "test-storage"; - let test_data = CommonMetricData::new("category", "name", test_storage); + let test_data = CommonMetricDataInternal::new("category", "name", test_storage); let test_metric_id = test_data.identifier(&glean); // Attempt to record metric with the record and record_with functions, diff --git a/glean-core/src/error_recording.rs b/glean-core/src/error_recording.rs index 4f13cf6c7..aaf850d01 100644 --- a/glean-core/src/error_recording.rs +++ b/glean-core/src/error_recording.rs @@ -15,6 +15,7 @@ use std::convert::TryFrom; use std::fmt::Display; +use crate::common_metric_data::CommonMetricDataInternal; use crate::error::{Error, ErrorKind}; use crate::metrics::labeled::{combine_base_identifier_and_label, strip_label}; use crate::metrics::CounterMetric; @@ -88,14 +89,14 @@ impl TryFrom for ErrorType { } /// For a given metric, get the metric in which to record errors -fn get_error_metric_for_metric(meta: &CommonMetricData, error: ErrorType) -> CounterMetric { +fn get_error_metric_for_metric(meta: &CommonMetricDataInternal, error: ErrorType) -> CounterMetric { // Can't use meta.identifier here, since that might cause infinite recursion // if the label on this metric needs to report an error. let identifier = meta.base_identifier(); let name = strip_label(&identifier); // Record errors in the pings the metric is in, as well as the metrics ping. - let mut send_in_pings = meta.send_in_pings.clone(); + let mut send_in_pings = meta.inner.send_in_pings.clone(); let ping_name = "metrics".to_string(); if !send_in_pings.contains(&ping_name) { send_in_pings.push(ping_name); @@ -128,7 +129,7 @@ fn get_error_metric_for_metric(meta: &CommonMetricData, error: ErrorType) -> Cou /// * `num_errors` - The number of errors of the same type to report. pub fn record_error>>( glean: &Glean, - meta: &CommonMetricData, + meta: &CommonMetricDataInternal, error: ErrorType, message: impl Display, num_errors: O, @@ -156,7 +157,7 @@ pub fn record_error>>( /// The number of errors reported. pub fn test_get_num_recorded_errors( glean: &Glean, - meta: &CommonMetricData, + meta: &CommonMetricDataInternal, error: ErrorType, ) -> Result { let metric = get_error_metric_for_metric(meta, error); diff --git a/glean-core/src/event_database/mod.rs b/glean-core/src/event_database/mod.rs index 3c9679aaf..c3f1c60e1 100644 --- a/glean-core/src/event_database/mod.rs +++ b/glean-core/src/event_database/mod.rs @@ -18,6 +18,7 @@ use chrono::{DateTime, FixedOffset}; use serde::{Deserialize, Serialize}; use serde_json::{json, Value as JsonValue}; +use crate::common_metric_data::CommonMetricDataInternal; use crate::coverage::record_coverage; use crate::error_recording::{record_error, ErrorType}; use crate::metrics::{DatetimeMetric, TimeUnit}; @@ -184,7 +185,7 @@ impl EventDatabase { let extra = [("glean.startup.date".into(), startup)].into(); self.record( glean, - &glean_restarted, + &glean_restarted.into(), crate::get_timestamp_ms(), Some(extra), ); @@ -237,7 +238,7 @@ impl EventDatabase { pub fn record( &self, glean: &Glean, - meta: &CommonMetricData, + meta: &CommonMetricDataInternal, timestamp: u64, extra: Option>, ) { @@ -249,7 +250,7 @@ impl EventDatabase { let mut submit_max_capacity_event_ping = false; { let mut db = self.event_stores.write().unwrap(); // safe unwrap, only error case is poisoning - for store_name in meta.send_in_pings.iter() { + for store_name in meta.inner.send_in_pings.iter() { let store = db.entry(store_name.to_string()).or_insert_with(Vec::new); let execution_counter = CounterMetric::new(CommonMetricData { name: "execution_counter".into(), @@ -263,8 +264,8 @@ impl EventDatabase { let event = StoredEvent { event: RecordedEvent { timestamp, - category: meta.category.to_string(), - name: meta.name.to_string(), + category: meta.inner.category.to_string(), + name: meta.inner.name.to_string(), extra: extra.clone(), }, execution_counter, @@ -404,7 +405,7 @@ impl EventDatabase { .map_err(|_| { record_error( glean, - &glean_restarted_meta(store_name), + &glean_restarted_meta(store_name).into(), ErrorType::InvalidState, format!("Unparseable glean.startup.date '{}'", date_str), None, @@ -444,7 +445,7 @@ impl EventDatabase { if inter_group_offset < highest_ts { record_error( glean, - &glean_restarted_meta(store_name), + &glean_restarted_meta(store_name).into(), ErrorType::InvalidValue, format!("Time between restart and ping start {} indicates client clock weirdness.", time_from_ping_start_to_glean_restarted), None, @@ -460,7 +461,7 @@ impl EventDatabase { if execution_counter != cur_ec { record_error( glean, - &glean_restarted_meta(store_name), + &glean_restarted_meta(store_name).into(), ErrorType::InvalidState, format!( "Inconsistent execution counter {} (expected {})", @@ -476,7 +477,7 @@ impl EventDatabase { // execution_counter or glean.startup.date math went awry. record_error( glean, - &glean_restarted_meta(store_name), + &glean_restarted_meta(store_name).into(), ErrorType::InvalidState, format!( "Inconsistent previous highest timestamp {} (expected <= {})", @@ -571,7 +572,7 @@ impl EventDatabase { /// This doesn't clear the stored value. pub fn test_get_value<'a>( &'a self, - meta: &'a CommonMetricData, + meta: &'a CommonMetricDataInternal, store_name: &str, ) -> Option> { record_coverage(&meta.base_identifier()); @@ -584,7 +585,7 @@ impl EventDatabase { .into_iter() .flatten() .map(|stored_event| stored_event.event.clone()) - .filter(|event| event.name == meta.name && event.category == meta.category) + .filter(|event| event.name == meta.inner.name && event.category == meta.inner.category) .collect(); if !value.is_empty() { Some(value) @@ -722,7 +723,7 @@ mod test { let test_category = "category"; let test_name = "name"; let test_timestamp = 2; - let test_meta = CommonMetricData::new(test_category, test_name, test_storage); + let test_meta = CommonMetricDataInternal::new(test_category, test_name, test_storage); let event_data = RecordedEvent { timestamp: test_timestamp, category: test_category.to_string(), @@ -1169,7 +1170,8 @@ mod test { send_in_pings: vec![store_name.into()], lifetime: Lifetime::Ping, ..Default::default() - }, + } + .into(), ErrorType::InvalidValue ) ); diff --git a/glean-core/src/lib.rs b/glean-core/src/lib.rs index a6e4bb7c7..17138625e 100644 --- a/glean-core/src/lib.rs +++ b/glean-core/src/lib.rs @@ -16,10 +16,12 @@ use std::borrow::Cow; use std::collections::HashMap; +use std::convert::TryFrom; use std::fmt; use std::sync::atomic::{AtomicBool, Ordering}; use std::sync::{Arc, Mutex}; +use metrics::MetricsDisabledConfig; use once_cell::sync::{Lazy, OnceCell}; use uuid::Uuid; @@ -695,6 +697,20 @@ pub fn glean_test_get_experiment_data(experiment_id: String) -> Option launch_with_glean(|glean| { + glean.set_metrics_disabled_config(cfg); + }), + Err(e) => { + log::error!("Error setting metrics feature config: {:?}", e); + } + } +} + /// Sets a debug view tag. /// /// When the debug view tag is set, pings are sent with a `X-Debug-ID` header with the diff --git a/glean-core/src/lib_unit_tests.rs b/glean-core/src/lib_unit_tests.rs index 8ba9f90e7..aa22287da 100644 --- a/glean-core/src/lib_unit_tests.rs +++ b/glean-core/src/lib_unit_tests.rs @@ -8,6 +8,8 @@ use std::collections::HashSet; use std::iter::FromIterator; +use serde_json::json; + use super::*; use crate::metrics::{StringMetric, TimeUnit, TimespanMetric, TimingDistributionMetric}; @@ -214,11 +216,12 @@ fn client_id_and_first_run_date_must_be_regenerated() { #[test] fn basic_metrics_should_be_cleared_when_uploading_is_disabled() { let (mut glean, _t) = new_glean(None); - let metric = StringMetric::new(CommonMetricData::new( - "category", - "string_metric", - "baseline", - )); + let metric = StringMetric::new(CommonMetricData { + category: "category".to_string(), + name: "string_metric".to_string(), + send_in_pings: vec!["baseline".to_string()], + ..Default::default() + }); metric.set_sync(&glean, "TEST VALUE"); assert!(metric.get_value(&glean, "baseline").is_some()); @@ -812,6 +815,174 @@ fn test_setting_log_pings() { assert!(!glean.log_pings()); } +#[test] +fn test_set_metrics_disabled() { + let (glean, _t) = new_glean(None); + let metric = StringMetric::new(CommonMetricData { + category: "category".to_string(), + name: "string_metric".to_string(), + send_in_pings: vec!["baseline".to_string()], + ..Default::default() + }); + let another_metric = LabeledString::new( + CommonMetricData { + category: "category".to_string(), + name: "labeled_string_metric".to_string(), + send_in_pings: vec!["baseline".to_string()], + ..Default::default() + }, + Some(vec!["label1".into()]), + ); + + // 1. Set the metrics with a "TEST_VALUE" and ensure it was set + metric.set_sync(&glean, "TEST_VALUE"); + assert_eq!( + "TEST_VALUE", + metric.get_value(&glean, "baseline").unwrap(), + "Initial value must match" + ); + another_metric.get("label1").set_sync(&glean, "TEST_VALUE"); + assert_eq!( + "TEST_VALUE", + another_metric + .get("label1") + .get_value(&glean, "baseline") + .unwrap(), + "Initial value must match" + ); + + // 2. Set a configuration to disable the metrics + let mut metrics_disabled_config = json!( + { + "category.string_metric": true, + "category.labeled_string_metric": true, + } + ) + .to_string(); + glean.set_metrics_disabled_config( + MetricsDisabledConfig::try_from(metrics_disabled_config).unwrap(), + ); + + // 3. Since the metrics were disabled, setting a new value will be ignored + metric.set_sync(&glean, "VALUE_AFTER_DISABLED"); + assert_eq!( + "TEST_VALUE", + metric.get_value(&glean, "baseline").unwrap(), + "Shouldn't set when disabled" + ); + another_metric + .get("label1") + .set_sync(&glean, "VALUE_AFTER_DISABLED"); + assert_eq!( + "TEST_VALUE", + another_metric + .get("label1") + .get_value(&glean, "baseline") + .unwrap(), + "Shouldn't set when disabled" + ); + + // 4. Set a new configuration where the metrics are enabled + metrics_disabled_config = json!({}).to_string(); + glean.set_metrics_disabled_config( + MetricsDisabledConfig::try_from(metrics_disabled_config).unwrap(), + ); + + // 5. Since the metrics are now enabled, setting a new value should work + metric.set_sync(&glean, "VALUE_AFTER_REENABLED"); + assert_eq!( + "VALUE_AFTER_REENABLED", + metric.get_value(&glean, "baseline").unwrap(), + "Should set when re-enabled" + ); + another_metric + .get("label1") + .set_sync(&glean, "VALUE_AFTER_REENABLED"); + assert_eq!( + "VALUE_AFTER_REENABLED", + another_metric + .get("label1") + .get_value(&glean, "baseline") + .unwrap(), + "Should set when re-enabled" + ); +} + +#[test] +fn test_remote_settings_epoch() { + let (glean, _t) = new_glean(None); + + // 1. Ensure the starting epoch + let mut current_epoch = glean.remote_settings_epoch.load(Ordering::Acquire); + assert_eq!(0u8, current_epoch, "Current epoch must start at 0"); + + // 2. Set a configuration which will trigger incrementing the epoch + let metrics_disabled_config = json!( + { + "category.string_metric": true + } + ) + .to_string(); + glean.set_metrics_disabled_config( + MetricsDisabledConfig::try_from(metrics_disabled_config).unwrap(), + ); + + // 3. Ensure the epoch updated + current_epoch = glean.remote_settings_epoch.load(Ordering::Acquire); + assert_eq!(1u8, current_epoch, "Current epoch must match"); +} + +#[test] +fn test_remote_settings_epoch_updates_in_metric() { + let (glean, _t) = new_glean(None); + let metric = StringMetric::new(CommonMetricData { + category: "category".to_string(), + name: "string_metric".to_string(), + send_in_pings: vec!["baseline".to_string()], + ..Default::default() + }); + + // 1. Set the metric with a "TEST_VALUE" and ensure it was set + metric.set_sync(&glean, "TEST_VALUE"); + assert_eq!( + "TEST_VALUE", + metric.get_value(&glean, "baseline").unwrap(), + "Initial value must match" + ); + + // 2. Set a configuration to disable the `category.string_metric` + let metrics_disabled_config = json!( + { + "category.string_metric": true + } + ) + .to_string(); + glean.set_metrics_disabled_config( + MetricsDisabledConfig::try_from(metrics_disabled_config).unwrap(), + ); + + // 3. Ensure the epoch was updated + let current_epoch = glean.remote_settings_epoch.load(Ordering::Acquire); + assert_eq!(1u8, current_epoch, "Current epoch must update"); + + // 4. Since the metric was disabled, setting a new value will be ignored + // AND the metric should update its epoch to match the `current_epoch` + metric.set_sync(&glean, "VALUE_AFTER_DISABLED"); + assert_eq!( + "TEST_VALUE", + metric.get_value(&glean, "baseline").unwrap(), + "Shouldn't set when disabled" + ); + + use crate::metrics::MetricType; + // The "epoch" resides in the upper nibble of the `inner.disabled` field + let epoch = metric.meta().disabled.load(Ordering::Acquire) >> 4; + assert_eq!( + current_epoch, epoch, + "Epoch must match between metric and Glean core" + ); +} + #[test] #[should_panic] fn test_empty_application_id() { diff --git a/glean-core/src/metrics/boolean.rs b/glean-core/src/metrics/boolean.rs index 82dffb45d..71ed2372c 100644 --- a/glean-core/src/metrics/boolean.rs +++ b/glean-core/src/metrics/boolean.rs @@ -4,6 +4,7 @@ use std::sync::Arc; +use crate::common_metric_data::CommonMetricDataInternal; use crate::error_recording::{test_get_num_recorded_errors, ErrorType}; use crate::metrics::Metric; use crate::metrics::MetricType; @@ -16,17 +17,17 @@ use crate::Glean; /// Records a simple flag. #[derive(Clone, Debug)] pub struct BooleanMetric { - meta: Arc, + meta: Arc, } impl MetricType for BooleanMetric { - fn meta(&self) -> &CommonMetricData { + fn meta(&self) -> &CommonMetricDataInternal { &self.meta } fn with_name(&self, name: String) -> Self { let mut meta = (*self.meta).clone(); - meta.name = name; + meta.inner.name = name; Self { meta: Arc::new(meta), } @@ -34,7 +35,7 @@ impl MetricType for BooleanMetric { fn with_dynamic_label(&self, label: String) -> Self { let mut meta = (*self.meta).clone(); - meta.dynamic_label = Some(label); + meta.inner.dynamic_label = Some(label); Self { meta: Arc::new(meta), } @@ -49,7 +50,7 @@ impl BooleanMetric { /// Creates a new boolean metric. pub fn new(meta: CommonMetricData) -> Self { Self { - meta: Arc::new(meta), + meta: Arc::new(meta.into()), } } @@ -87,13 +88,13 @@ impl BooleanMetric { /// This doesn't clear the stored value. #[doc(hidden)] pub fn get_value(&self, glean: &Glean, ping_name: Option<&str>) -> Option { - let queried_ping_name = ping_name.unwrap_or_else(|| &self.meta().send_in_pings[0]); + let queried_ping_name = ping_name.unwrap_or_else(|| &self.meta().inner.send_in_pings[0]); match StorageManager.snapshot_metric_for_test( glean.storage(), queried_ping_name, &self.meta.identifier(glean), - self.meta.lifetime, + self.meta.inner.lifetime, ) { Some(Metric::Boolean(b)) => Some(b), _ => None, @@ -118,7 +119,7 @@ impl BooleanMetric { /// /// * `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`. + /// metric for. inner to the first value in `send_in_pings`. /// /// # Returns /// diff --git a/glean-core/src/metrics/counter.rs b/glean-core/src/metrics/counter.rs index 3629ffc07..ac544df95 100644 --- a/glean-core/src/metrics/counter.rs +++ b/glean-core/src/metrics/counter.rs @@ -4,6 +4,7 @@ use std::sync::Arc; +use crate::common_metric_data::CommonMetricDataInternal; use crate::error_recording::{record_error, test_get_num_recorded_errors, ErrorType}; use crate::metrics::Metric; use crate::metrics::MetricType; @@ -17,17 +18,17 @@ use crate::Glean; /// The value can only be incremented, not decremented. #[derive(Clone, Debug)] pub struct CounterMetric { - meta: Arc, + meta: Arc, } impl MetricType for CounterMetric { - fn meta(&self) -> &CommonMetricData { + fn meta(&self) -> &CommonMetricDataInternal { &self.meta } fn with_name(&self, name: String) -> Self { let mut meta = (*self.meta).clone(); - meta.name = name; + meta.inner.name = name; Self { meta: Arc::new(meta), } @@ -35,7 +36,7 @@ impl MetricType for CounterMetric { fn with_dynamic_label(&self, label: String) -> Self { let mut meta = (*self.meta).clone(); - meta.dynamic_label = Some(label); + meta.inner.dynamic_label = Some(label); Self { meta: Arc::new(meta), } @@ -50,7 +51,7 @@ impl CounterMetric { /// Creates a new counter metric. pub fn new(meta: CommonMetricData) -> Self { Self { - meta: Arc::new(meta), + meta: Arc::new(meta.into()), } } @@ -116,13 +117,13 @@ impl CounterMetric { ) -> Option { let queried_ping_name = ping_name .into() - .unwrap_or_else(|| &self.meta().send_in_pings[0]); + .unwrap_or_else(|| &self.meta().inner.send_in_pings[0]); match StorageManager.snapshot_metric_for_test( glean.storage(), queried_ping_name, &self.meta.identifier(glean), - self.meta.lifetime, + self.meta.inner.lifetime, ) { Some(Metric::Counter(i)) => Some(i), _ => None, @@ -147,7 +148,7 @@ impl CounterMetric { /// /// * `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`. + /// metric for. inner to the first value in `send_in_pings`. /// /// # Returns /// diff --git a/glean-core/src/metrics/custom_distribution.rs b/glean-core/src/metrics/custom_distribution.rs index 595558b57..929e4863e 100644 --- a/glean-core/src/metrics/custom_distribution.rs +++ b/glean-core/src/metrics/custom_distribution.rs @@ -4,6 +4,7 @@ use std::sync::Arc; +use crate::common_metric_data::CommonMetricDataInternal; use crate::error_recording::{record_error, test_get_num_recorded_errors, ErrorType}; use crate::histogram::{Bucketing, Histogram, HistogramType}; use crate::metrics::{DistributionData, Metric, MetricType}; @@ -16,7 +17,7 @@ use crate::Glean; /// Memory distributions are used to accumulate and store memory sizes. #[derive(Clone, Debug)] pub struct CustomDistributionMetric { - meta: Arc, + meta: Arc, range_min: u64, range_max: u64, bucket_count: u64, @@ -39,7 +40,7 @@ pub(crate) fn snapshot(hist: &Histogram) -> DistributionData { } impl MetricType for CustomDistributionMetric { - fn meta(&self) -> &CommonMetricData { + fn meta(&self) -> &CommonMetricDataInternal { &self.meta } } @@ -58,7 +59,7 @@ impl CustomDistributionMetric { histogram_type: HistogramType, ) -> Self { Self { - meta: Arc::new(meta), + meta: Arc::new(meta.into()), range_min: range_min as u64, range_max: range_max as u64, bucket_count: bucket_count as u64, @@ -173,13 +174,13 @@ impl CustomDistributionMetric { ) -> Option { let queried_ping_name = ping_name .into() - .unwrap_or_else(|| &self.meta().send_in_pings[0]); + .unwrap_or_else(|| &self.meta().inner.send_in_pings[0]); match StorageManager.snapshot_metric_for_test( glean.storage(), queried_ping_name, &self.meta.identifier(glean), - self.meta.lifetime, + self.meta.inner.lifetime, ) { // Boxing the value, in order to return either of the possible buckets Some(Metric::CustomDistributionExponential(hist)) => Some(snapshot(&hist)), @@ -206,7 +207,7 @@ impl CustomDistributionMetric { /// /// * `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`. + /// metric for. inner to the first value in `send_in_pings`. /// /// # Returns /// diff --git a/glean-core/src/metrics/datetime.rs b/glean-core/src/metrics/datetime.rs index c4d959daf..3ef846a32 100644 --- a/glean-core/src/metrics/datetime.rs +++ b/glean-core/src/metrics/datetime.rs @@ -5,6 +5,7 @@ use std::fmt; use std::sync::Arc; +use crate::common_metric_data::CommonMetricDataInternal; use crate::error_recording::{record_error, test_get_num_recorded_errors, ErrorType}; use crate::metrics::time_unit::TimeUnit; use crate::metrics::Metric; @@ -83,12 +84,12 @@ impl Default for Datetime { /// the application. #[derive(Clone, Debug)] pub struct DatetimeMetric { - meta: Arc, + meta: Arc, time_unit: TimeUnit, } impl MetricType for DatetimeMetric { - fn meta(&self) -> &CommonMetricData { + fn meta(&self) -> &CommonMetricDataInternal { &self.meta } } @@ -119,7 +120,7 @@ impl DatetimeMetric { /// Creates a new datetime metric. pub fn new(meta: CommonMetricData, time_unit: TimeUnit) -> Self { Self { - meta: Arc::new(meta), + meta: Arc::new(meta.into()), time_unit, } } @@ -240,13 +241,13 @@ impl DatetimeMetric { glean: &Glean, ping_name: Option<&str>, ) -> Option<(ChronoDatetime, TimeUnit)> { - let queried_ping_name = ping_name.unwrap_or_else(|| &self.meta().send_in_pings[0]); + let queried_ping_name = ping_name.unwrap_or_else(|| &self.meta().inner.send_in_pings[0]); match StorageManager.snapshot_metric( glean.storage(), queried_ping_name, &self.meta.identifier(glean), - self.meta.lifetime, + self.meta.inner.lifetime, ) { Some(Metric::Datetime(d, tu)) => Some((d, tu)), _ => None, @@ -311,7 +312,7 @@ impl DatetimeMetric { /// /// * `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`. + /// metric for. inner to the first value in `send_in_pings`. /// /// # Returns /// diff --git a/glean-core/src/metrics/denominator.rs b/glean-core/src/metrics/denominator.rs index d1e5455ca..fb8087492 100644 --- a/glean-core/src/metrics/denominator.rs +++ b/glean-core/src/metrics/denominator.rs @@ -2,6 +2,7 @@ // 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 crate::common_metric_data::CommonMetricDataInternal; use crate::error_recording::{record_error, test_get_num_recorded_errors, ErrorType}; use crate::metrics::CounterMetric; use crate::metrics::Metric; @@ -25,7 +26,7 @@ pub struct DenominatorMetric { } impl MetricType for DenominatorMetric { - fn meta(&self) -> &CommonMetricData { + fn meta(&self) -> &CommonMetricDataInternal { self.counter.meta() } } @@ -103,13 +104,13 @@ impl DenominatorMetric { ) -> Option { let queried_ping_name = ping_name .into() - .unwrap_or_else(|| &self.meta().send_in_pings[0]); + .unwrap_or_else(|| &self.meta().inner.send_in_pings[0]); match StorageManager.snapshot_metric_for_test( glean.storage(), queried_ping_name, &self.meta().identifier(glean), - self.meta().lifetime, + self.meta().inner.lifetime, ) { Some(Metric::Counter(i)) => Some(i), _ => None, @@ -124,7 +125,7 @@ impl DenominatorMetric { /// /// * `error` - The type of error /// * `ping_name` - the optional name of the ping to retrieve the metric - /// for. Defaults to the first value in `send_in_pings`. + /// for. inner to the first value in `send_in_pings`. /// /// # Returns /// diff --git a/glean-core/src/metrics/event.rs b/glean-core/src/metrics/event.rs index 385d8e029..66de337d1 100644 --- a/glean-core/src/metrics/event.rs +++ b/glean-core/src/metrics/event.rs @@ -4,6 +4,7 @@ use std::collections::HashMap; +use crate::common_metric_data::CommonMetricDataInternal; use crate::error_recording::{record_error, test_get_num_recorded_errors, ErrorType}; use crate::event_database::RecordedEvent; use crate::metrics::MetricType; @@ -20,12 +21,12 @@ const MAX_LENGTH_EXTRA_KEY_VALUE: usize = 500; /// records a timestamp, the event's name and a set of custom values. #[derive(Clone, Debug)] pub struct EventMetric { - meta: CommonMetricData, + meta: CommonMetricDataInternal, allowed_extra_keys: Vec, } impl MetricType for EventMetric { - fn meta(&self) -> &CommonMetricData { + fn meta(&self) -> &CommonMetricDataInternal { &self.meta } } @@ -38,7 +39,7 @@ impl EventMetric { /// Creates a new event metric. pub fn new(meta: CommonMetricData, allowed_extra_keys: Vec) -> Self { Self { - meta, + meta: meta.into(), allowed_extra_keys, } } @@ -130,7 +131,7 @@ impl EventMetric { ) -> Option> { let queried_ping_name = ping_name .into() - .unwrap_or_else(|| &self.meta().send_in_pings[0]); + .unwrap_or_else(|| &self.meta().inner.send_in_pings[0]); glean .event_storage() @@ -155,7 +156,7 @@ impl EventMetric { /// /// * `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`. + /// metric for. inner to the first value in `send_in_pings`. /// /// # Returns /// diff --git a/glean-core/src/metrics/experiment.rs b/glean-core/src/metrics/experiment.rs index b394e56cb..23e6c41ce 100644 --- a/glean-core/src/metrics/experiment.rs +++ b/glean-core/src/metrics/experiment.rs @@ -4,14 +4,15 @@ use std::cmp; use std::collections::HashMap; +use std::sync::atomic::AtomicU8; +use crate::common_metric_data::CommonMetricDataInternal; use crate::error_recording::{record_error, ErrorType}; use crate::metrics::{Metric, MetricType, RecordedExperiment}; use crate::storage::{StorageManager, INTERNAL_STORAGE}; use crate::util::{truncate_string_at_boundary, truncate_string_at_boundary_with_error}; -use crate::CommonMetricData; -use crate::Glean; use crate::Lifetime; +use crate::{CommonMetricData, Glean}; /// 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. @@ -30,11 +31,11 @@ const MAX_EXPERIMENTS_EXTRAS_SIZE: usize = 20; /// This is used through the `set_experiment_active`/`set_experiment_inactive` Glean SDK API. #[derive(Clone, Debug)] pub struct ExperimentMetric { - meta: CommonMetricData, + meta: CommonMetricDataInternal, } impl MetricType for ExperimentMetric { - fn meta(&self) -> &CommonMetricData { + fn meta(&self) -> &CommonMetricDataInternal { &self.meta } } @@ -67,13 +68,16 @@ impl ExperimentMetric { }; let new_experiment = Self { - meta: CommonMetricData { - name: format!("{}#experiment", truncated_id), - // We don't need a category, the name is already unique - category: "".into(), - send_in_pings: vec![INTERNAL_STORAGE.into()], - lifetime: Lifetime::Application, - ..Default::default() + meta: CommonMetricDataInternal { + inner: CommonMetricData { + name: format!("{}#experiment", truncated_id), + // We don't need a category, the name is already unique + category: "".into(), + send_in_pings: vec![INTERNAL_STORAGE.into()], + lifetime: Lifetime::Application, + ..Default::default() + }, + disabled: AtomicU8::new(0), }, }; @@ -179,7 +183,7 @@ impl ExperimentMetric { if let Err(e) = glean.storage().remove_single_metric( Lifetime::Application, INTERNAL_STORAGE, - &self.meta.name, + &self.meta.inner.name, ) { log::error!("Failed to set experiment as inactive: {:?}", e); } @@ -196,7 +200,7 @@ impl ExperimentMetric { glean.storage(), INTERNAL_STORAGE, &self.meta.identifier(glean), - self.meta.lifetime, + self.meta.inner.lifetime, ) { Some(Metric::Experiment(e)) => Some(e), _ => None, diff --git a/glean-core/src/metrics/labeled.rs b/glean-core/src/metrics/labeled.rs index e679d9624..6ff957c3c 100644 --- a/glean-core/src/metrics/labeled.rs +++ b/glean-core/src/metrics/labeled.rs @@ -6,7 +6,7 @@ use std::borrow::Cow; use std::collections::{hash_map::Entry, HashMap}; use std::sync::{Arc, Mutex}; -use crate::common_metric_data::CommonMetricData; +use crate::common_metric_data::{CommonMetricData, CommonMetricDataInternal}; use crate::error_recording::{record_error, test_get_num_recorded_errors, ErrorType}; use crate::metrics::{BooleanMetric, CounterMetric, Metric, MetricType, StringMetric}; use crate::Glean; @@ -245,7 +245,7 @@ where Some(_) => { let label = self.static_label(label); self.new_metric_with_name(combine_base_identifier_and_label( - &self.submetric.meta().name, + &self.submetric.meta().inner.name, label, )) } @@ -304,13 +304,13 @@ pub fn strip_label(identifier: &str) -> &str { /// The errors are logged. pub fn validate_dynamic_label( glean: &Glean, - meta: &CommonMetricData, + meta: &CommonMetricDataInternal, base_identifier: &str, label: &str, ) -> String { let key = combine_base_identifier_and_label(base_identifier, label); - for store in &meta.send_in_pings { - if glean.storage().has_metric(meta.lifetime, store, &key) { + for store in &meta.inner.send_in_pings { + if glean.storage().has_metric(meta.inner.lifetime, store, &key) { return key; } } @@ -321,8 +321,8 @@ pub fn validate_dynamic_label( label_count += 1; }; - let lifetime = meta.lifetime; - for store in &meta.send_in_pings { + let lifetime = meta.inner.lifetime; + for store in &meta.inner.send_in_pings { glean .storage() .iter_store_from(lifetime, store, Some(prefix), &mut snapshotter); diff --git a/glean-core/src/metrics/memory_distribution.rs b/glean-core/src/metrics/memory_distribution.rs index 664ba64bd..e5945bcab 100644 --- a/glean-core/src/metrics/memory_distribution.rs +++ b/glean-core/src/metrics/memory_distribution.rs @@ -4,6 +4,7 @@ use std::sync::Arc; +use crate::common_metric_data::CommonMetricDataInternal; use crate::error_recording::{record_error, test_get_num_recorded_errors, ErrorType}; use crate::histogram::{Functional, Histogram}; use crate::metrics::memory_unit::MemoryUnit; @@ -27,7 +28,7 @@ const MAX_BYTES: u64 = 1 << 40; /// Memory distributions are used to accumulate and store memory sizes. #[derive(Clone, Debug)] pub struct MemoryDistributionMetric { - meta: Arc, + meta: Arc, memory_unit: MemoryUnit, } @@ -49,7 +50,7 @@ pub(crate) fn snapshot(hist: &Histogram) -> DistributionData { } impl MetricType for MemoryDistributionMetric { - fn meta(&self) -> &CommonMetricData { + fn meta(&self) -> &CommonMetricDataInternal { &self.meta } } @@ -62,7 +63,7 @@ impl MemoryDistributionMetric { /// Creates a new memory distribution metric. pub fn new(meta: CommonMetricData, memory_unit: MemoryUnit) -> Self { Self { - meta: Arc::new(meta), + meta: Arc::new(meta.into()), memory_unit, } } @@ -224,13 +225,13 @@ impl MemoryDistributionMetric { ) -> Option { let queried_ping_name = ping_name .into() - .unwrap_or_else(|| &self.meta().send_in_pings[0]); + .unwrap_or_else(|| &self.meta().inner.send_in_pings[0]); match StorageManager.snapshot_metric_for_test( glean.storage(), queried_ping_name, &self.meta.identifier(glean), - self.meta.lifetime, + self.meta.inner.lifetime, ) { Some(Metric::MemoryDistribution(hist)) => Some(snapshot(&hist)), _ => None, diff --git a/glean-core/src/metrics/metrics_disabled_config.rs b/glean-core/src/metrics/metrics_disabled_config.rs new file mode 100644 index 000000000..02364d7b0 --- /dev/null +++ b/glean-core/src/metrics/metrics_disabled_config.rs @@ -0,0 +1,42 @@ +// 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/. + +use std::{collections::HashMap, convert::TryFrom}; + +use serde::{Deserialize, Serialize}; + +/// Represents a list of metrics and their associated `disabled` property from the +/// remote-settings configuration store. The expected format of this data is stringified +/// JSON in the following format: +/// ```json +/// { +/// "category.metric_name": true +/// } +/// ``` +#[derive(Serialize, Deserialize, Debug, Clone, Default)] +pub struct MetricsDisabledConfig { + /// This is a `HashMap` consisting of base_identifiers as keys + /// and bool values representing an override for the `disabled` + /// property of the metric + #[serde(flatten)] + pub metrics_disabled: HashMap, +} + +impl MetricsDisabledConfig { + /// Creates a new MetricsDisabledConfig + pub fn new() -> Self { + Default::default() + } +} + +impl TryFrom for MetricsDisabledConfig { + type Error = crate::ErrorKind; + + fn try_from(json: String) -> Result { + match serde_json::from_str(json.as_str()) { + Ok(config) => Ok(config), + Err(e) => Err(crate::ErrorKind::Json(e)), + } + } +} diff --git a/glean-core/src/metrics/mod.rs b/glean-core/src/metrics/mod.rs index 1e8051447..6a730eff0 100644 --- a/glean-core/src/metrics/mod.rs +++ b/glean-core/src/metrics/mod.rs @@ -5,6 +5,7 @@ //! The different metric types supported by the Glean SDK to handle data. use std::collections::HashMap; +use std::sync::atomic::Ordering; use chrono::{DateTime, FixedOffset}; use serde::{Deserialize, Serialize}; @@ -20,6 +21,7 @@ mod experiment; pub(crate) mod labeled; mod memory_distribution; mod memory_unit; +mod metrics_disabled_config; mod numerator; mod ping; mod quantity; @@ -34,11 +36,11 @@ mod timing_distribution; mod url; mod uuid; +use crate::common_metric_data::CommonMetricDataInternal; pub use crate::event_database::RecordedEvent; use crate::histogram::{Functional, Histogram, PrecomputedExponential, PrecomputedLinear}; pub use crate::metrics::datetime::Datetime; use crate::util::get_iso_time_string; -use crate::CommonMetricData; use crate::Glean; pub use self::boolean::BooleanMetric; @@ -67,6 +69,8 @@ pub use self::uuid::UuidMetric; pub use crate::histogram::HistogramType; pub use recorded_experiment::RecordedExperiment; +pub use self::metrics_disabled_config::MetricsDisabledConfig; + /// A snapshot of all buckets and the accumulated sum of a distribution. // // Note: Be careful when changing this structure. @@ -142,7 +146,7 @@ pub enum Metric { /// A [`MetricType`] describes common behavior across all metrics. pub trait MetricType { /// Access the stored metadata - fn meta(&self) -> &CommonMetricData; + fn meta(&self) -> &CommonMetricDataInternal; /// Create a new metric from this with a new name. fn with_name(&self, _name: String) -> Self @@ -165,7 +169,57 @@ pub trait MetricType { /// This depends on the metrics own state, as determined by its metadata, /// and whether upload is enabled on the Glean object. fn should_record(&self, glean: &Glean) -> bool { - glean.is_upload_enabled() && self.meta().should_record() + if !glean.is_upload_enabled() { + return false; + } + + // Technically nothing prevents multiple calls to should_record() to run in parallel, + // meaning both are reading self.meta().disabled and later writing it. In between it can + // also read remote_settings_metrics_config, which also could be modified in between those 2 reads. + // This means we could write the wrong remote_settings_epoch | current_disabled value. All in all + // at worst we would see that metric enabled/disabled wrongly once. + // But since everything is tunneled through the dispatcher, this should never ever happen. + + // Get the current disabled field from the metric metadata, including + // the encoded remote_settings epoch + let disabled_field = self.meta().disabled.load(Ordering::Relaxed); + // Grab the epoch from the upper nibble + let epoch = disabled_field >> 4; + // Get the disabled flag from the lower nibble + let disabled = disabled_field & 0xF; + // Get the current remote_settings epoch to see if we need to bother with the + // more expensive HashMap lookup + let remote_settings_epoch = glean.remote_settings_epoch.load(Ordering::Acquire); + if epoch == remote_settings_epoch { + return disabled == 0; + } + // The epoch's didn't match so we need to look up the disabled flag + // by the base_identifier from the in-memory HashMap + let metrics_disabled = &glean + .remote_settings_metrics_config + .lock() + .unwrap() + .metrics_disabled; + // Get the value from the remote configuration if it is there, otherwise return the default value. + let current_disabled = { + let base_id = self.meta().base_identifier(); + let identifier = base_id + .split_once('/') + .map(|split| split.0) + .unwrap_or(&base_id); + if let Some(is_disabled) = metrics_disabled.get(identifier) { + u8::from(*is_disabled) + } else { + u8::from(self.meta().inner.disabled) + } + }; + + // Re-encode the epoch and enabled status and update the metadata + let new_disabled = (remote_settings_epoch << 4) | (current_disabled & 0xF); + self.meta().disabled.store(new_disabled, Ordering::Relaxed); + + // Return a boolean indicating whether or not the metric should be recorded + current_disabled == 0 } } diff --git a/glean-core/src/metrics/numerator.rs b/glean-core/src/metrics/numerator.rs index f6e6dbd60..3c340cab1 100644 --- a/glean-core/src/metrics/numerator.rs +++ b/glean-core/src/metrics/numerator.rs @@ -4,6 +4,7 @@ use std::sync::Arc; +use crate::common_metric_data::CommonMetricDataInternal; use crate::error_recording::ErrorType; use crate::metrics::MetricType; use crate::metrics::Rate; @@ -20,7 +21,7 @@ use crate::Glean; pub struct NumeratorMetric(pub(crate) Arc); impl MetricType for NumeratorMetric { - fn meta(&self) -> &CommonMetricData { + fn meta(&self) -> &CommonMetricDataInternal { self.0.meta() } } diff --git a/glean-core/src/metrics/quantity.rs b/glean-core/src/metrics/quantity.rs index 81e2525b0..c59d3a4a2 100644 --- a/glean-core/src/metrics/quantity.rs +++ b/glean-core/src/metrics/quantity.rs @@ -2,6 +2,7 @@ // 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 crate::common_metric_data::CommonMetricDataInternal; use crate::error_recording::{record_error, test_get_num_recorded_errors, ErrorType}; use crate::metrics::Metric; use crate::metrics::MetricType; @@ -14,11 +15,11 @@ use crate::Glean; /// Used to store explicit non-negative integers. #[derive(Clone, Debug)] pub struct QuantityMetric { - meta: CommonMetricData, + meta: CommonMetricDataInternal, } impl MetricType for QuantityMetric { - fn meta(&self) -> &CommonMetricData { + fn meta(&self) -> &CommonMetricDataInternal { &self.meta } } @@ -30,7 +31,7 @@ impl MetricType for QuantityMetric { impl QuantityMetric { /// Creates a new quantity metric. pub fn new(meta: CommonMetricData) -> Self { - Self { meta } + Self { meta: meta.into() } } /// Sets the value. Must be non-negative. @@ -79,13 +80,13 @@ impl QuantityMetric { ) -> Option { let queried_ping_name = ping_name .into() - .unwrap_or_else(|| &self.meta().send_in_pings[0]); + .unwrap_or_else(|| &self.meta().inner.send_in_pings[0]); match StorageManager.snapshot_metric_for_test( glean.storage(), queried_ping_name, &self.meta.identifier(glean), - self.meta.lifetime, + self.meta.inner.lifetime, ) { Some(Metric::Quantity(i)) => Some(i), _ => None, diff --git a/glean-core/src/metrics/rate.rs b/glean-core/src/metrics/rate.rs index f072bbceb..ba7f085b5 100644 --- a/glean-core/src/metrics/rate.rs +++ b/glean-core/src/metrics/rate.rs @@ -2,6 +2,7 @@ // 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 crate::common_metric_data::CommonMetricDataInternal; use crate::error_recording::{record_error, test_get_num_recorded_errors, ErrorType}; use crate::metrics::Metric; use crate::metrics::MetricType; @@ -36,11 +37,11 @@ impl From<(i32, i32)> for Rate { /// Both numerator and denominator can only be incremented, not decremented. #[derive(Clone, Debug)] pub struct RateMetric { - meta: CommonMetricData, + meta: CommonMetricDataInternal, } impl MetricType for RateMetric { - fn meta(&self) -> &CommonMetricData { + fn meta(&self) -> &CommonMetricDataInternal { &self.meta } } @@ -52,7 +53,7 @@ impl MetricType for RateMetric { impl RateMetric { /// Creates a new rate metric. pub fn new(meta: CommonMetricData) -> Self { - Self { meta } + Self { meta: meta.into() } } /// Increases the numerator by `amount`. @@ -154,13 +155,13 @@ impl RateMetric { ) -> Option { let queried_ping_name = ping_name .into() - .unwrap_or_else(|| &self.meta().send_in_pings[0]); + .unwrap_or_else(|| &self.meta().inner.send_in_pings[0]); match StorageManager.snapshot_metric_for_test( glean.storage(), queried_ping_name, &self.meta.identifier(glean), - self.meta.lifetime, + self.meta.inner.lifetime, ) { Some(Metric::Rate(n, d)) => Some((n, d).into()), _ => None, diff --git a/glean-core/src/metrics/string.rs b/glean-core/src/metrics/string.rs index 2eb20fc33..7f006714d 100644 --- a/glean-core/src/metrics/string.rs +++ b/glean-core/src/metrics/string.rs @@ -4,6 +4,7 @@ use std::sync::Arc; +use crate::common_metric_data::CommonMetricDataInternal; use crate::error_recording::{test_get_num_recorded_errors, ErrorType}; use crate::metrics::Metric; use crate::metrics::MetricType; @@ -20,17 +21,17 @@ const MAX_LENGTH_VALUE: usize = 100; /// Strings are length-limited to `MAX_LENGTH_VALUE` bytes. #[derive(Clone, Debug)] pub struct StringMetric { - meta: Arc, + meta: Arc, } impl MetricType for StringMetric { - fn meta(&self) -> &CommonMetricData { + fn meta(&self) -> &CommonMetricDataInternal { &self.meta } fn with_name(&self, name: String) -> Self { let mut meta = (*self.meta).clone(); - meta.name = name; + meta.inner.name = name; Self { meta: Arc::new(meta), } @@ -38,7 +39,7 @@ impl MetricType for StringMetric { fn with_dynamic_label(&self, label: String) -> Self { let mut meta = (*self.meta).clone(); - meta.dynamic_label = Some(label); + meta.inner.dynamic_label = Some(label); Self { meta: Arc::new(meta), } @@ -53,7 +54,7 @@ impl StringMetric { /// Creates a new string metric. pub fn new(meta: CommonMetricData) -> Self { Self { - meta: Arc::new(meta), + meta: Arc::new(meta.into()), } } @@ -93,13 +94,13 @@ impl StringMetric { ) -> Option { let queried_ping_name = ping_name .into() - .unwrap_or_else(|| &self.meta().send_in_pings[0]); + .unwrap_or_else(|| &self.meta().inner.send_in_pings[0]); match StorageManager.snapshot_metric_for_test( glean.storage(), queried_ping_name, &self.meta.identifier(glean), - self.meta.lifetime, + self.meta.inner.lifetime, ) { Some(Metric::String(s)) => Some(s), _ => None, diff --git a/glean-core/src/metrics/string_list.rs b/glean-core/src/metrics/string_list.rs index 4e23ccb9c..ab8657d3a 100644 --- a/glean-core/src/metrics/string_list.rs +++ b/glean-core/src/metrics/string_list.rs @@ -4,6 +4,7 @@ use std::sync::Arc; +use crate::common_metric_data::CommonMetricDataInternal; use crate::error_recording::{record_error, test_get_num_recorded_errors, ErrorType}; use crate::metrics::Metric; use crate::metrics::MetricType; @@ -22,11 +23,11 @@ const MAX_STRING_LENGTH: usize = 50; /// This allows appending a string value with arbitrary content to a list. #[derive(Clone, Debug)] pub struct StringListMetric { - meta: Arc, + meta: Arc, } impl MetricType for StringListMetric { - fn meta(&self) -> &CommonMetricData { + fn meta(&self) -> &CommonMetricDataInternal { &self.meta } } @@ -39,7 +40,7 @@ impl StringListMetric { /// Creates a new string list metric. pub fn new(meta: CommonMetricData) -> Self { Self { - meta: Arc::new(meta), + meta: Arc::new(meta.into()), } } @@ -152,13 +153,13 @@ impl StringListMetric { ) -> Option> { let queried_ping_name = ping_name .into() - .unwrap_or_else(|| &self.meta().send_in_pings[0]); + .unwrap_or_else(|| &self.meta().inner.send_in_pings[0]); match StorageManager.snapshot_metric_for_test( glean.storage(), queried_ping_name, &self.meta.identifier(glean), - self.meta.lifetime, + self.meta.inner.lifetime, ) { Some(Metric::StringList(values)) => Some(values), _ => None, diff --git a/glean-core/src/metrics/text.rs b/glean-core/src/metrics/text.rs index c9a8f33c9..9fb90f13a 100644 --- a/glean-core/src/metrics/text.rs +++ b/glean-core/src/metrics/text.rs @@ -4,6 +4,7 @@ use std::sync::Arc; +use crate::common_metric_data::CommonMetricDataInternal; use crate::error_recording::{test_get_num_recorded_errors, ErrorType}; use crate::metrics::Metric; use crate::metrics::MetricType; @@ -22,17 +23,17 @@ const MAX_LENGTH_VALUE: usize = 200 * 1024; /// Text is length-limited to `MAX_LENGTH_VALUE` bytes. #[derive(Clone, Debug)] pub struct TextMetric { - meta: Arc, + meta: Arc, } impl MetricType for TextMetric { - fn meta(&self) -> &CommonMetricData { + fn meta(&self) -> &CommonMetricDataInternal { &self.meta } fn with_name(&self, name: String) -> Self { let mut meta = (*self.meta).clone(); - meta.name = name; + meta.inner.name = name; Self { meta: Arc::new(meta), } @@ -40,7 +41,7 @@ impl MetricType for TextMetric { fn with_dynamic_label(&self, label: String) -> Self { let mut meta = (*self.meta).clone(); - meta.dynamic_label = Some(label); + meta.inner.dynamic_label = Some(label); Self { meta: Arc::new(meta), } @@ -55,7 +56,7 @@ impl TextMetric { /// Creates a new text metric. pub fn new(meta: CommonMetricData) -> Self { Self { - meta: Arc::new(meta), + meta: Arc::new(meta.into()), } } @@ -97,13 +98,13 @@ impl TextMetric { ) -> Option { let queried_ping_name = ping_name .into() - .unwrap_or_else(|| &self.meta().send_in_pings[0]); + .unwrap_or_else(|| &self.meta().inner.send_in_pings[0]); match StorageManager.snapshot_metric_for_test( glean.storage(), queried_ping_name, &self.meta.identifier(glean), - self.meta.lifetime, + self.meta.inner.lifetime, ) { Some(Metric::Text(s)) => Some(s), _ => None, diff --git a/glean-core/src/metrics/timespan.rs b/glean-core/src/metrics/timespan.rs index 6bed0e47c..b4d3bd590 100644 --- a/glean-core/src/metrics/timespan.rs +++ b/glean-core/src/metrics/timespan.rs @@ -6,6 +6,7 @@ use std::convert::TryInto; use std::sync::{Arc, RwLock}; use std::time::Duration; +use crate::common_metric_data::CommonMetricDataInternal; use crate::error_recording::{record_error, test_get_num_recorded_errors, ErrorType}; use crate::metrics::time_unit::TimeUnit; use crate::metrics::Metric; @@ -24,13 +25,13 @@ use crate::Glean; // Cloning `CommonMetricData` is not free, as it contains strings, so we also wrap that in an Arc. #[derive(Clone, Debug)] pub struct TimespanMetric { - meta: Arc, + meta: Arc, time_unit: TimeUnit, start_time: Arc>>, } impl MetricType for TimespanMetric { - fn meta(&self) -> &CommonMetricData { + fn meta(&self) -> &CommonMetricDataInternal { &self.meta } } @@ -43,7 +44,7 @@ impl TimespanMetric { /// Creates a new timespan metric. pub fn new(meta: CommonMetricData, time_unit: TimeUnit) -> Self { Self { - meta: Arc::new(meta), + meta: Arc::new(meta.into()), time_unit, start_time: Arc::new(RwLock::new(None)), } @@ -198,7 +199,7 @@ impl TimespanMetric { /// Explicitly sets the timespan value synchronously. #[doc(hidden)] pub fn set_raw_sync(&self, glean: &Glean, elapsed: Duration) { - if !self.meta.should_record() { + if !self.should_record(glean) { return; } @@ -271,13 +272,13 @@ impl TimespanMetric { ) -> Option { let queried_ping_name = ping_name .into() - .unwrap_or_else(|| &self.meta().send_in_pings[0]); + .unwrap_or_else(|| &self.meta().inner.send_in_pings[0]); match StorageManager.snapshot_metric_for_test( glean.storage(), queried_ping_name, &self.meta.identifier(glean), - self.meta.lifetime, + self.meta.inner.lifetime, ) { Some(Metric::Timespan(time, time_unit)) => Some(time_unit.duration_convert(time)), _ => None, diff --git a/glean-core/src/metrics/timing_distribution.rs b/glean-core/src/metrics/timing_distribution.rs index 49b767b88..fdab07ac8 100644 --- a/glean-core/src/metrics/timing_distribution.rs +++ b/glean-core/src/metrics/timing_distribution.rs @@ -6,6 +6,7 @@ use std::collections::HashMap; use std::sync::atomic::{AtomicUsize, Ordering}; use std::sync::{Arc, Mutex}; +use crate::common_metric_data::CommonMetricDataInternal; use crate::error_recording::{record_error, test_get_num_recorded_errors, ErrorType}; use crate::histogram::{Functional, Histogram}; use crate::metrics::time_unit::TimeUnit; @@ -56,7 +57,7 @@ impl From for TimerId { /// Timing distributions are used to accumulate and store time measurement, for analyzing distributions of the timing data. #[derive(Clone, Debug)] pub struct TimingDistributionMetric { - meta: Arc, + meta: Arc, time_unit: TimeUnit, next_id: Arc, start_times: Arc>>, @@ -80,7 +81,7 @@ pub(crate) fn snapshot(hist: &Histogram) -> DistributionData { } impl MetricType for TimingDistributionMetric { - fn meta(&self) -> &CommonMetricData { + fn meta(&self) -> &CommonMetricDataInternal { &self.meta } } @@ -93,7 +94,7 @@ impl TimingDistributionMetric { /// Creates a new timing distribution metric. pub fn new(meta: CommonMetricData, time_unit: TimeUnit) -> Self { Self { - meta: Arc::new(meta), + meta: Arc::new(meta.into()), time_unit, next_id: Arc::new(AtomicUsize::new(0)), start_times: Arc::new(Mutex::new(Default::default())), @@ -426,13 +427,13 @@ impl TimingDistributionMetric { ) -> Option { let queried_ping_name = ping_name .into() - .unwrap_or_else(|| &self.meta().send_in_pings[0]); + .unwrap_or_else(|| &self.meta().inner.send_in_pings[0]); match StorageManager.snapshot_metric_for_test( glean.storage(), queried_ping_name, &self.meta.identifier(glean), - self.meta.lifetime, + self.meta.inner.lifetime, ) { Some(Metric::TimingDistribution(hist)) => Some(snapshot(&hist)), _ => None, diff --git a/glean-core/src/metrics/url.rs b/glean-core/src/metrics/url.rs index 698ea34f9..332286f62 100644 --- a/glean-core/src/metrics/url.rs +++ b/glean-core/src/metrics/url.rs @@ -4,6 +4,7 @@ use std::sync::Arc; +use crate::common_metric_data::CommonMetricDataInternal; use crate::error_recording::{record_error, test_get_num_recorded_errors, ErrorType}; use crate::metrics::Metric; use crate::metrics::MetricType; @@ -21,11 +22,11 @@ const MAX_URL_LENGTH: usize = 8192; /// The URL is length-limited to `MAX_URL_LENGTH` bytes. #[derive(Clone, Debug)] pub struct UrlMetric { - meta: Arc, + meta: Arc, } impl MetricType for UrlMetric { - fn meta(&self) -> &CommonMetricData { + fn meta(&self) -> &CommonMetricDataInternal { &self.meta } } @@ -38,7 +39,7 @@ impl UrlMetric { /// Creates a new string metric. pub fn new(meta: CommonMetricData) -> Self { Self { - meta: Arc::new(meta), + meta: Arc::new(meta.into()), } } @@ -112,13 +113,13 @@ impl UrlMetric { ) -> Option { let queried_ping_name = ping_name .into() - .unwrap_or_else(|| &self.meta().send_in_pings[0]); + .unwrap_or_else(|| &self.meta().inner.send_in_pings[0]); match StorageManager.snapshot_metric_for_test( glean.storage(), queried_ping_name, &self.meta.identifier(glean), - self.meta.lifetime, + self.meta.inner.lifetime, ) { Some(Metric::Url(s)) => Some(s), _ => None, diff --git a/glean-core/src/metrics/uuid.rs b/glean-core/src/metrics/uuid.rs index 219f93566..e78d15ad3 100644 --- a/glean-core/src/metrics/uuid.rs +++ b/glean-core/src/metrics/uuid.rs @@ -6,6 +6,7 @@ use std::sync::Arc; use uuid::Uuid; +use crate::common_metric_data::CommonMetricDataInternal; use crate::error_recording::{record_error, test_get_num_recorded_errors, ErrorType}; use crate::metrics::Metric; use crate::metrics::MetricType; @@ -18,11 +19,11 @@ use crate::Glean; /// Stores UUID v4 (randomly generated) values. #[derive(Clone, Debug)] pub struct UuidMetric { - meta: Arc, + meta: Arc, } impl MetricType for UuidMetric { - fn meta(&self) -> &CommonMetricData { + fn meta(&self) -> &CommonMetricDataInternal { &self.meta } } @@ -35,7 +36,7 @@ impl UuidMetric { /// Creates a new UUID metric pub fn new(meta: CommonMetricData) -> Self { Self { - meta: Arc::new(meta), + meta: Arc::new(meta.into()), } } @@ -109,13 +110,13 @@ impl UuidMetric { ) -> Option { let queried_ping_name = ping_name .into() - .unwrap_or_else(|| &self.meta().send_in_pings[0]); + .unwrap_or_else(|| &self.meta().inner.send_in_pings[0]); match StorageManager.snapshot_metric_for_test( glean.storage(), queried_ping_name, &self.meta.identifier(glean), - self.meta.lifetime, + self.meta.inner.lifetime, ) { Some(Metric::Uuid(uuid)) => Uuid::parse_str(&uuid).ok(), _ => None, diff --git a/glean-core/src/ping/mod.rs b/glean-core/src/ping/mod.rs index 903b2b56f..fef664f0e 100644 --- a/glean-core/src/ping/mod.rs +++ b/glean-core/src/ping/mod.rs @@ -76,7 +76,7 @@ impl PingMaker { glean.storage(), INTERNAL_STORAGE, &seq.meta().identifier(glean), - seq.meta().lifetime, + seq.meta().inner.lifetime, ) { Some(Metric::Counter(i)) => i, _ => 0, diff --git a/glean-core/src/util.rs b/glean-core/src/util.rs index 47bfbe87b..52cc5d57c 100644 --- a/glean-core/src/util.rs +++ b/glean-core/src/util.rs @@ -4,9 +4,9 @@ use chrono::{DateTime, FixedOffset, Local}; +use crate::common_metric_data::CommonMetricDataInternal; use crate::error_recording::{record_error, ErrorType}; use crate::metrics::TimeUnit; -use crate::CommonMetricData; use crate::Glean; /// Generates a pipeline-friendly string @@ -136,7 +136,7 @@ pub(crate) fn truncate_string_at_boundary>(value: S, length: usi /// A string, with at most `length` bytes. pub(crate) fn truncate_string_at_boundary_with_error>( glean: &Glean, - meta: &CommonMetricData, + meta: &CommonMetricDataInternal, value: S, length: usize, ) -> String { diff --git a/glean-core/tests/event.rs b/glean-core/tests/event.rs index e038cc24f..06ecae210 100644 --- a/glean-core/tests/event.rs +++ b/glean-core/tests/event.rs @@ -324,16 +324,29 @@ fn ensure_custom_ping_events_dont_overflow() { }; let event = EventMetric::new(event_meta.clone(), vec![]); - assert!(test_get_num_recorded_errors(&glean, &event_meta, ErrorType::InvalidOverflow).is_err()); + assert!(test_get_num_recorded_errors( + &glean, + &(event_meta.clone()).into(), + ErrorType::InvalidOverflow + ) + .is_err()); for _ in 0..500 { event.record_sync(&glean, 0, HashMap::new()); } - assert!(test_get_num_recorded_errors(&glean, &event_meta, ErrorType::InvalidOverflow).is_err()); + assert!(test_get_num_recorded_errors( + &glean, + &(event_meta.clone()).into(), + ErrorType::InvalidOverflow + ) + .is_err()); // That should top us right up to the limit. Now for going over. event.record_sync(&glean, 0, HashMap::new()); - assert!(test_get_num_recorded_errors(&glean, &event_meta, ErrorType::InvalidOverflow).is_err()); + assert!( + test_get_num_recorded_errors(&glean, &event_meta.into(), ErrorType::InvalidOverflow) + .is_err() + ); assert_eq!(501, event.get_value(&glean, store_name).unwrap().len()); }