Append suffix to synthetic trials when writing to proto

Part [2/2]

Part 1 CL: crrev.com/c/3615130

This resolves the bug where the "UKM" suffix is not appended to
synthetic trial and group names when building UKM reports.

This CL also does some minor refactoring in
field_trials_provider_unittest.cc so that the field trials under test
are actually registered.

Bug: 1320847,b/227744487
Change-Id: Ieee8007e4d87ce8d832cf74b5614097b58e06581
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3629980
Commit-Queue: Luc Nguyen <lucnguyen@google.com>
Reviewed-by: Robert Kaplow <rkaplow@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1002774}

components/metrics/field_trials_provider.cc

@@ -35,7 +35,6 @@ FieldTrialsProvider::~FieldTrialsProvider() = default;
 
 void FieldTrialsProvider::GetFieldTrialIds(
     std::vector<ActiveGroupId>* field_trial_ids) const {
-  // We use the default field trial suffixing (no suffix).
   variations::GetFieldTrialActiveGroupIds(suffix_, field_trial_ids);
 }
 
@@ -89,7 +88,7 @@ void FieldTrialsProvider::GetAndWriteFieldTrials(
   if (registry_) {
     std::vector<ActiveGroupId> synthetic_trials;
     registry_->GetSyntheticFieldTrialsOlderThan(log_creation_time_,
-                                                &synthetic_trials);
+                                                &synthetic_trials, suffix_);
     WriteFieldTrials(synthetic_trials, system_profile_proto);
   }
 }

components/metrics/field_trials_provider.h

