Skip to content

Commit

Permalink
update for pr feedback
Browse files Browse the repository at this point in the history
  • Loading branch information
@jeddai
jeddai committed Jul 12, 2023
1 parent ed57214 commit 7b81a76
Show file tree
Hide file tree
Showing 5 changed files with 150 additions and 76 deletions.
3 changes: 1 addition & 2 deletions CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,12 +21,11 @@

### 🦊 What's Changed 🦊

- When a rollout audience size decreases, the enrollment is re-evaluated and the client is unenrolled if no longer in the bucket. ([#5687](https://github.com/mozilla/application-services/pull/5687)).
- When a rollout audience size changes, the enrollment is re-evaluated and the client is un-enrolled if no longer in the bucket or re-enrolled if they had previously been disqualified from bucketing. ([#5687](https://github.com/mozilla/application-services/pull/5687), [#5716](https://github.com/mozilla/application-services/pull/5716)).
- The record of enrollment will be available in the new `enrollments` targeting attribute.
- Add `enrollments` value to `TargetingAttributes` — it is a set of strings containing all enrollments, past and present ([#5685](https://github.com/mozilla/application-services/pull/5685)).
- _Note: This change only applies to stateful uses of the Nimbus SDK, e.g. mobile_
- Add ability to enroll selected features multiple times (coenrollment) ([#5684](https://github.com/mozilla/application-services/pull/5684), [#5697](https://github.com/mozilla/application-services/pull/5697)).
- Update enrollment logic to re-evaluate enrollment for rollouts ([#5716](https://github.com/mozilla/application-services/pull/5716)).

### ⚠️ Breaking Changes ⚠️

Expand Down
13 changes: 5 additions & 8 deletions components/nimbus/src/enrollment.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -169,7 +169,7 @@ impl ExperimentEnrollment {
targeting_helper: &NimbusTargetingHelper,
out_enrollment_events: &mut Vec<EnrollmentChangeEvent>,
) -> Result<Self> {
Ok(match self.status {
Ok(match &self.status {
EnrollmentStatus::NotEnrolled { .. } | EnrollmentStatus::Error { .. } => {
if !is_user_participating || updated_experiment.is_enrollment_paused {
self.clone()
Expand Down Expand Up @@ -260,6 +260,7 @@ impl ExperimentEnrollment {
EnrollmentStatus::Disqualified {
ref branch,
enrollment_id,
reason,
..
} => {
if !is_user_participating {
Expand All @@ -271,18 +272,14 @@ impl ExperimentEnrollment {
slug: self.slug.clone(),
status: EnrollmentStatus::Disqualified {
reason: DisqualifiedReason::OptOut,
enrollment_id,
enrollment_id: *enrollment_id,
branch: branch.clone(),
},
}
} else if updated_experiment.is_rollout
&& matches!(
self.status,
EnrollmentStatus::Disqualified {
reason: DisqualifiedReason::NotSelected
| DisqualifiedReason::NotTargeted,
..
}
reason,
DisqualifiedReason::NotSelected | DisqualifiedReason::NotTargeted,
)
{
let evaluated_enrollment = evaluate_enrollment(
Expand Down
7 changes: 1 addition & 6 deletions components/nimbus/src/nimbus_client.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@
// License, v. 2.0. If a copy of the MPL was not distributed with this
// file, You can obtain one at https://mozilla.org/MPL/2.0/.

use crate::enrollment::DisqualifiedReason;
use crate::{
behavior::EventStore,
client::{create_client, SettingsClient},
Expand Down Expand Up @@ -326,11 +325,7 @@ impl NimbusClient {
is_enrolled_set.insert(ee.slug.clone());
all_enrolled_set.insert(ee.slug.clone());
}
EnrollmentStatus::WasEnrolled { .. }
| EnrollmentStatus::Disqualified {
reason: DisqualifiedReason::NotSelected | DisqualifiedReason::NotTargeted,
..
} => {
EnrollmentStatus::WasEnrolled { .. } | EnrollmentStatus::Disqualified { .. } => {
all_enrolled_set.insert(ee.slug.clone());
}
_ => {}
Expand Down
49 changes: 49 additions & 0 deletions components/nimbus/src/tests/helpers.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ use crate::{
AppContext, EnrollmentStatus, Experiment, FeatureConfig, NimbusTargetingHelper,
TargetingAttributes,
};
use serde::Serialize;
use serde_json::{json, Value};
use std::collections::HashSet;
use uuid::Uuid;
Expand Down Expand Up @@ -385,3 +386,51 @@ pub fn get_multi_feature_experiment(
pub fn no_coenrolling_features() -> HashSet<&'static str> {
Default::default()
}

#[cfg_attr(not(feature = "stateful"), allow(unused))]
pub fn to_local_experiments_string<S>(experiments: &[S]) -> crate::Result<String>
where
S: Serialize,
{
Ok(serde_json::to_string(&json!({ "data": experiments }))?)
}

#[cfg_attr(not(feature = "stateful"), allow(unused))]
pub fn get_targeted_experiment(slug: &str, targeting: &str) -> serde_json::Value {
json!({
"schemaVersion": "1.0.0",
"slug": slug,
"endDate": null,
"featureIds": ["some-feature-1"],
"branches": [
{
"slug": "control",
"ratio": 1
},
{
"slug": "treatment",
"ratio": 1
}
],
"channel": "nightly",
"probeSets": [],
"startDate": null,
"appName": "fenix",
"appId": "org.mozilla.fenix",
"bucketConfig": {
"count": 10000,
"start": 0,
"total": 10000,
"namespace": "secure-gold",
"randomizationUnit": "nimbus_id"
},
"targeting": targeting,
"userFacingName": "test experiment",
"referenceBranch": "control",
"isEnrollmentPaused": false,
"proposedEnrollment": 7,
"userFacingDescription": "This is a test experiment for testing purposes.",
"id": "secure-copper",
"last_modified": 1_602_197_324_372i64,
})
}
154 changes: 94 additions & 60 deletions components/nimbus/src/tests/stateful/test_nimbus.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,10 @@
* 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/. */

use crate::tests::helpers::get_bucketed_rollout;
use crate::enrollment::DisqualifiedReason;
use crate::tests::helpers::{
get_bucketed_rollout, get_targeted_experiment, to_local_experiments_string,
};
use crate::{
behavior::{
EventStore, Interval, IntervalConfig, IntervalData, MultiIntervalCounter,
Expand Down Expand Up @@ -1007,49 +1010,6 @@ fn test_fetch_enabled() -> Result<()> {
Ok(())
}

fn generate_experiment(slug: &str, targeting: &str) -> serde_json::Value {
json!({
"schemaVersion": "1.0.0",
"slug": slug,
"endDate": null,
"featureIds": ["some-feature-1"],
"branches": [
{
"slug": "control",
"ratio": 1
},
{
"slug": "treatment",
"ratio": 1
}
],
"channel": "nightly",
"probeSets": [],
"startDate": null,
"appName": "fenix",
"appId": "org.mozilla.fenix",
"bucketConfig": {
"count": 10000,
"start": 0,
"total": 10000,
"namespace": "secure-gold",
"randomizationUnit": "nimbus_id"
},
"targeting": targeting,
"userFacingName": "test experiment",
"referenceBranch": "control",
"isEnrollmentPaused": false,
"proposedEnrollment": 7,
"userFacingDescription": "This is a test experiment for testing purposes.",
"id": "secure-copper",
"last_modified": 1_602_197_324_372i64,
})
}

fn experiments_to_nimbus_string(experiments: Vec<serde_json::Value>) -> Result<String> {
Ok(serde_json::to_string(&json!({ "data": experiments }))?)
}

#[test]
fn test_active_enrollment_in_targeting() -> Result<()> {
let mock_client_id = "client-1".to_string();
Expand Down Expand Up @@ -1080,8 +1040,8 @@ fn test_active_enrollment_in_targeting() -> Result<()> {
client.initialize()?;

// Apply an initial experiment
let exp = generate_experiment("test-1", "true");
client.set_experiments_locally(experiments_to_nimbus_string(vec![exp])?)?;
let exp = get_targeted_experiment("test-1", "true");
client.set_experiments_locally(to_local_experiments_string(&[exp])?)?;
client.apply_pending_experiments()?;

let active_experiments = client.get_active_experiments()?;
Expand All @@ -1091,8 +1051,8 @@ fn test_active_enrollment_in_targeting() -> Result<()> {
assert!(targeting_helper.eval_jexl("'test-1' in active_experiments".to_string())?);

// Apply experiment that targets the above experiment is in enrollments
let exp = generate_experiment("test-2", "'test-1' in enrollments");
client.set_experiments_locally(experiments_to_nimbus_string(vec![exp])?)?;
let exp = get_targeted_experiment("test-2", "'test-1' in enrollments");
client.set_experiments_locally(to_local_experiments_string(&[exp])?)?;
client.apply_pending_experiments()?;

let active_experiments = client.get_active_experiments()?;
Expand All @@ -1113,9 +1073,11 @@ fn test_previous_enrollments_in_targeting() -> Result<()> {

let temp_dir = tempfile::tempdir()?;

let slug_1 = "test-1";
let slug_2 = "test-2";
let slug_3 = "ro-1";
let slug_1 = "experiment-1-was-enrolled";
let slug_2 = "experiment-2-dq-not-targeted";
let slug_3 = "experiment-3-dq-error";
let slug_4 = "experiment-4-dq-opt-out";
let slug_5 = "rollout-1-dq-not-selected";

let app_context = AppContext {
app_name: "fenix".to_string(),
Expand All @@ -1133,6 +1095,7 @@ fn test_previous_enrollments_in_targeting() -> Result<()> {
..AvailableRandomizationUnits::default()
},
)?;

let targeting_attributes = TargetingAttributes {
app_context,
..Default::default()
Expand All @@ -1141,42 +1104,113 @@ fn test_previous_enrollments_in_targeting() -> Result<()> {
client.initialize()?;

// Apply an initial experiment
let exp_1 = generate_experiment(slug_1, "true");
let exp_2 = generate_experiment(slug_2, "true");
let ro_1 = get_bucketed_rollout(slug_3, 10_000);
client.set_experiments_locally(experiments_to_nimbus_string(vec![
let exp_1 = get_targeted_experiment(slug_1, "true");
let exp_2 = get_targeted_experiment(slug_2, "true");
let exp_3 = get_targeted_experiment(slug_3, "true");
let exp_4 = get_targeted_experiment(slug_4, "true");
let ro_1 = get_bucketed_rollout(slug_5, 10_000);
client.set_experiments_locally(to_local_experiments_string(&[
exp_1,
exp_2,
exp_3,
exp_4,
serde_json::to_value(ro_1)?,
])?)?;
client.apply_pending_experiments()?;

let active_experiments = client.get_active_experiments()?;
assert_eq!(active_experiments.len(), 3);
assert_eq!(active_experiments.len(), 5);

let targeting_helper = client.create_targeting_helper(None)?;
assert!(targeting_helper.eval_jexl(format!("'{}' in active_experiments", slug_1))?);
assert!(targeting_helper.eval_jexl(format!("'{}' in active_experiments", slug_2))?);
assert!(targeting_helper.eval_jexl(format!("'{}' in active_experiments", slug_3))?);
assert!(targeting_helper.eval_jexl(format!("'{}' in active_experiments", slug_4))?);
assert!(targeting_helper.eval_jexl(format!("'{}' in active_experiments", slug_5))?);
assert!(targeting_helper.eval_jexl(format!("'{}' in enrollments", slug_1))?);
assert!(targeting_helper.eval_jexl(format!("'{}' in enrollments", slug_2))?);
assert!(targeting_helper.eval_jexl(format!("'{}' in enrollments", slug_3))?);
assert!(targeting_helper.eval_jexl(format!("'{}' in enrollments", slug_4))?);
assert!(targeting_helper.eval_jexl(format!("'{}' in enrollments", slug_5))?);

// Apply empty first experiment, disqualifying second experiment, and decreased bucket rollout
let exp_2 = generate_experiment(slug_2, "false");
let ro_1 = get_bucketed_rollout(slug_3, 0);
let experiment_json =
serde_json::to_string(&json!({"data": [exp_2, serde_json::to_value(ro_1)?]}))?;
let exp_2 = get_targeted_experiment(slug_2, "false");
let exp_3 = get_targeted_experiment(slug_3, "error_out");
let exp_4 = get_targeted_experiment(slug_4, "true");
let ro_1 = get_bucketed_rollout(slug_5, 0);
let experiment_json = serde_json::to_string(
&json!({"data": [exp_2, exp_3, exp_4, serde_json::to_value(ro_1)?]}),
)?;
client.set_experiments_locally(experiment_json)?;
client.apply_pending_experiments()?;
client.opt_out(slug_4.to_string())?;

let active_experiments = client.get_active_experiments()?;
assert_eq!(active_experiments.len(), 0);
let db = client.db()?;
let enrollments: Vec<ExperimentEnrollment> = db
.get_store(StoreId::Enrollments)
.collect_all(&db.write()?)?;
assert_eq!(enrollments.len(), 5);
assert!(matches!(
enrollments.get(0).unwrap(),
ExperimentEnrollment {
slug: _slug_1,
status: EnrollmentStatus::WasEnrolled { .. }
}
));
assert!(matches!(
enrollments.get(1).unwrap(),
ExperimentEnrollment {
slug: _slug_2,
status: EnrollmentStatus::Disqualified {
reason: DisqualifiedReason::NotTargeted,
..
}
}
));
assert!(matches!(
enrollments.get(2).unwrap(),
ExperimentEnrollment {
slug: _slug_3,
status: EnrollmentStatus::Disqualified {
reason: DisqualifiedReason::Error,
..
}
}
));
assert!(matches!(
enrollments.get(3).unwrap(),
ExperimentEnrollment {
slug: _slug_4,
status: EnrollmentStatus::Disqualified {
reason: DisqualifiedReason::OptOut,
..
}
}
));
assert!(matches!(
enrollments.get(4).unwrap(),
ExperimentEnrollment {
slug: _slug_5,
status: EnrollmentStatus::Disqualified {
reason: DisqualifiedReason::NotSelected,
..
}
}
));

let targeting_helper = client.create_targeting_helper(None)?;
assert!(!targeting_helper.eval_jexl(format!("'{}' in active_experiments", slug_1))?);
assert!(!targeting_helper.eval_jexl(format!("'{}' in active_experiments", slug_2))?);
assert!(!targeting_helper.eval_jexl(format!("'{}' in active_experiments", slug_3))?);
assert!(!targeting_helper.eval_jexl(format!("'{}' in active_experiments", slug_4))?);
assert!(!targeting_helper.eval_jexl(format!("'{}' in active_experiments", slug_5))?);
assert!(targeting_helper.eval_jexl(format!("'{}' in enrollments", slug_1))?);
assert!(targeting_helper.eval_jexl(format!("'{}' in enrollments", slug_2))?);
assert!(targeting_helper.eval_jexl(format!("'{}' in enrollments", slug_3))?);
assert!(targeting_helper.eval_jexl(format!("'{}' in enrollments", slug_4))?);
assert!(targeting_helper.eval_jexl(format!("'{}' in enrollments", slug_5))?);

Ok(())
}

0 comments on commit 7b81a76

Please sign in to comment.