commit ba045db40f9da6b6e095466ee06c4cad8d458537
parent 0a8ddc64d403f808586f4a867a6837dcf07d4121
Author: Alexandru Marc <amarc@mozilla.com>
Date: Tue, 6 Jan 2026 22:41:06 +0200
Revert "Bug 2006752 - Move enrollment status events to targeting context ping r=nimbus-reviewers,beth" for causing xpcshell failures @ test_nimbusTelemetry.js
This reverts commit 56b39b51d2f92c37c7c892f7c76282c7aa4b5f83.
Diffstat:
11 files changed, 95 insertions(+), 115 deletions(-)
diff --git a/toolkit/components/nimbus/lib/RemoteSettingsExperimentLoader.sys.mjs b/toolkit/components/nimbus/lib/RemoteSettingsExperimentLoader.sys.mjs
@@ -396,89 +396,82 @@ export class RemoteSettingsExperimentLoader {
// See-also: https://bugzilla.mozilla.org/show_bug.cgi?id=1936317
// See-also: https://bugzilla.mozilla.org/show_bug.cgi?id=1936319
if (lazy.TARGETING_CONTEXT_TELEMETRY_ENABLED) {
- await lazy.recordTargetingContext();
+ lazy.recordTargetingContext();
}
- try {
- // Since this method is async, the enabled pref could change between await
- // points. We don't want to half validate experiments, so we cache this to
- // keep it consistent throughout updating.
- const validationEnabled = this.validationEnabled;
-
- let recipeValidator;
-
- if (validationEnabled) {
- recipeValidator = new lazy.JsonSchema.Validator(
- await SCHEMAS.NimbusExperiment
- );
- }
-
- let allRecipes = null;
- try {
- allRecipes = await this.getRecipesFromAllCollections({
- forceSync,
- trigger,
- });
- } catch (e) {
- lazy.log.debug("Failed to update", e);
- }
+ // Since this method is async, the enabled pref could change between await
+ // points. We don't want to half validate experiments, so we cache this to
+ // keep it consistent throughout updating.
+ const validationEnabled = this.validationEnabled;
- if (allRecipes !== null) {
- const unenrolledExperimentSlugs = lazy.NimbusEnrollments
- .syncEnrollmentsEnabled
- ? await lazy.NimbusEnrollments.loadUnenrolledExperimentSlugsFromOtherProfiles()
- : undefined;
+ let recipeValidator;
- const enrollmentsCtx = new EnrollmentsContext(
- this.manager,
- recipeValidator,
- {
- validationEnabled,
- labsEnabled: lazy.ExperimentAPI.labsEnabled,
- studiesEnabled: lazy.ExperimentAPI.studiesEnabled,
- shouldCheckTargeting: true,
- unenrolledExperimentSlugs,
- }
- );
+ if (validationEnabled) {
+ recipeValidator = new lazy.JsonSchema.Validator(
+ await SCHEMAS.NimbusExperiment
+ );
+ }
- const { existingEnrollments, recipes } =
- this._partitionRecipes(allRecipes);
+ let allRecipes = null;
+ try {
+ allRecipes = await this.getRecipesFromAllCollections({
+ forceSync,
+ trigger,
+ });
+ } catch (e) {
+ lazy.log.debug("Failed to update", e);
+ }
- for (const { enrollment, recipe } of existingEnrollments) {
- const result = recipe
- ? await enrollmentsCtx.checkRecipe(recipe)
- : CheckRecipeResult.Ok(MatchStatus.NOT_SEEN);
+ if (allRecipes !== null) {
+ const unenrolledExperimentSlugs = lazy.NimbusEnrollments
+ .syncEnrollmentsEnabled
+ ? await lazy.NimbusEnrollments.loadUnenrolledExperimentSlugsFromOtherProfiles()
+ : undefined;
- await this.manager.updateEnrollment(
- enrollment,
- recipe,
- this.SOURCE,
- result
- );
+ const enrollmentsCtx = new EnrollmentsContext(
+ this.manager,
+ recipeValidator,
+ {
+ validationEnabled,
+ labsEnabled: lazy.ExperimentAPI.labsEnabled,
+ studiesEnabled: lazy.ExperimentAPI.studiesEnabled,
+ shouldCheckTargeting: true,
+ unenrolledExperimentSlugs,
}
+ );
- for (const recipe of recipes) {
- const result = await enrollmentsCtx.checkRecipe(recipe);
- await this.manager.onRecipe(recipe, this.SOURCE, result);
- }
+ const { existingEnrollments, recipes } =
+ this._partitionRecipes(allRecipes);
- lazy.log.debug(`${enrollmentsCtx.matches} recipes matched.`);
- }
+ for (const { enrollment, recipe } of existingEnrollments) {
+ const result = recipe
+ ? await enrollmentsCtx.checkRecipe(recipe)
+ : CheckRecipeResult.Ok(MatchStatus.NOT_SEEN);
- if (trigger !== "timer") {
- const lastUpdateTime = Math.round(Date.now() / 1000);
- Services.prefs.setIntPref(TIMER_LAST_UPDATE_PREF, lastUpdateTime);
+ await this.manager.updateEnrollment(
+ enrollment,
+ recipe,
+ this.SOURCE,
+ result
+ );
}
- if (allRecipes !== null) {
- // Enrollments have not changed, so we don't need to notify.
- Services.obs.notifyObservers(null, "nimbus:enrollments-updated");
- }
- } finally {
- if (lazy.TARGETING_CONTEXT_TELEMETRY_ENABLED) {
- // Submit targeting context ping after all enrollment status events should be generated
- GleanPings.nimbusTargetingContext.submit();
+ for (const recipe of recipes) {
+ const result = await enrollmentsCtx.checkRecipe(recipe);
+ await this.manager.onRecipe(recipe, this.SOURCE, result);
}
+
+ lazy.log.debug(`${enrollmentsCtx.matches} recipes matched.`);
+ }
+
+ if (trigger !== "timer") {
+ const lastUpdateTime = Math.round(Date.now() / 1000);
+ Services.prefs.setIntPref(TIMER_LAST_UPDATE_PREF, lastUpdateTime);
+ }
+
+ if (allRecipes !== null) {
+ // Enrollments have not changed, so we don't need to notify.
+ Services.obs.notifyObservers(null, "nimbus:enrollments-updated");
}
}
diff --git a/toolkit/components/nimbus/lib/TargetingContextRecorder.sys.mjs b/toolkit/components/nimbus/lib/TargetingContextRecorder.sys.mjs
@@ -401,4 +401,6 @@ export async function recordTargetingContext() {
// This will ensure that the profile group ID metric has been set.
await lazy.ClientID.getProfileGroupID();
+
+ GleanPings.nimbusTargetingContext.submit();
}
diff --git a/toolkit/components/nimbus/metrics.yaml b/toolkit/components/nimbus/metrics.yaml
@@ -1043,12 +1043,10 @@ nimbus_events:
- https://bugzilla.mozilla.org/show_bug.cgi?id=1817481
- https://bugzilla.mozilla.org/show_bug.cgi?id=1955169
- https://bugzilla.mozilla.org/show_bug.cgi?id=1997467
- - https://bugzilla.mozilla.org/show_bug.cgi?id=2006752
data_reviews:
- https://bugzilla.mozilla.org/show_bug.cgi?id=1817481
- https://bugzilla.mozilla.org/show_bug.cgi?id=1955169
- https://bugzilla.mozilla.org/show_bug.cgi?id=1997467
- - https://bugzilla.mozilla.org/show_bug.cgi?id=2006752
data_sensitivity:
- technical
notification_emails:
@@ -1056,7 +1054,6 @@ nimbus_events:
- project-nimbus@mozilla.com
expires: never
disabled: true
- send_in_pings: *targeting_context_pings
migration:
type: event
diff --git a/toolkit/components/nimbus/test/unit/test_ExperimentManager_enroll.js b/toolkit/components/nimbus/test/unit/test_ExperimentManager_enroll.js
@@ -359,7 +359,7 @@ add_task(async function test_failure_name_conflict() {
Assert.deepEqual(
Glean.nimbusEvents.enrollmentStatus
- .testGetValue("nimbus-targeting-context")
+ .testGetValue("events")
.map(ev => ev.extra),
[
{
@@ -708,7 +708,7 @@ add_task(async function test_forceEnroll_cleanup() {
Assert.deepEqual(
Glean.nimbusEvents.enrollmentStatus
- .testGetValue("nimbus-targeting-context")
+ .testGetValue("events")
?.map(ev => ev.extra),
[
{
diff --git a/toolkit/components/nimbus/test/unit/test_ExperimentManager_prefs.js b/toolkit/components/nimbus/test/unit/test_ExperimentManager_prefs.js
@@ -1869,9 +1869,7 @@ add_task(async function test_prefChange() {
}
const enrollmentStatusEvents = (
- Glean.nimbusEvents.enrollmentStatus.testGetValue(
- "nimbus-targeting-context"
- ) ?? []
+ Glean.nimbusEvents.enrollmentStatus.testGetValue("events") ?? []
).map(ev => ev.extra);
for (const expectedEvent of expectedEnrollmentStatusEvents) {
diff --git a/toolkit/components/nimbus/test/unit/test_ExperimentManager_unenroll.js b/toolkit/components/nimbus/test/unit/test_ExperimentManager_unenroll.js
@@ -363,7 +363,7 @@ add_task(async function test_unenroll_individualOptOut_statusTelemetry() {
Assert.deepEqual(
Glean.nimbusEvents.enrollmentStatus
- .testGetValue("nimbus-targeting-context")
+ .testGetValue("events")
?.map(ev => ev.extra),
[
{
@@ -408,7 +408,7 @@ add_task(async function testUnenrollBogusReason() {
Assert.deepEqual(
Glean.nimbusEvents.enrollmentStatus
- .testGetValue("nimbus-targeting-context")
+ .testGetValue("events")
?.map(ev => ev.extra),
[
{
diff --git a/toolkit/components/nimbus/test/unit/test_FirefoxLabs.js b/toolkit/components/nimbus/test/unit/test_FirefoxLabs.js
@@ -245,7 +245,7 @@ add_task(async function test_enroll() {
Assert.deepEqual(
Glean.nimbusEvents.enrollmentStatus
- .testGetValue("nimbus-targeting-context")
+ .testGetValue("events")
?.map(ev => ev.extra),
[
{
@@ -378,7 +378,7 @@ add_task(async function test_unenroll() {
Assert.deepEqual(
Glean.nimbusEvents.enrollmentStatus
- .testGetValue("nimbus-targeting-context")
+ .testGetValue("events")
?.map(ev => ev.extra),
[
{
diff --git a/toolkit/components/nimbus/test/unit/test_Migrations.js b/toolkit/components/nimbus/test/unit/test_Migrations.js
@@ -1706,7 +1706,7 @@ add_task(async function testGraduateFirefoxLabsAutoPip() {
Assert.deepEqual(
Glean.nimbusEvents.enrollmentStatus
- .testGetValue("nimbus-targeting-context")
+ .testGetValue("events")
.map(event => event.extra),
[
{
diff --git a/toolkit/components/nimbus/test/unit/test_RemoteSettingsExperimentLoader_updateRecipes.js b/toolkit/components/nimbus/test/unit/test_RemoteSettingsExperimentLoader_updateRecipes.js
@@ -1880,9 +1880,7 @@ add_task(async function test_updateRecipes_enrollmentStatus_telemetry() {
await loader.updateRecipes("test");
- const events = Glean.nimbusEvents.enrollmentStatus.testGetValue(
- "nimbus-targeting-context"
- );
+ const events = Glean.nimbusEvents.enrollmentStatus.testGetValue("events");
Assert.deepEqual(events?.map(ev => ev.extra) ?? [], [
{
@@ -2069,7 +2067,7 @@ add_task(async function test_updateRecipes_enrollmentStatus_notEnrolled() {
Assert.deepEqual(
Glean.nimbusEvents.enrollmentStatus
- .testGetValue("nimbus-targeting-context")
+ .testGetValue("events")
?.map(ev => ev.extra),
[
{
@@ -2446,7 +2444,7 @@ add_task(async function testUnenrolledInAnotherProfileBeforeUpdate() {
Assert.deepEqual(
Glean.nimbusEvents.enrollmentStatus
- .testGetValue("nimbus-targeting-context")
+ .testGetValue("events")
?.map(ev => ev.extra),
[
{
@@ -2616,7 +2614,7 @@ add_task(async function testUnenrolledInAnotherProfileBetweenUpdates() {
Assert.deepEqual(
Glean.nimbusEvents.enrollmentStatus
- .testGetValue("nimbus-targeting-context")
+ .testGetValue("events")
?.map(ev => ev.extra),
[
{
diff --git a/toolkit/components/nimbus/test/unit/test_TargetingContextRecorder.js b/toolkit/components/nimbus/test/unit/test_TargetingContextRecorder.js
@@ -59,14 +59,6 @@ async function setupTest({ ...args } = {}) {
}
/**
- * Call recordTargetingContext and submit nimbusTargetingContext ping
- */
-async function recordTargetingContextAndSubmit() {
- await recordTargetingContext();
- GleanPings.nimbusTargetingContext.submit();
-}
-
-/**
* Return an object containing each nimbus_targeting_context metric mapped to
* its recorded value.
*
@@ -214,7 +206,7 @@ add_task(async function testNimbusTargetingContextAllKeysPresent() {
`nimbusTargetingContext.${metric} was recorded ${JSON.stringify(values[metric])}`
);
}
- }, recordTargetingContextAndSubmit);
+ }, recordTargetingContext);
manager.unenroll("experiment");
manager.unenroll("rollout");
@@ -239,7 +231,7 @@ add_task(async function testNimbusTargetingEnvironmentUserSetPrefs() {
!prefs.includes("nimbus.testing.testSetString"),
"nimbus.testing.testInt is not set and not in telemetry"
);
- }, recordTargetingContextAndSubmit);
+ }, recordTargetingContext);
// This pref is a fallbackPref, so should not appear in the list.
Services.prefs.setIntPref("nimbus.testing.testInt", 123);
@@ -260,7 +252,7 @@ add_task(async function testNimbusTargetingEnvironmentUserSetPrefs() {
prefs.includes("nimbus.testing.testSetString"),
"nimbus.testing.testSetString is set and in telemetry"
);
- }, recordTargetingContextAndSubmit);
+ }, recordTargetingContext);
Services.prefs.deleteBranch("nimbus.testing.testInt");
Services.prefs.deleteBranch("nimbus.testing.testSetString");
@@ -283,7 +275,7 @@ add_task(async function testNimbusTargetingEnvironmentPrefValues() {
!Object.hasOwn(prefs, PREF_KEY),
`${PREF} not set and not present in telemetry`
);
- }, recordTargetingContextAndSubmit);
+ }, recordTargetingContext);
Services.prefs.getDefaultBranch(null).setStringPref(PREF, "default");
@@ -296,7 +288,7 @@ add_task(async function testNimbusTargetingEnvironmentPrefValues() {
"default",
`${PREF} set on the default branch and present in telemetry`
);
- }, recordTargetingContextAndSubmit);
+ }, recordTargetingContext);
Services.prefs.setStringPref(PREF, "user");
@@ -309,7 +301,7 @@ add_task(async function testNimbusTargetingEnvironmentPrefValues() {
"user",
`${PREF} set on the user branch and present in telemetry`
);
- }, recordTargetingContextAndSubmit);
+ }, recordTargetingContext);
Services.prefs.deleteBranch(PREF);
@@ -331,7 +323,7 @@ add_task(async function testExperimentMetrics() {
Assert.deepEqual(values.activeExperiments, []);
Assert.deepEqual(values.activeRollouts, []);
Assert.deepEqual(values.enrollmentsMap, []);
- }, recordTargetingContextAndSubmit);
+ }, recordTargetingContext);
await manager.enroll(
NimbusTestUtils.factories.recipe.withFeatureConfig("experiment-1", {
@@ -379,7 +371,7 @@ add_task(async function testExperimentMetrics() {
{ experimentSlug: "rollout-1", branchSlug: "rollout" },
].sort()
);
- }, recordTargetingContextAndSubmit);
+ }, recordTargetingContext);
manager.unenroll("experiment-1", { reason: "test" });
manager.unenroll("experiment-2", { reason: "test" });
@@ -400,7 +392,7 @@ add_task(async function testExperimentMetrics() {
{ experimentSlug: "rollout-1", branchSlug: "rollout" },
].sort()
);
- }, recordTargetingContextAndSubmit);
+ }, recordTargetingContext);
await cleanup();
});
@@ -431,7 +423,7 @@ add_task(async function testErrorMetrics() {
!Object.hasOwn(prefs, PREF_KEY),
`${PREF_KEY} not set and not present in telemetry`
);
- }, recordTargetingContextAndSubmit);
+ }, recordTargetingContext);
info(
"testing prefs with the wrong type are recorded in the pref_type_errors metric"
@@ -449,7 +441,7 @@ add_task(async function testErrorMetrics() {
!Object.hasOwn(prefs, PREF_KEY),
"nimbus.qa.pref-1 not set and not present in telemetry"
);
- }, recordTargetingContextAndSubmit);
+ }, recordTargetingContext);
Services.prefs.deleteBranch(PREF);
@@ -473,7 +465,7 @@ add_task(async function testErrorMetrics() {
assertRecordingFailures({
attrEvalErrors: ["currentDate", "isFirstStartup"],
});
- }, recordTargetingContextAndSubmit);
+ }, recordTargetingContext);
await cleanup();
@@ -522,7 +514,7 @@ add_task(async function testRecordingErrors() {
null,
"The targetingContextValue metric is not recorded by default."
);
- }, recordTargetingContextAndSubmit);
+ }, recordTargetingContext);
// We triggered glean to record error metrics. Ensure that we don't double count.
Services.fog.testResetFOG();
@@ -584,7 +576,7 @@ add_task(async function testRecordingErrors() {
],
"activeExperiments should have the invalid value in the targetingContextValue metric"
);
- }, recordTargetingContextAndSubmit);
+ }, recordTargetingContext);
await cleanup();
@@ -612,7 +604,7 @@ add_task(async function testAddonsInfo() {
[],
"No recorded addon info"
);
- }, recordTargetingContextAndSubmit);
+ }, recordTargetingContext);
const ext1 = ExtensionTestUtils.loadExtension({
useAddonManager: "temporary",
@@ -637,7 +629,7 @@ add_task(async function testAddonsInfo() {
"hasInstalledAddons is true"
);
Assert.deepEqual(values.addonsInfo.addons, [ext1.id], "Has one addon");
- }, recordTargetingContextAndSubmit);
+ }, recordTargetingContext);
const ext2 = ExtensionTestUtils.loadExtension({
useAddonManager: "temporary",
@@ -666,7 +658,7 @@ add_task(async function testAddonsInfo() {
[ext1.id, ext2.id].sort(),
"Has two addons"
);
- }, recordTargetingContextAndSubmit);
+ }, recordTargetingContext);
await ext1.unload();
await ext2.unload();
@@ -688,7 +680,7 @@ add_task(async function testAddonsInfo() {
[],
"No recorded addon info"
);
- }, recordTargetingContextAndSubmit);
+ }, recordTargetingContext);
await cleanup();
});
diff --git a/toolkit/components/nimbus/test/unit/test_prefFlips.js b/toolkit/components/nimbus/test/unit/test_prefFlips.js
@@ -2253,7 +2253,7 @@ add_task(async function test_prefFlip_setPref_restore() {
);
Assert.deepEqual(
Glean.nimbusEvents.enrollmentStatus
- .testGetValue("nimbus-targeting-context")
+ .testGetValue("events")
?.map(ev => ev.extra),
[
{