@@ -43,9 +43,9 @@ class FieldTrialsProvider : public metrics::MetricsProvider {
   void SetLogCreationTimeForTesting(base::TimeTicks time);
 
  private:
-  // Overrideable for testing.
-  virtual void GetFieldTrialIds(
-      std::vector<ActiveGroupId>* field_trial_ids) const;
+  // Populates |field_trial_ids| with currently active field trials groups. The
+  // trial and group names are suffixed with |suffix_| before being hashed.
+  void GetFieldTrialIds(std::vector<ActiveGroupId>* field_trial_ids) const;
 
   // Gets active FieldTrials and SyntheticFieldTrials and populates
   // |system_profile_proto| with them.

components/metrics/field_trials_provider_unittest.cc

@@ -4,45 +4,44 @@
 
 #include "components/metrics/field_trials_provider.h"
 
+#include "base/metrics/field_trial.h"
 #include "base/threading/platform_thread.h"
 #include "components/variations/active_field_trials.h"
 #include "components/variations/synthetic_trial_registry.h"
 #include "components/variations/synthetic_trials.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "third_party/metrics_proto/system_profile.pb.h"
 
+using ActiveGroup = base::FieldTrial::ActiveGroup;
+
 namespace variations {
 
 namespace {
 
-const ActiveGroupId kFieldTrialIds[] = {MakeActiveGroupId("Trial1", "Group1"),
-                                        MakeActiveGroupId("Trial2", "Group2"),
-                                        MakeActiveGroupId("Trial3", "Group3")};
-const SyntheticTrialGroup kSyntheticFieldTrials[] = {
-    SyntheticTrialGroup("Synthetic1",
-                        "SyntheticGroup1",
-                        variations::SyntheticTrialAnnotationMode::kNextLog),
-    SyntheticTrialGroup("Synthetic2",
-                        "SyntheticGroup2",
-                        variations::SyntheticTrialAnnotationMode::kNextLog)};
-const ActiveGroupId kAllTrialIds[] = {
-    kFieldTrialIds[0], kFieldTrialIds[1], kFieldTrialIds[2],
-    kSyntheticFieldTrials[0].id(), kSyntheticFieldTrials[1].id()};
+constexpr const char* kSuffix = "UKM";
 
-class TestProvider : public FieldTrialsProvider {
- public:
-  TestProvider(SyntheticTrialRegistry* registry, base::StringPiece suffix)
-      : FieldTrialsProvider(registry, suffix) {}
-  ~TestProvider() override {}
-
-  void GetFieldTrialIds(
-      std::vector<ActiveGroupId>* field_trial_ids) const override {
-    ASSERT_TRUE(field_trial_ids->empty());
-    for (const ActiveGroupId& id : kFieldTrialIds) {
-      field_trial_ids->push_back(id);
-    }
-  }
-};
+const ActiveGroup kFieldTrials[] = {{"Trial1", "Group1"},
+                                    {"Trial2", "Group2"},
+                                    {"Trial3", "Group3"}};
+const ActiveGroup kSyntheticFieldTrials[] = {{"Synthetic1", "SyntheticGroup1"},
+                                             {"Synthetic2", "SyntheticGroup2"}};
+
+ActiveGroupId ToActiveGroupId(ActiveGroup active_group,
+                              std::string suffix = "");
+
+const ActiveGroupId kFieldTrialIds[] = {ToActiveGroupId(kFieldTrials[0]),
+                                        ToActiveGroupId(kFieldTrials[1]),
+                                        ToActiveGroupId(kFieldTrials[2])};
+const ActiveGroupId kAllTrialIds[] = {
+    ToActiveGroupId(kFieldTrials[0]), ToActiveGroupId(kFieldTrials[1]),
+    ToActiveGroupId(kFieldTrials[2]), ToActiveGroupId(kSyntheticFieldTrials[0]),
+    ToActiveGroupId(kSyntheticFieldTrials[1])};
+const ActiveGroupId kAllTrialIdsWithSuffixes[] = {
+    ToActiveGroupId(kFieldTrials[0], kSuffix),
+    ToActiveGroupId(kFieldTrials[1], kSuffix),
+    ToActiveGroupId(kFieldTrials[2], kSuffix),
+    ToActiveGroupId(kSyntheticFieldTrials[0], kSuffix),
+    ToActiveGroupId(kSyntheticFieldTrials[1], kSuffix)};
 
 // Check that the field trials in |system_profile| correspond to |expected|.
 void CheckFieldTrialsInSystemProfile(
@@ -56,24 +55,43 @@ void CheckFieldTrialsInSystemProfile(
   }
 }
 
+ActiveGroupId ToActiveGroupId(ActiveGroup active_group, std::string suffix) {
+  return MakeActiveGroupId(active_group.trial_name + suffix,
+                           active_group.group_name + suffix);
+}
+
 }  // namespace
 
 class FieldTrialsProviderTest : public ::testing::Test {
  public:
-  FieldTrialsProviderTest() {}
-  ~FieldTrialsProviderTest() override {}
+  FieldTrialsProviderTest() = default;
+  ~FieldTrialsProviderTest() override = default;
 
  protected:
+  void SetUp() override {
+    // Register the field trials.
+    for (const ActiveGroup& trial : kFieldTrials) {
+      base::FieldTrial* field_trial = base::FieldTrialList::CreateFieldTrial(
+          trial.trial_name, trial.group_name);
+      // Call group() to finalize and mark the field trial as active.
+      field_trial->group();
+    }
+  }
+
   // Register trials which should get recorded.
   void RegisterExpectedSyntheticTrials() {
-    for (const SyntheticTrialGroup& synthetic_trial : kSyntheticFieldTrials) {
-      registry_.RegisterSyntheticFieldTrial(synthetic_trial);
+    for (const ActiveGroup& trial : kSyntheticFieldTrials) {
+      registry_.RegisterSyntheticFieldTrial(SyntheticTrialGroup(
+          trial.trial_name, trial.group_name,
+          /*annotation_mode=*/
+          variations::SyntheticTrialAnnotationMode::kNextLog));
     }
   }
   // Register trial which shouldn't get recorded.
   void RegisterExtraSyntheticTrial() {
     registry_.RegisterSyntheticFieldTrial(SyntheticTrialGroup(
         "ExtraSynthetic", "ExtraGroup",
+        /*annotation_mode=*/
         variations::SyntheticTrialAnnotationMode::kNextLog));
   }
 
@@ -89,7 +107,7 @@ class FieldTrialsProviderTest : public ::testing::Test {
 };
 
 TEST_F(FieldTrialsProviderTest, ProvideSyntheticTrials) {
-  TestProvider provider(&registry_, base::StringPiece());
+  FieldTrialsProvider provider(&registry_, base::StringPiece());
 
   RegisterExpectedSyntheticTrials();
   // Make sure these trials are older than the log.
@@ -113,7 +131,7 @@ TEST_F(FieldTrialsProviderTest, ProvideSyntheticTrials) {
 }
 
 TEST_F(FieldTrialsProviderTest, NoSyntheticTrials) {
-  TestProvider provider(nullptr, base::StringPiece());
+  FieldTrialsProvider provider(nullptr, base::StringPiece());
 
   metrics::SystemProfileProto proto;
   provider.ProvideSystemProfileMetricsWithLogCreationTime(base::TimeTicks(),
@@ -136,7 +154,7 @@ TEST_F(FieldTrialsProviderTest, ProvideCurrentSessionData) {
   trial->set_name_id(1);
   trial->set_group_id(1);
 
-  TestProvider provider(&registry_, base::StringPiece());
+  FieldTrialsProvider provider(&registry_, base::StringPiece());
   RegisterExpectedSyntheticTrials();
   WaitUntilTimeChanges(base::TimeTicks::Now());
   provider.SetLogCreationTimeForTesting(base::TimeTicks::Now());
@@ -148,4 +166,21 @@ TEST_F(FieldTrialsProviderTest, ProvideCurrentSessionData) {
   CheckFieldTrialsInSystemProfile(uma_log.system_profile(), kAllTrialIds);
 }
 
+TEST_F(FieldTrialsProviderTest, GetAndWriteFieldTrialsWithSuffixes) {
+  metrics::ChromeUserMetricsExtension uma_log;
+  uma_log.system_profile();
+
+  FieldTrialsProvider provider(&registry_, kSuffix);
+  RegisterExpectedSyntheticTrials();
+  WaitUntilTimeChanges(base::TimeTicks::Now());
+  provider.SetLogCreationTimeForTesting(base::TimeTicks::Now());
+
+  provider.ProvideCurrentSessionData(&uma_log);
+
+  EXPECT_EQ(std::size(kAllTrialIdsWithSuffixes),
+            static_cast<size_t>(uma_log.system_profile().field_trial_size()));
+  CheckFieldTrialsInSystemProfile(uma_log.system_profile(),
+                                  kAllTrialIdsWithSuffixes);
+}
+
 }  // namespace variations

components/variations/active_field_trials.cc

@@ -22,20 +22,6 @@ namespace {
 
 base::LazyInstance<std::string>::Leaky g_seed_version;
 
-// Populates |name_group_ids| based on |active_groups|. Field trial names are
-// suffixed with |suffix| before hashing is executed.
-void GetFieldTrialActiveGroupIdsForActiveGroups(
-    base::StringPiece suffix,
-    const base::FieldTrial::ActiveGroups& active_groups,
-    std::vector<ActiveGroupId>* name_group_ids) {
-  DCHECK(name_group_ids->empty());
-  for (auto it = active_groups.begin(); it != active_groups.end(); ++it) {
-    name_group_ids->push_back(
-        MakeActiveGroupId(it->trial_name + std::string(suffix),
-                          it->group_name + std::string(suffix)));
-  }
-}
-
 void AppendActiveGroupIdsAsStrings(
     const std::vector<ActiveGroupId> name_group_ids,
     std::vector<std::string>* output) {
@@ -55,6 +41,18 @@ ActiveGroupId MakeActiveGroupId(base::StringPiece trial_name,
   return id;
 }
 
+void GetFieldTrialActiveGroupIdsForActiveGroups(
+    base::StringPiece suffix,
+    const base::FieldTrial::ActiveGroups& active_groups,
+    std::vector<ActiveGroupId>* name_group_ids) {
+  DCHECK(name_group_ids->empty());
+  for (const auto& active_group : active_groups) {
+    name_group_ids->push_back(
+        MakeActiveGroupId(active_group.trial_name + std::string(suffix),
+                          active_group.group_name + std::string(suffix)));
+  }
+}
+
 void GetFieldTrialActiveGroupIds(base::StringPiece suffix,
                                  std::vector<ActiveGroupId>* name_group_ids) {
   DCHECK(name_group_ids->empty());

components/variations/active_field_trials.h

@@ -40,6 +40,14 @@ struct COMPONENT_EXPORT(VARIATIONS) ActiveGroupIdCompare {
   }
 };
 
+// Populates |name_group_ids| based on |active_groups|. Field trial names are
+// suffixed with |suffix| before hashing is executed.
+COMPONENT_EXPORT(VARIATIONS)
+void GetFieldTrialActiveGroupIdsForActiveGroups(
+    base::StringPiece suffix,
+    const base::FieldTrial::ActiveGroups& active_groups,
+    std::vector<ActiveGroupId>* name_group_ids);
+
 // Fills the supplied vector |name_group_ids| (which must be empty when called)
 // with unique ActiveGroupIds for each Field Trial that has a chosen group.
 // Field Trials for which a group has not been chosen yet are NOT returned in

components/variations/synthetic_trial_registry.cc

@@ -10,6 +10,7 @@
 #include "base/metrics/histogram_functions.h"
 #include "base/observer_list.h"
 #include "base/strings/string_number_conversions.h"
+#include "components/variations/active_field_trials.h"
 #include "components/variations/hashing.h"
 #include "components/variations/variations_associated_data.h"
 
@@ -165,14 +166,19 @@ void SyntheticTrialRegistry::NotifySyntheticTrialObservers() {
 
 void SyntheticTrialRegistry::GetSyntheticFieldTrialsOlderThan(
     base::TimeTicks time,
-    std::vector<ActiveGroupId>* synthetic_trials) const {
+    std::vector<ActiveGroupId>* synthetic_trials,
+    base::StringPiece suffix) const {
   DCHECK(synthetic_trials);
   synthetic_trials->clear();
+  base::FieldTrial::ActiveGroups active_groups;
   for (const auto& entry : synthetic_trial_groups_) {
     if (entry.start_time() <= time ||
         entry.annotation_mode() == SyntheticTrialAnnotationMode::kCurrentLog)
-      synthetic_trials->push_back(entry.id());
+      active_groups.push_back(entry.active_group());
   }
+
+  GetFieldTrialActiveGroupIdsForActiveGroups(suffix, active_groups,
+                                             synthetic_trials);
 }
 
 }  // namespace variations

components/variations/synthetic_trial_registry.h

@@ -84,6 +84,8 @@ class COMPONENT_EXPORT(VARIATIONS) SyntheticTrialRegistry {
   friend FieldTrialsProviderTest;
   friend SyntheticTrialRegistryTest;
   FRIEND_TEST_ALL_PREFIXES(SyntheticTrialRegistryTest, RegisterSyntheticTrial);
+  FRIEND_TEST_ALL_PREFIXES(SyntheticTrialRegistryTest,
+                           GetSyntheticFieldTrialsOlderThanSuffix);
   FRIEND_TEST_ALL_PREFIXES(SyntheticTrialRegistryTest,
                            GetSyntheticFieldTrialActiveGroups);
   FRIEND_TEST_ALL_PREFIXES(VariationsCrashKeysTest, BasicFunctionality);
@@ -119,10 +121,12 @@ class COMPONENT_EXPORT(VARIATIONS) SyntheticTrialRegistry {
                                          const std::string& experiment_id);
 
   // Returns a list of synthetic field trials that are either (1) older than
-  // |time|, or (2) specify |kCurrentLog| as |annotation_mode|.
+  // |time|, or (2) specify |kCurrentLog| as |annotation_mode|. The trial and
+  // group names are suffixed with |suffix| before being hashed.
   void GetSyntheticFieldTrialsOlderThan(
       base::TimeTicks time,
-      std::vector<ActiveGroupId>* synthetic_trials) const;
+      std::vector<ActiveGroupId>* synthetic_trials,
+      base::StringPiece suffix = "") const;
 
   // Notifies observers on a synthetic trial list change.
   void NotifySyntheticTrialObservers();

components/variations/synthetic_trial_registry_unittest.cc

@@ -134,6 +134,28 @@ TEST_F(SyntheticTrialRegistryTest, RegisterSyntheticTrial) {
   EXPECT_TRUE(HasSyntheticTrial(synthetic_trials, "TestTrial4", "Group4"));
 }
 
+TEST_F(SyntheticTrialRegistryTest, GetSyntheticFieldTrialsOlderThanSuffix) {
+  SyntheticTrialRegistry registry;
+  SyntheticTrialGroup trial("TestTrial", "Group",
+                            SyntheticTrialAnnotationMode::kCurrentLog);
+  registry.RegisterSyntheticFieldTrial(trial);
+
+  std::vector<ActiveGroupId> synthetic_trials;
+  // Get list of synthetic trials, but with no added suffixes to the trial and
+  // group names.
+  registry.GetSyntheticFieldTrialsOlderThan(base::TimeTicks::Now(),
+                                            &synthetic_trials);
+  ASSERT_EQ(1U, synthetic_trials.size());
+  EXPECT_TRUE(HasSyntheticTrial(synthetic_trials, "TestTrial", "Group"));
+
+  // Get list of synthetic trials, but with "UKM" suffixed to the trial and
+  // group names.
+  registry.GetSyntheticFieldTrialsOlderThan(base::TimeTicks::Now(),
+                                            &synthetic_trials, "UKM");
+  ASSERT_EQ(1U, synthetic_trials.size());
+  EXPECT_TRUE(HasSyntheticTrial(synthetic_trials, "TestTrialUKM", "GroupUKM"));
+}
+
 TEST_F(SyntheticTrialRegistryTest, RegisterExternalExperiments_NoAllowlist) {
   SyntheticTrialRegistry registry(false);
   const std::string context = "TestTrial1";

components/variations/synthetic_trials.cc

@@ -19,12 +19,12 @@ SyntheticTrialGroup::SyntheticTrialGroup(
 SyntheticTrialGroup::SyntheticTrialGroup(const SyntheticTrialGroup&) = default;
 
 void SyntheticTrialGroup::SetTrialName(base::StringPiece trial_name) {
-  trial_name_ = std::string(trial_name);
+  active_group_.trial_name = std::string(trial_name);
   id_.name = variations::HashName(trial_name);
 }
 
 void SyntheticTrialGroup::SetGroupName(base::StringPiece group_name) {
-  group_name_ = std::string(group_name);
+  active_group_.group_name = std::string(group_name);
   id_.group = variations::HashName(group_name);
 }
 

components/variations/synthetic_trials.h

@@ -44,8 +44,9 @@ class COMPONENT_EXPORT(VARIATIONS) SyntheticTrialGroup {
 
   ~SyntheticTrialGroup() = default;
 
-  base::StringPiece trial_name() const { return trial_name_; }
-  base::StringPiece group_name() const { return group_name_; }
+  base::FieldTrial::ActiveGroup active_group() const { return active_group_; }
+  base::StringPiece trial_name() const { return active_group_.trial_name; }
+  base::StringPiece group_name() const { return active_group_.group_name; }
   ActiveGroupId id() const { return id_; }
   base::TimeTicks start_time() const { return start_time_; }
   SyntheticTrialAnnotationMode annotation_mode() const {
@@ -62,8 +63,7 @@ class COMPONENT_EXPORT(VARIATIONS) SyntheticTrialGroup {
   void SetIsExternal(bool is_external) { is_external_ = is_external; }
 
  private:
-  std::string trial_name_;
-  std::string group_name_;
+  base::FieldTrial::ActiveGroup active_group_;
   ActiveGroupId id_;
   base::TimeTicks start_time_;