Skip to content

WIP: Bug 2037747 - Add per-feature update notifications to the Androind Nimbus client [ci full] #7354

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
freshstrangemusic wants to merge 1 commit into
base: main
Choose a base branch
from
+101 −18
Open

WIP: Bug 2037747 - Add per-feature update notifications to the Androind Nimbus client [ci full] #7354

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
89 changes: 81 additions & 8 deletions components/nimbus/android/src/main/java/org/mozilla/experiments/nimbus/Nimbus.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -179,6 +179,65 @@ open class Nimbus(
}
}

public val updates = object {
private val lock = ReentrantLock()
private val callbackMap: MutableMap<String, MutableSet<() -> Unit>> = mutableSetOf()

@AnyThread
public fun register(featureId: String, callback: () -> Unit) {
lock.runBlock {
callbackMap
.getOrPut(featureId, arrayListOf)
.add(callback)
}
}

@AnyThread
public fun unregister(featureId: String, callback: () -> Unit) {
lock.runBlock {
callbackMap.get(featureId)?.run { remove(callback) }
}
}

@AnyThread
public fun notifyChanged(events: List<EnrollmentChangeEvent>) {
if (events.isEmpty()) {
return
}

val featureIds: MutableSet<String> = mutableSetOf()

for (event in events) {
for (featureId in event.featureIds) {
featureIds.add(featureId)
}
}

notifyFeatures(featureIds)
}

@AnyThread
public fun notifyFeatures(featureIds: Set<String>) {
val toUpdate = arrayListOf()

lock.runBlock {
for (featureId in featureIds) {
callbackMap.get(featureId)?.also { callbacks ->
for (callback of callbacks) {
toUpdate.add(callback)
}
}
}
}

scope.launch(Dispatchers.Main) {
for (callback of toUpdate) {
callback()
}
}
}
}

