add new Configuration parameter, fix RLB ping tests

CHANGELOG.md

@@ -4,6 +4,7 @@
 
 * Rust
  * Don't return a result from `submit_ping`. The boolean return value indicates whether a ping was submitted ([#1613](https:/mozilla/glean/pull/1613))
+ * **Breaking Change**: Glean now schedules "metrics" pings, accepting a new Configuration parameter. ([#1599](https:/mozilla/glean/pull/1599))
 
 # v37.0.0 (2021-04-30)
 

glean-core/rlb/examples/prototype.rs

@@ -52,6 +52,7 @@ fn main() {
  channel: None,
  server_endpoint: Some("invalid-test-host".into()),
  uploader: None,
+ use_core_mps: true,
  };
 
  let client_info = ClientInfoMetrics {

glean-core/rlb/src/common_test.rs

@@ -49,6 +49,7 @@ pub(crate) fn new_glean(
  channel: Some("testing".into()),
  server_endpoint: Some("invalid-test-host".into()),
  uploader: None,
+ use_core_mps: false,
  },
  };
 

glean-core/rlb/src/configuration.rs

@@ -30,4 +30,6 @@ pub struct Configuration {
  pub server_endpoint: Option<String>,
  /// The instance of the uploader used to send pings.
  pub uploader: Option<Box<dyn PingUploader + 'static>>,
+ /// Whether Glean should schedule "metrics" pings for you.
+ pub use_core_mps: bool,
 }

glean-core/rlb/src/lib.rs

@@ -28,6 +28,7 @@
 //! channel: None,
 //! server_endpoint: None,
 //! uploader: None,
+//! use_core_mps: false,
 //! };
 //! glean::initialize(cfg, ClientInfoMetrics::unknown());
 //!
@@ -188,7 +189,7 @@ pub fn initialize(cfg: Configuration, client_info: ClientInfoMetrics) {
  max_events: cfg.max_events,
  delay_ping_lifetime_io: cfg.delay_ping_lifetime_io,
  app_build: client_info.app_build.clone(),
- use_core_mps: true,
+ use_core_mps: cfg.use_core_mps,
  };
 
  let glean = match Glean::new(core_cfg) {
@@ -304,7 +305,7 @@ pub fn initialize(cfg: Configuration, client_info: ClientInfoMetrics) {
  // Set up information and scheduling for Glean owned pings. Ideally, the "metrics"
  // ping startup check should be performed before any other ping, since it relies
  // on being dispatched to the API context before any other metric.
- // TODO: start the metrics ping scheduler, will happen in bug 1672951.
+ glean.start_metrics_ping_scheduler();
 
  // Check if the "dirty flag" is set. That means the product was probably
  // force-closed. If that's the case, submit a 'baseline' ping with the
@@ -363,7 +364,7 @@ pub fn shutdown() {
  }
 
  crate::launch_with_glean_mut(|glean| {
- glean_core::cancel_metrics_ping_scheduler();
+ glean.cancel_metrics_ping_scheduler();
  glean.set_dirty_flag(false);
  });
 
@@ -464,16 +465,15 @@ pub fn set_upload_enabled(enabled: bool) {
  let old_enabled = glean.is_upload_enabled();
  glean.set_upload_enabled(enabled);
 
- // TODO: Cancel upload and any outstanding metrics ping scheduler
- // task. Will happen on bug 1672951.
-
  if !old_enabled && enabled {
+ glean.start_metrics_ping_scheduler();
  // If uploading is being re-enabled, we have to restore the
  // application-lifetime metrics.
  initialize_core_metrics(&glean, &state.client_info, state.channel.clone());
  }
 
  if old_enabled && !enabled {
+ glean.cancel_metrics_ping_scheduler();
  // If uploading is disabled, we need to send the deletion-request ping:
  // note that glean-core takes care of generating it.
  state.upload_manager.trigger_upload();
@@ -684,8 +684,6 @@ pub(crate) fn destroy_glean(clear_stores: bool) {
 pub fn test_reset_glean(cfg: Configuration, client_info: ClientInfoMetrics, clear_stores: bool) {
  destroy_glean(clear_stores);
 
- // Always log pings for tests
- //Glean.setLogPings(true)
  initialize(cfg, client_info);
 }
 

glean-core/rlb/src/test.rs

@@ -45,6 +45,7 @@ fn send_a_ping() {
  channel: Some("testing".into()),
  server_endpoint: Some("invalid-test-host".into()),
  uploader: Some(Box::new(FakeUploader { sender: s })),
+ use_core_mps: false,
  };
 
  let _t = new_glean(Some(cfg), true);
@@ -155,6 +156,7 @@ fn test_experiments_recording_before_glean_inits() {
  channel: Some("testing".into()),
  server_endpoint: Some("invalid-test-host".into()),
  uploader: None,
+ use_core_mps: false,
  },
  ClientInfoMetrics::unknown(),
  false,
@@ -215,6 +217,7 @@ fn sending_of_foreground_background_pings() {
  channel: Some("testing".into()),
  server_endpoint: Some("invalid-test-host".into()),
  uploader: Some(Box::new(FakeUploader { sender: s })),
+ use_core_mps: false,
  };
 
  let _t = new_glean(Some(cfg), true);
@@ -299,6 +302,7 @@ fn sending_of_startup_baseline_ping() {
  channel: Some("testing".into()),
  server_endpoint: Some("invalid-test-host".into()),
  uploader: Some(Box::new(FakeUploader { sender: s })),
+ use_core_mps: false,
  },
  ClientInfoMetrics::unknown(),
  false,
@@ -359,6 +363,7 @@ fn no_dirty_baseline_on_clean_shutdowns() {
  channel: Some("testing".into()),
  server_endpoint: Some("invalid-test-host".into()),
  uploader: Some(Box::new(FakeUploader { sender: s })),
+ use_core_mps: false,
  },
  ClientInfoMetrics::unknown(),
  false,
@@ -390,6 +395,7 @@ fn initialize_must_not_crash_if_data_dir_is_messed_up() {
  channel: Some("testing".into()),
  server_endpoint: Some("invalid-test-host".into()),
  uploader: None,
+ use_core_mps: false,
  };
 
  test_reset_glean(cfg, ClientInfoMetrics::unknown(), false);
@@ -453,6 +459,7 @@ fn initializing_twice_is_a_noop() {
  channel: Some("testing".into()),
  server_endpoint: Some("invalid-test-host".into()),
  uploader: None,
+ use_core_mps: false,
  },
  ClientInfoMetrics::unknown(),
  true,
@@ -470,6 +477,7 @@ fn initializing_twice_is_a_noop() {
  channel: Some("testing".into()),
  server_endpoint: Some("invalid-test-host".into()),
  uploader: None,
+ use_core_mps: false,
  },
  ClientInfoMetrics::unknown(),
  false,
@@ -508,6 +516,7 @@ fn the_app_channel_must_be_correctly_set_if_requested() {
  channel: None,
  server_endpoint: Some("invalid-test-host".into()),
  uploader: None,
+ use_core_mps: false,
  },
  ClientInfoMetrics::unknown(),
  true,
@@ -622,6 +631,7 @@ fn sending_deletion_ping_if_disabled_outside_of_run() {
  channel: Some("testing".into()),
  server_endpoint: Some("invalid-test-host".into()),
  uploader: None,
+ use_core_mps: false,
  };
 
  let _t = new_glean(Some(cfg), true);
@@ -640,6 +650,7 @@ fn sending_deletion_ping_if_disabled_outside_of_run() {
  channel: Some("testing".into()),
  server_endpoint: Some("invalid-test-host".into()),
  uploader: Some(Box::new(FakeUploader { sender: s })),
+ use_core_mps: false,
  },
  ClientInfoMetrics::unknown(),
  false,
@@ -687,6 +698,7 @@ fn no_sending_of_deletion_ping_if_unchanged_outside_of_run() {
  channel: Some("testing".into()),
  server_endpoint: Some("invalid-test-host".into()),
  uploader: None,
+ use_core_mps: false,
  };
 
  let _t = new_glean(Some(cfg), true);
@@ -705,6 +717,7 @@ fn no_sending_of_deletion_ping_if_unchanged_outside_of_run() {
  channel: Some("testing".into()),
  server_endpoint: Some("invalid-test-host".into()),
  uploader: Some(Box::new(FakeUploader { sender: s })),
+ use_core_mps: false,
  },
  ClientInfoMetrics::unknown(),
  false,
@@ -770,6 +783,7 @@ fn setting_debug_view_tag_before_initialization_should_not_crash() {
  channel: Some("testing".into()),
  server_endpoint: Some("invalid-test-host".into()),
  uploader: Some(Box::new(FakeUploader { sender: s })),
+ use_core_mps: false,
  };
 
  let _t = new_glean(Some(cfg), true);
@@ -829,6 +843,7 @@ fn setting_source_tags_before_initialization_should_not_crash() {
  channel: Some("testing".into()),
  server_endpoint: Some("invalid-test-host".into()),
  uploader: Some(Box::new(FakeUploader { sender: s })),
+ use_core_mps: false,
  };
 
  let _t = new_glean(Some(cfg), true);
@@ -889,6 +904,7 @@ fn setting_source_tags_after_initialization_should_not_crash() {
  channel: Some("testing".into()),
  server_endpoint: Some("invalid-test-host".into()),
  uploader: Some(Box::new(FakeUploader { sender: s })),
+ use_core_mps: false,
  };
 
  let _t = new_glean(Some(cfg), true);
@@ -961,6 +977,7 @@ fn flipping_upload_enabled_respects_order_of_events() {
  channel: Some("testing".into()),
  server_endpoint: Some("invalid-test-host".into()),
  uploader: Some(Box::new(FakeUploader { sender: s })),
+ use_core_mps: false,
  };
 
  // We create a ping and a metric before we initialize Glean
@@ -1031,6 +1048,7 @@ fn registering_pings_before_init_must_work() {
  channel: Some("testing".into()),
  server_endpoint: Some("invalid-test-host".into()),
  uploader: Some(Box::new(FakeUploader { sender: s })),
+ use_core_mps: false,
  };
 
  let _t = new_glean(Some(cfg), true);
@@ -1081,6 +1099,7 @@ fn test_a_ping_before_submission() {
  channel: Some("testing".into()),
  server_endpoint: Some("invalid-test-host".into()),
  uploader: Some(Box::new(FakeUploader { sender: s })),
+ use_core_mps: false,
  };
 
  let _t = new_glean(Some(cfg), true);

glean-core/rlb/tests/init_fails.rs

@@ -68,6 +68,7 @@ fn init_fails() {
  channel: Some("testing".into()),
  server_endpoint: Some("invalid-test-host".into()),
  uploader: None,
+ use_core_mps: false,
  };
  common::initialize(cfg);
 

glean-core/rlb/tests/no_time_to_init.rs

@@ -66,6 +66,7 @@ fn init_fails() {
  channel: Some("testing".into()),
  server_endpoint: Some("invalid-test-host".into()),
  uploader: None,
+ use_core_mps: false,
  };
  common::initialize(cfg);
 

glean-core/rlb/tests/overflowing_preinit.rs

@@ -71,6 +71,7 @@ fn overflowing_the_task_queue_records_telemetry() {
  channel: Some("testing".into()),
  server_endpoint: Some("invalid-test-host".into()),
  uploader: None,
+ use_core_mps: false,
  };
 
  // Insert a bunch of tasks to overflow the queue.

glean-core/rlb/tests/schema.rs

@@ -36,6 +36,7 @@ fn new_glean(configuration: Option<Configuration>) -> tempfile::TempDir {
  channel: Some("testing".into()),
  server_endpoint: Some("invalid-test-host".into()),
  uploader: None,
+ use_core_mps: false,
  },
  };
 
@@ -86,6 +87,7 @@ fn validate_against_schema() {
  channel: Some("testing".into()),
  server_endpoint: Some("invalid-test-host".into()),
  uploader: Some(Box::new(ValidatingUploader { sender: s })),
+ use_core_mps: false,
  };
  let _ = new_glean(Some(cfg));
 

glean-core/rlb/tests/simple.rs

@@ -68,6 +68,7 @@ fn simple_lifecycle() {
  channel: Some("testing".into()),
  server_endpoint: Some("invalid-test-host".into()),
  uploader: None,
+ use_core_mps: false,
  };
  common::initialize(cfg);
 

glean-core/src/lib.rs

@@ -98,12 +98,7 @@ pub fn setup_glean(glean: Glean) -> Result<()> {
  // calling `initialize` on the global singleton and further operations check that it has been
  // initialized.
  if GLEAN.get().is_none() {
- if GLEAN.set(Mutex::new(glean)).is_ok() {
- let glean = &GLEAN.get().unwrap().lock().unwrap();
- if glean.schedule_metrics_pings {
- scheduler::schedule(&glean);
- }
- } else {
+ if GLEAN.set(Mutex::new(glean)).is_err() {
  log::warn!(
  "Global Glean object is initialized already. This probably happened concurrently."
  )
@@ -1017,6 +1012,22 @@ impl Glean {
  // We don't care about this failing, maybe the data does just not exist.
  let _ = self.event_data_store.clear_all();
  }
+
+ /// Instructs the Metrics Ping Scheduler's thread to exit cleanly.
+ /// If Glean was configured with `use_core_mps: false`, this has no effect.
+ pub fn cancel_metrics_ping_scheduler(&self) {
+ if self.schedule_metrics_pings {
+ scheduler::cancel();
+ }
+ }
+
+ /// Instructs the Metrics Ping Scheduler to being scheduling metrics pings.
+ /// If Glean wsa configured with `use_core_mps: false`, this has no effect.
+ pub fn start_metrics_ping_scheduler(&self) {
+ if self.schedule_metrics_pings {
+ scheduler::schedule(&self);
+ }
+ }
 }
 
 /// Returns a timestamp corresponding to "now" with millisecond precision.
@@ -1025,12 +1036,6 @@ pub fn get_timestamp_ms() -> u64 {
  zeitstempel::now() / NANOS_PER_MILLI
 }
 
-/// Instructs the Metrics Ping Scheduler's thread to exit cleanly.
-/// If Glean was configured with `use_core_mps: false`, this has no effect.
-pub fn cancel_metrics_ping_scheduler() {
- scheduler::cancel();
-}
-
 // Split unit tests to a separate file, to reduce the file of this one.
 #[cfg(test)]
 #[path = "lib_unit_tests.rs"]

glean-core/src/scheduler.rs

@@ -75,6 +75,12 @@ impl MetricsPingScheduler for GleanMetricsPingScheduler {
 pub fn schedule(glean: &Glean) {
  let now = local_now_with_offset().0;
 
+ let (cancelled_lock, _condvar) = &**TASK_CONDVAR;
+ if *cancelled_lock.lock().unwrap() {
+ log::debug!("Told to schedule, but already cancelled. Are we in a test?");
+ }
+ *cancelled_lock.lock().unwrap() = false; // Uncancel the thread.
+
  let submitter = GleanMetricsPingSubmitter {};
  let scheduler = GleanMetricsPingScheduler {};