fix(nimbus): populate config_slug on mobile (#7367) (d2faad8d) · Commits · The Tor Project / Applications / application-services

components/nimbus/src/enrollment.rs

+11 −7

Original line number	Diff line number	Diff line
		@@ -61,7 +61,7 @@ pub enum NotEnrolledReason {
		/// The experiment enrollment is paused.
		EnrollmentsPaused,
		/// The experiment used a feature that was already under experiment.
		FeatureConflict,
		FeatureConflict { conflict_slug: Option<String> },
		/// The evaluator bucketing did not choose us.
		NotSelected,
		/// We are not being targeted for this experiment.
		@@ -100,7 +100,7 @@ impl Display for NotEnrolledReason {
		NotEnrolledReason::DifferentAppName => "DifferentAppName",
		NotEnrolledReason::DifferentChannel => "DifferentChannel",
		NotEnrolledReason::EnrollmentsPaused => "EnrollmentsPaused",
		NotEnrolledReason::FeatureConflict => "FeatureConflict",
		NotEnrolledReason::FeatureConflict { .. } => "FeatureConflict",
		NotEnrolledReason::NotSelected => "NotSelected",
		NotEnrolledReason::NotTargeted => "NotTargeted",
		NotEnrolledReason::ExperimentsOptOut => "ExperimentsOptOut",
		@@ -984,10 +984,11 @@ impl<'a> EnrollmentsEvolver<'a> {

		for prev_enrollment in prev_enrollments {
		if matches!(
		prev_enrollment.status,
		&prev_enrollment.status,
		EnrollmentStatus::NotEnrolled {
		reason: NotEnrolledReason::FeatureConflict
		reason: NotEnrolledReason::FeatureConflict { conflict_slug },
		}
		if conflict_slug.is_some()
		) {
		continue;
		}
		@@ -1066,7 +1067,9 @@ impl<'a> EnrollmentsEvolver<'a> {
		next_enrollments.push(ExperimentEnrollment {
		slug: slug.clone(),
		status: EnrollmentStatus::NotEnrolled {
		reason: NotEnrolledReason::FeatureConflict,
		reason: NotEnrolledReason::FeatureConflict {
		conflict_slug: Some(needed_features_in_use[0].slug.clone()),
		},
		},
		});

		@@ -1094,10 +1097,11 @@ impl<'a> EnrollmentsEvolver<'a> {

		if prev_enrollment.is_none()
		\|\| matches!(
		prev_enrollment.unwrap().status,
		&prev_enrollment.unwrap().status,
		EnrollmentStatus::NotEnrolled {
		reason: NotEnrolledReason::FeatureConflict
		reason: NotEnrolledReason::FeatureConflict { conflict_slug }
		}
		if conflict_slug.is_some()
		)
		{
		let next_enrollment = match self.evolve_enrollment(

components/nimbus/src/metrics.rs

+6 −1

Original line number	Diff line number	Diff line
		@@ -10,6 +10,7 @@ use crate::enrollment::ExperimentEnrollment;
		use crate::enrollment::PreviousGeckoPrefState;
		pub use crate::metrics::detail::*;

		use crate::enrollment::NotEnrolledReason;
		#[derive(Serialize, Deserialize, Clone)]
		#[cfg_attr(test, derive(Debug))]
		pub struct EnrollmentStatusExtraDef {
		@@ -31,6 +32,7 @@ impl From<ExperimentEnrollment> for EnrollmentStatusExtraDef {
		let mut branch_value: Option<String> = None;
		let mut reason_value: Option<String> = None;
		let mut error_value: Option<String> = None;
		let mut conflict_slug_value: Option<String> = None;
		match &enrollment.status {
		EnrollmentStatus::Enrolled { reason, branch, .. } => {
		branch_value = Some(branch.to_owned());
		@@ -42,6 +44,9 @@ impl From<ExperimentEnrollment> for EnrollmentStatusExtraDef {
		}
		EnrollmentStatus::NotEnrolled { reason } => {
		reason_value = Some(reason.to_string());
		if let NotEnrolledReason::FeatureConflict { conflict_slug } = reason {
		conflict_slug_value = conflict_slug.clone();
		}
		}
		EnrollmentStatus::WasEnrolled { branch, .. } => branch_value = Some(branch.to_owned()),
		EnrollmentStatus::Error { reason } => {
		@@ -50,7 +55,7 @@ impl From<ExperimentEnrollment> for EnrollmentStatusExtraDef {
		}
		EnrollmentStatusExtraDef {
		branch: branch_value,
		conflict_slug: None,
		conflict_slug: conflict_slug_value,
		error_string: error_value,
		reason: reason_value,
		slug: Some(enrollment.slug),

components/nimbus/src/stateful/enrollment.rs

+106 −1

Original line number	Diff line number	Diff line
		@@ -6,7 +6,8 @@ use std::iter;

		use crate::enrollment::Participation;
		use crate::enrollment::{
		EnrollmentChangeEvent, EnrollmentChangeEventType, EnrollmentsEvolver, ExperimentEnrollment,
		DisqualifiedReason, EnrolledReason, EnrollmentChangeEvent, EnrollmentChangeEventType,
		EnrollmentsEvolver, ExperimentEnrollment, NotEnrolledReason, PreviousGeckoPrefState,
		map_enrollments,
		};
		use crate::error::{Result, debug, warn};
		@@ -295,3 +296,107 @@ pub fn reset_telemetry_identifiers(
		}
		Ok(events)
		}

		pub mod v3 {
		// This module contains legacy enrollment structs that mirror the schema of enrollments stored as they were in the database as of v3. These are used for deserializing pre-migration enrollments during the migration process, and should not be used outside of that context.

		use super::*;
		use serde::{Deserialize, Serialize};

		#[derive(Deserialize, Serialize, Debug, Clone, Hash, Eq, PartialEq)]
		pub enum LegacyNotEnrolledReason {
		DifferentAppName,
		DifferentChannel,
		EnrollmentsPaused,
		FeatureConflict,
		NotSelected,
		NotTargeted,
		OptOut,
		}

		#[derive(Deserialize, Serialize, Debug, Clone, PartialEq, Eq)]
		pub struct LegacyExperimentEnrollment {
		pub slug: String,
		pub status: LegacyEnrollmentStatus,
		}

		#[derive(Deserialize, Serialize, Debug, Clone, Hash, Eq, PartialEq)]
		pub enum LegacyEnrollmentStatus {
		Enrolled {
		reason: EnrolledReason,
		branch: String,
		#[serde(skip_serializing_if = "Option::is_none")]
		prev_gecko_pref_states: Option<Vec<PreviousGeckoPrefState>>,
		},
		NotEnrolled {
		reason: LegacyNotEnrolledReason,
		},
		Disqualified {
		reason: DisqualifiedReason,
		branch: String,
		},
		WasEnrolled {
		branch: String,
		experiment_ended_at: u64,
		},
		Error {
		reason: String,
		},
		}

		impl From<LegacyNotEnrolledReason> for NotEnrolledReason {
		#[allow(deprecated)]
		fn from(value: LegacyNotEnrolledReason) -> Self {
		match value {
		LegacyNotEnrolledReason::DifferentAppName => NotEnrolledReason::DifferentAppName,
		LegacyNotEnrolledReason::DifferentChannel => NotEnrolledReason::DifferentChannel,
		LegacyNotEnrolledReason::EnrollmentsPaused => NotEnrolledReason::EnrollmentsPaused,
		LegacyNotEnrolledReason::FeatureConflict => NotEnrolledReason::FeatureConflict {
		conflict_slug: None,
		},
		LegacyNotEnrolledReason::NotSelected => NotEnrolledReason::NotSelected,
		LegacyNotEnrolledReason::NotTargeted => NotEnrolledReason::NotTargeted,
		LegacyNotEnrolledReason::OptOut => NotEnrolledReason::OptOut,
		}
		}
		}

		impl From<LegacyEnrollmentStatus> for EnrollmentStatus {
		fn from(value: LegacyEnrollmentStatus) -> Self {
		match value {
		LegacyEnrollmentStatus::Enrolled {
		reason,
		branch,
		prev_gecko_pref_states,
		} => EnrollmentStatus::Enrolled {
		reason,
		branch,
		prev_gecko_pref_states,
		},
		LegacyEnrollmentStatus::NotEnrolled { reason } => EnrollmentStatus::NotEnrolled {
		reason: reason.into(),
		},
		LegacyEnrollmentStatus::Disqualified { reason, branch } => {
		EnrollmentStatus::Disqualified { reason, branch }
		}
		LegacyEnrollmentStatus::WasEnrolled {
		branch,
		experiment_ended_at,
		} => EnrollmentStatus::WasEnrolled {
		branch,
		experiment_ended_at,
		},
		LegacyEnrollmentStatus::Error { reason } => EnrollmentStatus::Error { reason },
		}
		}
		}

		impl From<LegacyExperimentEnrollment> for ExperimentEnrollment {
		fn from(value: LegacyExperimentEnrollment) -> Self {
		ExperimentEnrollment {
		slug: value.slug,
		status: value.status.into(),
		}
		}
		}
		}

components/nimbus/src/stateful/persistence.rs

+28 −1

Original line number	Diff line number	Diff line
		@@ -10,8 +10,10 @@ use std::fs;
		use std::path::Path;
		use std::sync::Arc;

		use crate::enrollment::ExperimentEnrollment;
		use crate::error::{ErrorCode, NimbusError, Result, debug, info, warn};
		use crate::metrics::{DatabaseLoadExtraDef, DatabaseMigrationExtraDef, MetricsHandler};
		use crate::stateful::enrollment::v3;

		// This uses the lmdb backend for rkv, which is unstable.
		// We use it for now since glean didn't seem to have trouble with it (although
		@@ -27,7 +29,7 @@ use crate::metrics::{DatabaseLoadExtraDef, DatabaseMigrationExtraDef, MetricsHan
		pub(crate) const DB_KEY_DB_VERSION: &str = "db_version";

		/// The current database version.
		pub(crate) const DB_VERSION: u16 = 3;
		pub(crate) const DB_VERSION: u16 = 4;

		pub(crate) const DB_KEY_DB_WAS_CORRUPT: &str = "db-was-corrupt";

		@@ -554,6 +556,14 @@ impl Database {
		DatabaseMigrationReason::Upgrade,
		)?;

		self.apply_migration(
		writer,
		\|writer\| self.migrate_v3_to_v4(writer),
		&mut current_version,
		4,
		DatabaseMigrationReason::Upgrade,
		)?;

		Ok(())
		}

		@@ -677,6 +687,23 @@ impl Database {
		Ok(())
		}

		fn migrate_v3_to_v4(&self, writer: &mut Writer) -> Result<()> {
		info!("Upgrading from version 3 to version 4");

		let enrollment_store = self.get_store(StoreId::Enrollments);
		let enrollments: Vec<v3::LegacyExperimentEnrollment> =
		enrollment_store.try_collect_all(writer)?;

		let enrollments: Vec<ExperimentEnrollment> =
		enrollments.into_iter().map(Into::into).collect();

		for enrollment in enrollments {
		enrollment_store.put(writer, &enrollment.slug, &enrollment)?;
		}

		Ok(())
		}

		/// Gets a Store object, which used with the writer returned by
		/// `self.write()` to update the database in a transaction.
		pub fn get_store(&self, store_id: StoreId) -> &SingleStore {

components/nimbus/src/tests/stateful/test_persistence.rs

+96 −32

Original line number	Diff line number	Diff line
		@@ -82,6 +82,12 @@ fn test_db_upgrade_unknown_version() -> Result<()> {
		to_version: 3,
		error: None,
		},
		DatabaseMigrationExtraDef {
		reason: DatabaseMigrationReason::Upgrade.to_string(),
		from_version: 3,
		to_version: 4,
		error: None,
		},
		]
		);

		@@ -150,6 +156,12 @@ fn test_corrupt_db() -> Result<()> {
		from_version: 2,
		to_version: 3,
		error: None,
		},
		DatabaseMigrationExtraDef {
		reason: DatabaseMigrationReason::Upgrade.to_string(),
		from_version: 3,
		to_version: 4,
		error: None,
		}
		]
		);
		@@ -248,6 +260,12 @@ fn test_corrupt_db_get_calculated_attributes() -> Result<()> {
		from_version: 2,
		to_version: 3,
		error: None,
		},
		DatabaseMigrationExtraDef {
		reason: DatabaseMigrationReason::Upgrade.to_string(),
		from_version: 3,
		to_version: 4,
		error: None,
		}
		]
		);
		@@ -289,7 +307,7 @@ fn test_corrupt_db_get_calculated_attributes() -> Result<()> {
		}

		#[test]
		fn test_migrate_db_v2_to_v3_user_opted_out() -> Result<()> {
		fn test_migrate_db_from_v2_user_opted_out() -> Result<()> {
		error_support::init_for_tests();
		let tmp_dir = tempfile::tempdir()?;

		@@ -300,8 +318,8 @@ fn test_migrate_db_v2_to_v3_user_opted_out() -> Result<()> {
		let metrics = TestMetrics::new();
		let db = Database::new(&tmp_dir, metrics.clone())?;

		// Check the database was upgraded to v3
		assert_eq!(db.get(StoreId::Meta, DB_KEY_DB_VERSION)?, Some(3u16));
		// Check the database was upgraded to the latest version
		assert_eq!(db.get(StoreId::Meta, DB_KEY_DB_VERSION)?, Some(DB_VERSION));

		// Check that separate flags were set correctly for opted-out user
		let reader = db.read()?;
		@@ -324,26 +342,34 @@ fn test_migrate_db_v2_to_v3_user_opted_out() -> Result<()> {
		corrupt: Some(false),
		error: None,
		initial_version: Some(2),
		migrated_version: Some(3),
		migrated_version: Some(DB_VERSION),
		migration_error: None,
		}],
		);

		assert_eq!(
		metrics.get_database_migration_events(),
		[DatabaseMigrationExtraDef {
		[
		DatabaseMigrationExtraDef {
		reason: DatabaseMigrationReason::Upgrade.to_string(),
		from_version: 2,
		to_version: 3,
		error: None,
		}],
		},
		DatabaseMigrationExtraDef {
		reason: DatabaseMigrationReason::Upgrade.to_string(),
		from_version: 3,
		to_version: 4,
		error: None,
		}
		],
		);

		Ok(())
		}

		#[test]
		fn test_migrate_db_v2_to_v3_user_opted_in() -> Result<()> {
		fn test_migrate_db_from_v2_user_opted_in() -> Result<()> {
		error_support::init_for_tests();
		let tmp_dir = tempfile::tempdir()?;

		@@ -353,8 +379,8 @@ fn test_migrate_db_v2_to_v3_user_opted_in() -> Result<()> {
		let metrics = TestMetrics::new();
		let db = Database::new(&tmp_dir, metrics.clone())?;

		// Check the database was upgraded to v3
		assert_eq!(db.get(StoreId::Meta, DB_KEY_DB_VERSION)?, Some(3u16));
		// Check the database was upgraded to the latest version
		assert_eq!(db.get(StoreId::Meta, DB_KEY_DB_VERSION)?, Some(DB_VERSION));

		// Check that separate flags were set correctly for opted-in user
		let reader = db.read()?;
		@@ -377,19 +403,27 @@ fn test_migrate_db_v2_to_v3_user_opted_in() -> Result<()> {
		corrupt: Some(false),
		error: None,
		initial_version: Some(2),
		migrated_version: Some(3),
		migrated_version: Some(4),
		migration_error: None,
		}],
		);

		assert_eq!(
		metrics.get_database_migration_events(),
		[DatabaseMigrationExtraDef {
		[
		DatabaseMigrationExtraDef {
		reason: DatabaseMigrationReason::Upgrade.to_string(),
		from_version: 2,
		to_version: 3,
		error: None,
		}],
		},
		DatabaseMigrationExtraDef {
		reason: DatabaseMigrationReason::Upgrade.to_string(),
		from_version: 3,
		to_version: 4,
		error: None,
		}
		],
		);

		Ok(())
		@@ -405,7 +439,10 @@ fn test_migrate_empty() -> Result<()> {
		let db = Database::new(&tmp_dir, metrics.clone())?;
		let meta = db.get_store(StoreId::Meta);
		let reader = db.read()?;
		assert_eq!(meta.get::<u16, _>(&reader, DB_KEY_DB_VERSION)?, Some(3));
		assert_eq!(
		meta.get::<u16, _>(&reader, DB_KEY_DB_VERSION)?,
		Some(DB_VERSION)
		);
		assert_eq!(
		meta.get::<bool, _>(&reader, DB_KEY_GLOBAL_USER_PARTICIPATION)?,
		None
		@@ -425,7 +462,7 @@ fn test_migrate_empty() -> Result<()> {
		corrupt: Some(false),
		error: None,
		initial_version: Some(0),
		migrated_version: Some(3),
		migrated_version: Some(DB_VERSION),
		migration_error: None,
		}],
		);
		@@ -445,6 +482,12 @@ fn test_migrate_empty() -> Result<()> {
		to_version: 3,
		error: None,
		},
		DatabaseMigrationExtraDef {
		reason: DatabaseMigrationReason::Upgrade.to_string(),
		from_version: 3,
		to_version: 4,
		error: None,
		},
		],
		);

		@@ -452,7 +495,7 @@ fn test_migrate_empty() -> Result<()> {
		}

		#[test]
		fn test_migrate_db_v1_to_v3_cumulative_participation_enabled() -> Result<()> {
		fn test_migrate_db_from_v1_cumulative_participation_enabled() -> Result<()> {
		error_support::init_for_tests();

		let tmp_dir = tempfile::tempdir()?;
		@@ -472,7 +515,10 @@ fn test_migrate_db_v1_to_v3_cumulative_participation_enabled() -> Result<()> {
		let db = Database::new(&tmp_dir, metrics.clone())?;
		let meta = db.get_store(StoreId::Meta);
		let reader = db.read()?;
		assert_eq!(meta.get::<u16, _>(&reader, DB_KEY_DB_VERSION)?, Some(3));
		assert_eq!(
		meta.get::<u16, _>(&reader, DB_KEY_DB_VERSION)?,
		Some(DB_VERSION)
		);
		assert_eq!(
		meta.get::<bool, _>(&reader, DB_KEY_GLOBAL_USER_PARTICIPATION)?,
		None
		@@ -492,7 +538,7 @@ fn test_migrate_db_v1_to_v3_cumulative_participation_enabled() -> Result<()> {
		corrupt: Some(false),
		error: None,
		initial_version: Some(1),
		migrated_version: Some(3),
		migrated_version: Some(DB_VERSION),
		migration_error: None,
		}],
		);
		@@ -511,6 +557,12 @@ fn test_migrate_db_v1_to_v3_cumulative_participation_enabled() -> Result<()> {
		from_version: 2,
		to_version: 3,
		error: None,
		},
		DatabaseMigrationExtraDef {
		reason: DatabaseMigrationReason::Upgrade.to_string(),
		from_version: 3,
		to_version: 4,
		error: None,
		}
		],
		);
		@@ -519,7 +571,7 @@ fn test_migrate_db_v1_to_v3_cumulative_participation_enabled() -> Result<()> {
		}

		#[test]
		fn test_migrate_db_v1_to_v3_cumulative_participation_disabled() -> Result<()> {
		fn test_migrate_db_from_v1_cumulative_participation_disabled() -> Result<()> {
		error_support::init_for_tests();

		let tmp_dir = tempfile::tempdir()?;
		@@ -539,7 +591,10 @@ fn test_migrate_db_v1_to_v3_cumulative_participation_disabled() -> Result<()> {
		let db = Database::new(&tmp_dir, metrics.clone())?;
		let meta = db.get_store(StoreId::Meta);
		let reader = db.read()?;
		assert_eq!(meta.get::<u16, _>(&reader, DB_KEY_DB_VERSION)?, Some(3));
		assert_eq!(
		meta.get::<u16, _>(&reader, DB_KEY_DB_VERSION)?,
		Some(DB_VERSION)
		);
		assert_eq!(
		meta.get::<bool, _>(&reader, DB_KEY_GLOBAL_USER_PARTICIPATION)?,
		None
		@@ -559,7 +614,7 @@ fn test_migrate_db_v1_to_v3_cumulative_participation_disabled() -> Result<()> {
		corrupt: Some(false),
		error: None,
		initial_version: Some(1),
		migrated_version: Some(3),
		migrated_version: Some(DB_VERSION),
		migration_error: None,
		}],
		);
		@@ -578,6 +633,12 @@ fn test_migrate_db_v1_to_v3_cumulative_participation_disabled() -> Result<()> {
		from_version: 2,
		to_version: 3,
		error: None,
		},
		DatabaseMigrationExtraDef {
		reason: DatabaseMigrationReason::Upgrade.to_string(),
		from_version: 3,
		to_version: 4,
		error: None,
		}
		],
		);
		@@ -586,7 +647,7 @@ fn test_migrate_db_v1_to_v3_cumulative_participation_disabled() -> Result<()> {
		}

		#[test]
		fn test_migrate_db_v3_idempotent() -> Result<()> {
		fn test_migrate_db_idempotent() -> Result<()> {
		error_support::init_for_tests();

		let tmp_dir = tempfile::tempdir()?;
		@@ -596,7 +657,7 @@ fn test_migrate_db_v3_idempotent() -> Result<()> {
		let meta = SingleStore::new(rkv.open_single("meta", StoreOptions::create())?);

		let mut writer = rkv.write()?;
		meta.put(&mut writer, DB_KEY_DB_VERSION, &3)?;
		meta.put(&mut writer, DB_KEY_DB_VERSION, &DB_VERSION)?;
		meta.put(&mut writer, DB_KEY_EXPERIMENT_PARTICIPATION, &false)?;
		meta.put(&mut writer, DB_KEY_ROLLOUT_PARTICIPATION, &true)?;
		writer.commit()?;
		@@ -607,7 +668,10 @@ fn test_migrate_db_v3_idempotent() -> Result<()> {
		let db = Database::new(&tmp_dir, metrics.clone())?;
		let meta = db.get_store(StoreId::Meta);
		let reader = db.read()?;
		assert_eq!(meta.get::<u16, _>(&reader, DB_KEY_DB_VERSION)?, Some(3));
		assert_eq!(
		meta.get::<u16, _>(&reader, DB_KEY_DB_VERSION)?,
		Some(DB_VERSION)
		);
		assert_eq!(
		meta.get::<bool, _>(&reader, DB_KEY_GLOBAL_USER_PARTICIPATION)?,
		None
		@@ -626,7 +690,7 @@ fn test_migrate_db_v3_idempotent() -> Result<()> {
		[DatabaseLoadExtraDef {
		corrupt: Some(false),
		error: None,
		initial_version: Some(3),
		initial_version: Some(DB_VERSION),
		migrated_version: None,
		migration_error: None,
		}],