init {
NullVariables.instance.setContext(context)

Expand Down Expand Up @@ -279,7 +338,7 @@ open class Nimbus(
@VisibleForTesting(otherwise = VisibleForTesting.PRIVATE)
internal fun initializeOnThisThread() = withCatchAll("initialize") {
nimbusClient.initialize()
postEnrolmentCalculation()
postEnrolmentCalculation(true)
}

override fun fetchExperiments() {
Expand Down Expand Up @@ -344,7 +403,8 @@ open class Nimbus(
NimbusHealth.applyPendingExperimentsTime.accumulateSingleSample(time)
recordExperimentTelemetryEvents(events!!)
// Get the experiments to record in telemetry
postEnrolmentCalculation()
postEnrolmentCalculation(false)
updates.notifyChanged(events!!)
} catch (e: NimbusException.InvalidExperimentFormat) {
reportError("Invalid experiment format", e)
}
Expand Down Expand Up @@ -379,11 +439,22 @@ open class Nimbus(
}

@WorkerThread
private fun postEnrolmentCalculation() {
nimbusClient.getActiveExperiments().let {
recordExperimentTelemetry(it)
private fun postEnrolmentCalculation(initial: Bool) {
nimbusClient.getActiveExperiments().also { experiments ->
recordExperimentTelemetry(experiments)
updateObserver { observer ->
observer.onUpdatesApplied(it)
observer.onUpdatesApplied(experiments)
}

if (initial) {
val featureIds = mutableSetOf()
for (experiment in experiments) {
for (featureId in experiment.featureIds) {
featureIds.add(featureId)
}
}

updates.notifyFeatures(featureIds)
}
}
}
Expand Down Expand Up @@ -431,7 +502,8 @@ open class Nimbus(
val enrolmentChanges = nimbusClient.setExperimentParticipation(active)
if (enrolmentChanges.isNotEmpty()) {
recordExperimentTelemetryEvents(enrolmentChanges)
postEnrolmentCalculation()
postEnrolmentCalculation(false)
updates.notifyChanged(enrolmentChanges)
}
}

Expand All @@ -442,7 +514,8 @@ open class Nimbus(
val enrolmentChanges = nimbusClient.setRolloutParticipation(active)
if (enrolmentChanges.isNotEmpty()) {
recordExperimentTelemetryEvents(enrolmentChanges)
postEnrolmentCalculation()
postEnrolmentCalculation(false)
updates.notifyChanged(enrolmentChanges)
}
}

Expand Down
23 changes: 14 additions & 9 deletions components/nimbus/src/enrollment.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -227,7 +227,7 @@ impl ExperimentEnrollment {
&enrollment.slug, &enrollment
);
if matches!(enrollment.status, EnrollmentStatus::Enrolled { .. }) {
out_enrollment_events.push(enrollment.get_change_event())
out_enrollment_events.push(enrollment.get_change_event(experiment))
}
enrollment
})
Expand All @@ -246,6 +246,7 @@ impl ExperimentEnrollment {
branch_slug: branch_slug.to_string(),
reason: Some("does-not-exist".to_string()),
change: EnrollmentChangeEventType::EnrollFailed,
feature_ids: experiment.get_feature_ids(),
});

return Err(NimbusError::NoSuchBranch(
Expand All @@ -257,7 +258,7 @@ impl ExperimentEnrollment {
slug: experiment.slug.clone(),
status: EnrollmentStatus::new_enrolled(EnrolledReason::OptIn, branch_slug),
};
out_enrollment_events.push(enrollment.get_change_event());
out_enrollment_events.push(enrollment.get_change_event(experiment));
Ok(enrollment)
}

Expand Down Expand Up @@ -287,7 +288,7 @@ impl ExperimentEnrollment {
&self.slug, &self, updated_enrollment
);
if matches!(updated_enrollment.status, EnrollmentStatus::Enrolled { .. }) {
out_enrollment_events.push(updated_enrollment.get_change_event());
out_enrollment_events.push(updated_enrollment.get_change_event(experiment));
}
updated_enrollment
}
Expand All @@ -303,15 +304,15 @@ impl ExperimentEnrollment {
self.maybe_revert_all_gecko_pref_states(gecko_pref_store);
let updated_enrollment =
self.disqualify_from_enrolled(DisqualifiedReason::OptOut);
out_enrollment_events.push(updated_enrollment.get_change_event());
out_enrollment_events.push(updated_enrollment.get_change_event(experiment));
updated_enrollment
} else if !updated_experiment.has_branch(branch) {
// The branch we were in disappeared!
#[cfg(feature = "stateful")]
self.maybe_revert_all_gecko_pref_states(gecko_pref_store);
let updated_enrollment =
self.disqualify_from_enrolled(DisqualifiedReason::Error);
out_enrollment_events.push(updated_enrollment.get_change_event());
out_enrollment_events.push(updated_enrollment.get_change_event(experiment));
updated_enrollment
} else if matches!(reason, EnrolledReason::OptIn) {
// we check if we opted-in an experiment, if so
Expand Down Expand Up @@ -341,7 +342,7 @@ impl ExperimentEnrollment {
EnrollmentStatus::Error { .. } => {
let updated_enrollment =
self.disqualify_from_enrolled(DisqualifiedReason::Error);
out_enrollment_events.push(updated_enrollment.get_change_event());
out_enrollment_events.push(updated_enrollment.get_change_event(experiment));
updated_enrollment
}
EnrollmentStatus::NotEnrolled {
Expand All @@ -359,7 +360,7 @@ impl ExperimentEnrollment {
);
let updated_enrollment =
self.disqualify_from_enrolled(DisqualifiedReason::NotTargeted);
out_enrollment_events.push(updated_enrollment.get_change_event());
out_enrollment_events.push(updated_enrollment.get_change_event(experiment));
updated_enrollment
}
EnrollmentStatus::NotEnrolled {
Expand All @@ -369,7 +370,7 @@ impl ExperimentEnrollment {
//
let updated_enrollment =
self.disqualify_from_enrolled(DisqualifiedReason::NotSelected);
out_enrollment_events.push(updated_enrollment.get_change_event());
out_enrollment_events.push(updated_enrollment.get_change_event(experiment));
updated_enrollment
}
EnrollmentStatus::NotEnrolled { .. }
Expand Down Expand Up @@ -455,6 +456,7 @@ impl ExperimentEnrollment {
#[cfg_attr(not(feature = "stateful"), allow(unused))]
pub(crate) fn on_explicit_opt_out(
&self,
experiment: &Experiment,
out_enrollment_events: &mut Vec<EnrollmentChangeEvent>,
#[cfg(feature = "stateful")] gecko_pref_store: Option<&GeckoPrefStore>,
) -> ExperimentEnrollment {
Expand All @@ -464,7 +466,7 @@ impl ExperimentEnrollment {
self.maybe_revert_all_gecko_pref_states(gecko_pref_store);

let enrollment = self.disqualify_from_enrolled(DisqualifiedReason::OptOut);
out_enrollment_events.push(enrollment.get_change_event());
out_enrollment_events.push(enrollment.get_change_event(experiment));
enrollment
}
EnrollmentStatus::NotEnrolled { .. } => Self {
Expand Down Expand Up @@ -994,6 +996,7 @@ impl<'a> EnrollmentsEvolver<'a> {
branch_slug: "N/A".to_string(),
reason: Some("feature-conflict".to_string()),
change: EnrollmentChangeEventType::EnrollFailed,
feature_ids: next_experiment.get_feature_ids(),
})
}
// Whether it's our experiment or not that is using these features, no further enrollment can
Expand Down Expand Up @@ -1152,6 +1155,7 @@ impl<'a> EnrollmentsEvolver<'a> {
)?),
// Experiment deleted remotely.
(Some(_), None, Some(enrollment)) => enrollment.on_experiment_ended(
prev_experiment,
#[cfg(feature = "stateful")]
gecko_pref_store,
out_enrollment_events,
Expand Down Expand Up @@ -1466,6 +1470,7 @@ pub struct EnrollmentChangeEvent {
pub branch_slug: String,
pub reason: Option<String>,
pub change: EnrollmentChangeEventType,
pub feature_ids: Vec<String>,
}

impl EnrollmentChangeEvent {
Expand Down
4 changes: 3 additions & 1 deletion components/nimbus/src/nimbus.udl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,8 @@ dictionary EnrollmentChangeEvent {
string branch_slug;
string? reason;
EnrollmentChangeEventType change;

sequence<string> feature_ids;
};

enum EnrollmentChangeEventType {
Expand Down Expand Up @@ -234,7 +236,7 @@ interface NimbusClient {
string dbpath,
MetricsHandler metrics_handler,
GeckoPrefHandler? gecko_pref_handler,
NimbusServerSettings? remote_settings_info
NimbusServerSettings? remote_settings_info,
);

/// Initializes the database and caches enough information so that the
Expand Down
3 changes: 3 additions & 0 deletions components/nimbus/src/stateful/enrollment.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,6 +128,7 @@ pub fn opt_in_with_branch(
branch_slug: branch.to_string(),
reason: Some("does-not-exist".to_string()),
change: EnrollmentChangeEventType::EnrollFailed,
feature_ids: exp.get_feature_ids(),
});
}

Expand All @@ -153,6 +154,7 @@ pub fn opt_out(
branch_slug: "N/A".to_string(),
reason: Some("does-not-exist".to_string()),
change: EnrollmentChangeEventType::UnenrollFailed,
feature_ids: vec![],
});
}

Expand Down Expand Up @@ -184,6 +186,7 @@ pub fn unenroll_for_pref(
branch_slug: "N/A".to_string(),
reason: Some("does-not-exist".to_string()),
change: EnrollmentChangeEventType::UnenrollFailed,
feature_ids: vec![],
});
}

Expand Down