[M112] Reland "[fyfre] Implement long-term cohort tracking for FRE st…

…udy" This reverts commit 6b9e4ce. Reason for revert: Fixed and re-landed the parent CL Original change's description: > Revert "[fyfre] Implement long-term cohort tracking for FRE study" > > This reverts commit 1b15ffb. > > Reason for revert: Linux ASan failures caused by parent CL (crrev.com/c/4280190). Example: https://ci.chromium.org/ui/p/chromium/builders/ci/Linux%20ASan%20LSan%20Tests%20(1)/112172/overview > > Original change's description: > > [fyfre] Implement long-term cohort tracking for FRE study > > > > Makes clients start registering with a study cohort (name of > > the cohort obtained via a new param of the ForYouFreStudy feature) > > when they attempt to run the FRE. The cohort and its name don't > > affect the behaviour of the client by themselves, this is done > > through the ForYouFre feature and its params. > > > > The cohort registration is done through a synthetic trial, where > > the group gets pulled from prefs, so that we can annotate sessions > > with the group name in subsequent startups to be able to track the > > outcome of a specific first run config in the long run. > > > > See http://go/for-you-fre-exp for more details. > > > > Bug: 1402712 > > Change-Id: I4d392cc9aaaaf31385fadda569d621d086611900 > > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4244081 > > Reviewed-by: David Roger <droger@chromium.org> > > Reviewed-by: Alex Ilin <alexilin@chromium.org> > > Commit-Queue: Nicolas Dossou-Gbété <dgn@chromium.org> > > Reviewed-by: Alexei Svitkine <asvitkine@chromium.org> > > Cr-Commit-Position: refs/heads/main@{#1109150} > > Bug: 1402712 > Change-Id: Ib5ff82fb95f39e6805fa52c2624e281994e8db3f > No-Presubmit: true > No-Tree-Checks: true > No-Try: true > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4284865 > Commit-Queue: Igor Ruvinov <igorruvinov@chromium.org> > Auto-Submit: Igor Ruvinov <igorruvinov@chromium.org> > Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com> > Owners-Override: Igor Ruvinov <igorruvinov@chromium.org> > Cr-Commit-Position: refs/heads/main@{#1109206} (cherry picked from commit 56a5a10) Bug: 1402712 Change-Id: I74274882373486d864ab634e8eecb0133e7b3473 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4291113 Reviewed-by: David Roger <droger@chromium.org> Commit-Queue: Nicolas Dossou-Gbété <dgn@chromium.org> Cr-Original-Commit-Position: refs/heads/main@{#1109564} Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4300632 Auto-Submit: Nicolas Dossou-Gbété <dgn@chromium.org> Cr-Commit-Position: refs/branch-heads/5615@{#128} Cr-Branched-From: 9c6408e-refs/heads/main@{#1109224}
blueboxd · Mar 2, 2023 · 33429e8 · 33429e8
1 parent fe6b2a0
commit 33429e8
Show file tree

Hide file tree

Showing 9 changed files with 227 additions and 8 deletions.
diff --git a/chrome/browser/chrome_browser_field_trials.cc b/chrome/browser/chrome_browser_field_trials.cc
@@ -20,10 +20,12 @@
 #include "chrome/browser/metrics/chrome_browser_sampling_trials.h"
 #include "chrome/browser/metrics/chrome_metrics_service_accessor.h"
 #include "chrome/browser/metrics/chrome_metrics_service_client.h"
+#include "chrome/browser/ui/startup/first_run_service.h"
 #include "chrome/common/chrome_paths.h"
 #include "chrome/common/chrome_switches.h"
 #include "components/metrics/metrics_pref_names.h"
 #include "components/metrics/persistent_histograms.h"
+#include "components/signin/public/base/signin_buildflags.h"
 #include "components/version_info/version_info.h"
 
 #if BUILDFLAG(IS_ANDROID)
