From a1a04ade0b7f20b3c6902a0ee06fda79f381584d Mon Sep 17 00:00:00 2001 From: Jan-Erik Rediger Date: Fri, 22 Nov 2024 14:00:21 +0100 Subject: [PATCH 01/18] Refactoring: Define utility test function globally --- glean-core/python/tests/conftest.py | 33 +++++++++++++++++++++ glean-core/python/tests/test_glean.py | 41 ++++++--------------------- 2 files changed, 41 insertions(+), 33 deletions(-) diff --git a/glean-core/python/tests/conftest.py b/glean-core/python/tests/conftest.py index 9d1429d4a3..e05747517b 100644 --- a/glean-core/python/tests/conftest.py +++ b/glean-core/python/tests/conftest.py @@ -3,7 +3,9 @@ # file, You can obtain one at http://mozilla.org/MPL/2.0/. +import json import logging +import os import socket import time from pathlib import Path @@ -110,6 +112,37 @@ def wait_for_server(httpserver, timeout=30): raise TimeoutError() +class Helpers: + @staticmethod + def wait_for_ping(path, timeout=2) -> (str, str): + """ + Wait for a ping to appear in `path` for at most `timeout` seconds. + + Raises a `TimeoutError` if the file doesn't exist within the timeout. + + Returns a tuple of (url path, payload). + """ + + start_time = time.time() + while not path.exists(): + time.sleep(0.1) + if time.time() - start_time > timeout: + raise TimeoutError(f"No ping appeared in {path} within {timeout} seconds") + + with path.open("r") as fd: + url_path = fd.readline() + serialized_ping = fd.readline() + payload = json.loads(serialized_ping) + + os.remove(path) + return (url_path, payload) + + +@pytest.fixture +def helpers(): + return Helpers + + # Setup a default webserver that pings will go to by default, so we don't hit # the real telemetry endpoint from unit tests. Some tests that care about the # pings actually being sent may still set up their own webservers using the diff --git a/glean-core/python/tests/test_glean.py b/glean-core/python/tests/test_glean.py index 05b66ed061..0f7affeede 100644 --- a/glean-core/python/tests/test_glean.py +++ b/glean-core/python/tests/test_glean.py @@ -4,7 +4,6 @@ import io -import os import json from pathlib import Path import re @@ -54,30 +53,6 @@ def wait_for_requests(server, n=1, timeout=2): ) -def wait_for_ping(path, timeout=2) -> (str, str): - """ - Wait for a ping to appear in `path` for at most `timeout` seconds. - - Raises a `TimeoutError` if the file doesn't exist within the timeout. - - Returns a tuple of (url path, payload). - """ - - start_time = time.time() - while not path.exists(): - time.sleep(0.1) - if time.time() - start_time > timeout: - raise TimeoutError(f"No ping appeared in {path} within {timeout} seconds") - - with path.open("r") as fd: - url_path = fd.readline() - serialized_ping = fd.readline() - payload = json.loads(serialized_ping) - - os.remove(path) - return (url_path, payload) - - def test_setting_upload_enabled_before_initialization_should_not_crash(): Glean._reset() Glean.set_upload_enabled(True) @@ -817,7 +792,7 @@ def test_app_display_version_unknown(): @pytest.mark.skipif(sys.platform == "win32", reason="bug 1771157: Windows failures") -def test_flipping_upload_enabled_respects_order_of_events(tmpdir, monkeypatch): +def test_flipping_upload_enabled_respects_order_of_events(tmpdir, monkeypatch, helpers): # This test relies on testing mode to be disabled, since we need to prove the # real-world async behaviour of this. Glean._reset() @@ -849,7 +824,7 @@ def test_flipping_upload_enabled_respects_order_of_events(tmpdir, monkeypatch): # Submit a custom ping. ping.submit() - url_path, payload = wait_for_ping(info_path) + url_path, payload = helpers.wait_for_ping(info_path) # Validate we got the deletion-request ping assert "deletion-request" == url_path.split("/")[3] @@ -868,7 +843,7 @@ def test_data_dir_is_required(): @pytest.mark.skipif(sys.platform == "win32", reason="bug 1771157: Windows failures") -def test_client_activity_api(tmpdir, monkeypatch): +def test_client_activity_api(tmpdir, monkeypatch, helpers): Glean._reset() info_path = Path(str(tmpdir)) / "info.txt" @@ -891,7 +866,7 @@ def test_client_activity_api(tmpdir, monkeypatch): # Making it active Glean.handle_client_active() - url_path, payload = wait_for_ping(info_path) + url_path, payload = helpers.wait_for_ping(info_path) assert "baseline" == url_path.split("/")[3] assert payload["ping_info"]["reason"] == "active" # It's an empty ping. @@ -903,7 +878,7 @@ def test_client_activity_api(tmpdir, monkeypatch): # Making it inactive Glean.handle_client_inactive() - url_path, payload = wait_for_ping(info_path) + url_path, payload = helpers.wait_for_ping(info_path) assert "baseline" == url_path.split("/")[3] assert payload["ping_info"]["reason"] == "inactive" assert "glean.baseline.duration" in payload["metrics"]["timespan"] @@ -914,7 +889,7 @@ def test_client_activity_api(tmpdir, monkeypatch): # Once more active Glean.handle_client_active() - url_path, payload = wait_for_ping(info_path) + url_path, payload = helpers.wait_for_ping(info_path) assert "baseline" == url_path.split("/")[3] assert payload["ping_info"]["reason"] == "active" assert "timespan" not in payload["metrics"] @@ -964,7 +939,7 @@ def check_custom_ping(reason): @pytest.mark.skipif(sys.platform == "win32", reason="uploader isn't started fast enough") -def test_max_events_overflow(tmpdir): +def test_max_events_overflow(tmpdir, helpers): info_path = Path(str(tmpdir)) / "info.txt" data_dir = Path(str(tmpdir)) / "glean" @@ -995,7 +970,7 @@ def test_max_events_overflow(tmpdir): # Records the event and triggers the ping due to max_events=1 event.record() - url_path, payload = wait_for_ping(info_path) + url_path, payload = helpers.wait_for_ping(info_path) assert "events" == url_path.split("/")[3] events = payload["events"] From e56b68574167b615ad68a102a7932586e6b425ee Mon Sep 17 00:00:00 2001 From: Jan-Erik Rediger Date: Thu, 31 Oct 2024 13:38:56 +0100 Subject: [PATCH 02/18] Check if a ping is enabled before recording a metric for it --- .../glean/private/EventMetricTypeTest.kt | 4 +- glean-core/src/core/mod.rs | 30 +++++++++++ glean-core/src/database/mod.rs | 50 +++++++++---------- glean-core/src/event_database/mod.rs | 4 ++ glean-core/src/lib_unit_tests.rs | 1 + glean-core/src/metrics/mod.rs | 4 -- glean-core/tests/event.rs | 42 +++++++++------- 7 files changed, 85 insertions(+), 50 deletions(-) diff --git a/glean-core/android/src/test/java/mozilla/telemetry/glean/private/EventMetricTypeTest.kt b/glean-core/android/src/test/java/mozilla/telemetry/glean/private/EventMetricTypeTest.kt index 8aeea17db4..1ae3eecaa7 100644 --- a/glean-core/android/src/test/java/mozilla/telemetry/glean/private/EventMetricTypeTest.kt +++ b/glean-core/android/src/test/java/mozilla/telemetry/glean/private/EventMetricTypeTest.kt @@ -455,10 +455,10 @@ class EventMetricTypeTest { } @Test - fun `overdue events are discarded if ping is not registered`() { + fun `events are discarded if ping is not registered`() { // This is similar to the above test, // except that we register the custom ping AFTER initialize. - // Overdue events are thus discarded because the ping is unknown at initialization time. + // Events are thus discarded upon `record` because the ping is unknown. val server = getMockWebServer() val context = getContext() diff --git a/glean-core/src/core/mod.rs b/glean-core/src/core/mod.rs index adcf9899fd..b355a60eb3 100644 --- a/glean-core/src/core/mod.rs +++ b/glean-core/src/core/mod.rs @@ -474,6 +474,36 @@ impl Glean { self.upload_enabled } + /// Check if a ping is enabled. + /// + /// Note that some internal "ping" names are considered to be always enabled. + /// + /// If a ping is not known to Glean ("unregistered") it is always considered disabled. + /// If a ping is known, it can be enabled/disabled at any point. + /// Only data for enabled pings is recorded. + /// Disabled pings are never submitted. + pub fn is_ping_enabled(&self, ping: &str) -> bool { + // We "abuse" pings/storage names for internal data. + const DEFAULT_ENABLED: &[&str] = &[ + "glean_client_info", + "glean_internal_info", + // for `experimentation_id`. + // That should probably have gone into `glean_internal_info` instead. + "all-pings", + ]; + + // `client_info`-like stuff is always enabled. + if DEFAULT_ENABLED.contains(&ping) { + return true; + } + + let Some(ping) = self.ping_registry.get(ping) else { + return false; + }; + + ping.enabled(self) + } + /// Handles the changing of state from upload disabled to enabled. /// /// Should only be called when the state actually changes. diff --git a/glean-core/src/database/mod.rs b/glean-core/src/database/mod.rs index 398137db3f..45fac4b0a5 100644 --- a/glean-core/src/database/mod.rs +++ b/glean-core/src/database/mod.rs @@ -448,21 +448,19 @@ impl Database { /// Records a metric in the underlying storage system. 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; - } - let name = data.identifier(glean); - for ping_name in data.storage_names() { - if let Err(e) = self.record_per_lifetime(data.inner.lifetime, ping_name, &name, value) { - log::error!( - "Failed to record metric '{}' into {}: {:?}", - data.base_identifier(), - ping_name, - e - ); + if glean.is_ping_enabled(ping_name) { + if let Err(e) = + self.record_per_lifetime(data.inner.lifetime, ping_name, &name, value) + { + log::error!( + "Failed to record metric '{}' into {}: {:?}", + data.base_identifier(), + ping_name, + e + ); + } } } } @@ -518,22 +516,22 @@ impl Database { where F: FnMut(Option) -> Metric, { - // If upload is disabled we don't want to record. - if !glean.is_upload_enabled() { - return; - } - let name = data.identifier(glean); for ping_name in data.storage_names() { - if let Err(e) = - self.record_per_lifetime_with(data.inner.lifetime, ping_name, &name, &mut transform) - { - log::error!( - "Failed to record metric '{}' into {}: {:?}", - data.base_identifier(), + if glean.is_ping_enabled(ping_name) { + if let Err(e) = self.record_per_lifetime_with( + data.inner.lifetime, ping_name, - e - ); + &name, + &mut transform, + ) { + log::error!( + "Failed to record metric '{}' into {}: {:?}", + data.base_identifier(), + ping_name, + e + ); + } } } } diff --git a/glean-core/src/event_database/mod.rs b/glean-core/src/event_database/mod.rs index 34eab80108..3c88c0c619 100644 --- a/glean-core/src/event_database/mod.rs +++ b/glean-core/src/event_database/mod.rs @@ -284,6 +284,10 @@ impl EventDatabase { { let mut db = self.event_stores.write().unwrap(); // safe unwrap, only error case is poisoning for store_name in meta.inner.send_in_pings.iter() { + if !glean.is_ping_enabled(store_name) { + continue; + } + let store = db.entry(store_name.to_string()).or_default(); let execution_counter = CounterMetric::new(CommonMetricData { name: "execution_counter".into(), diff --git a/glean-core/src/lib_unit_tests.rs b/glean-core/src/lib_unit_tests.rs index eff5184c29..2b844c646f 100644 --- a/glean-core/src/lib_unit_tests.rs +++ b/glean-core/src/lib_unit_tests.rs @@ -626,6 +626,7 @@ fn test_first_run() { #[test] fn test_dirty_bit() { + let _ = env_logger::builder().try_init(); let dir = tempfile::tempdir().unwrap(); let tmpname = dir.path().display().to_string(); { diff --git a/glean-core/src/metrics/mod.rs b/glean-core/src/metrics/mod.rs index 741fe1fbc5..478fdd543c 100644 --- a/glean-core/src/metrics/mod.rs +++ b/glean-core/src/metrics/mod.rs @@ -178,10 +178,6 @@ 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 { - 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_config, which also could be modified in between those 2 reads. diff --git a/glean-core/tests/event.rs b/glean-core/tests/event.rs index 075751f218..4d411d82c0 100644 --- a/glean-core/tests/event.rs +++ b/glean-core/tests/event.rs @@ -10,10 +10,10 @@ use std::fs; use serde_json::json; -use glean_core::metrics::*; use glean_core::{ get_timestamp_ms, test_get_num_recorded_errors, CommonMetricData, ErrorType, Lifetime, }; +use glean_core::{metrics::*, Glean}; #[test] fn record_properly_records_without_optional_arguments() { @@ -507,27 +507,12 @@ fn event_storage_trimming() { }, vec![], ); - // First, record the event in the two pings. - // Successfully records just fine because nothing's checking on record that these pings - // exist and are registered. - { - let (glean, dir) = new_glean(Some(tempdir)); - tempdir = dir; - event.record_sync(&glean, 10, HashMap::new(), 0); - assert_eq!(1, event.get_value(&glean, store_name).unwrap().len()); - assert_eq!(1, event.get_value(&glean, store_name_2).unwrap().len()); - } - // Second, construct (but don't init) Glean over again. - // Register exactly one of the two pings. - // Then process the part of init that does the trimming (`on_ready_to_submit_pings`). - // This ought to load the data from the registered ping and trim the data from the unregistered one. - { - let (mut glean, _dir) = new_glean(Some(tempdir)); + let new_ping = |glean: &mut Glean, ping: &str| { // In Rust, pings are registered via construction. // But that's done asynchronously, so we do it synchronously here: glean.register_ping_type(&PingType::new( - store_name.to_string(), + ping.to_string(), true, false, true, @@ -536,6 +521,27 @@ fn event_storage_trimming() { vec![], vec![], )); + }; + + // First, register both pings, so that we can record the event in the two pings. + { + let (mut glean, dir) = new_glean(Some(tempdir)); + tempdir = dir; + + new_ping(&mut glean, store_name); + new_ping(&mut glean, store_name_2); + + event.record_sync(&glean, 10, HashMap::new(), 0); + + assert_eq!(1, event.get_value(&glean, store_name).unwrap().len()); + assert_eq!(1, event.get_value(&glean, store_name_2).unwrap().len()); + } + // Second, construct (but don't init) Glean again. + // Register exactly one of the two pings. + // Then process the part of init that does the trimming (`on_ready_to_submit_pings`). + { + let (mut glean, _dir) = new_glean(Some(tempdir)); + new_ping(&mut glean, store_name); glean.on_ready_to_submit_pings(true); From 5dfda6bbf5b29bb932a752057823da806d1ddc2f Mon Sep 17 00:00:00 2001 From: Jan-Erik Rediger Date: Fri, 1 Nov 2024 16:53:57 +0100 Subject: [PATCH 03/18] Add the new `set_collection_enabled` API This effectively deprecates `set_upload_enabled`. --- .../java/mozilla/telemetry/glean/Glean.kt | 26 ++++++++++++++++++- .../java/mozilla/telemetry/glean/GleanTest.kt | 18 ++++++------- .../telemetry/glean/pings/DeletionPingTest.kt | 6 ++--- .../glean/private/EventMetricTypeTest.kt | 6 ++--- .../glean/private/ObjectMetricTypeTest.kt | 4 +-- glean-core/ios/Glean/Glean.swift | 23 +++++++++++++++- glean-core/ios/GleanTests/GleanTests.swift | 2 +- .../GleanTests/Metrics/EventMetricTests.swift | 6 ++--- .../Net/DeletionRequestPingTests.swift | 4 +-- glean-core/python/glean/glean.py | 26 ++++++++++++++++++- glean-core/rlb/src/lib.rs | 12 ++++++++- glean-core/src/lib.rs | 12 ++++++++- .../mozilla/samples/gleancore/MainActivity.kt | 4 +-- .../app/glean-sample-app/ViewController.swift | 2 +- 14 files changed, 120 insertions(+), 31 deletions(-) diff --git a/glean-core/android/src/main/java/mozilla/telemetry/glean/Glean.kt b/glean-core/android/src/main/java/mozilla/telemetry/glean/Glean.kt index 1ff3ff20b5..325d759efe 100644 --- a/glean-core/android/src/main/java/mozilla/telemetry/glean/Glean.kt +++ b/glean-core/android/src/main/java/mozilla/telemetry/glean/Glean.kt @@ -289,7 +289,7 @@ open class GleanInternalAPI internal constructor() { } /** - * Enable or disable Glean collection and upload. + * **DEPRECATED** Enable or disable Glean collection and upload. * * Metric collection is enabled by default. * @@ -300,12 +300,36 @@ open class GleanInternalAPI internal constructor() { * * When enabling, the core Glean metrics are recreated. * + * **DEPRECATION NOTICE**: + * This API is deprecated. Use `setCollectionEnabled` instead. + * * @param enabled When true, enable metric collection. */ + @Deprecated("Use `setCollectionEnabled` instead.") fun setUploadEnabled(enabled: Boolean) { gleanSetUploadEnabled(enabled) } + /** + * Enable or disable Glean collection and upload. + * + * Metric collection is enabled by default. + * + * When collection is disabled, metrics aren't recorded at all and no data + * is uploaded. + * **Note**: Individual pings can be enabled if they don't follow this setting. + * See `PingType.setEnabled`. + * + * When disabling, all pending metrics, events and queued pings are cleared. + * + * When enabling, the core Glean metrics are recreated. + * + * @param enabled When true, enable metric collection. + */ + fun setCollectionEnabled(enabled: Boolean) { + gleanSetUploadEnabled(enabled) + } + /** * Indicate that an experiment is running. Glean will then add an * experiment annotation to the environment which is sent with pings. This diff --git a/glean-core/android/src/test/java/mozilla/telemetry/glean/GleanTest.kt b/glean-core/android/src/test/java/mozilla/telemetry/glean/GleanTest.kt index a787a083e7..9dffe2537d 100644 --- a/glean-core/android/src/test/java/mozilla/telemetry/glean/GleanTest.kt +++ b/glean-core/android/src/test/java/mozilla/telemetry/glean/GleanTest.kt @@ -62,7 +62,7 @@ class GleanTest { @After fun resetGlobalState() { - Glean.setUploadEnabled(true) + Glean.setCollectionEnabled(true) } @Test @@ -121,7 +121,7 @@ class GleanTest { sendInPings = listOf("store1"), ), ) - Glean.setUploadEnabled(false) + Glean.setCollectionEnabled(false) stringMetric.set("foo") assertNull(stringMetric.testGetValue()) @@ -542,12 +542,12 @@ class GleanTest { stringMetric.set("TEST VALUE") assertEquals("TEST VALUE", stringMetric.testGetValue()!!) - Glean.setUploadEnabled(false) + Glean.setCollectionEnabled(false) assertNull(stringMetric.testGetValue()) stringMetric.set("TEST VALUE") assertNull(stringMetric.testGetValue()) - Glean.setUploadEnabled(true) + Glean.setCollectionEnabled(true) assertNull(stringMetric.testGetValue()) stringMetric.set("TEST VALUE") assertEquals("TEST VALUE", stringMetric.testGetValue()!!) @@ -557,10 +557,10 @@ class GleanTest { fun `Core metrics should be cleared and restored when disabling and enabling uploading`() { assertNotNull(GleanInternalMetrics.os.testGetValue()) - Glean.setUploadEnabled(false) + Glean.setCollectionEnabled(false) assertNull(GleanInternalMetrics.os.testGetValue()) - Glean.setUploadEnabled(true) + Glean.setCollectionEnabled(true) assertNotNull(GleanInternalMetrics.os.testGetValue()) } @@ -585,7 +585,7 @@ class GleanTest { Glean.metricsPingScheduler!!.timer != null, ) - Glean.setUploadEnabled(true) + Glean.setCollectionEnabled(true) // Verify that the workers are still enqueued to show that setting upload enabled to true // doesn't affect any already queued workers, since we ask consumers to set upload enabled @@ -600,7 +600,7 @@ class GleanTest { ) // Toggle upload enabled to false - Glean.setUploadEnabled(false) + Glean.setCollectionEnabled(false) // Verify workers have been cancelled assertTrue( @@ -871,7 +871,7 @@ class GleanTest { Glean.initialize(context, true, config, GleanBuildInfo.buildInfo) // Glean might still be initializing. Disable upload. - Glean.setUploadEnabled(false) + Glean.setCollectionEnabled(false) // Set data and try to submit a custom ping. val testValue = "SomeTestValue" diff --git a/glean-core/android/src/test/java/mozilla/telemetry/glean/pings/DeletionPingTest.kt b/glean-core/android/src/test/java/mozilla/telemetry/glean/pings/DeletionPingTest.kt index 635a8ca4f5..f27e30e59e 100644 --- a/glean-core/android/src/test/java/mozilla/telemetry/glean/pings/DeletionPingTest.kt +++ b/glean-core/android/src/test/java/mozilla/telemetry/glean/pings/DeletionPingTest.kt @@ -95,7 +95,7 @@ class DeletionPingTest { val pendingDeletionRequestDir = File(Glean.getDataDir(), DELETION_PING_DIR) // Disabling upload generates a deletion ping - Glean.setUploadEnabled(false) + Glean.setCollectionEnabled(false) triggerWorkManager(context) val request = server.takeRequest(2L, TimeUnit.SECONDS)!! @@ -107,7 +107,7 @@ class DeletionPingTest { // Re-setting upload to `false` should not generate an additional ping // and no worker should be scheduled. - Glean.setUploadEnabled(false) + Glean.setCollectionEnabled(false) assertFalse(getWorkerStatus(context, PingUploadWorker.PING_WORKER_TAG).isEnqueued) // No new file should have been written @@ -182,7 +182,7 @@ class DeletionPingTest { ) // Disabling upload generates a deletion ping - Glean.setUploadEnabled(false) + Glean.setCollectionEnabled(false) triggerWorkManager(context) val request = server.takeRequest(2L, TimeUnit.SECONDS)!! diff --git a/glean-core/android/src/test/java/mozilla/telemetry/glean/private/EventMetricTypeTest.kt b/glean-core/android/src/test/java/mozilla/telemetry/glean/private/EventMetricTypeTest.kt index 1ae3eecaa7..e83a10182f 100644 --- a/glean-core/android/src/test/java/mozilla/telemetry/glean/private/EventMetricTypeTest.kt +++ b/glean-core/android/src/test/java/mozilla/telemetry/glean/private/EventMetricTypeTest.kt @@ -258,14 +258,14 @@ class EventMetricTypeTest { ), allowedExtraKeys = listOf("test_name"), ) - Glean.setUploadEnabled(true) + Glean.setCollectionEnabled(true) eventMetric.record(EventMetricExtras(testName = "event1")) val snapshot1 = eventMetric.testGetValue()!! assertEquals(1, snapshot1.size) - Glean.setUploadEnabled(false) + Glean.setCollectionEnabled(false) eventMetric.record(EventMetricExtras(testName = "event2")) assertNull(eventMetric.testGetValue()) - Glean.setUploadEnabled(true) + Glean.setCollectionEnabled(true) eventMetric.record(EventMetricExtras(testName = "event3")) val snapshot3 = eventMetric.testGetValue()!! assertEquals(1, snapshot3.size) diff --git a/glean-core/android/src/test/java/mozilla/telemetry/glean/private/ObjectMetricTypeTest.kt b/glean-core/android/src/test/java/mozilla/telemetry/glean/private/ObjectMetricTypeTest.kt index 06caf7cc4b..3a6a6f23d3 100644 --- a/glean-core/android/src/test/java/mozilla/telemetry/glean/private/ObjectMetricTypeTest.kt +++ b/glean-core/android/src/test/java/mozilla/telemetry/glean/private/ObjectMetricTypeTest.kt @@ -163,7 +163,7 @@ class ObjectMetricTypeTest { ), ) - Glean.setUploadEnabled(true) + Glean.setCollectionEnabled(true) var balloons = BalloonsObject() balloons.add(BalloonsObjectItem(colour = "yellow", diameter = 10)) @@ -177,7 +177,7 @@ class ObjectMetricTypeTest { var expected: JsonElement = Json.decodeFromString(expectedJson) assertEquals(expected, metric.testGetValue()!!) - Glean.setUploadEnabled(false) + Glean.setCollectionEnabled(false) balloons = BalloonsObject() balloons.add(BalloonsObjectItem(colour = "blue", diameter = 1)) metric.set(balloons) diff --git a/glean-core/ios/Glean/Glean.swift b/glean-core/ios/Glean/Glean.swift index 9739a7517e..83a701d336 100644 --- a/glean-core/ios/Glean/Glean.swift +++ b/glean-core/ios/Glean/Glean.swift @@ -220,7 +220,7 @@ public class Glean { ) } - /// Enable or disable Glean collection and upload. + /// **DEPRECATED** Enable or disable Glean collection and upload. /// /// Metric collection is enabled by default. /// @@ -231,12 +231,33 @@ public class Glean { /// /// When enabling, the core Glean metrics are recreated. /// + /// **DEPRECATION NOTICE**: + /// This API is deprecated. Use `setCollectionEnabled` instead. + /// /// - parameters: /// * enabled: When true, enable metric collection. + @available(*, deprecated, message: "Use `setCollectionEnabled` instead.") public func setUploadEnabled(_ enabled: Bool) { gleanSetUploadEnabled(enabled) } + /// Enable or disable Glean collection and upload. + /// + /// Metric collection is enabled by default. + /// + /// When uploading is disabled, metrics aren't recorded at all and no data + /// is uploaded. + /// + /// When disabling, all pending metrics, events and queued pings are cleared. + /// + /// When enabling, the core Glean metrics are recreated. + /// + /// - parameters: + /// * enabled: When true, enable metric collection. + public func setCollectionEnabled(_ enabled: Bool) { + gleanSetUploadEnabled(enabled) + } + /// Used to indicate that an experiment is running. /// /// Glean will add an experiment annotation that is sent with pings. This information is _not_ diff --git a/glean-core/ios/GleanTests/GleanTests.swift b/glean-core/ios/GleanTests/GleanTests.swift index 812e92ea6e..fc08970d1a 100644 --- a/glean-core/ios/GleanTests/GleanTests.swift +++ b/glean-core/ios/GleanTests/GleanTests.swift @@ -283,7 +283,7 @@ class GleanTests: XCTestCase { Glean.shared.resetGlean(clearStores: false) // Glean might still be initializing. Disable upload. - Glean.shared.setUploadEnabled(false) + Glean.shared.setCollectionEnabled(false) // Set data and try to submit a custom ping. counter.add(1) diff --git a/glean-core/ios/GleanTests/Metrics/EventMetricTests.swift b/glean-core/ios/GleanTests/Metrics/EventMetricTests.swift index 895d63c51c..ae9e61d1f8 100644 --- a/glean-core/ios/GleanTests/Metrics/EventMetricTests.swift +++ b/glean-core/ios/GleanTests/Metrics/EventMetricTests.swift @@ -240,16 +240,16 @@ class EventMetricTypeTests: XCTestCase { disabled: false ), ["test_name"]) - Glean.shared.setUploadEnabled(true) + Glean.shared.setCollectionEnabled(true) metric.record(TestExtras(testName: "event1")) let snapshot1 = metric.testGetValue()! XCTAssertEqual(1, snapshot1.count) - Glean.shared.setUploadEnabled(false) + Glean.shared.setCollectionEnabled(false) metric.record(TestExtras(testName: "event2")) XCTAssertNil(metric.testGetValue()) - Glean.shared.setUploadEnabled(true) + Glean.shared.setCollectionEnabled(true) metric.record(TestExtras(testName: "event3")) let snapshot3 = metric.testGetValue()! XCTAssertEqual(1, snapshot3.count) diff --git a/glean-core/ios/GleanTests/Net/DeletionRequestPingTests.swift b/glean-core/ios/GleanTests/Net/DeletionRequestPingTests.swift index 6f3eedf6ad..43857fb7ac 100644 --- a/glean-core/ios/GleanTests/Net/DeletionRequestPingTests.swift +++ b/glean-core/ios/GleanTests/Net/DeletionRequestPingTests.swift @@ -94,7 +94,7 @@ class DeletionRequestPingTests: XCTestCase { setupHttpResponseStub("deletion-request") expectation = expectation(description: "Completed upload") - Glean.shared.setUploadEnabled(false) + Glean.shared.setCollectionEnabled(false) waitForExpectations(timeout: 5.0) { error in XCTAssertNil(error, "Test timed out waiting for upload: \(error!)") @@ -156,7 +156,7 @@ class DeletionRequestPingTests: XCTestCase { setupHttpResponseStub("deletion-request") expectation = expectation(description: "Completed upload") - Glean.shared.setUploadEnabled(false) + Glean.shared.setCollectionEnabled(false) waitForExpectations(timeout: 5.0) { error in XCTAssertNil(error, "Test timed out waiting for upload: \(error!)") diff --git a/glean-core/python/glean/glean.py b/glean-core/python/glean/glean.py index 088306427f..1c5a7ab6c4 100644 --- a/glean-core/python/glean/glean.py +++ b/glean-core/python/glean/glean.py @@ -325,7 +325,7 @@ def is_initialized(cls) -> bool: @classmethod def set_upload_enabled(cls, enabled: bool) -> None: """ - Enable or disable Glean collection and upload. + **DEPRECATED** Enable or disable Glean collection and upload. Metric collection is enabled by default. @@ -336,6 +336,9 @@ def set_upload_enabled(cls, enabled: bool) -> None: When enabling, the core Glean metrics are recreated. + **DEPRECATION NOTICE**: + This API is deprecated. Use `set_collection_enabled` instead. + Args: enabled (bool): When True, enable metric collection. """ @@ -347,6 +350,27 @@ def set_upload_enabled(cls, enabled: bool) -> None: # we can safely enqueue here and it will execute after initialization. _uniffi.glean_set_upload_enabled(enabled) + @classmethod + def set_collection_enabled(cls, enabled: bool) -> None: + """ + Enable or disable Glean collection and upload. + + Metric collection is enabled by default. + + When collection is disabled, metrics aren't recorded at all and no data + is uploaded. + **Note**: Individual pings can be enabled if they don't follow this setting. + See `PingType.set_enabled`. + + When disabling, all pending metrics, events and queued pings are cleared. + + When enabling, the core Glean metrics are recreated. + + Args: + enabled (bool): When True, enable metric collection. + """ + cls.set_upload_enabled(enabled) + @classmethod def set_experiment_active( cls, experiment_id: str, branch: str, extra: Optional[Dict[str, str]] = None diff --git a/glean-core/rlb/src/lib.rs b/glean-core/rlb/src/lib.rs index 4cffa2deb6..72b6f053f0 100644 --- a/glean-core/rlb/src/lib.rs +++ b/glean-core/rlb/src/lib.rs @@ -137,13 +137,23 @@ pub fn shutdown() { glean_core::shutdown() } -/// Sets whether upload is enabled or not. +/// **DEPRECATED** Sets whether upload is enabled or not. +/// +/// **DEPRECATION NOTICE**: +/// This API is deprecated. Use `set_collection_enabled` instead. /// /// See [`glean_core::Glean::set_upload_enabled`]. pub fn set_upload_enabled(enabled: bool) { glean_core::glean_set_upload_enabled(enabled) } +/// Sets whether upload is enabled or not. +/// +/// See [`glean_core::Glean::set_upload_enabled`]. +pub fn set_collection_enabled(enabled: bool) { + glean_core::glean_set_collection_enabled(enabled) +} + /// Collects and submits a ping for eventual uploading by name. /// /// Note that this needs to be public in order for RLB consumers to diff --git a/glean-core/src/lib.rs b/glean-core/src/lib.rs index b22e45708d..7079c46ab5 100644 --- a/glean-core/src/lib.rs +++ b/glean-core/src/lib.rs @@ -819,7 +819,10 @@ pub extern "C" fn glean_enable_logging() { } } -/// Sets whether upload is enabled or not. +/// **DEPRECATED** Sets whether upload is enabled or not. +/// +/// **DEPRECATION NOTICE**: +/// This API is deprecated. Use `set_collection_enabled` instead. pub fn glean_set_upload_enabled(enabled: bool) { if !was_initialize_called() { return; @@ -852,6 +855,13 @@ pub fn glean_set_upload_enabled(enabled: bool) { }) } +/// Sets whether collection is enabled or not. +/// +/// This replaces `set_upload_enabled`. +pub fn glean_set_collection_enabled(enabled: bool) { + glean_set_upload_enabled(enabled) +} + /// Register a new [`PingType`](PingType). pub(crate) fn register_ping_type(ping: &PingType) { // If this happens after Glean.initialize is called (and returns), diff --git a/samples/android/app/src/main/java/org/mozilla/samples/gleancore/MainActivity.kt b/samples/android/app/src/main/java/org/mozilla/samples/gleancore/MainActivity.kt index 36b6bfe023..c5e34ee72e 100644 --- a/samples/android/app/src/main/java/org/mozilla/samples/gleancore/MainActivity.kt +++ b/samples/android/app/src/main/java/org/mozilla/samples/gleancore/MainActivity.kt @@ -85,10 +85,10 @@ open class MainActivity : AppCompatActivity() { binding.uploadSwitch.setOnCheckedChangeListener { _, isChecked -> if (isChecked) { binding.gleanEnabledText.setText("Glean is enabled") - Glean.setUploadEnabled(true) + Glean.setCollectionEnabled(true) } else { binding.gleanEnabledText.setText("Glean is disabled") - Glean.setUploadEnabled(false) + Glean.setCollectionEnabled(false) } } diff --git a/samples/ios/app/glean-sample-app/ViewController.swift b/samples/ios/app/glean-sample-app/ViewController.swift index 72da0d1d94..6968a2546e 100644 --- a/samples/ios/app/glean-sample-app/ViewController.swift +++ b/samples/ios/app/glean-sample-app/ViewController.swift @@ -87,7 +87,7 @@ class ViewController: UIViewController { } @IBAction func enableToggled(_: Any) { - Glean.shared.setUploadEnabled(enableSwitch.isOn) + Glean.shared.setCollectionEnabled(enableSwitch.isOn) UserDefaults.standard.set(enableSwitch.isOn, forKey: telemetryPrefKey) enabledLabel.text = "Glean is \(enableSwitch.isOn ? "enabled" : "disabled")" } From ed3bda3a6b0154476af675a7ac11d4e2ee366680 Mon Sep 17 00:00:00 2001 From: Jan-Erik Rediger Date: Fri, 1 Nov 2024 17:52:40 +0100 Subject: [PATCH 04/18] Track whether a ping follows `collection_enabled` --- .../mozilla/telemetry/glean/private/PingType.kt | 2 ++ glean-core/ios/Glean/Metrics/Ping.swift | 6 ++++-- glean-core/python/glean/_loader.py | 2 ++ glean-core/python/glean/metrics/ping.py | 6 ++++-- glean-core/rlb/src/private/ping.rs | 2 ++ glean-core/src/glean.udl | 12 +++++++++++- glean-core/src/metrics/ping.rs | 13 +++++++++++++ 7 files changed, 38 insertions(+), 5 deletions(-) diff --git a/glean-core/android/src/main/java/mozilla/telemetry/glean/private/PingType.kt b/glean-core/android/src/main/java/mozilla/telemetry/glean/private/PingType.kt index e528474ffa..d8d6f13e53 100644 --- a/glean-core/android/src/main/java/mozilla/telemetry/glean/private/PingType.kt +++ b/glean-core/android/src/main/java/mozilla/telemetry/glean/private/PingType.kt @@ -54,6 +54,7 @@ class PingType ( enabled: Boolean, val schedulesPings: List, val reasonCodes: List, + followsCollectionEnabled: Boolean, ) where ReasonCodesEnum : Enum, ReasonCodesEnum : ReasonCode { private var testCallback: ((ReasonCodesEnum?) -> Unit)? = null private val innerPing: GleanPingType @@ -68,6 +69,7 @@ class PingType ( schedulesPings = schedulesPings, reasonCodes = reasonCodes, enabled = enabled, + followsCollectionEnabled = followsCollectionEnabled, ) } diff --git a/glean-core/ios/Glean/Metrics/Ping.swift b/glean-core/ios/Glean/Metrics/Ping.swift index 05abd40f4a..7d125260e6 100644 --- a/glean-core/ios/Glean/Metrics/Ping.swift +++ b/glean-core/ios/Glean/Metrics/Ping.swift @@ -43,7 +43,8 @@ public class Ping { includeInfoSections: Bool, enabled: Bool, schedulesPings: [String], - reasonCodes: [String] + reasonCodes: [String], + followsCollectionEnabled: Bool ) { self.name = name self.reasonCodes = reasonCodes @@ -55,7 +56,8 @@ public class Ping { includeInfoSections, enabled, schedulesPings, - reasonCodes + reasonCodes, + followsCollectionEnabled ) } diff --git a/glean-core/python/glean/_loader.py b/glean-core/python/glean/_loader.py index 6ca3cba7e7..acf5c7980d 100644 --- a/glean-core/python/glean/_loader.py +++ b/glean-core/python/glean/_loader.py @@ -64,6 +64,8 @@ "precise_timestamps", "include_info_sections", "schedules_pings", + "enabled", + "follows_collection_enabled", "time_unit", ] diff --git a/glean-core/python/glean/metrics/ping.py b/glean-core/python/glean/metrics/ping.py index a68bb3b7a4..a0cc3195ca 100644 --- a/glean-core/python/glean/metrics/ping.py +++ b/glean-core/python/glean/metrics/ping.py @@ -17,9 +17,10 @@ def __init__( send_if_empty: bool, precise_timestamps: bool, include_info_sections: bool, - # enabled: bool, -- Not currently enabled for python bindings schedules_pings: List[str], reason_codes: List[str], + enabled: bool = True, + follows_collection_enabled: bool = True, ): """ This implements the developer facing API for custom pings. @@ -34,9 +35,10 @@ def __init__( send_if_empty, precise_timestamps, include_info_sections, - True, + enabled, schedules_pings, reason_codes, + follows_collection_enabled, ) self._test_callback = None # type: Optional[Callable[[Optional[str]], None]] diff --git a/glean-core/rlb/src/private/ping.rs b/glean-core/rlb/src/private/ping.rs index b54eec91a6..b5d58f57c7 100644 --- a/glean-core/rlb/src/private/ping.rs +++ b/glean-core/rlb/src/private/ping.rs @@ -44,6 +44,7 @@ impl PingType { enabled: bool, schedules_pings: Vec, reason_codes: Vec, + follows_collection_enabled: bool, ) -> Self { let inner = glean_core::metrics::PingType::new( name.into(), @@ -54,6 +55,7 @@ impl PingType { enabled, schedules_pings, reason_codes, + follows_collection_enabled, ); Self { diff --git a/glean-core/src/glean.udl b/glean-core/src/glean.udl index 5e3b767835..8741c6c53b 100644 --- a/glean-core/src/glean.udl +++ b/glean-core/src/glean.udl @@ -300,7 +300,17 @@ enum ErrorType { }; interface PingType { - constructor(string name, boolean include_client_id, boolean send_if_empty, boolean precise_timestamps, boolean include_info_sections, boolean enabled, sequence schedules_pings, sequence reason_codes); + constructor( + string name, + boolean include_client_id, + boolean send_if_empty, + boolean precise_timestamps, + boolean include_info_sections, + boolean enabled, + sequence schedules_pings, + sequence reason_codes, + boolean follows_collection_enabled + ); void submit(optional string? reason = null); }; diff --git a/glean-core/src/metrics/ping.rs b/glean-core/src/metrics/ping.rs index b4d1e95a31..3ee3457e37 100644 --- a/glean-core/src/metrics/ping.rs +++ b/glean-core/src/metrics/ping.rs @@ -3,6 +3,7 @@ // file, You can obtain one at https://mozilla.org/MPL/2.0/. use std::fmt; +use std::sync::atomic::AtomicBool; use std::sync::Arc; use crate::ping::PingMaker; @@ -35,6 +36,10 @@ struct InnerPing { pub schedules_pings: Vec, /// The "reason" codes that this ping can send pub reason_codes: Vec, + + /// True when it follows the `collection_enabled` flag (aka `upload_enabled`) flag. + /// Otherwise it needs to be enabled through `enabled_pings`. + follows_collection_enabled: AtomicBool, } impl fmt::Debug for PingType { @@ -48,6 +53,10 @@ impl fmt::Debug for PingType { .field("enabled", &self.0.enabled) .field("schedules_pings", &self.0.schedules_pings) .field("reason_codes", &self.0.reason_codes) + .field( + "follows_collection_enabled", + &self.0.follows_collection_enabled.load(Ordering::Relaxed), + ) .finish() } } @@ -80,6 +89,7 @@ impl PingType { enabled: bool, schedules_pings: Vec, reason_codes: Vec, + follows_collection_enabled: bool, ) -> Self { Self::new_internal( name, @@ -90,6 +100,7 @@ impl PingType { enabled, schedules_pings, reason_codes, + follows_collection_enabled, ) } @@ -103,6 +114,7 @@ impl PingType { enabled: bool, schedules_pings: Vec, reason_codes: Vec, + follows_collection_enabled: bool, ) -> Self { let this = Self(Arc::new(InnerPing { name: name.into(), @@ -113,6 +125,7 @@ impl PingType { enabled, schedules_pings, reason_codes, + follows_collection_enabled: AtomicBool::new(follows_collection_enabled), })); // Register this ping. From d7f33eac16e95b79daad68f7c59a7b21bf66511d Mon Sep 17 00:00:00 2001 From: Jan-Erik Rediger Date: Mon, 25 Nov 2024 13:18:27 +0100 Subject: [PATCH 05/18] Upgrade to glean_parser 16.1.0 --- CHANGELOG.md | 1 + Cargo.lock | 2 +- Makefile | 2 +- glean-core/Cargo.toml | 2 +- glean-core/build/Cargo.toml | 2 +- glean-core/build/src/lib.rs | 2 +- glean-core/ios/sdk_generator.sh | 2 +- glean-core/python/glean/__init__.py | 2 +- .../telemetry/glean-gradle-plugin/GleanGradlePlugin.groovy | 2 +- pyproject.toml | 2 +- 10 files changed, 10 insertions(+), 9 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 62be416a35..e29cb3bb7a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,7 @@ * General * Add methods to access current Glean debugging settings and the list of currently registered pings([Bug 1921976](https://bugzilla.mozilla.org/show_bug.cgi?id=1921976)). + * Require `glean_parser` v16.1.0 ([#3006](https://github.com/mozilla/glean/pull/3006)) * Rust * Permit Glean shutdown to interrupt UploadManager Wait tasks ([bug 1928288](https://bugzilla.mozilla.org/show_bug.cgi?id=1928288)) diff --git a/Cargo.lock b/Cargo.lock index 15a9f896af..be9bd36938 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -336,7 +336,7 @@ dependencies = [ [[package]] name = "glean-build" -version = "15.2.0" +version = "16.1.0" dependencies = [ "tempfile", "xshell-venv", diff --git a/Makefile b/Makefile index b6d9fd7246..98421d1fa2 100644 --- a/Makefile +++ b/Makefile @@ -157,7 +157,7 @@ docs-python: build-python ## Build the Python documentation .PHONY: docs docs-rust docs-swift docs-metrics: setup-python ## Build the internal metrics documentation - $(GLEAN_PYENV)/bin/pip install glean_parser~=15.2 + $(GLEAN_PYENV)/bin/pip install glean_parser~=16.1 $(GLEAN_PYENV)/bin/glean_parser translate --allow-reserved \ -f markdown \ -o ./docs/user/user/collected-metrics \ diff --git a/glean-core/Cargo.toml b/glean-core/Cargo.toml index a81a82edb5..e5a6891667 100644 --- a/glean-core/Cargo.toml +++ b/glean-core/Cargo.toml @@ -21,7 +21,7 @@ include = [ rust-version = "1.76" [package.metadata.glean] -glean-parser = "15.2.0" +glean-parser = "16.1.0" [badges] circle-ci = { repository = "mozilla/glean", branch = "main" } diff --git a/glean-core/build/Cargo.toml b/glean-core/build/Cargo.toml index 2fab9072e7..207b3b17a8 100644 --- a/glean-core/build/Cargo.toml +++ b/glean-core/build/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "glean-build" -version = "15.2.0" +version = "16.1.0" edition = "2021" description = "Glean SDK Rust build helper" repository = "https://github.com/mozilla/glean" diff --git a/glean-core/build/src/lib.rs b/glean-core/build/src/lib.rs index a098e9b427..7ac8e923d8 100644 --- a/glean-core/build/src/lib.rs +++ b/glean-core/build/src/lib.rs @@ -39,7 +39,7 @@ use std::{env, path::PathBuf}; use xshell_venv::{Result, Shell, VirtualEnv}; -const GLEAN_PARSER_VERSION: &str = "15.2.0"; +const GLEAN_PARSER_VERSION: &str = "16.1.0"; /// A Glean Rust bindings generator. pub struct Builder { diff --git a/glean-core/ios/sdk_generator.sh b/glean-core/ios/sdk_generator.sh index 38d276c307..311abe4542 100755 --- a/glean-core/ios/sdk_generator.sh +++ b/glean-core/ios/sdk_generator.sh @@ -25,7 +25,7 @@ set -e -GLEAN_PARSER_VERSION=15.2 +GLEAN_PARSER_VERSION=16.1 # CMDNAME is used in the usage text below. # shellcheck disable=SC2034 diff --git a/glean-core/python/glean/__init__.py b/glean-core/python/glean/__init__.py index 1db3e09d1b..548c7d0e93 100644 --- a/glean-core/python/glean/__init__.py +++ b/glean-core/python/glean/__init__.py @@ -30,7 +30,7 @@ __email__ = "glean-team@mozilla.com" -GLEAN_PARSER_VERSION = "15.2.0" +GLEAN_PARSER_VERSION = "16.1.0" parser_version = VersionInfo.parse(GLEAN_PARSER_VERSION) parser_version_next_major = parser_version.bump_major() diff --git a/gradle-plugin/src/main/groovy/mozilla/telemetry/glean-gradle-plugin/GleanGradlePlugin.groovy b/gradle-plugin/src/main/groovy/mozilla/telemetry/glean-gradle-plugin/GleanGradlePlugin.groovy index 0a466efbe8..1dc15d7f7b 100644 --- a/gradle-plugin/src/main/groovy/mozilla/telemetry/glean-gradle-plugin/GleanGradlePlugin.groovy +++ b/gradle-plugin/src/main/groovy/mozilla/telemetry/glean-gradle-plugin/GleanGradlePlugin.groovy @@ -50,7 +50,7 @@ abstract class GleanMetricsYamlTransform implements TransformAction { // The version of glean_parser to install from PyPI. - private String GLEAN_PARSER_VERSION = "15.2" + private String GLEAN_PARSER_VERSION = "16.1" // The version of Miniconda is explicitly specified. // Miniconda3-4.5.12 is known to not work on Windows. private String MINICONDA_VERSION = "24.3.0-0" diff --git a/pyproject.toml b/pyproject.toml index 30b4997814..87a9f254fe 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -22,7 +22,7 @@ maintainers = [ dependencies = [ "semver>=2.13.0", - "glean_parser~=15.2", + "glean_parser~=16.1", "importlib_resources>=1.3; python_version=='3.8'" ] From 3767996ab19c4efc72b561ba96460a1e703ffdf8 Mon Sep 17 00:00:00 2001 From: Jan-Erik Rediger Date: Fri, 1 Nov 2024 17:52:40 +0100 Subject: [PATCH 06/18] Add new `follows_collection_enabled` parameter to all code --- .../telemetry/glean/testing/GleanTestRule.kt | 3 + .../java/mozilla/telemetry/glean/GleanTest.kt | 2 + .../glean/debug/GleanDebugActivityTest.kt | 1 + .../telemetry/glean/pings/CustomPingTest.kt | 4 ++ .../glean/pings/RidealongPingTest.kt | 1 + .../glean/private/EventMetricTypeTest.kt | 1 + .../telemetry/glean/private/PingTypeTest.kt | 5 ++ glean-core/ios/GleanTests/GleanTests.swift | 6 +- .../ios/GleanTests/Metrics/PingTests.swift | 12 ++-- .../ios/GleanTests/RidealongPingTests.swift | 3 +- glean-core/ios/GleanTests/TestUtils.swift | 9 ++- glean-core/rlb/examples/crashing-threads.rs | 15 ++++- glean-core/rlb/examples/delayed-ping-data.rs | 15 ++++- glean-core/rlb/examples/long-running.rs | 15 ++++- .../rlb/examples/ping-lifetime-flush.rs | 15 ++++- glean-core/rlb/examples/prototype.rs | 15 ++++- glean-core/rlb/src/common_test.rs | 2 +- glean-core/rlb/src/lib.rs | 2 +- glean-core/rlb/src/test.rs | 49 +++++++------- glean-core/rlb/tests/common/mod.rs | 2 +- glean-core/rlb/tests/init_fails.rs | 12 +++- .../rlb/tests/interruptible_shutdown.rs | 12 +++- glean-core/rlb/tests/never_init.rs | 12 +++- glean-core/rlb/tests/no_time_to_init.rs | 12 +++- glean-core/rlb/tests/schema.rs | 13 +++- glean-core/rlb/tests/simple.rs | 12 +++- glean-core/rlb/tests/upload_timing.rs | 12 +++- glean-core/src/core/mod.rs | 2 +- glean-core/src/internal_pings.rs | 4 ++ glean-core/src/lib_unit_tests.rs | 27 +++++++- glean-core/src/upload/directory.rs | 6 +- glean-core/src/upload/mod.rs | 12 ++++ glean-core/tests/common/mod.rs | 12 ++-- glean-core/tests/event.rs | 15 ++++- glean-core/tests/ping.rs | 48 ++++++++----- glean-core/tests/ping_maker.rs | 67 ++++++++++++++----- 36 files changed, 357 insertions(+), 98 deletions(-) diff --git a/glean-core/android/src/main/java/mozilla/telemetry/glean/testing/GleanTestRule.kt b/glean-core/android/src/main/java/mozilla/telemetry/glean/testing/GleanTestRule.kt index 5977cf918d..f8b2eebe0a 100644 --- a/glean-core/android/src/main/java/mozilla/telemetry/glean/testing/GleanTestRule.kt +++ b/glean-core/android/src/main/java/mozilla/telemetry/glean/testing/GleanTestRule.kt @@ -79,6 +79,7 @@ class GleanTestRule( enabled = true, schedulesPings = emptyList(), reasonCodes = emptyList(), + followsCollectionEnabled = true, ) PingType( name = "store2", @@ -89,6 +90,7 @@ class GleanTestRule( enabled = true, schedulesPings = emptyList(), reasonCodes = emptyList(), + followsCollectionEnabled = true, ) PingType( name = "store3", @@ -99,6 +101,7 @@ class GleanTestRule( enabled = true, schedulesPings = emptyList(), reasonCodes = emptyList(), + followsCollectionEnabled = true, ) Glean.resetGlean( diff --git a/glean-core/android/src/test/java/mozilla/telemetry/glean/GleanTest.kt b/glean-core/android/src/test/java/mozilla/telemetry/glean/GleanTest.kt index 9dffe2537d..155a41bb1a 100644 --- a/glean-core/android/src/test/java/mozilla/telemetry/glean/GleanTest.kt +++ b/glean-core/android/src/test/java/mozilla/telemetry/glean/GleanTest.kt @@ -483,6 +483,7 @@ class GleanTest { enabled = true, schedulesPings = emptyList(), reasonCodes = listOf(), + followsCollectionEnabled = true, ) val stringMetric = StringMetricType( CommonMetricData( @@ -853,6 +854,7 @@ class GleanTest { enabled = true, schedulesPings = emptyList(), reasonCodes = listOf(), + followsCollectionEnabled = true, ) val stringMetric = StringMetricType( CommonMetricData( diff --git a/glean-core/android/src/test/java/mozilla/telemetry/glean/debug/GleanDebugActivityTest.kt b/glean-core/android/src/test/java/mozilla/telemetry/glean/debug/GleanDebugActivityTest.kt index 20b1b978c0..2e9c236c7c 100644 --- a/glean-core/android/src/test/java/mozilla/telemetry/glean/debug/GleanDebugActivityTest.kt +++ b/glean-core/android/src/test/java/mozilla/telemetry/glean/debug/GleanDebugActivityTest.kt @@ -253,6 +253,7 @@ class GleanDebugActivityTest { enabled = true, schedulesPings = emptyList(), reasonCodes = listOf(), + followsCollectionEnabled = true, ) // Set the extra values and start the intent. diff --git a/glean-core/android/src/test/java/mozilla/telemetry/glean/pings/CustomPingTest.kt b/glean-core/android/src/test/java/mozilla/telemetry/glean/pings/CustomPingTest.kt index c6c90a2f62..3676e6755d 100644 --- a/glean-core/android/src/test/java/mozilla/telemetry/glean/pings/CustomPingTest.kt +++ b/glean-core/android/src/test/java/mozilla/telemetry/glean/pings/CustomPingTest.kt @@ -83,6 +83,7 @@ class CustomPingTest { enabled = true, schedulesPings = emptyList(), reasonCodes = emptyList(), + followsCollectionEnabled = true, ) customPing.submit() @@ -115,6 +116,7 @@ class CustomPingTest { enabled = true, schedulesPings = emptyList(), reasonCodes = emptyList(), + followsCollectionEnabled = true, ) // Trigger the ping twice. @@ -174,6 +176,7 @@ class CustomPingTest { enabled = true, schedulesPings = emptyList(), reasonCodes = emptyList(), + followsCollectionEnabled = true, ) // Define a 'click' event @@ -252,6 +255,7 @@ class CustomPingTest { enabled = true, schedulesPings = emptyList(), reasonCodes = emptyList(), + followsCollectionEnabled = true, ) customPing.submit() diff --git a/glean-core/android/src/test/java/mozilla/telemetry/glean/pings/RidealongPingTest.kt b/glean-core/android/src/test/java/mozilla/telemetry/glean/pings/RidealongPingTest.kt index d7933ac6cd..1acdb6ba94 100644 --- a/glean-core/android/src/test/java/mozilla/telemetry/glean/pings/RidealongPingTest.kt +++ b/glean-core/android/src/test/java/mozilla/telemetry/glean/pings/RidealongPingTest.kt @@ -72,6 +72,7 @@ class RidealongPingTest { enabled = true, schedulesPings = emptyList(), reasonCodes = emptyList(), + followsCollectionEnabled = true, ) Glean.handleBackgroundEvent() diff --git a/glean-core/android/src/test/java/mozilla/telemetry/glean/private/EventMetricTypeTest.kt b/glean-core/android/src/test/java/mozilla/telemetry/glean/private/EventMetricTypeTest.kt index e83a10182f..908c88336a 100644 --- a/glean-core/android/src/test/java/mozilla/telemetry/glean/private/EventMetricTypeTest.kt +++ b/glean-core/android/src/test/java/mozilla/telemetry/glean/private/EventMetricTypeTest.kt @@ -511,6 +511,7 @@ class EventMetricTypeTest { enabled = true, schedulesPings = emptyList(), reasonCodes = listOf(), + followsCollectionEnabled = true, ) // Trigger worker task to upload the pings in the background. diff --git a/glean-core/android/src/test/java/mozilla/telemetry/glean/private/PingTypeTest.kt b/glean-core/android/src/test/java/mozilla/telemetry/glean/private/PingTypeTest.kt index 42f6717327..cb56a6da76 100644 --- a/glean-core/android/src/test/java/mozilla/telemetry/glean/private/PingTypeTest.kt +++ b/glean-core/android/src/test/java/mozilla/telemetry/glean/private/PingTypeTest.kt @@ -58,6 +58,7 @@ class PingTypeTest { enabled = true, schedulesPings = emptyList(), reasonCodes = listOf(), + followsCollectionEnabled = true, ) val counter = CounterMetricType( @@ -127,6 +128,7 @@ class PingTypeTest { enabled = true, schedulesPings = emptyList(), reasonCodes = listOf(), + followsCollectionEnabled = true, ) val counter = CounterMetricType( @@ -177,6 +179,7 @@ class PingTypeTest { enabled = true, schedulesPings = emptyList(), reasonCodes = listOf(), + followsCollectionEnabled = true, ) val counter = CounterMetricType( @@ -227,6 +230,7 @@ class PingTypeTest { enabled = true, schedulesPings = emptyList(), reasonCodes = listOf(), + followsCollectionEnabled = true, ) val counter = CounterMetricType( @@ -278,6 +282,7 @@ class PingTypeTest { includeInfoSections = true, enabled = true, reasonCodes = listOf(), + followsCollectionEnabled = true, ) val counter = CounterMetricType( diff --git a/glean-core/ios/GleanTests/GleanTests.swift b/glean-core/ios/GleanTests/GleanTests.swift index fc08970d1a..de3d890dfc 100644 --- a/glean-core/ios/GleanTests/GleanTests.swift +++ b/glean-core/ios/GleanTests/GleanTests.swift @@ -261,7 +261,8 @@ class GleanTests: XCTestCase { includeInfoSections: true, enabled: true, schedulesPings: [], - reasonCodes: [] + reasonCodes: [], + followsCollectionEnabled: true ) let counter = CounterMetricType(CommonMetricData( @@ -401,7 +402,8 @@ class GleanTests: XCTestCase { includeInfoSections: true, enabled: true, schedulesPings: [], - reasonCodes: [] + reasonCodes: [], + followsCollectionEnabled: true ) let counter = CounterMetricType(CommonMetricData( diff --git a/glean-core/ios/GleanTests/Metrics/PingTests.swift b/glean-core/ios/GleanTests/Metrics/PingTests.swift index 1631e77384..0e3cc9e73e 100644 --- a/glean-core/ios/GleanTests/Metrics/PingTests.swift +++ b/glean-core/ios/GleanTests/Metrics/PingTests.swift @@ -43,7 +43,8 @@ class PingTests: XCTestCase { includeInfoSections: true, enabled: true, schedulesPings: [], - reasonCodes: [] + reasonCodes: [], + followsCollectionEnabled: true ) let counter = CounterMetricType(CommonMetricData( @@ -92,7 +93,8 @@ class PingTests: XCTestCase { includeInfoSections: true, enabled: true, schedulesPings: [], - reasonCodes: [] + reasonCodes: [], + followsCollectionEnabled: true ) let counter = CounterMetricType(CommonMetricData( @@ -131,7 +133,8 @@ class PingTests: XCTestCase { includeInfoSections: true, enabled: true, schedulesPings: [], - reasonCodes: [] + reasonCodes: [], + followsCollectionEnabled: true ) // This is simply a duplicate of the experimentationId metric that is defined in @@ -225,7 +228,8 @@ class PingTests: XCTestCase { includeInfoSections: true, enabled: true, schedulesPings: [], - reasonCodes: ["was_tested"] + reasonCodes: ["was_tested"], + followsCollectionEnabled: true ) setupHttpResponseStub("custom2") diff --git a/glean-core/ios/GleanTests/RidealongPingTests.swift b/glean-core/ios/GleanTests/RidealongPingTests.swift index 26a05783e3..05028234a3 100644 --- a/glean-core/ios/GleanTests/RidealongPingTests.swift +++ b/glean-core/ios/GleanTests/RidealongPingTests.swift @@ -30,7 +30,8 @@ final class RidealongPingTests: XCTestCase { includeInfoSections: true, enabled: true, schedulesPings: [], - reasonCodes: [] + reasonCodes: [], + followsCollectionEnabled: true ) // We receive a baseline ping, and a ridealong ping. diff --git a/glean-core/ios/GleanTests/TestUtils.swift b/glean-core/ios/GleanTests/TestUtils.swift index bc2a406536..f059570534 100644 --- a/glean-core/ios/GleanTests/TestUtils.swift +++ b/glean-core/ios/GleanTests/TestUtils.swift @@ -96,7 +96,8 @@ func resetGleanDiscardingInitialPings(testCase: XCTestCase, includeInfoSections: true, enabled: true, schedulesPings: [], - reasonCodes: [] + reasonCodes: [], + followsCollectionEnabled: true ) _ = Ping( name: "store2", @@ -106,7 +107,8 @@ func resetGleanDiscardingInitialPings(testCase: XCTestCase, includeInfoSections: true, enabled: true, schedulesPings: [], - reasonCodes: [] + reasonCodes: [], + followsCollectionEnabled: true ) _ = Ping( name: "store3", @@ -116,7 +118,8 @@ func resetGleanDiscardingInitialPings(testCase: XCTestCase, includeInfoSections: true, enabled: true, schedulesPings: [], - reasonCodes: [] + reasonCodes: [], + followsCollectionEnabled: true ) Glean.shared.resetGlean(configuration: configuration, clearStores: clearStores) diff --git a/glean-core/rlb/examples/crashing-threads.rs b/glean-core/rlb/examples/crashing-threads.rs index 404476317e..af74782f56 100644 --- a/glean-core/rlb/examples/crashing-threads.rs +++ b/glean-core/rlb/examples/crashing-threads.rs @@ -87,8 +87,19 @@ pub mod glean_metrics { } #[allow(non_upper_case_globals)] -pub static PrototypePing: Lazy = - Lazy::new(|| PingType::new("prototype", true, true, true, true, true, vec![], vec![])); +pub static PrototypePing: Lazy = Lazy::new(|| { + PingType::new( + "prototype", + true, + true, + true, + true, + true, + vec![], + vec![], + true, + ) +}); fn main() { env_logger::init(); diff --git a/glean-core/rlb/examples/delayed-ping-data.rs b/glean-core/rlb/examples/delayed-ping-data.rs index 1e772a1c9e..f7aaeb41cd 100644 --- a/glean-core/rlb/examples/delayed-ping-data.rs +++ b/glean-core/rlb/examples/delayed-ping-data.rs @@ -74,8 +74,19 @@ impl net::PingUploader for MovingUploader { } #[allow(non_upper_case_globals)] -pub static PrototypePing: Lazy = - Lazy::new(|| PingType::new("prototype", true, true, false, true, true, vec![], vec![])); +pub static PrototypePing: Lazy = Lazy::new(|| { + PingType::new( + "prototype", + true, + true, + false, + true, + true, + vec![], + vec![], + true, + ) +}); fn main() { env_logger::init(); diff --git a/glean-core/rlb/examples/long-running.rs b/glean-core/rlb/examples/long-running.rs index dda7037180..b9cbed8bb4 100644 --- a/glean-core/rlb/examples/long-running.rs +++ b/glean-core/rlb/examples/long-running.rs @@ -40,8 +40,19 @@ impl net::PingUploader for FakeUploader { } #[allow(non_upper_case_globals)] -pub static PrototypePing: Lazy = - Lazy::new(|| PingType::new("prototype", true, true, true, true, true, vec![], vec![])); +pub static PrototypePing: Lazy = Lazy::new(|| { + PingType::new( + "prototype", + true, + true, + true, + true, + true, + vec![], + vec![], + true, + ) +}); fn main() { env_logger::init(); diff --git a/glean-core/rlb/examples/ping-lifetime-flush.rs b/glean-core/rlb/examples/ping-lifetime-flush.rs index 660183833c..0123e36c7e 100644 --- a/glean-core/rlb/examples/ping-lifetime-flush.rs +++ b/glean-core/rlb/examples/ping-lifetime-flush.rs @@ -75,8 +75,19 @@ impl net::PingUploader for MovingUploader { } #[allow(non_upper_case_globals)] -pub static PrototypePing: Lazy = - Lazy::new(|| PingType::new("prototype", true, true, false, true, true, vec![], vec![])); +pub static PrototypePing: Lazy = Lazy::new(|| { + PingType::new( + "prototype", + true, + true, + false, + true, + true, + vec![], + vec![], + true, + ) +}); fn main() { env_logger::init(); diff --git a/glean-core/rlb/examples/prototype.rs b/glean-core/rlb/examples/prototype.rs index 774634520c..32f25986fc 100644 --- a/glean-core/rlb/examples/prototype.rs +++ b/glean-core/rlb/examples/prototype.rs @@ -28,8 +28,19 @@ pub mod glean_metrics { } #[allow(non_upper_case_globals)] -pub static PrototypePing: Lazy = - Lazy::new(|| PingType::new("prototype", true, true, true, true, true, vec![], vec![])); +pub static PrototypePing: Lazy = Lazy::new(|| { + PingType::new( + "prototype", + true, + true, + true, + true, + true, + vec![], + vec![], + true, + ) +}); fn main() { env_logger::init(); diff --git a/glean-core/rlb/src/common_test.rs b/glean-core/rlb/src/common_test.rs index 12c460b3bb..13efb6c723 100644 --- a/glean-core/rlb/src/common_test.rs +++ b/glean-core/rlb/src/common_test.rs @@ -46,7 +46,7 @@ pub(crate) fn new_glean( .build(), }; - _ = PingType::new("store1", true, true, true, true, true, vec![], vec![]); + _ = PingType::new("store1", true, true, true, true, true, vec![], vec![], true); crate::test_reset_glean(cfg, ClientInfoMetrics::unknown(), clear_stores); dir diff --git a/glean-core/rlb/src/lib.rs b/glean-core/rlb/src/lib.rs index 72b6f053f0..13944d77bf 100644 --- a/glean-core/rlb/src/lib.rs +++ b/glean-core/rlb/src/lib.rs @@ -23,7 +23,7 @@ //! let cfg = ConfigurationBuilder::new(true, "/tmp/data", "org.mozilla.glean_core.example").build(); //! glean::initialize(cfg, ClientInfoMetrics::unknown()); //! -//! let prototype_ping = PingType::new("prototype", true, true, true, true, true, vec!(), vec!()); +//! let prototype_ping = PingType::new("prototype", true, true, true, true, true, vec!(), vec!(), true); //! //! prototype_ping.submit(None); //! ``` diff --git a/glean-core/rlb/src/test.rs b/glean-core/rlb/src/test.rs index 15c57b6ffa..cc38afed94 100644 --- a/glean-core/rlb/src/test.rs +++ b/glean-core/rlb/src/test.rs @@ -18,6 +18,10 @@ use crate::private::{BooleanMetric, CounterMetric, EventMetric, StringMetric, Te use super::*; use crate::common_test::{lock_test, new_glean, GLOBAL_APPLICATION_ID}; +fn new_test_ping(name: &str) -> PingType { + PingType::new(name, true, true, true, true, true, vec![], vec![], true) +} + #[test] fn send_a_ping() { let _lock = lock_test(); @@ -49,8 +53,7 @@ fn send_a_ping() { // Define a new ping and submit it. const PING_NAME: &str = "test-ping"; - let custom_ping = - private::PingType::new(PING_NAME, true, true, true, true, true, vec![], vec![]); + let custom_ping = new_test_ping(PING_NAME); custom_ping.submit(None); // Wait for the ping to arrive. @@ -91,8 +94,17 @@ fn send_a_ping_without_info_sections() { // Define a new ping and submit it. const PING_NAME: &str = "noinfo-ping"; - let custom_ping = - private::PingType::new(PING_NAME, true, true, true, false, true, vec![], vec![]); + let custom_ping = PingType::new( + PING_NAME, + true, + true, + true, + /* include_info_sections */ false, + true, + vec![], + vec![], + true, + ); custom_ping.submit(None); // Wait for the ping to arrive. @@ -596,7 +608,7 @@ fn ping_collection_must_happen_after_concurrently_scheduled_metrics_recordings() ); let ping_name = "custom_ping_1"; - let ping = private::PingType::new(ping_name, true, false, true, true, true, vec![], vec![]); + let ping = new_test_ping(ping_name); let metric = private::StringMetric::new(CommonMetricData { name: "string_metric".into(), category: "telemetry".into(), @@ -1099,16 +1111,7 @@ fn flipping_upload_enabled_respects_order_of_events() { .build(); // We create a ping and a metric before we initialize Glean - let sample_ping = PingType::new( - "sample-ping-1", - true, - false, - true, - true, - true, - vec![], - vec![], - ); + let sample_ping = new_test_ping("sample-ping-1"); let metric = private::StringMetric::new(CommonMetricData { name: "string_metric".into(), category: "telemetry".into(), @@ -1152,7 +1155,7 @@ fn registering_pings_before_init_must_work() { } // Create a custom ping and attempt its registration. - let sample_ping = PingType::new("pre-register", true, true, true, true, true, vec![], vec![]); + let sample_ping = new_test_ping("pre-register"); // Create a custom configuration to use a fake uploader. let dir = tempfile::tempdir().unwrap(); @@ -1165,7 +1168,7 @@ fn registering_pings_before_init_must_work() { let _t = new_glean(Some(cfg), true); - // Submit a baseline ping. + // Submit a test ping. sample_ping.submit(None); // Wait for the ping to arrive. @@ -1204,7 +1207,7 @@ fn test_a_ping_before_submission() { let _t = new_glean(Some(cfg), true); // Create a custom ping and register it. - let sample_ping = PingType::new("custom1", true, true, true, true, true, vec![], vec![]); + let sample_ping = new_test_ping("custom1"); let metric = CounterMetric::new(CommonMetricData { name: "counter_metric".into(), @@ -1321,8 +1324,7 @@ fn signaling_done() { // Define a new ping and submit it. const PING_NAME: &str = "test-ping"; - let custom_ping = - private::PingType::new(PING_NAME, true, true, true, true, true, vec![], vec![]); + let custom_ping = new_test_ping(PING_NAME); custom_ping.submit(None); custom_ping.submit(None); @@ -1393,8 +1395,7 @@ fn configure_ping_throttling() { // Define a new ping. const PING_NAME: &str = "test-ping"; - let custom_ping = - private::PingType::new(PING_NAME, true, true, true, true, true, vec![], vec![]); + let custom_ping = new_test_ping(PING_NAME); // Submit and receive it `pings_per_interval` times. for _ in 0..pings_per_interval { @@ -1457,9 +1458,7 @@ fn pings_ride_along_builtin_pings() { let _t = new_glean(Some(cfg), true); - let reasons = vec!["active".to_string()]; - let _ride_along_ping = - private::PingType::new("ride-along", true, true, true, true, true, vec![], reasons); + let _ride_along_ping = new_test_ping("ride-along"); // Simulate becoming active. handle_client_active(); diff --git a/glean-core/rlb/tests/common/mod.rs b/glean-core/rlb/tests/common/mod.rs index 26b0ae9842..da58a7cee7 100644 --- a/glean-core/rlb/tests/common/mod.rs +++ b/glean-core/rlb/tests/common/mod.rs @@ -48,6 +48,6 @@ pub fn initialize(cfg: Configuration) { locale: Some("xx-XX".to_string()), }; - _ = PingType::new("store1", true, true, true, true, true, vec![], vec![]); + _ = PingType::new("store1", true, true, true, true, true, vec![], vec![], true); glean::initialize(cfg, client_info); } diff --git a/glean-core/rlb/tests/init_fails.rs b/glean-core/rlb/tests/init_fails.rs index 920bcaf42a..98657e166b 100644 --- a/glean-core/rlb/tests/init_fails.rs +++ b/glean-core/rlb/tests/init_fails.rs @@ -43,7 +43,17 @@ mod pings { #[allow(non_upper_case_globals)] pub static validation: Lazy = Lazy::new(|| { - glean::private::PingType::new("validation", true, true, true, true, true, vec![], vec![]) + glean::private::PingType::new( + "validation", + true, + true, + true, + true, + true, + vec![], + vec![], + true, + ) }); } diff --git a/glean-core/rlb/tests/interruptible_shutdown.rs b/glean-core/rlb/tests/interruptible_shutdown.rs index db950ba116..62f0d7e9f1 100644 --- a/glean-core/rlb/tests/interruptible_shutdown.rs +++ b/glean-core/rlb/tests/interruptible_shutdown.rs @@ -44,7 +44,17 @@ mod pings { #[allow(non_upper_case_globals)] pub static validation: Lazy = Lazy::new(|| { - glean::private::PingType::new("validation", true, true, true, true, true, vec![], vec![]) + glean::private::PingType::new( + "validation", + true, + true, + true, + true, + true, + vec![], + vec![], + true, + ) }); } diff --git a/glean-core/rlb/tests/never_init.rs b/glean-core/rlb/tests/never_init.rs index a991ce467e..b40cdc1871 100644 --- a/glean-core/rlb/tests/never_init.rs +++ b/glean-core/rlb/tests/never_init.rs @@ -39,7 +39,17 @@ mod pings { #[allow(non_upper_case_globals)] pub static validation: Lazy = Lazy::new(|| { - glean::private::PingType::new("validation", true, true, true, true, true, vec![], vec![]) + glean::private::PingType::new( + "validation", + true, + true, + true, + true, + true, + vec![], + vec![], + true, + ) }); } diff --git a/glean-core/rlb/tests/no_time_to_init.rs b/glean-core/rlb/tests/no_time_to_init.rs index c14129c168..4412214996 100644 --- a/glean-core/rlb/tests/no_time_to_init.rs +++ b/glean-core/rlb/tests/no_time_to_init.rs @@ -41,7 +41,17 @@ mod pings { #[allow(non_upper_case_globals)] pub static validation: Lazy = Lazy::new(|| { - glean::private::PingType::new("validation", true, true, true, true, true, vec![], vec![]) + glean::private::PingType::new( + "validation", + true, + true, + true, + true, + true, + vec![], + vec![], + true, + ) }); } diff --git a/glean-core/rlb/tests/schema.rs b/glean-core/rlb/tests/schema.rs index 2909ab1dd3..df53095536 100644 --- a/glean-core/rlb/tests/schema.rs +++ b/glean-core/rlb/tests/schema.rs @@ -170,8 +170,17 @@ fn validate_against_schema() { text_metric.set("loooooong text".repeat(100)); // Define a new ping and submit it. - let custom_ping = - glean::private::PingType::new(PING_NAME, true, true, true, true, true, vec![], vec![]); + let custom_ping = glean::private::PingType::new( + PING_NAME, + true, + true, + true, + true, + true, + vec![], + vec![], + true, + ); custom_ping.submit(None); // Wait for the ping to arrive. diff --git a/glean-core/rlb/tests/simple.rs b/glean-core/rlb/tests/simple.rs index a9360eeb2d..59e2f325df 100644 --- a/glean-core/rlb/tests/simple.rs +++ b/glean-core/rlb/tests/simple.rs @@ -41,7 +41,17 @@ mod pings { #[allow(non_upper_case_globals)] pub static validation: Lazy = Lazy::new(|| { - glean::private::PingType::new("validation", true, true, true, true, true, vec![], vec![]) + glean::private::PingType::new( + "validation", + true, + true, + true, + true, + true, + vec![], + vec![], + true, + ) }); } diff --git a/glean-core/rlb/tests/upload_timing.rs b/glean-core/rlb/tests/upload_timing.rs index 49007cf8dc..4b5a7798e0 100644 --- a/glean-core/rlb/tests/upload_timing.rs +++ b/glean-core/rlb/tests/upload_timing.rs @@ -97,7 +97,17 @@ mod pings { #[allow(non_upper_case_globals)] pub static validation: Lazy = Lazy::new(|| { - glean::private::PingType::new("validation", true, true, true, true, true, vec![], vec![]) + glean::private::PingType::new( + "validation", + true, + true, + true, + true, + true, + vec![], + vec![], + true, + ) }); } diff --git a/glean-core/src/core/mod.rs b/glean-core/src/core/mod.rs index b355a60eb3..f65af6f459 100644 --- a/glean-core/src/core/mod.rs +++ b/glean-core/src/core/mod.rs @@ -127,7 +127,7 @@ where /// ping_lifetime_max_time: 2000, /// }; /// let mut glean = Glean::new(cfg).unwrap(); -/// let ping = PingType::new("sample", true, false, true, true, true, vec![], vec![]); +/// let ping = PingType::new("sample", true, false, true, true, true, vec![], vec![], true); /// glean.register_ping_type(&ping); /// /// let call_counter: CounterMetric = CounterMetric::new(CommonMetricData { diff --git a/glean-core/src/internal_pings.rs b/glean-core/src/internal_pings.rs index 4fe15352b2..9b03fb125a 100644 --- a/glean-core/src/internal_pings.rs +++ b/glean-core/src/internal_pings.rs @@ -34,6 +34,7 @@ impl InternalPings { "dirty_startup".to_string(), "inactive".to_string(), ], + true, ), metrics: PingType::new( "metrics", @@ -50,6 +51,7 @@ impl InternalPings { "tomorrow".to_string(), "upgrade".to_string(), ], + true, ), events: PingType::new( "events", @@ -64,6 +66,7 @@ impl InternalPings { "inactive".to_string(), "max_capacity".to_string(), ], + true, ), deletion_request: PingType::new( "deletion-request", @@ -74,6 +77,7 @@ impl InternalPings { true, // The deletion-request should not be disabled vec![], vec!["at_init".to_string(), "set_upload_enabled".to_string()], + true, ), } } diff --git a/glean-core/src/lib_unit_tests.rs b/glean-core/src/lib_unit_tests.rs index 2b844c646f..7924dfd15c 100644 --- a/glean-core/src/lib_unit_tests.rs +++ b/glean-core/src/lib_unit_tests.rs @@ -25,9 +25,29 @@ pub fn new_glean(tempdir: Option) -> (Glean, tempfile::TempDi _ = InternalPings::new(true); // store{1, 2} is used throughout tests - let ping = PingType::new_internal("store1", true, false, true, true, true, vec![], vec![]); + let ping = PingType::new_internal( + "store1", + true, + false, + true, + true, + true, + vec![], + vec![], + true, + ); glean.register_ping_type(&ping); - let ping = PingType::new_internal("store2", true, false, true, true, true, vec![], vec![]); + let ping = PingType::new_internal( + "store2", + true, + false, + true, + true, + true, + vec![], + vec![], + true, + ); glean.register_ping_type(&ping); (glean, dir) } @@ -1197,6 +1217,7 @@ fn disabled_pings_are_not_submitted() { false, vec![], vec![], + true, ); glean.register_ping_type(&ping); @@ -1249,6 +1270,7 @@ fn pings_are_controllable_from_remote_settings_config() { false, vec![], vec![], + true, ); glean.register_ping_type(&disabled_ping); let enabled_ping = PingType::new( @@ -1260,6 +1282,7 @@ fn pings_are_controllable_from_remote_settings_config() { true, vec![], vec![], + true, ); glean.register_ping_type(&enabled_ping); diff --git a/glean-core/src/upload/directory.rs b/glean-core/src/upload/directory.rs index b083fa8f1f..9c3d83f83a 100644 --- a/glean-core/src/upload/directory.rs +++ b/glean-core/src/upload/directory.rs @@ -337,7 +337,7 @@ mod test { let (mut glean, dir) = new_glean(None); // Register a ping for testing - let ping_type = PingType::new("test", true, true, true, true, true, vec![], vec![]); + let ping_type = PingType::new("test", true, true, true, true, true, vec![], vec![], true); glean.register_ping_type(&ping_type); // Submit the ping to populate the pending_pings directory @@ -364,7 +364,7 @@ mod test { let (mut glean, dir) = new_glean(None); // Register a ping for testing - let ping_type = PingType::new("test", true, true, true, true, true, vec![], vec![]); + let ping_type = PingType::new("test", true, true, true, true, true, vec![], vec![], true); glean.register_ping_type(&ping_type); // Submit the ping to populate the pending_pings directory @@ -400,7 +400,7 @@ mod test { let (mut glean, dir) = new_glean(None); // Register a ping for testing - let ping_type = PingType::new("test", true, true, true, true, true, vec![], vec![]); + let ping_type = PingType::new("test", true, true, true, true, true, vec![], vec![], true); glean.register_ping_type(&ping_type); // Submit the ping to populate the pending_pings directory diff --git a/glean-core/src/upload/mod.rs b/glean-core/src/upload/mod.rs index 83b06cca65..9cfe3e50ad 100644 --- a/glean-core/src/upload/mod.rs +++ b/glean-core/src/upload/mod.rs @@ -1034,6 +1034,7 @@ mod test { true, vec![], vec![], + true, ); glean.register_ping_type(&ping_type); @@ -1075,6 +1076,7 @@ mod test { true, vec![], vec![], + true, ); glean.register_ping_type(&ping_type); @@ -1114,6 +1116,7 @@ mod test { true, vec![], vec![], + true, ); glean.register_ping_type(&ping_type); @@ -1153,6 +1156,7 @@ mod test { true, vec![], vec![], + true, ); glean.register_ping_type(&ping_type); @@ -1192,6 +1196,7 @@ mod test { true, vec![], vec![], + true, ); glean.register_ping_type(&ping_type); @@ -1233,6 +1238,7 @@ mod test { true, vec![], vec![], + true, ); glean.register_ping_type(&ping_type); @@ -1350,6 +1356,7 @@ mod test { true, vec![], vec![], + true, ); glean.register_ping_type(&ping_type); @@ -1425,6 +1432,7 @@ mod test { true, vec![], vec![], + true, ); glean.register_ping_type(&ping_type); @@ -1484,6 +1492,7 @@ mod test { true, vec![], vec![], + true, ); glean.register_ping_type(&ping_type); @@ -1564,6 +1573,7 @@ mod test { true, vec![], vec![], + true, ); glean.register_ping_type(&ping_type); @@ -1645,6 +1655,7 @@ mod test { true, vec![], vec![], + true, ); glean.register_ping_type(&ping_type); @@ -1728,6 +1739,7 @@ mod test { true, vec![], vec![], + true, ); glean.register_ping_type(&ping_type); diff --git a/glean-core/tests/common/mod.rs b/glean-core/tests/common/mod.rs index 5e99d4c51a..5d11f4eeab 100644 --- a/glean-core/tests/common/mod.rs +++ b/glean-core/tests/common/mod.rs @@ -71,14 +71,18 @@ pub fn new_glean(tempdir: Option) -> (Glean, tempfile::TempDi let mut glean = Glean::new(cfg).unwrap(); // store{1,2} is used throughout tests - let ping = PingType::new("store1", true, false, true, true, true, vec![], vec![]); - glean.register_ping_type(&ping); - let ping = PingType::new("store2", true, false, true, true, true, vec![], vec![]); - glean.register_ping_type(&ping); + _ = new_test_ping(&mut glean, "store1"); + _ = new_test_ping(&mut glean, "store2"); (glean, dir) } +pub fn new_test_ping(glean: &mut Glean, name: &str) -> PingType { + let ping = PingType::new(name, true, false, true, true, true, vec![], vec![], true); + glean.register_ping_type(&ping); + ping +} + /// Converts an iso8601::DateTime to a chrono::DateTime pub fn iso8601_to_chrono(datetime: &iso8601::DateTime) -> chrono::DateTime { if let YMD { year, month, day } = datetime.date { diff --git a/glean-core/tests/event.rs b/glean-core/tests/event.rs index 4d411d82c0..5ea9ab615d 100644 --- a/glean-core/tests/event.rs +++ b/glean-core/tests/event.rs @@ -172,6 +172,7 @@ fn test_sending_of_event_ping_when_it_fills_up() { true, vec![], vec!["max_capacity".to_string()], + true, )); } @@ -239,6 +240,7 @@ fn test_server_knobs_config_changing_max_events() { true, vec![], vec!["max_capacity".to_string()], + true, )); } @@ -520,6 +522,7 @@ fn event_storage_trimming() { true, vec![], vec![], + true, )); }; @@ -575,7 +578,17 @@ fn with_event_timestamps() { ping_lifetime_max_time: 0, }; let mut glean = Glean::new(cfg).unwrap(); - let ping = PingType::new("store1", true, false, true, true, true, vec![], vec![]); + let ping = PingType::new( + "store1", + true, + false, + true, + true, + true, + vec![], + vec![], + true, + ); glean.register_ping_type(&ping); let store_name = "store1"; diff --git a/glean-core/tests/ping.rs b/glean-core/tests/ping.rs index 467f2ba294..4ede7a25c4 100644 --- a/glean-core/tests/ping.rs +++ b/glean-core/tests/ping.rs @@ -16,14 +16,13 @@ use glean_core::Lifetime; fn write_ping_to_disk() { let (mut glean, _temp) = new_glean(None); - let ping = PingType::new("metrics", true, false, true, true, true, vec![], vec![]); - glean.register_ping_type(&ping); + let ping = new_test_ping(&mut glean, "store1"); // We need to store a metric as an empty ping is not stored. let counter = CounterMetric::new(CommonMetricData { name: "counter".into(), category: "local".into(), - send_in_pings: vec!["metrics".into()], + send_in_pings: vec!["store1".into()], ..Default::default() }); counter.add_sync(&glean, 1); @@ -37,14 +36,13 @@ fn write_ping_to_disk() { fn disabling_upload_clears_pending_pings() { let (mut glean, _t) = new_glean(None); - let ping = PingType::new("metrics", true, false, true, true, true, vec![], vec![]); - glean.register_ping_type(&ping); + let ping = new_test_ping(&mut glean, "store1"); // We need to store a metric as an empty ping is not stored. let counter = CounterMetric::new(CommonMetricData { name: "counter".into(), category: "local".into(), - send_in_pings: vec!["metrics".into()], + send_in_pings: vec!["store1".into()], ..Default::default() }); @@ -106,7 +104,17 @@ fn deletion_request_only_when_toggled_from_on_to_off() { fn empty_pings_with_flag_are_sent() { let (mut glean, _t) = new_glean(None); - let ping1 = PingType::new("custom-ping1", true, true, true, true, true, vec![], vec![]); + let ping1 = PingType::new( + "custom-ping1", + true, + true, + true, + true, + true, + vec![], + vec![], + true, + ); glean.register_ping_type(&ping1); let ping2 = PingType::new( "custom-ping2", @@ -117,6 +125,7 @@ fn empty_pings_with_flag_are_sent() { true, vec![], vec![], + true, ); glean.register_ping_type(&ping2); @@ -151,11 +160,8 @@ fn test_pings_submitted_metric() { None, ); - let metrics_ping = PingType::new("metrics", true, false, true, true, true, vec![], vec![]); - glean.register_ping_type(&metrics_ping); - - let baseline_ping = PingType::new("baseline", true, false, true, true, true, vec![], vec![]); - glean.register_ping_type(&baseline_ping); + let metrics_ping = new_test_ping(&mut glean, "metrics"); + let baseline_ping = new_test_ping(&mut glean, "baseline"); // We need to store a metric as an empty ping is not stored. let counter = CounterMetric::new(CommonMetricData { @@ -230,8 +236,7 @@ fn test_pings_submitted_metric() { fn events_ping_with_metric_but_no_events_is_not_sent() { let (mut glean, _t) = new_glean(None); - let events_ping = PingType::new("events", true, true, true, true, true, vec![], vec![]); - glean.register_ping_type(&events_ping); + let events_ping = new_test_ping(&mut glean, "events"); let counter = CounterMetric::new(CommonMetricData { name: "counter".into(), category: "local".into(), @@ -264,7 +269,17 @@ fn events_ping_with_metric_but_no_events_is_not_sent() { fn test_scheduled_pings_are_sent() { let (mut glean, _t) = new_glean(None); - let piggyback_ping = PingType::new("piggyback", true, true, true, true, true, vec![], vec![]); + let piggyback_ping = PingType::new( + "piggyback", + true, + true, + true, + true, + true, + vec![], + vec![], + true, + ); glean.register_ping_type(&piggyback_ping); let trigger_ping = PingType::new( @@ -276,6 +291,7 @@ fn test_scheduled_pings_are_sent() { true, vec!["piggyback".into()], vec![], + true, ); glean.register_ping_type(&trigger_ping); @@ -288,7 +304,7 @@ fn test_scheduled_pings_are_sent() { fn database_write_timings_get_recorded() { let (mut glean, _t) = new_glean(None); - let metrics_ping = PingType::new("metrics", true, false, true, true, true, vec![], vec![]); + let metrics_ping = new_test_ping(&mut glean, "metrics"); glean.register_ping_type(&metrics_ping); // We need to store a metric to record something. diff --git a/glean-core/tests/ping_maker.rs b/glean-core/tests/ping_maker.rs index 32d5fc777a..1eaad964e3 100644 --- a/glean-core/tests/ping_maker.rs +++ b/glean-core/tests/ping_maker.rs @@ -13,8 +13,7 @@ fn set_up_basic_ping() -> (Glean, PingMaker, PingType, tempfile::TempDir) { let (tempdir, _) = tempdir(); let (mut glean, t) = new_glean(Some(tempdir)); let ping_maker = PingMaker::new(); - let ping_type = PingType::new("store1", true, false, true, true, true, vec![], vec![]); - glean.register_ping_type(&ping_type); + let ping_type = new_test_ping(&mut glean, "store1"); // Record something, so the ping will have data let metric = BooleanMetric::new(CommonMetricData { @@ -98,7 +97,17 @@ fn test_metrics_must_report_experimentation_id() { }) .unwrap(); let ping_maker = PingMaker::new(); - let ping_type = PingType::new("store1", true, false, true, true, true, vec![], vec![]); + let ping_type = PingType::new( + "store1", + true, + false, + true, + true, + true, + vec![], + vec![], + true, + ); glean.register_ping_type(&ping_type); // Record something, so the ping will have data @@ -155,7 +164,17 @@ fn experimentation_id_is_removed_if_send_if_empty_is_false() { .unwrap(); let ping_maker = PingMaker::new(); - let unknown_ping_type = PingType::new("unknown", true, false, true, true, true, vec![], vec![]); + let unknown_ping_type = PingType::new( + "unknown", + true, + false, + true, + true, + true, + vec![], + vec![], + true, + ); glean.register_ping_type(&unknown_ping_type); assert!(ping_maker @@ -171,7 +190,17 @@ fn collect_must_report_none_when_no_data_is_stored() { let (mut glean, ping_maker, ping_type, _t) = set_up_basic_ping(); - let unknown_ping_type = PingType::new("unknown", true, false, true, true, true, vec![], vec![]); + let unknown_ping_type = PingType::new( + "unknown", + true, + false, + true, + true, + true, + vec![], + vec![], + true, + ); glean.register_ping_type(&ping_type); assert!(ping_maker @@ -181,7 +210,7 @@ fn collect_must_report_none_when_no_data_is_stored() { #[test] fn seq_number_must_be_sequential() { - let (glean, ping_maker, _ping_type, _t) = set_up_basic_ping(); + let (mut glean, ping_maker, _ping_type, _t) = set_up_basic_ping(); let metric = BooleanMetric::new(CommonMetricData { name: "boolean_metric".into(), @@ -195,8 +224,17 @@ fn seq_number_must_be_sequential() { for i in 0..=1 { for ping_name in ["store1", "store2"].iter() { - let ping_type = - PingType::new(*ping_name, true, false, true, true, true, vec![], vec![]); + let ping_type = PingType::new( + *ping_name, + true, + false, + true, + true, + true, + vec![], + vec![], + true, + ); let ping = ping_maker .collect(&glean, &ping_type, None, "", "") .unwrap(); @@ -209,7 +247,7 @@ fn seq_number_must_be_sequential() { // Test that ping sequence numbers increase independently. { - let ping_type = PingType::new("store1", true, false, true, true, true, vec![], vec![]); + let ping_type = new_test_ping(&mut glean, "store1"); // 3rd ping of store1 let ping = ping_maker @@ -227,7 +265,7 @@ fn seq_number_must_be_sequential() { } { - let ping_type = PingType::new("store2", true, false, true, true, true, vec![], vec![]); + let ping_type = new_test_ping(&mut glean, "store2"); // 3rd ping of store2 let ping = ping_maker @@ -238,7 +276,7 @@ fn seq_number_must_be_sequential() { } { - let ping_type = PingType::new("store1", true, false, true, true, true, vec![], vec![]); + let ping_type = new_test_ping(&mut glean, "store1"); // 5th ping of store1 let ping = ping_maker @@ -253,8 +291,7 @@ fn seq_number_must_be_sequential() { fn clear_pending_pings() { let (mut glean, _t) = new_glean(None); let ping_maker = PingMaker::new(); - let ping_type = PingType::new("store1", true, false, true, true, true, vec![], vec![]); - glean.register_ping_type(&ping_type); + let ping_type = new_test_ping(&mut glean, "store1"); // Record something, so the ping will have data let metric = BooleanMetric::new(CommonMetricData { @@ -281,7 +318,7 @@ fn no_pings_submitted_if_upload_disabled() { // Regression test, bug 1603571 let (mut glean, _t) = new_glean(None); - let ping_type = PingType::new("store1", true, true, true, true, true, vec![], vec![]); + let ping_type = PingType::new("store1", true, true, true, true, true, vec![], vec![], true); glean.register_ping_type(&ping_type); assert!(ping_type.submit_sync(&glean, None)); @@ -299,7 +336,7 @@ fn no_pings_submitted_if_upload_disabled() { fn metadata_is_correctly_added_when_necessary() { let (mut glean, _t) = new_glean(None); glean.set_debug_view_tag("valid-tag"); - let ping_type = PingType::new("store1", true, true, true, true, true, vec![], vec![]); + let ping_type = PingType::new("store1", true, true, true, true, true, vec![], vec![], true); glean.register_ping_type(&ping_type); assert!(ping_type.submit_sync(&glean, None)); From e5f2d5481a11b1d61dc1d0dedbab35d2e4f8a915 Mon Sep 17 00:00:00 2001 From: Jan-Erik Rediger Date: Fri, 1 Nov 2024 17:52:40 +0100 Subject: [PATCH 07/18] Add a new API to set pings enabled/disabled --- .../telemetry/glean/private/PingType.kt | 10 ++++ glean-core/ios/Glean/Metrics/Ping.swift | 8 +++ glean-core/python/glean/metrics/ping.py | 9 ++++ glean-core/rlb/src/private/ping.rs | 8 +++ glean-core/src/core/mod.rs | 11 ++++ glean-core/src/glean.udl | 2 + glean-core/src/lib.rs | 20 +++++++ glean-core/src/metrics/ping.rs | 53 +++++++++++++++---- 8 files changed, 110 insertions(+), 11 deletions(-) diff --git a/glean-core/android/src/main/java/mozilla/telemetry/glean/private/PingType.kt b/glean-core/android/src/main/java/mozilla/telemetry/glean/private/PingType.kt index d8d6f13e53..bf39aff3a9 100644 --- a/glean-core/android/src/main/java/mozilla/telemetry/glean/private/PingType.kt +++ b/glean-core/android/src/main/java/mozilla/telemetry/glean/private/PingType.kt @@ -110,4 +110,14 @@ class PingType ( val reasonString = reason?.let { this.reasonCodes[it.code()] } this.innerPing.submit(reasonString) } + + /** + * Enable or disable a ping. + * + * Disabling a ping causes all data for that ping to be removed from storage + * and all pending pings of that type to be deleted. + */ + fun setEnabled(enabled: Boolean) { + this.innerPing.setEnabled(enabled) + } } diff --git a/glean-core/ios/Glean/Metrics/Ping.swift b/glean-core/ios/Glean/Metrics/Ping.swift index 7d125260e6..3b77b594f2 100644 --- a/glean-core/ios/Glean/Metrics/Ping.swift +++ b/glean-core/ios/Glean/Metrics/Ping.swift @@ -99,4 +99,12 @@ public class Ping { } innerPing.submit(reasonString) } + + /// Enable or disable a ping. + /// + /// Disabling a ping causes all data for that ping to be removed from storage + /// and all pending pings of that type to be deleted. + public func setEnabled(enabled: Bool) { + innerPing.setEnabled(enabled) + } } diff --git a/glean-core/python/glean/metrics/ping.py b/glean-core/python/glean/metrics/ping.py index a0cc3195ca..fd29891b2d 100644 --- a/glean-core/python/glean/metrics/ping.py +++ b/glean-core/python/glean/metrics/ping.py @@ -75,3 +75,12 @@ def submit(self, reason: Optional[int] = None) -> None: self._test_callback = None self._inner.submit(reason_string) + + def set_enabled(self, enabled: bool) -> None: + """ + Enable or disable a ping. + + Disabling a ping causes all data for that ping to be removed from storage + and all pending pings of that type to be deleted. + """ + self._inner.set_enabled(enabled) diff --git a/glean-core/rlb/src/private/ping.rs b/glean-core/rlb/src/private/ping.rs index b5d58f57c7..b6c0b2c2c8 100644 --- a/glean-core/rlb/src/private/ping.rs +++ b/glean-core/rlb/src/private/ping.rs @@ -64,6 +64,14 @@ impl PingType { } } + /// Enable or disable a ping. + /// + /// Disabling a ping causes all data for that ping to be removed from storage + /// and all pending pings of that type to be deleted. + pub fn set_enabled(&self, enabled: bool) { + self.inner.set_enabled(enabled) + } + /// Submits the ping for eventual uploading. /// /// The ping content is assembled as soon as possible, but upload is not diff --git a/glean-core/src/core/mod.rs b/glean-core/src/core/mod.rs index f65af6f459..349d46834b 100644 --- a/glean-core/src/core/mod.rs +++ b/glean-core/src/core/mod.rs @@ -467,6 +467,17 @@ impl Glean { } } + /// Enable or disable a ping. + /// + /// Disabling a ping causes all data for that ping to be removed from storage + /// and all pending pings of that type to be deleted. + /// + /// **Note**: Do not use directly. Call `PingType::set_enabled` instead. + #[doc(hidden)] + pub fn set_ping_enabled(&mut self, ping: &PingType, enabled: bool) { + ping.store_enabled(enabled); + } + /// Determines whether upload is enabled. /// /// When upload is disabled, no data will be recorded. diff --git a/glean-core/src/glean.udl b/glean-core/src/glean.udl index 8741c6c53b..ec75cf0109 100644 --- a/glean-core/src/glean.udl +++ b/glean-core/src/glean.udl @@ -312,6 +312,8 @@ interface PingType { boolean follows_collection_enabled ); void submit(optional string? reason = null); + + void set_enabled(boolean enabled); }; // The common set of data shared across all different metric types. diff --git a/glean-core/src/lib.rs b/glean-core/src/lib.rs index 7079c46ab5..dc615c27ad 100644 --- a/glean-core/src/lib.rs +++ b/glean-core/src/lib.rs @@ -99,6 +99,7 @@ static PRE_INIT_SOURCE_TAGS: Mutex> = Mutex::new(Vec::new()); /// Keep track of pings registered before Glean is initialized. static PRE_INIT_PING_REGISTRATION: Mutex> = Mutex::new(Vec::new()); +static PRE_INIT_PING_ENABLED: Mutex> = Mutex::new(Vec::new()); /// Global singleton of the handles of the glean.init threads. /// For joining. For tests. @@ -441,6 +442,10 @@ fn initialize_inner( for ping in pings.iter() { glean.register_ping_type(ping); } + let pings = PRE_INIT_PING_ENABLED.lock().unwrap(); + for (ping, enabled) in pings.iter() { + glean.set_ping_enabled(ping, *enabled); + } // If this is the first time ever the Glean SDK runs, make sure to set // some initial core metrics in case we need to generate early pings. @@ -862,6 +867,21 @@ pub fn glean_set_collection_enabled(enabled: bool) { glean_set_upload_enabled(enabled) } +/// Enable or disable a ping. +/// +/// Disabling a ping causes all data for that ping to be removed from storage +/// and all pending pings of that type to be deleted. +pub fn set_ping_enabled(ping: &PingType, enabled: bool) { + let ping = ping.clone(); + if was_initialize_called() { + crate::launch_with_glean_mut(move |glean| glean.set_ping_enabled(&ping, enabled)); + } else { + let m = &PRE_INIT_PING_ENABLED; + let mut lock = m.lock().unwrap(); + lock.push((ping, enabled)); + } +} + /// Register a new [`PingType`](PingType). pub(crate) fn register_ping_type(ping: &PingType) { // If this happens after Glean.initialize is called (and returns), diff --git a/glean-core/src/metrics/ping.rs b/glean-core/src/metrics/ping.rs index 3ee3457e37..49ac767282 100644 --- a/glean-core/src/metrics/ping.rs +++ b/glean-core/src/metrics/ping.rs @@ -3,7 +3,7 @@ // file, You can obtain one at https://mozilla.org/MPL/2.0/. use std::fmt; -use std::sync::atomic::AtomicBool; +use std::sync::atomic::{AtomicBool, Ordering}; use std::sync::Arc; use crate::ping::PingMaker; @@ -31,7 +31,7 @@ struct InnerPing { /// Whether to include the {client|ping}_info sections on assembly. pub include_info_sections: bool, /// Whether this ping is enabled. - pub enabled: bool, + pub enabled: AtomicBool, /// Other pings that should be scheduled when this ping is sent. pub schedules_pings: Vec, /// The "reason" codes that this ping can send @@ -50,7 +50,7 @@ impl fmt::Debug for PingType { .field("send_if_empty", &self.0.send_if_empty) .field("precise_timestamps", &self.0.precise_timestamps) .field("include_info_sections", &self.0.include_info_sections) - .field("enabled", &self.0.enabled) + .field("enabled", &self.0.enabled.load(Ordering::Relaxed)) .field("schedules_pings", &self.0.schedules_pings) .field("reason_codes", &self.0.reason_codes) .field( @@ -122,7 +122,7 @@ impl PingType { send_if_empty, precise_timestamps, include_info_sections, - enabled, + enabled: AtomicBool::new(enabled), schedules_pings, reason_codes, follows_collection_enabled: AtomicBool::new(follows_collection_enabled), @@ -155,16 +155,43 @@ impl PingType { self.0.include_info_sections } + /// Enable or disable a ping. + /// + /// Disabling a ping causes all data for that ping to be removed from storage + /// and all pending pings of that type to be deleted. + pub fn set_enabled(&self, enabled: bool) { + crate::set_ping_enabled(self, enabled) + } + + /// Store whether this ping is enabled or not. + /// + /// **Note**: For internal use only. Only stores the flag. Does not touch any stored data. + /// Use the public API `PingType::set_enabled` instead. + pub(crate) fn store_enabled(&self, enabled: bool) { + self.0.enabled.store(enabled, Ordering::Release); + } + pub(crate) fn enabled(&self, glean: &Glean) -> bool { - let remote_settings_config = &glean.remote_settings_config.lock().unwrap(); + if self.0.follows_collection_enabled.load(Ordering::Relaxed) { + // if this follows collection_enabled: + // 1. check that first. if disabled, we're done + // 2. if enabled, check server-knobs + // 3. If that is not set, fall-through checking the ping + if !glean.is_upload_enabled() { + return false; + } + + let remote_settings_config = &glean.remote_settings_config.lock().unwrap(); - if !remote_settings_config.pings_enabled.is_empty() { - if let Some(remote_enabled) = remote_settings_config.pings_enabled.get(self.name()) { - return *remote_enabled; + if !remote_settings_config.pings_enabled.is_empty() { + if let Some(remote_enabled) = remote_settings_config.pings_enabled.get(self.name()) + { + return *remote_enabled; + } } } - self.0.enabled + self.0.enabled.load(Ordering::Relaxed) } pub(crate) fn schedules_pings(&self) -> &[String] { @@ -207,8 +234,12 @@ impl PingType { /// Whether the ping was succesfully assembled and queued. #[doc(hidden)] pub fn submit_sync(&self, glean: &Glean, reason: Option<&str>) -> bool { - if !glean.is_upload_enabled() { - log::info!("Glean disabled: not submitting any pings."); + if !self.enabled(glean) { + log::info!( + "The ping '{}' is disabled and will be discarded and not submitted", + self.0.name + ); + return false; } From a28ced653792bcda0db13f4b15237ee2bcd5beea Mon Sep 17 00:00:00 2001 From: Jan-Erik Rediger Date: Mon, 11 Nov 2024 15:52:49 +0100 Subject: [PATCH 08/18] Clear internal storage and storage for all known pings and all lifetimes --- glean-core/src/core/mod.rs | 11 ++++++++++- glean-core/src/database/mod.rs | 28 ++++++++++++++++++++++++++++ glean-core/src/metrics/ping.rs | 4 ++++ 3 files changed, 42 insertions(+), 1 deletion(-) diff --git a/glean-core/src/core/mod.rs b/glean-core/src/core/mod.rs index 349d46834b..9e93f4299f 100644 --- a/glean-core/src/core/mod.rs +++ b/glean-core/src/core/mod.rs @@ -577,7 +577,16 @@ impl Glean { // Note that this also includes the ping sequence numbers, so it has // the effect of resetting those to their initial values. if let Some(data) = self.data_store.as_ref() { - data.clear_all() + _ = data.clear_lifetime_storage(Lifetime::User, "glean_internal_info"); + _ = data.clear_lifetime_storage(Lifetime::User, "glean_client_info"); + _ = data.clear_lifetime_storage(Lifetime::Application, "glean_client_info"); + for (ping_name, ping) in &self.ping_registry { + if ping.follows_collection_enabled() { + _ = data.clear_ping_lifetime_storage(ping_name); + _ = data.clear_lifetime_storage(Lifetime::User, ping_name); + _ = data.clear_lifetime_storage(Lifetime::Application, ping_name); + } + } } if let Err(err) = self.event_data_store.clear_all() { log::warn!("Error clearing pending events: {}", err); diff --git a/glean-core/src/database/mod.rs b/glean-core/src/database/mod.rs index 45fac4b0a5..74f03125bb 100644 --- a/glean-core/src/database/mod.rs +++ b/glean-core/src/database/mod.rs @@ -656,6 +656,34 @@ impl Database { }) } + pub fn clear_lifetime_storage(&self, lifetime: Lifetime, storage_name: &str) -> Result<()> { + self.write_with_store(lifetime, |mut writer, store| { + let mut metrics = Vec::new(); + { + let mut iter = store.iter_from(&writer, storage_name)?; + while let Some(Ok((metric_id, _))) = iter.next() { + if let Ok(metric_id) = std::str::from_utf8(metric_id) { + if !metric_id.starts_with(storage_name) { + break; + } + metrics.push(metric_id.to_owned()); + } + } + } + + let mut res = Ok(()); + for to_delete in metrics { + if let Err(e) = store.delete(&mut writer, to_delete) { + log::warn!("Can't delete from store: {:?}", e); + res = Err(e); + } + } + + measure_commit!(self, writer.commit())?; + Ok(res?) + }) + } + /// Removes a single metric from the storage. /// /// # Arguments diff --git a/glean-core/src/metrics/ping.rs b/glean-core/src/metrics/ping.rs index 49ac767282..328bf2ee01 100644 --- a/glean-core/src/metrics/ping.rs +++ b/glean-core/src/metrics/ping.rs @@ -194,6 +194,10 @@ impl PingType { self.0.enabled.load(Ordering::Relaxed) } + pub(crate) fn follows_collection_enabled(&self) -> bool { + self.0.follows_collection_enabled.load(Ordering::Relaxed) + } + pub(crate) fn schedules_pings(&self) -> &[String] { &self.0.schedules_pings } From 13d627918e737b64d708a3e6a1bd3eda572e1d33 Mon Sep 17 00:00:00 2001 From: Jan-Erik Rediger Date: Fri, 8 Nov 2024 16:49:49 +0100 Subject: [PATCH 09/18] Rust sample: use new enable API for testing purposes --- samples/rust/metrics.yaml | 1 + samples/rust/pings.yaml | 15 +++++++++++++++ samples/rust/src/main.rs | 11 +++++++++++ 3 files changed, 27 insertions(+) diff --git a/samples/rust/metrics.yaml b/samples/rust/metrics.yaml index e4d1c48269..3c66c127d6 100644 --- a/samples/rust/metrics.yaml +++ b/samples/rust/metrics.yaml @@ -24,6 +24,7 @@ test.metrics: expires: never send_in_pings: - prototype + - usage-reporting sample_labeled_counter: &defaults type: labeled_counter diff --git a/samples/rust/pings.yaml b/samples/rust/pings.yaml index 5abc28a675..f26898f93a 100644 --- a/samples/rust/pings.yaml +++ b/samples/rust/pings.yaml @@ -20,3 +20,18 @@ prototype: - N/A notification_emails: - CHANGE-ME@example.com + +usage-reporting: + description: | + A sample custom ping. + include_client_id: false + send_if_empty: true + bugs: + - https://bugzilla.mozilla.org/123456789 + data_reviews: + - N/A + notification_emails: + - CHANGE-ME@example.com + metadata: + follows_collection_enabled: false + include_info_sections: false diff --git a/samples/rust/src/main.rs b/samples/rust/src/main.rs index 0390f8f43a..df28069166 100644 --- a/samples/rust/src/main.rs +++ b/samples/rust/src/main.rs @@ -111,6 +111,8 @@ fn main() { locale: None, }; + _ = &*glean_metrics::prototype; + _ = &*glean_metrics::usage_reporting; glean::initialize(cfg, client_info); glean_metrics::test_metrics::sample_boolean.set(true); @@ -151,6 +153,15 @@ fn main() { } glean_metrics::prototype.submit(None); + glean_metrics::usage_reporting.submit(None); + + glean::set_upload_enabled(false); + glean_metrics::usage_reporting.set_enabled(true); + glean_metrics::test_metrics::sample_boolean.set(true); + _ = glean_metrics::test_metrics::sample_boolean.test_get_value(None); + glean_metrics::prototype.submit(None); + glean_metrics::usage_reporting.submit(None); + // Need to wait a short time for Glean to actually act. thread::sleep(Duration::from_millis(100)); From db3e8f32565a83904a57e105a2c652c3554b1e6a Mon Sep 17 00:00:00 2001 From: Jan-Erik Rediger Date: Tue, 12 Nov 2024 14:43:40 +0100 Subject: [PATCH 10/18] Clear data when ping gets explicitly disabled --- glean-core/src/core/mod.rs | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/glean-core/src/core/mod.rs b/glean-core/src/core/mod.rs index 9e93f4299f..e5a4f7c148 100644 --- a/glean-core/src/core/mod.rs +++ b/glean-core/src/core/mod.rs @@ -476,6 +476,13 @@ impl Glean { #[doc(hidden)] pub fn set_ping_enabled(&mut self, ping: &PingType, enabled: bool) { ping.store_enabled(enabled); + if !enabled { + if let Some(data) = self.data_store.as_ref() { + _ = data.clear_ping_lifetime_storage(ping.name()); + _ = data.clear_lifetime_storage(Lifetime::User, ping.name()); + _ = data.clear_lifetime_storage(Lifetime::Application, ping.name()); + } + } } /// Determines whether upload is enabled. From 2eca6990f16ec4b8fa91ad315265e6b93f516ab4 Mon Sep 17 00:00:00 2001 From: Jan-Erik Rediger Date: Tue, 12 Nov 2024 12:14:31 +0100 Subject: [PATCH 11/18] Fix up Kotlin/Swift tests under the new ping behavior No data is recorded for unknown or disabled pings. --- .../mozilla/telemetry/glean/private/EventMetricTypeTest.kt | 4 ++-- .../java/mozilla/telemetry/glean/private/PingTypeTest.kt | 3 ++- glean-core/ios/GleanTests/Metrics/PingTests.swift | 5 ++++- 3 files changed, 8 insertions(+), 4 deletions(-) diff --git a/glean-core/android/src/test/java/mozilla/telemetry/glean/private/EventMetricTypeTest.kt b/glean-core/android/src/test/java/mozilla/telemetry/glean/private/EventMetricTypeTest.kt index 908c88336a..c789e97dd3 100644 --- a/glean-core/android/src/test/java/mozilla/telemetry/glean/private/EventMetricTypeTest.kt +++ b/glean-core/android/src/test/java/mozilla/telemetry/glean/private/EventMetricTypeTest.kt @@ -484,9 +484,9 @@ class EventMetricTypeTest { allowedExtraKeys = listOf("some_extra"), ) - // Let's record a single event. This will be queued up but not be sent. + // Let's record a single event. This will NOT be queued up because the ping is not registered. event.record(TestEventExtras(someExtra = "alternative")) - assertEquals(1, event.testGetValue()!!.size) + assertNull(event.testGetValue()) // Let's act as if the app was stopped Glean.testDestroyGleanHandle() diff --git a/glean-core/android/src/test/java/mozilla/telemetry/glean/private/PingTypeTest.kt b/glean-core/android/src/test/java/mozilla/telemetry/glean/private/PingTypeTest.kt index cb56a6da76..25b2980081 100644 --- a/glean-core/android/src/test/java/mozilla/telemetry/glean/private/PingTypeTest.kt +++ b/glean-core/android/src/test/java/mozilla/telemetry/glean/private/PingTypeTest.kt @@ -337,8 +337,9 @@ class PingTypeTest { ), ) + // Recording to an unknown ping won't record anything. counter.add() - assertEquals(1, counter.testGetValue()) + assertNull(counter.testGetValue()) // We might have some work queued by init that we'll need to clear. triggerWorkManager(context) diff --git a/glean-core/ios/GleanTests/Metrics/PingTests.swift b/glean-core/ios/GleanTests/Metrics/PingTests.swift index 0e3cc9e73e..e986c1fd78 100644 --- a/glean-core/ios/GleanTests/Metrics/PingTests.swift +++ b/glean-core/ios/GleanTests/Metrics/PingTests.swift @@ -188,7 +188,10 @@ class PingTests: XCTestCase { )) counter.add() - XCTAssertNotNil(counter.testGetValue()) + // Nothing stored for unknown ping + XCTAssertNil(counter.testGetValue()) + // Data recorded for baseline + XCTAssertNotNil(counter.testGetValue("baseline")) setupHttpResponseStub("INVALID") // Fail if the server receives data From b3deb1fd5420debf0319f6341e4132be4657f3de Mon Sep 17 00:00:00 2001 From: Jan-Erik Rediger Date: Tue, 15 Oct 2024 12:25:05 +0200 Subject: [PATCH 12/18] Test the new behavior for enabled pings --- .circleci/config.yml | 4 + glean-core/python/tests/data/pings.yaml | 17 +- .../python/tests/test_collection_enabled.py | 117 ++++++++++ glean-core/rlb/examples/enabled-pings.rs | 200 ++++++++++++++++++ glean-core/rlb/tests/test-enabled-pings.sh | 60 ++++++ glean-core/tests/collection_enabled.rs | 171 +++++++++++++++ 6 files changed, 568 insertions(+), 1 deletion(-) create mode 100644 glean-core/python/tests/test_collection_enabled.py create mode 100644 glean-core/rlb/examples/enabled-pings.rs create mode 100755 glean-core/rlb/tests/test-enabled-pings.sh create mode 100644 glean-core/tests/collection_enabled.rs diff --git a/.circleci/config.yml b/.circleci/config.yml index 826b019120..97dccb1ba3 100644 --- a/.circleci/config.yml +++ b/.circleci/config.yml @@ -93,6 +93,10 @@ commands: name: Run Rust RLB flush test command: | glean-core/rlb/tests/test-ping-lifetime-flush.sh + - run: + name: Run Rust RLB enabled-pings test + command: | + glean-core/rlb/tests/test-enabled-pings.sh install-rustup: steps: diff --git a/glean-core/python/tests/data/pings.yaml b/glean-core/python/tests/data/pings.yaml index ca67a50009..00c332b511 100644 --- a/glean-core/python/tests/data/pings.yaml +++ b/glean-core/python/tests/data/pings.yaml @@ -3,7 +3,7 @@ # file, You can obtain one at http://mozilla.org/MPL/2.0/. --- -$schema: moz://mozilla.org/schemas/glean/pings/1-0-0 +$schema: moz://mozilla.org/schemas/glean/pings/2-0-0 kebab-case: description: | @@ -16,3 +16,18 @@ kebab-case: - https://example.comd notification_emails: - glean-team@mozilla.com + +nofollows-defined: + description: | + A sample custom ping with follows_collection_enabled=False. + include_client_id: false + send_if_empty: true + bugs: + - https://bugzilla.mozilla.org/123456789 + data_reviews: + - N/A + notification_emails: + - CHANGE-ME@example.com + metadata: + follows_collection_enabled: false + include_info_sections: false diff --git a/glean-core/python/tests/test_collection_enabled.py b/glean-core/python/tests/test_collection_enabled.py new file mode 100644 index 0000000000..0a5f1b118a --- /dev/null +++ b/glean-core/python/tests/test_collection_enabled.py @@ -0,0 +1,117 @@ +# 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 http://mozilla.org/MPL/2.0/. + + +from pathlib import Path + + +from glean import Configuration, Glean, load_pings +from glean import __version__ as glean_version +from glean.metrics import ( + CounterMetricType, + CommonMetricData, + Lifetime, + PingType, +) +from glean.testing import _RecordingUploader + +GLEAN_APP_ID = "glean-python-test" +ROOT = Path(__file__).parent + + +def test_pings_with_follows_false_follow_their_own_setting(tmpdir, helpers): + info_path = Path(str(tmpdir)) / "info.txt" + data_dir = Path(str(tmpdir)) / "glean" + + Glean._reset() + Glean.initialize( + application_id=GLEAN_APP_ID, + application_version=glean_version, + upload_enabled=True, + data_dir=data_dir, + configuration=Configuration(ping_uploader=_RecordingUploader(info_path)), + ) + + ping = PingType( + "nofollows", + include_client_id=False, + send_if_empty=True, + precise_timestamps=True, + include_info_sections=False, + enabled=False, + schedules_pings=[], + reason_codes=[], + follows_collection_enabled=False, + ) + + counter_metric = CounterMetricType( + CommonMetricData( + disabled=False, + category="local", + lifetime=Lifetime.PING, + name="counter", + send_in_pings=["nofollows"], + dynamic_label=None, + ) + ) + + counter_metric.add(1) + assert not counter_metric.test_get_value() + ping.submit() + assert not counter_metric.test_get_value() + + ping.set_enabled(True) + counter_metric.add(2) + assert 2 == counter_metric.test_get_value() + ping.submit() + assert not counter_metric.test_get_value() + + url_path, payload = helpers.wait_for_ping(info_path) + + assert "nofollows" == url_path.split("/")[3] + assert 2 == payload["metrics"]["counter"]["local.counter"] + + +def test_loader_sets_flags(tmpdir, helpers): + info_path = Path(str(tmpdir)) / "info.txt" + data_dir = Path(str(tmpdir)) / "glean" + + Glean._reset() + + pings = load_pings(ROOT / "data" / "pings.yaml") + + counter_metric = CounterMetricType( + CommonMetricData( + disabled=False, + category="local", + lifetime=Lifetime.PING, + name="counter", + send_in_pings=["nofollows-defined"], + dynamic_label=None, + ) + ) + + Glean.initialize( + application_id=GLEAN_APP_ID, + application_version=glean_version, + upload_enabled=True, + data_dir=data_dir, + configuration=Configuration(ping_uploader=_RecordingUploader(info_path)), + ) + + counter_metric.add(1) + assert not counter_metric.test_get_value() + pings.nofollows_defined.submit() + assert not counter_metric.test_get_value() + + pings.nofollows_defined.set_enabled(True) + counter_metric.add(2) + assert 2 == counter_metric.test_get_value() + pings.nofollows_defined.submit() + assert not counter_metric.test_get_value() + + url_path, payload = helpers.wait_for_ping(info_path) + + assert "nofollows-defined" == url_path.split("/")[3] + assert 2 == payload["metrics"]["counter"]["local.counter"] diff --git a/glean-core/rlb/examples/enabled-pings.rs b/glean-core/rlb/examples/enabled-pings.rs new file mode 100644 index 0000000000..40fb73b4f6 --- /dev/null +++ b/glean-core/rlb/examples/enabled-pings.rs @@ -0,0 +1,200 @@ +// 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/. + +//! Test that pings can be enabled/disabled at runtime. + +use std::env; +use std::fs::File; +use std::io::{Read, Write}; +use std::path::PathBuf; + +use flate2::read::GzDecoder; +use glean::net; +use glean::{ClientInfoMetrics, ConfigurationBuilder}; + +/// A timing_distribution +mod metrics { + use glean::private::*; + use glean::Lifetime; + use glean_core::CommonMetricData; + use once_cell::sync::Lazy; + + #[allow(non_upper_case_globals)] + pub static boo: Lazy = Lazy::new(|| { + CounterMetric::new(CommonMetricData { + name: "boo".into(), + category: "sample".into(), + send_in_pings: vec!["one".into(), "two".into()], + lifetime: Lifetime::Ping, + disabled: false, + ..Default::default() + }) + }); +} + +mod pings { + use glean::private::PingType; + use once_cell::sync::Lazy; + + #[allow(non_upper_case_globals)] + pub static one: Lazy = Lazy::new(|| { + glean::private::PingType::new("one", true, true, true, true, false, vec![], vec![], false) + }); + + #[allow(non_upper_case_globals)] + pub static two: Lazy = Lazy::new(|| { + glean::private::PingType::new("two", true, true, true, true, false, vec![], vec![], false) + }); +} + +#[derive(Debug)] +struct MovingUploader(PathBuf); + +impl MovingUploader { + fn new(mut path: PathBuf) -> Self { + path.push("sent_pings"); + std::fs::create_dir_all(&path).unwrap(); + Self(path) + } +} + +impl net::PingUploader for MovingUploader { + fn upload(&self, upload_request: net::PingUploadRequest) -> net::UploadResult { + let net::PingUploadRequest { + body, url, headers, .. + } = upload_request; + let mut gzip_decoder = GzDecoder::new(&body[..]); + let mut s = String::with_capacity(body.len()); + + let data = gzip_decoder + .read_to_string(&mut s) + .ok() + .map(|_| &s[..]) + .or_else(|| std::str::from_utf8(&body).ok()) + .unwrap(); + + let docid = url.rsplit('/').next().unwrap(); + let out_path = self.0.join(format!("{docid}.json")); + let mut fp = File::create(out_path).unwrap(); + + // pseudo-JSON, let's hope this works. + writeln!(fp, "{{").unwrap(); + writeln!(fp, " \"url\": {url},").unwrap(); + for (key, val) in headers { + writeln!(fp, " \"{key}\": \"{val}\",").unwrap(); + } + writeln!(fp, "}}").unwrap(); + writeln!(fp, "{data}").unwrap(); + + net::UploadResult::http_status(200) + } +} + +#[derive(Debug, Clone, Copy)] +enum State { + One, + Two, + Both, +} + +impl From<&str> for State { + fn from(state: &str) -> Self { + match state { + "default" => State::One, + "enable-both" => State::Both, + "enable-only-two" => State::Two, + _ => { + panic!("unknown argument: {state}"); + } + } + } +} + +fn main() { + env_logger::init(); + + let mut args = env::args().skip(1); + + let data_path = PathBuf::from(args.next().expect("need data path")); + let state = args.next().unwrap_or_default(); + + _ = &*pings::one; + _ = &*pings::two; + + let uploader = MovingUploader::new(data_path.clone()); + let cfg = ConfigurationBuilder::new(true, data_path, "glean.enabled-pings") + .with_server_endpoint("invalid-test-host") + .with_use_core_mps(false) + .with_uploader(uploader) + .build(); + + let client_info = ClientInfoMetrics { + app_build: env!("CARGO_PKG_VERSION").to_string(), + app_display_version: env!("CARGO_PKG_VERSION").to_string(), + channel: None, + locale: None, + }; + + glean::initialize(cfg, client_info); + + // Wait for init to finish, + // otherwise we might be to quick with calling `shutdown`. + let _ = metrics::boo.test_get_value(None); + + let state = State::from(&*state); + match state { + State::One => { + pings::one.set_enabled(true); + pings::two.set_enabled(false); + } + State::Two => { + pings::one.set_enabled(false); + pings::two.set_enabled(true); + } + State::Both => { + pings::one.set_enabled(true); + pings::two.set_enabled(true); + } + } + metrics::boo.add(1); + if matches!(state, State::One | State::Both) { + assert_eq!( + Some(1), + metrics::boo.test_get_value(Some("one".to_string())) + ); + } + + if matches!(state, State::Two | State::Both) { + assert_eq!( + Some(1), + metrics::boo.test_get_value(Some("two".to_string())) + ); + } + + pings::one.submit(None); + assert!(metrics::boo + .test_get_value(Some("one".to_string())) + .is_none()); + + if matches!(state, State::Two | State::Both) { + assert_eq!( + Some(1), + metrics::boo.test_get_value(Some("two".to_string())) + ); + } + + pings::two.submit(None); + if matches!(state, State::One | State::Both) { + assert!(metrics::boo + .test_get_value(Some("one".to_string())) + .is_none()); + } + if matches!(state, State::Two | State::Both) { + assert!(metrics::boo + .test_get_value(Some("two".to_string())) + .is_none()); + } + + glean::shutdown(); // Cleanly shut down at the end of the test. +} diff --git a/glean-core/rlb/tests/test-enabled-pings.sh b/glean-core/rlb/tests/test-enabled-pings.sh new file mode 100755 index 0000000000..a965bb6aa0 --- /dev/null +++ b/glean-core/rlb/tests/test-enabled-pings.sh @@ -0,0 +1,60 @@ +#!/bin/bash + +# Test harness for testing the RLB processes from the outside. +# +# Some behavior can only be observed when properly exiting the process running Glean, +# e.g. when an uploader runs in another thread. +# On exit the threads will be killed, regardless of their state. + +# Remove the temporary data path on all exit conditions +cleanup() { + if [ -n "$datapath" ]; then + rm -r "$datapath" + fi +} +trap cleanup INT ABRT TERM EXIT + +tmp="${TMPDIR:-/tmp}" +datapath=$(mktemp -d "${tmp}/glean_enabled_pings.XXXX") + +# Build it once +cargo build -p glean --example enabled-pings + +cmd="cargo run -q -p glean --example enabled-pings -- $datapath" + +$cmd default +count=$(ls -1q "$datapath/sent_pings" | wc -l) +if [[ "$count" -ne 1 ]]; then + echo "1: test result: FAILED." + exit 101 +fi + +if ! grep -q "invalid-test-host/submit/glean-enabled-pings/one/" "$datapath/sent_pings"/*; then + echo "2: test result: FAILED." + exit 101 +fi + +rm -r $datapath +$cmd enable-both +count=$(ls -1q "$datapath/sent_pings" | wc -l) +if [[ "$count" -ne 2 ]]; then + echo "3: test result: FAILED." + exit 101 +fi + +rm -r $datapath +$cmd enable-only-two +count=$(ls -1q "$datapath/sent_pings" | wc -l) +if [[ "$count" -ne 1 ]]; then + echo "4: test result: FAILED." + exit 101 +fi + +if ! grep -q "invalid-test-host/submit/glean-enabled-pings/two/" "$datapath/sent_pings"/*; then + echo "5: test result: FAILED." + exit 101 +fi + + +echo "test result: ok." +exit 0 diff --git a/glean-core/tests/collection_enabled.rs b/glean-core/tests/collection_enabled.rs new file mode 100644 index 0000000000..3538ba9606 --- /dev/null +++ b/glean-core/tests/collection_enabled.rs @@ -0,0 +1,171 @@ +// 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 crate::common::*; + +use glean_core::metrics::*; +use glean_core::CommonMetricData; +use glean_core::Glean; +use glean_core::Lifetime; + +fn nofollows_ping(glean: &mut Glean) -> PingType { + // When `follows_collection_enabled=false` then by default `enabled=false` + let ping = PingType::new( + "nofollows", + /* include_client_id */ false, + /* send_if_empty */ true, + /* precise_timestamps */ true, + /* include_info_sections */ false, + /* enabled */ false, + vec![], + vec![], + /* follows_collection_enabled */ false, + ); + glean.register_ping_type(&ping); + ping +} + +fn manual_ping(glean: &mut Glean) -> PingType { + let ping = PingType::new( + "manual", + /* include_client_id */ true, + /* send_if_empty */ false, + /* precise_timestamps */ true, + /* include_info_sections */ true, + /* enabled */ true, + vec![], + vec![], + /* collection_enabled */ true, + ); + glean.register_ping_type(&ping); + ping +} + +#[test] +fn pings_with_follows_false_are_exempt() { + let (mut glean, _t) = new_glean(None); + + let ping = nofollows_ping(&mut glean); + + // We need to store a metric as an empty ping is not stored. + let counter = CounterMetric::new(CommonMetricData { + name: "counter".into(), + category: "local".into(), + send_in_pings: vec!["nofollows".into()], + ..Default::default() + }); + + counter.add_sync(&glean, 1); + assert!(!ping.submit_sync(&glean, None)); + + glean.set_ping_enabled(&ping, true); + counter.add_sync(&glean, 2); + assert!(ping.submit_sync(&glean, None)); + + let mut queued_pings = get_queued_pings(glean.get_data_path()).unwrap(); + assert_eq!(1, queued_pings.len()); + + let json = queued_pings.pop().unwrap().1; + let counter_val = json["metrics"]["counter"]["local.counter"] + .as_i64() + .unwrap(); + assert_eq!(2, counter_val); + + glean.set_upload_enabled(false); + assert_eq!(0, get_queued_pings(glean.get_data_path()).unwrap().len()); + // Disabling upload generates a deletion ping + assert_eq!(1, get_deletion_pings(glean.get_data_path()).unwrap().len()); + + // Regardless, the `nofollows` ping is still enabled. + counter.add_sync(&glean, 10); + assert!(ping.submit_sync(&glean, None)); + + let queued_pings = get_queued_pings(glean.get_data_path()).unwrap(); + assert_eq!(1, queued_pings.len()); + + let mut values = vec![2, 10]; + for ping in queued_pings { + let json = ping.1; + let counter_val = json["metrics"]["counter"]["local.counter"] + .as_i64() + .unwrap(); + + assert!(values.contains(&counter_val)); + values.retain(|&x| x != counter_val); + } +} + +#[test] +fn nofollows_ping_can_ride_along() { + let (mut glean, _t) = new_glean(None); + + let nofollows_ping = nofollows_ping(&mut glean); + // Basically `manual_ping` but with a ride-along + let manual_ping = PingType::new( + "manual", + /* include_client_id */ true, + /* send_if_empty */ false, + /* precise_timestamps */ true, + /* include_info_sections */ true, + /* enabled */ true, + vec!["nofollows".to_string()], + vec![], + /* collection_enabled */ true, + ); + glean.register_ping_type(&manual_ping); + + // We need to store a metric as an empty ping is not stored. + let counter = CounterMetric::new(CommonMetricData { + name: "counter".into(), + category: "local".into(), + send_in_pings: vec!["manual".into(), "nofollows".into()], + lifetime: Lifetime::Application, + ..Default::default() + }); + + // Trigger a ping with data. + counter.add_sync(&glean, 1); + assert!(manual_ping.submit_sync(&glean, None)); + assert_eq!(1, get_queued_pings(glean.get_data_path()).unwrap().len()); + + // Enable one more ping, trigger it with more data + glean.set_ping_enabled(&nofollows_ping, true); + counter.add_sync(&glean, 2); + assert!(manual_ping.submit_sync(&glean, Some("triggered"))); + // The previous one, plus 2 new ones + assert_eq!(3, get_queued_pings(glean.get_data_path()).unwrap().len()); + + // Disable it again + glean.set_ping_enabled("nofollows".to_string(), false); + counter.add_sync(&glean, 5); + assert!(manual_ping.submit_sync(&glean, Some("triggered"))); + let queued_pings = get_queued_pings(glean.get_data_path()).unwrap(); + // The 3 previous ones plus only one new one + assert_eq!(4, queued_pings.len()); + + // Check that all follows are as expected. + // We cannot guarantee order, so we need to look at some values + for (_url, json, info) in queued_pings { + let Some(obj) = info else { + panic!("no ping info") + }; + let counter_val = json["metrics"]["counter"]["local.counter"] + .as_i64() + .unwrap(); + + if obj["ping_name"].as_str().unwrap() == "nofollows" { + assert_eq!(2, counter_val, "{:?}", json); + } else { + let seq = json["ping_info"]["seq"].as_i64().unwrap(); + + match seq { + 0 => assert_eq!(1, counter_val), + 1 => assert_eq!(3, counter_val), + 2 => assert_eq!(8, counter_val), + _ => panic!("unexpected sequence number: {}", seq), + } + } + } +} From 74e34a010dd74424e90296a3e676fd2083a685f4 Mon Sep 17 00:00:00 2001 From: Jan-Erik Rediger Date: Wed, 13 Nov 2024 16:49:50 +0100 Subject: [PATCH 13/18] Only delete queued pings that are disabled --- glean-core/src/core/mod.rs | 10 ++++-- glean-core/src/ping/mod.rs | 44 +++++++++++++++++++++++--- glean-core/src/upload/directory.rs | 2 +- glean-core/src/upload/mod.rs | 1 + glean-core/tests/collection_enabled.rs | 34 ++++++++++++++++++-- 5 files changed, 80 insertions(+), 11 deletions(-) diff --git a/glean-core/src/core/mod.rs b/glean-core/src/core/mod.rs index e5a4f7c148..ea8a5c6579 100644 --- a/glean-core/src/core/mod.rs +++ b/glean-core/src/core/mod.rs @@ -574,9 +574,15 @@ impl Glean { .first_run_date .get_value(self, "glean_client_info"); - // Clear any pending pings. + // Clear any pending pings that follow `collection_enabled`. let ping_maker = PingMaker::new(); - if let Err(err) = ping_maker.clear_pending_pings(self.get_data_path()) { + let disabled_pings = self + .ping_registry + .iter() + .filter(|&(_ping_name, ping)| ping.follows_collection_enabled()) + .map(|(ping_name, _ping)| &ping_name[..]) + .collect::>(); + if let Err(err) = ping_maker.clear_pending_pings(self.get_data_path(), &disabled_pings) { log::warn!("Error clearing pending pings: {}", err); } diff --git a/glean-core/src/ping/mod.rs b/glean-core/src/ping/mod.rs index 27d82f4d45..f3483a2874 100644 --- a/glean-core/src/ping/mod.rs +++ b/glean-core/src/ping/mod.rs @@ -4,8 +4,8 @@ //! Ping collection, assembly & submission. -use std::fs::{create_dir_all, File}; -use std::io::Write; +use std::fs::{self, create_dir_all, File}; +use std::io::{BufRead, BufReader, Write}; use std::path::{Path, PathBuf}; use log::info; @@ -404,11 +404,45 @@ impl PingMaker { } /// Clears any pending pings in the queue. - pub fn clear_pending_pings(&self, data_path: &Path) -> Result<()> { + pub fn clear_pending_pings(&self, data_path: &Path, ping_names: &[&str]) -> Result<()> { let pings_dir = self.get_pings_dir(data_path, None)?; - std::fs::remove_dir_all(&pings_dir)?; - create_dir_all(&pings_dir)?; + // TODO(bug 1932909): Refactor this into its own function + // and share it with `upload::directory`. + let entries = pings_dir.read_dir()?; + for entry in entries.filter_map(|entry| entry.ok()) { + if let Ok(file_type) = entry.file_type() { + if !file_type.is_file() { + continue; + } + } else { + continue; + } + + let file = match File::open(entry.path()) { + Ok(file) => file, + Err(_) => { + continue; + } + }; + + let mut lines = BufReader::new(file).lines(); + if let (Some(Ok(path)), Some(Ok(_body)), Ok(metadata)) = + (lines.next(), lines.next(), lines.next().transpose()) + { + let PingMetadata { ping_name, .. } = metadata + .and_then(|m| crate::upload::process_metadata(&path, &m)) + .unwrap_or_default(); + let ping_name = + ping_name.unwrap_or_else(|| path.split('/').nth(3).unwrap_or("").into()); + + if ping_names.contains(&&ping_name[..]) { + _ = fs::remove_file(entry.path()); + } + } else { + continue; + } + } log::debug!("All pending pings deleted"); diff --git a/glean-core/src/upload/directory.rs b/glean-core/src/upload/directory.rs index 9c3d83f83a..fb90675be3 100644 --- a/glean-core/src/upload/directory.rs +++ b/glean-core/src/upload/directory.rs @@ -94,7 +94,7 @@ pub struct PingMetadata { /// currently it contains only additonal headers to be added to each ping request. /// Therefore, we will process the contents of this line /// and return a HeaderMap of the persisted headers. -fn process_metadata(path: &str, metadata: &str) -> Option { +pub fn process_metadata(path: &str, metadata: &str) -> Option { if let Ok(metadata) = serde_json::from_str::(metadata) { return Some(metadata); } else { diff --git a/glean-core/src/upload/mod.rs b/glean-core/src/upload/mod.rs index 9cfe3e50ad..87d637c06e 100644 --- a/glean-core/src/upload/mod.rs +++ b/glean-core/src/upload/mod.rs @@ -25,6 +25,7 @@ use chrono::Utc; use crate::error::ErrorKind; use crate::TimerId; use crate::{internal_metrics::UploadMetrics, Glean}; +pub use directory::process_metadata; use directory::{PingDirectoryManager, PingPayloadsByDirectory}; use policy::Policy; use request::create_date_header_value; diff --git a/glean-core/tests/collection_enabled.rs b/glean-core/tests/collection_enabled.rs index 3538ba9606..947cd0389f 100644 --- a/glean-core/tests/collection_enabled.rs +++ b/glean-core/tests/collection_enabled.rs @@ -74,7 +74,7 @@ fn pings_with_follows_false_are_exempt() { assert_eq!(2, counter_val); glean.set_upload_enabled(false); - assert_eq!(0, get_queued_pings(glean.get_data_path()).unwrap().len()); + assert_eq!(1, get_queued_pings(glean.get_data_path()).unwrap().len()); // Disabling upload generates a deletion ping assert_eq!(1, get_deletion_pings(glean.get_data_path()).unwrap().len()); @@ -133,14 +133,14 @@ fn nofollows_ping_can_ride_along() { // Enable one more ping, trigger it with more data glean.set_ping_enabled(&nofollows_ping, true); counter.add_sync(&glean, 2); - assert!(manual_ping.submit_sync(&glean, Some("triggered"))); + assert!(manual_ping.submit_sync(&glean, None)); // The previous one, plus 2 new ones assert_eq!(3, get_queued_pings(glean.get_data_path()).unwrap().len()); // Disable it again glean.set_ping_enabled("nofollows".to_string(), false); counter.add_sync(&glean, 5); - assert!(manual_ping.submit_sync(&glean, Some("triggered"))); + assert!(manual_ping.submit_sync(&glean, None)); let queued_pings = get_queued_pings(glean.get_data_path()).unwrap(); // The 3 previous ones plus only one new one assert_eq!(4, queued_pings.len()); @@ -169,3 +169,31 @@ fn nofollows_ping_can_ride_along() { } } } + +#[test] +fn queued_nofollows_pings_are_not_removed() { + let (mut glean, _t) = new_glean(None); + + let nofollows_ping = nofollows_ping(&mut glean); + let manual_ping = manual_ping(&mut glean); + + glean.set_ping_enabled(&nofollows_ping, true); + + // We need to store a metric as an empty ping is not stored. + let counter = CounterMetric::new(CommonMetricData { + name: "counter".into(), + category: "local".into(), + send_in_pings: vec!["manual".into(), "nofollows".into()], + lifetime: Lifetime::Application, + ..Default::default() + }); + + // Trigger a ping with data. + counter.add_sync(&glean, 1); + assert!(manual_ping.submit_sync(&glean, None)); + assert!(nofollows_ping.submit_sync(&glean, None)); + assert_eq!(2, get_queued_pings(glean.get_data_path()).unwrap().len()); + + glean.set_upload_enabled(false); + assert_eq!(1, get_queued_pings(glean.get_data_path()).unwrap().len()); +} From c65417c8f58b3c48911d3640e7cdac87ed35814f Mon Sep 17 00:00:00 2001 From: Jan-Erik Rediger Date: Wed, 13 Nov 2024 16:58:26 +0100 Subject: [PATCH 14/18] Clean out pending pings when they get disabled --- glean-core/src/core/mod.rs | 5 +++ glean-core/tests/collection_enabled.rs | 50 +++++++++++++++++++------- glean-core/tests/ping_maker.rs | 3 +- 3 files changed, 44 insertions(+), 14 deletions(-) diff --git a/glean-core/src/core/mod.rs b/glean-core/src/core/mod.rs index ea8a5c6579..040a8dca41 100644 --- a/glean-core/src/core/mod.rs +++ b/glean-core/src/core/mod.rs @@ -482,6 +482,11 @@ impl Glean { _ = data.clear_lifetime_storage(Lifetime::User, ping.name()); _ = data.clear_lifetime_storage(Lifetime::Application, ping.name()); } + let ping_maker = PingMaker::new(); + let disabled_pings = &[ping.name()][..]; + if let Err(err) = ping_maker.clear_pending_pings(self.get_data_path(), disabled_pings) { + log::warn!("Error clearing pending pings: {}", err); + } } } diff --git a/glean-core/tests/collection_enabled.rs b/glean-core/tests/collection_enabled.rs index 947cd0389f..4e07a5e7d9 100644 --- a/glean-core/tests/collection_enabled.rs +++ b/glean-core/tests/collection_enabled.rs @@ -83,7 +83,8 @@ fn pings_with_follows_false_are_exempt() { assert!(ping.submit_sync(&glean, None)); let queued_pings = get_queued_pings(glean.get_data_path()).unwrap(); - assert_eq!(1, queued_pings.len()); + // both `nofollows` pings remain in the queue + assert_eq!(2, queued_pings.len()); let mut values = vec![2, 10]; for ping in queued_pings { @@ -135,18 +136,8 @@ fn nofollows_ping_can_ride_along() { counter.add_sync(&glean, 2); assert!(manual_ping.submit_sync(&glean, None)); // The previous one, plus 2 new ones - assert_eq!(3, get_queued_pings(glean.get_data_path()).unwrap().len()); - - // Disable it again - glean.set_ping_enabled("nofollows".to_string(), false); - counter.add_sync(&glean, 5); - assert!(manual_ping.submit_sync(&glean, None)); let queued_pings = get_queued_pings(glean.get_data_path()).unwrap(); - // The 3 previous ones plus only one new one - assert_eq!(4, queued_pings.len()); - - // Check that all follows are as expected. - // We cannot guarantee order, so we need to look at some values + assert_eq!(3, queued_pings.len()); for (_url, json, info) in queued_pings { let Some(obj) = info else { panic!("no ping info") @@ -168,11 +159,40 @@ fn nofollows_ping_can_ride_along() { } } } + + // Disable it again + glean.set_ping_enabled(&nofollows_ping, false); + counter.add_sync(&glean, 5); + assert!(manual_ping.submit_sync(&glean, None)); + let queued_pings = get_queued_pings(glean.get_data_path()).unwrap(); + // The 2 previous `manual` pings, the `nofollows` was removed, plus the new `manual` one + assert_eq!(3, queued_pings.len()); + + // Check that all follows are as expected. + // We cannot guarantee order, so we need to look at some values + for (_url, json, info) in queued_pings { + let Some(obj) = info else { + panic!("no ping info") + }; + let counter_val = json["metrics"]["counter"]["local.counter"] + .as_i64() + .unwrap(); + + assert_ne!("nofollows", obj["ping_name"].as_str().unwrap()); + let seq = json["ping_info"]["seq"].as_i64().unwrap(); + + match seq { + 0 => assert_eq!(1, counter_val), + 1 => assert_eq!(3, counter_val), + 2 => assert_eq!(8, counter_val), + _ => panic!("unexpected sequence number: {}", seq), + } + } } #[test] fn queued_nofollows_pings_are_not_removed() { - let (mut glean, _t) = new_glean(None); + let (mut glean, t) = new_glean(None); let nofollows_ping = nofollows_ping(&mut glean); let manual_ping = manual_ping(&mut glean); @@ -196,4 +216,8 @@ fn queued_nofollows_pings_are_not_removed() { glean.set_upload_enabled(false); assert_eq!(1, get_queued_pings(glean.get_data_path()).unwrap().len()); + + // Ping is still there after a Glean restart + let (glean, _t) = new_glean(Some(t)); + assert_eq!(1, get_queued_pings(glean.get_data_path()).unwrap().len()); } diff --git a/glean-core/tests/ping_maker.rs b/glean-core/tests/ping_maker.rs index 1eaad964e3..7bd9dd823f 100644 --- a/glean-core/tests/ping_maker.rs +++ b/glean-core/tests/ping_maker.rs @@ -307,8 +307,9 @@ fn clear_pending_pings() { assert!(ping_type.submit_sync(&glean, None)); assert_eq!(1, get_queued_pings(glean.get_data_path()).unwrap().len()); + let disabled_pings = &["store1"][..]; assert!(ping_maker - .clear_pending_pings(glean.get_data_path()) + .clear_pending_pings(glean.get_data_path(), disabled_pings) .is_ok()); assert_eq!(0, get_queued_pings(glean.get_data_path()).unwrap().len()); } From 09126f5cd00bd527445d0e57a6939f7a5a751a60 Mon Sep 17 00:00:00 2001 From: Jan-Erik Rediger Date: Wed, 13 Nov 2024 17:29:07 +0100 Subject: [PATCH 15/18] Don't attempt to increase sequence number for disabled ping --- glean-core/src/ping/mod.rs | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/glean-core/src/ping/mod.rs b/glean-core/src/ping/mod.rs index f3483a2874..da80fec901 100644 --- a/glean-core/src/ping/mod.rs +++ b/glean-core/src/ping/mod.rs @@ -66,6 +66,11 @@ impl PingMaker { /// Gets, and then increments, the sequence number for a given ping. fn get_ping_seq(&self, glean: &Glean, storage_name: &str) -> usize { + // Don't attempt to increase sequence number for disabled ping + if !glean.is_ping_enabled(storage_name) { + return 0; + } + // Sequence numbers are stored as a counter under a name that includes the storage name let seq = CounterMetric::new(CommonMetricData { name: format!("{}#sequence", storage_name), From c22b13cd21e9fdceacbd92e17b97e2f079432f09 Mon Sep 17 00:00:00 2001 From: Jan-Erik Rediger Date: Wed, 20 Nov 2024 15:25:03 +0100 Subject: [PATCH 16/18] Try clearing metrics out again once we know about more pings --- glean-core/src/core/mod.rs | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/glean-core/src/core/mod.rs b/glean-core/src/core/mod.rs index 040a8dca41..6700ea3fb5 100644 --- a/glean-core/src/core/mod.rs +++ b/glean-core/src/core/mod.rs @@ -428,7 +428,16 @@ impl Glean { /// # Returns /// /// Whether the "events" ping was submitted. - pub fn on_ready_to_submit_pings(&self, trim_data_to_registered_pings: bool) -> bool { + pub fn on_ready_to_submit_pings(&mut self, trim_data_to_registered_pings: bool) -> bool { + // When upload is disabled on init we already clear out metrics. + // However at that point not all pings are registered and so we keep that data around. + // By the time we would be ready to submit we try again cleaning out metrics from + // now-known pings. + if !self.upload_enabled { + log::debug!("on_ready_to_submit_pings. let's clear pings once again."); + self.clear_metrics(); + } + self.event_data_store .flush_pending_events_on_startup(self, trim_data_to_registered_pings) } From 6a2cb13c09827e6c0ebdc4026d13d434724416fe Mon Sep 17 00:00:00 2001 From: Jan-Erik Rediger Date: Wed, 20 Nov 2024 15:20:16 +0100 Subject: [PATCH 17/18] New test to ensure pending pings are removed before `init` finishes --- .circleci/config.yml | 4 + .../rlb/examples/pending-gets-removed.rs | 225 ++++++++++++++++++ .../rlb/tests/test-pending-gets-removed.sh | 54 +++++ 3 files changed, 283 insertions(+) create mode 100644 glean-core/rlb/examples/pending-gets-removed.rs create mode 100755 glean-core/rlb/tests/test-pending-gets-removed.sh diff --git a/.circleci/config.yml b/.circleci/config.yml index 97dccb1ba3..71317b4ae2 100644 --- a/.circleci/config.yml +++ b/.circleci/config.yml @@ -97,6 +97,10 @@ commands: name: Run Rust RLB enabled-pings test command: | glean-core/rlb/tests/test-enabled-pings.sh + - run: + name: Run Rust RLB pending-gets-removed test + command: | + glean-core/rlb/tests/test-pending-gets-removed.sh install-rustup: steps: diff --git a/glean-core/rlb/examples/pending-gets-removed.rs b/glean-core/rlb/examples/pending-gets-removed.rs new file mode 100644 index 0000000000..57691e0eba --- /dev/null +++ b/glean-core/rlb/examples/pending-gets-removed.rs @@ -0,0 +1,225 @@ +// 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/. + +//! Test that pings can be enabled/disabled at runtime. + +use std::env; +use std::fs::{read_dir, File}; +use std::io::{BufRead, BufReader}; +use std::path::{Path, PathBuf}; + +use glean::{net, Configuration}; +use glean::{ClientInfoMetrics, ConfigurationBuilder}; +use serde_json::Value as JsonValue; + +/// A timing_distribution +mod metrics { + use glean::private::*; + use glean::Lifetime; + use glean_core::CommonMetricData; + use once_cell::sync::Lazy; + + #[allow(non_upper_case_globals)] + pub static boo: Lazy = Lazy::new(|| { + CounterMetric::new(CommonMetricData { + name: "boo".into(), + category: "sample".into(), + send_in_pings: vec!["validation".into()], + lifetime: Lifetime::Ping, + disabled: false, + ..Default::default() + }) + }); +} + +mod pings { + use glean::private::PingType; + use once_cell::sync::Lazy; + + #[allow(non_upper_case_globals)] + pub static validation: Lazy = Lazy::new(|| { + glean::private::PingType::new( + "validation", + true, + true, + true, + true, + true, + vec![], + vec![], + true, + ) + }); + + #[allow(non_upper_case_globals)] + pub static nofollows: Lazy = Lazy::new(|| { + glean::private::PingType::new( + "nofollows", + true, + true, + true, + true, + false, + vec![], + vec![], + false, + ) + }); +} + +// Define a fake uploader that sleeps. +#[derive(Debug)] +struct FakeUploader; + +impl net::PingUploader for FakeUploader { + fn upload(&self, _upload_request: net::PingUploadRequest) -> net::UploadResult { + // Recoverable upload failure, will be retried 3 times, + // but then keeps the pending ping around. + net::UploadResult::http_status(500) + } +} + +fn get_pings(pings_dir: &Path) -> Vec<(String, JsonValue, Option)> { + let Ok(entries) = read_dir(pings_dir) else { + return vec![]; + }; + entries + .filter_map(|entry| entry.ok()) + .filter(|entry| match entry.file_type() { + Ok(file_type) => file_type.is_file(), + Err(_) => false, + }) + .filter_map(|entry| File::open(entry.path()).ok()) + .filter_map(|file| { + let mut lines = BufReader::new(file).lines(); + if let (Some(Ok(url)), Some(Ok(body)), Ok(metadata)) = + (lines.next(), lines.next(), lines.next().transpose()) + { + let parsed_metadata = metadata.map(|m| { + serde_json::from_str::(&m).expect("metadata should be valid JSON") + }); + if let Ok(parsed_body) = serde_json::from_str::(&body) { + Some((url, parsed_body, parsed_metadata)) + } else { + None + } + } else { + None + } + }) + .collect() +} + +fn get_queued_pings(data_path: &Path) -> Vec<(String, JsonValue, Option)> { + get_pings(&data_path.join("pending_pings")) +} + +fn get_deletion_pings(data_path: &Path) -> Vec<(String, JsonValue, Option)> { + get_pings(&data_path.join("deletion_request")) +} + +fn get_config(data_path: &Path, upload_enabled: bool) -> Configuration { + ConfigurationBuilder::new(upload_enabled, data_path, "glean.pending-removed") + .with_server_endpoint("invalid-test-host") + .with_use_core_mps(false) + .with_uploader(FakeUploader) + .build() +} + +fn main() { + env_logger::init(); + + let mut args = env::args().skip(1); + + let data_path = PathBuf::from(args.next().expect("need data path")); + let state = args.next().unwrap_or_default(); + let client_info = ClientInfoMetrics { + app_build: env!("CARGO_PKG_VERSION").to_string(), + app_display_version: env!("CARGO_PKG_VERSION").to_string(), + channel: None, + locale: None, + }; + + // Ensure this ping is always registered early. + _ = &*pings::validation; + pings::nofollows.set_enabled(true); + + match &state[..] { + "1" => { + assert_eq!( + 0, + get_queued_pings(&data_path).len(), + "no pending ping should exist before init" + ); + + let cfg = get_config(&data_path, true); + glean::initialize(cfg, client_info); + + // Wait for init to finish. + let _ = metrics::boo.test_get_value(None); + + pings::validation.submit(None); + pings::nofollows.submit(None); + glean::shutdown(); + + assert_eq!(2, get_queued_pings(&data_path).len()); + } + "2" => { + assert_eq!( + 2, + get_queued_pings(&data_path).len(), + "two pending pings should exist before init" + ); + + let cfg = get_config(&data_path, false); + glean::initialize(cfg, client_info); + + // Wait for init to finish. + let _ = metrics::boo.test_get_value(None); + + assert_eq!( + 1, + get_queued_pings(&data_path).len(), + "one pending ping should exist after init" + ); + assert_eq!( + 1, + get_deletion_pings(&data_path).len(), + "one deletion-request ping should exist after init" + ); + } + "3" => { + assert_eq!( + 1, + get_queued_pings(&data_path).len(), + "one pending ping should exist before init" + ); + assert_eq!( + 1, + get_deletion_pings(&data_path).len(), + "one deletion-request ping should exist before init (leftover from previous run)" + ); + + let cfg = get_config(&data_path, false); + glean::initialize(cfg, client_info); + + pings::nofollows.set_enabled(false); + + // Wait for init to finish. + let _ = metrics::boo.test_get_value(None); + + assert_eq!( + 0, + get_queued_pings(&data_path).len(), + "no pending ping should exist after ping is disabled" + ); + assert_eq!( + 1, + get_deletion_pings(&data_path).len(), + "one deletion-request ping should exist after init (leftover from previous run)" + ); + } + _ => panic!("unknown state: {state:?}"), + }; +} diff --git a/glean-core/rlb/tests/test-pending-gets-removed.sh b/glean-core/rlb/tests/test-pending-gets-removed.sh new file mode 100755 index 0000000000..e66eb9cd35 --- /dev/null +++ b/glean-core/rlb/tests/test-pending-gets-removed.sh @@ -0,0 +1,54 @@ +#!/bin/bash + +# Test harness for testing the RLB processes from the outside. +# +# Some behavior can only be observed when properly exiting the process running Glean, +# e.g. when an uploader runs in another thread. +# On exit the threads will be killed, regardless of their state. + +# Remove the temporary data path on all exit conditions +cleanup() { + if [ -n "$datapath" ]; then + rm -r "$datapath" + fi +} +trap cleanup INT ABRT TERM EXIT + +set -e + +tmp="${TMPDIR:-/tmp}" +datapath=$(mktemp -d "${tmp}/pending-gets-removed.XXXX") + +# Build it once +cargo build -p glean --example pending-gets-removed + +cmd="cargo run -q -p glean --example pending-gets-removed -- $datapath" + +$cmd 1 +count=$(ls -1q "$datapath/pending_pings" | wc -l) +if [[ "$count" -ne 2 ]]; then + echo "1: test result: FAILED." + exit 101 +fi + +$cmd 2 +count=$(ls -1q "$datapath/pending_pings" | wc -l) +if [[ "$count" -ne 1 ]]; then + echo "2: test result: FAILED." + exit 101 +fi + +if ! grep -q "/submit/glean-pending-removed/nofollows/" "$datapath/pending_pings"/*; then + echo "3: test result: FAILED." + exit 101 +fi + +$cmd 3 +count=$(ls -1q "$datapath/pending_pings" | wc -l) +if [[ "$count" -ne 0 ]]; then + echo "4: test result: FAILED." + exit 101 +fi + +echo "test result: ok." +exit 0 From 38da56878319013d2cf28029ff9373b4dbc6fbfa Mon Sep 17 00:00:00 2001 From: Jan-Erik Rediger Date: Mon, 25 Nov 2024 15:45:26 +0100 Subject: [PATCH 18/18] Changelog entry for the new `collection-enabled` mode --- CHANGELOG.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index e29cb3bb7a..1aa0b61a76 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,9 @@ * General * Add methods to access current Glean debugging settings and the list of currently registered pings([Bug 1921976](https://bugzilla.mozilla.org/show_bug.cgi?id=1921976)). * Require `glean_parser` v16.1.0 ([#3006](https://github.com/mozilla/glean/pull/3006)) + * BREAKING CHANGE: Add new `collection-enabled` mode (and `follows_collection_enabled` setting for pings). + This allows to control a subset of pings independently from the Glean-wide `upload-enabled` flag. + This deprecates the `setUploadEnabled` API in favor of `setCollectionEnabled`. ([#3006](https://github.com/mozilla/glean/pull/3006)) * Rust * Permit Glean shutdown to interrupt UploadManager Wait tasks ([bug 1928288](https://bugzilla.mozilla.org/show_bug.cgi?id=1928288))