@@ -179,4 +181,7 @@ void ChromeBrowserFieldTrials::RegisterSyntheticTrials() {
         fre_consistency_trial_variation_id_);
   }
 #endif  // BUILDFLAG(IS_ANDROID)
+#if BUILDFLAG(ENABLE_DICE_SUPPORT)
+  FirstRunService::EnsureStickToFirstRunCohort();
+#endif
 }
diff --git a/chrome/browser/metrics/chrome_metrics_service_accessor.h b/chrome/browser/metrics/chrome_metrics_service_accessor.h
@@ -136,6 +136,7 @@ class ChromeMetricsServiceAccessor : public metrics::MetricsServiceAccessor {
   friend class OptimizationGuideKeyedService;
   friend class WebUITabStripFieldTrial;
   friend class feed::FeedServiceDelegateImpl;
+  friend class FirstRunService;
   friend class browser_sync::DeviceInfoSyncClientImpl;
   friend class feed::WebFeedSubscriptionCoordinator;
   friend class HttpsFirstModeService;

diff --git a/chrome/browser/signin/signin_features.cc b/chrome/browser/signin/signin_features.cc
@@ -11,7 +11,7 @@
 // Enables the new style, "For You" First Run Experience
 BASE_FEATURE(kForYouFre, "ForYouFre", base::FEATURE_DISABLED_BY_DEFAULT);
 
-#if !BUILDFLAG(IS_CHROMEOS_LACROS)
+#if BUILDFLAG(ENABLE_DICE_SUPPORT)
 // Whether the browser should be opened when the user closes the FRE window. If
 // false, we just exit Chrome and the user will get straight to the browser on
 // the next process launch.
@@ -30,8 +30,19 @@ const base::FeatureParam<SigninPromoVariant> kForYouFreSignInPromoVariant{
     &kForYouFre, /*name=*/"signin_promo_variant",
     /*default_value=*/SigninPromoVariant::kSignIn,
     /*options=*/&kSignInPromoVariantOptions};
-#endif
-#endif
+
+// Feature that indicates that we should put the client in a study group to
+// be able to look at metrics in the long term. Does not affect the client's
+// behavior by itself, instead this is done through the `kForYouFre` feature.
+BASE_FEATURE(kForYouFreStudy,
+             "ForYouFreStudy",
+             base::FEATURE_DISABLED_BY_DEFAULT);
+// String that refers to the study group in which this install was enrolled.
+// Used to implement the sticky experiment tracking.
+const base::FeatureParam<std::string> kForYouFreStudyGroup{
+    &kForYouFreStudy, /*name=*/"group_name", /*default_value=*/""};
+#endif  // BUILDFLAG(ENABLE_DICE_SUPPORT)
+#endif  // !BUILDFLAG(IS_CHROMEOS_ASH) && !BUILDFLAG(IS_ANDROID)
 
 // Enables the client-side processing of the HTTP response header
 // Google-Accounts-RemoveLocalAccount.

diff --git a/chrome/browser/signin/signin_features.h b/chrome/browser/signin/signin_features.h
@@ -7,18 +7,23 @@
 
 #include "base/feature_list.h"
 #include "base/metrics/field_trial_params.h"
+#include "components/signin/public/base/signin_buildflags.h"
 
 #if !BUILDFLAG(IS_CHROMEOS_ASH) && !BUILDFLAG(IS_ANDROID)
 BASE_DECLARE_FEATURE(kForYouFre);
 
-#if !BUILDFLAG(IS_CHROMEOS_LACROS)
+#if BUILDFLAG(ENABLE_DICE_SUPPORT)
 extern const base::FeatureParam<bool> kForYouFreCloseShouldProceed;
 
 enum class SigninPromoVariant { kSignIn, kMakeYourOwn, kDoMore };
 extern const base::FeatureParam<SigninPromoVariant>
     kForYouFreSignInPromoVariant;
-#endif
-#endif
+
+BASE_DECLARE_FEATURE(kForYouFreStudy);
+
+extern const base::FeatureParam<std::string> kForYouFreStudyGroup;
+#endif  // BUILDFLAG(ENABLE_DICE_SUPPORT)
+#endif  // !BUILDFLAG(IS_CHROMEOS_ASH) && !BUILDFLAG(IS_ANDROID)
 
 BASE_DECLARE_FEATURE(kProcessGaiaRemoveLocalAccountHeader);
 

diff --git a/chrome/browser/ui/startup/first_run_service.cc b/chrome/browser/ui/startup/first_run_service.cc
@@ -17,6 +17,7 @@
 #include "base/notreached.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/first_run/first_run.h"
+#include "chrome/browser/metrics/chrome_metrics_service_accessor.h"
 #include "chrome/browser/policy/chrome_browser_policy_connector.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/profiles/profile_metrics.h"
@@ -34,6 +35,7 @@
 #include "components/signin/public/base/consent_level.h"
 #include "components/signin/public/base/signin_pref_names.h"
 #include "components/signin/public/identity_manager/identity_manager.h"
+#include "components/variations/synthetic_trials.h"
 #include "content/public/browser/browser_context.h"
 
 #if BUILDFLAG(IS_CHROMEOS_LACROS)
@@ -233,8 +235,68 @@ void OnFirstRunHasExited(ResumeTaskCallback original_intent_callback,
 // static
 void FirstRunService::RegisterLocalStatePrefs(PrefRegistrySimple* registry) {
   registry->RegisterBooleanPref(prefs::kFirstRunFinished, false);
+  registry->RegisterStringPref(prefs::kFirstRunStudyGroup, "");
 }
 
+#if BUILDFLAG(ENABLE_DICE_SUPPORT)
+// static
+void FirstRunService::EnsureStickToFirstRunCohort() {
+  PrefService* local_state = g_browser_process->local_state();
+  if (!local_state) {
+    return;  // Can be null in unit tests;
+  }
+
+  if (!IsFirstRunMarkedFinishedInPrefs()) {
+    // This user did not see the FRE. Their first run either happened before the
+    // feature was enabled, or it's happening right now. In the former case we
+    // don't enroll them, and in the latter, they will be enrolled right before
+    // starting the FRE.
+    return;
+  }
+
+  auto enrolled_study_group =
+      local_state->GetString(prefs::kFirstRunStudyGroup);
+  if (enrolled_study_group.empty()) {
+    // The user was not enrolled or exited the study at some point.
+    return;
+  }
+
+  RegisterSyntheticFieldTrial(enrolled_study_group);
+}
+
+// static
+void FirstRunService::JoinFirstRunCohort() {
+  PrefService* local_state = g_browser_process->local_state();
+  if (!local_state) {
+    return;  // Can be null in unit tests;
+  }
+
+  // The First Run experience depends on experiment groups (see params
+  // associated with the `kForYouFre` feature). To measure the long terms impact
+  // of this one-shot experience, we save an associated group name to prefs so
+  // we can report it as a synthetic trial for understanding the effects for
+  // each specific configuration, disambiguating it from other clients who had
+  // a different experience (or did not see the FRE for some reason).
+  std::string active_study_group = kForYouFreStudyGroup.Get();
+  if (active_study_group.empty()) {
+    return;  // No active study, no need to sign up.
+  }
+
+  local_state->SetString(prefs::kFirstRunStudyGroup, active_study_group);
+  RegisterSyntheticFieldTrial(active_study_group);
+}
+
+// static
+void FirstRunService::RegisterSyntheticFieldTrial(
+    const std::string& group_name) {
+  DCHECK(!group_name.empty());
+
+  ChromeMetricsServiceAccessor::RegisterSyntheticFieldTrial(
+      "ForYouFreSynthetic", group_name,
+      variations::SyntheticTrialAnnotationMode::kCurrentLog);
+}
+#endif  // BUILDFLAG(ENABLE_DICE_SUPPORT)
+
 FirstRunService::FirstRunService(Profile* profile) : profile_(profile) {}
 FirstRunService::~FirstRunService() = default;
 
@@ -391,6 +453,17 @@ KeyedService* FirstRunServiceFactory::BuildServiceInstanceFor(
   }
 
 #if BUILDFLAG(ENABLE_DICE_SUPPORT)
+  if (base::FeatureList::IsEnabled(kForYouFreStudy)) {
+    // We use this point to register for the study as it can give us a good
+    // counterfactual, before checking the state of the feature itself. The
+    // service is created on demand so we are in a code path that will require
+    // the FRE to be shown.
+    // Besides being suppressed by enterprise policy, if the FRE doesn't run, it
+    // would be related to handling some corner cases, and should not impact our
+    // metrics too much.
+    FirstRunService::JoinFirstRunCohort();
+  }
+
   if (!base::FeatureList::IsEnabled(kForYouFre)) {
     return nullptr;
   }

diff --git a/chrome/browser/ui/startup/first_run_service.h b/chrome/browser/ui/startup/first_run_service.h
@@ -13,6 +13,7 @@
 #include "build/chromeos_buildflags.h"
 #include "chrome/browser/profiles/profile_keyed_service_factory.h"
 #include "components/keyed_service/core/keyed_service.h"
+#include "components/signin/public/base/signin_buildflags.h"
 
 class PrefRegistrySimple;
 class Profile;
@@ -44,6 +45,13 @@ class FirstRunService : public KeyedService {
 
   static void RegisterLocalStatePrefs(PrefRegistrySimple* registry);
 
+#if BUILDFLAG(ENABLE_DICE_SUPPORT)
+  // Ensures that the user's experiment group is appropriately reported
+  // to track the effect of the first run experience over time. Should be called
+  // once per browser process startup.
+  static void EnsureStickToFirstRunCohort();
+#endif
+
   explicit FirstRunService(Profile* profile);
   ~FirstRunService() override;
 
@@ -73,6 +81,23 @@ class FirstRunService : public KeyedService {
 
  private:
   friend class FirstRunServiceFactory;
+#if BUILDFLAG(ENABLE_DICE_SUPPORT)
+  // Enrolls this client with a synthetic field trial based on the Finch params.
+  // Should be called when the FRE is launched, then the client needs to
+  // register again on each process startup by calling
+  // `RegisterSyntheticFieldTrial()`.
+  static void JoinFirstRunCohort();
+
+  // Reports to the launch study for the First Run rollout.
+  // Notes:
+  // - This is declared here so it can have access to some private functions
+  // that need to be friended to be used.
+  // - The function is Dice-only as on Lacros (where this build flag is not set)
+  // the ForYouFre feature rollout will not go through this study process. The
+  // feature only guards an internal refactoring that does not have a
+  // user-visible effect. If will only have a killswitch.
+  static void RegisterSyntheticFieldTrial(const std::string& group_name);
+#endif
 
   // Asynchronously attempts to complete the first run silently.
   // By the time `callback` is run (if non-null), either:

diff --git a/chrome/browser/ui/startup/first_run_service_browsertest.cc b/chrome/browser/ui/startup/first_run_service_browsertest.cc
@@ -33,6 +33,7 @@
 #include "chrome/common/pref_names.h"
 #include "chrome/common/webui_url_constants.h"
 #include "chrome/test/base/in_process_browser_test.h"
+#include "components/metrics/metrics_service.h"
 #include "components/policy/core/common/mock_configuration_policy_provider.h"
 #include "components/policy/core/common/policy_namespace.h"
 #include "components/policy/core/common/policy_service.h"
@@ -42,10 +43,12 @@
 #include "components/signin/public/identity_manager/identity_manager.h"
 #include "components/signin/public/identity_manager/identity_test_environment.h"
 #include "components/signin/public/identity_manager/primary_account_mutator.h"
+#include "components/variations/synthetic_trials_active_group_id_provider.h"
 #include "content/public/browser/web_contents.h"
 #include "content/public/test/browser_test.h"
 #include "google_apis/gaia/core_account_id.h"
 #include "testing/gtest/include/gtest/gtest.h"
+#include "third_party/abseil-cpp/absl/types/optional.h"
 #include "ui/views/controls/webview/webview.h"
 
 #if BUILDFLAG(IS_CHROMEOS_LACROS)
@@ -105,8 +108,9 @@ void SetIsFirstRun(bool is_first_run) {
   // browser that the test fixtures rely on.
   // So are manipulating flags here instead of during `SetUpX` methods on
   // purpose.
-  if (first_run::IsChromeFirstRun() == is_first_run)
+  if (first_run::IsChromeFirstRun() == is_first_run) {
     return;
+  }
 
   if (is_first_run) {
     // This switch is added by InProcessBrowserTest
@@ -215,6 +219,13 @@ IN_PROC_BROWSER_TEST_F(FirstRunServiceBrowserTest,
   profiles::testing::WaitForPickerWidgetCreated();
   EXPECT_FALSE(GetFirstRunFinishedPrefValue());
 
+  // We don't expect synthetic trials to be registered here, since no group
+  // is configured with the feature. For the positive test case, see
+  // `FirstRunServiceCohortBrowserTest.GroupRegisteredAfterFre`.
+  PrefService* local_state = g_browser_process->local_state();
+  EXPECT_FALSE(local_state->HasPrefPath(prefs::kFirstRunStudyGroup));
+  EXPECT_FALSE(variations::HasSyntheticTrial("ForYouFreSynthetic"));
+
   ProfilePicker::Hide();
   run_loop.Run();
 
@@ -385,7 +396,89 @@ IN_PROC_BROWSER_TEST_F(FirstRunServiceNotForYouBrowserTest,
   EXPECT_TRUE(ShouldOpenFirstRun(profile()));
   EXPECT_EQ(nullptr, fre_service());
 }
-#endif
+
+class FirstRunServiceCohortBrowserTest : public FirstRunServiceBrowserTest {
+ public:
+  static constexpr char kStudyTestGroupName1[] = "test_group_1";
+  static constexpr char kStudyTestGroupName2[] = "test_group_2";
+
+  FirstRunServiceCohortBrowserTest() {
+    variations::SyntheticTrialsActiveGroupIdProvider::GetInstance()
+        ->ResetForTesting();
+
+    scoped_feature_list_.InitWithFeaturesAndParameters(
+        {
+            {kForYouFreStudy, {{"group_name", kStudyTestGroupName1}}},
+            {kForYouFre, {}},
+        },
+        {});
+  }
+
+ private:
+  base::test::ScopedFeatureList scoped_feature_list_;
+};
+
+IN_PROC_BROWSER_TEST_F(FirstRunServiceCohortBrowserTest,
+                       PRE_GroupRegisteredAfterFre) {
+  EXPECT_TRUE(ShouldOpenFirstRun(browser()->profile()));
+
+  // We don't expect the synthetic trial to be registered before the FRE runs.
+  PrefService* local_state = g_browser_process->local_state();
+  EXPECT_FALSE(local_state->HasPrefPath(prefs::kFirstRunStudyGroup));
+  EXPECT_FALSE(variations::HasSyntheticTrial("ForYouFreSynthetic"));
+
+  base::RunLoop run_loop;
+  fre_service()->OpenFirstRunIfNeeded(
+      FirstRunService::EntryPoint::kOther,
+      ExpectProceed(true).Then(run_loop.QuitClosure()));
+
+  // Opening the FRE triggers recording of the group.
+  EXPECT_EQ(kStudyTestGroupName1,
+            local_state->GetString(prefs::kFirstRunStudyGroup));
+  EXPECT_TRUE(variations::HasSyntheticTrial("ForYouFreSynthetic"));
+  EXPECT_TRUE(variations::IsInSyntheticTrialGroup("ForYouFreSynthetic",
+                                                  kStudyTestGroupName1));
+
+  profiles::testing::WaitForPickerWidgetCreated();
+  ProfilePicker::Hide();
+  profiles::testing::WaitForPickerClosed();
+  run_loop.Run();
+}
+IN_PROC_BROWSER_TEST_F(FirstRunServiceCohortBrowserTest,
+                       GroupRegisteredAfterFre) {
+  EXPECT_FALSE(ShouldOpenFirstRun(browser()->profile()));
+
+  PrefService* local_state = g_browser_process->local_state();
+  EXPECT_EQ(kStudyTestGroupName1,
+            local_state->GetString(prefs::kFirstRunStudyGroup));
+  EXPECT_TRUE(variations::IsInSyntheticTrialGroup("ForYouFreSynthetic",
+                                                  kStudyTestGroupName1));
+}
+
+IN_PROC_BROWSER_TEST_F(FirstRunServiceCohortBrowserTest,
+                       PRE_PRE_GroupViaPrefs) {
+  // Setting the pref, we expect it to get picked up in an upcoming startup.
+  PrefService* local_state = g_browser_process->local_state();
+  local_state->SetString(prefs::kFirstRunStudyGroup, kStudyTestGroupName2);
+
+  EXPECT_FALSE(variations::HasSyntheticTrial("ForYouFreSynthetic"));
+}
+IN_PROC_BROWSER_TEST_F(FirstRunServiceCohortBrowserTest, PRE_GroupViaPrefs) {
+  // The synthetic group should not be registered yet since we didn't go through
+  // the FRE.
+  EXPECT_FALSE(variations::HasSyntheticTrial("ForYouFreSynthetic"));
+
+  // Setting this should make the next run finally register the synthetic trial.
+  PrefService* local_state = g_browser_process->local_state();
+  local_state->SetBoolean(prefs::kFirstRunFinished, true);
+}
+IN_PROC_BROWSER_TEST_F(FirstRunServiceCohortBrowserTest, GroupViaPrefs) {
+  // The registered group is read from the prefs, not from the feature param.
+  EXPECT_TRUE(variations::IsInSyntheticTrialGroup("ForYouFreSynthetic",
+                                                  kStudyTestGroupName2));
+}
+
+#endif  // BUILDFLAG(ENABLE_DICE_SUPPORT)
 
 struct PolicyTestParam {
   const std::string test_suffix;

diff --git a/chrome/common/pref_names.cc b/chrome/common/pref_names.cc
@@ -1630,6 +1630,11 @@ const char kWebRtcTextLogCollectionAllowed[] =
 // Boolean that indicates that the first run experience has been finished (or
 // skipped by some policy) for this browser install.
 const char kFirstRunFinished[] = "browser.first_run_finished";
+
+// String that refers to the study group in which this install was enrolled.
+// Used to implement the sticky experiment tracking.
+// TODO(crbug.com/1418017): Clean up experiment setup.
+const char kFirstRunStudyGroup[] = "browser.first_run_study_group";
 #endif
 
 #if !BUILDFLAG(IS_ANDROID)

diff --git a/chrome/common/pref_names.h b/chrome/common/pref_names.h
@@ -538,6 +538,7 @@ extern const char kWebRtcTextLogCollectionAllowed[];
 
 #if BUILDFLAG(IS_CHROMEOS_LACROS) || BUILDFLAG(ENABLE_DICE_SUPPORT)
 extern const char kFirstRunFinished[];
+extern const char kFirstRunStudyGroup[];
 #endif
 
 #if !BUILDFLAG(IS_ANDROID)