Reland "[floc] Remove the EventTrigger field in the FlocIdComputed event"

This is a reland of e16309d9

The diff: disable floc event logging for 2 user-event related sync test
suite: SingleClientUserEventsSyncTest & TwoClientUserEventsSyncTest.
Add a wait-for-floc-network-response call at the end of
ProfileSyncServiceHarness::AwaitSyncSetupCompletion, so that the user
event logging behavior now becomes more deterministic and new flaky
test won't be added by accident.

Original change's description:
> [floc] Remove the EventTrigger field in the FlocIdComputed event
>
> Why: That field is never used & not useful for the server side analysis.
>
> This CL also removes a workaround that disables the initial floc
> loggings when permission disallows
> (in FlocIdProviderImpl::LogFlocComputedEvent). We had this workaround
> because otherwise the logging would mess up with the user event
> expectations in SingleClientUserEventsSyncTest, but that workaround is
> not ideal. This CL fixes it by disabling kFlocIdComputedEventLogging for
> the test suite, and adds a new test for the enabled case. The impact
> of this fix is it will cause additional events to be logged.
>
>
> Bug: 1148358
> Change-Id: I70cef531b89b434b5572b419acc8dd2412888498
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2572733
> Reviewed-by: Josh Karlin <jkarlin@chromium.org>
> Reviewed-by: Marc Treib <treib@chromium.org>
> Commit-Queue: Yao Xiao <yaoxia@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#835468}

Bug: 1148358
Change-Id: I12c8230ee5edc3de7337e9daa8c07143ae83949d
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2584607
Commit-Queue: Yao Xiao <yaoxia@chromium.org>
Reviewed-by: Marc Treib <treib@chromium.org>
Cr-Commit-Position: refs/heads/master@{#836637}

chrome/browser/federated_learning/floc_id_provider_browsertest.cc

@@ -481,8 +481,6 @@ IN_PROC_BROWSER_TEST_F(FlocIdProviderWithCustomizedServicesBrowserTest,
 
   const sync_pb::UserEventSpecifics_FlocIdComputed& event =
       specifics.floc_id_computed_event();
-  EXPECT_EQ(sync_pb::UserEventSpecifics::FlocIdComputed::NEW,
-            event.event_trigger());
   EXPECT_EQ(FlocId::SimHashHistory({test_host()}), event.floc_id());
 }
 
@@ -501,10 +499,18 @@ IN_PROC_BROWSER_TEST_F(FlocIdProviderWithCustomizedServicesBrowserTest,
 
   InitializeHistorySync();
 
-  // Expect that the FlocIdComputed user event is not recorded, as we won't
-  // record the 1st event after browser/sync startup if there are permission
-  // errors. The floc is should also be invalid.
-  ASSERT_EQ(0u, user_event_service()->GetRecordedUserEvents().size());
+  // Expect that the FlocIdComputed user event is recorded with sim-hash not
+  // set. The floc should also be invalid.
+  ASSERT_EQ(1u, user_event_service()->GetRecordedUserEvents().size());
+  const sync_pb::UserEventSpecifics& specifics =
+      user_event_service()->GetRecordedUserEvents()[0];
+  EXPECT_EQ(sync_pb::UserEventSpecifics::kFlocIdComputedEvent,
+            specifics.event_case());
+
+  const sync_pb::UserEventSpecifics_FlocIdComputed& event =
+      specifics.floc_id_computed_event();
+  EXPECT_FALSE(event.has_floc_id());
+
   EXPECT_FALSE(GetFlocId().IsValid());
 }
 
@@ -539,8 +545,6 @@ IN_PROC_BROWSER_TEST_F(FlocIdProviderWithCustomizedServicesBrowserTest,
 
   const sync_pb::UserEventSpecifics_FlocIdComputed& event =
       specifics.floc_id_computed_event();
-  EXPECT_EQ(sync_pb::UserEventSpecifics::FlocIdComputed::HISTORY_DELETE,
-            event.event_trigger());
   EXPECT_FALSE(event.has_floc_id());
 }
 

chrome/browser/federated_learning/floc_id_provider_impl.cc

@@ -58,8 +58,6 @@ FlocIdProviderImpl::FlocIdProviderImpl(
   base::Time last_compute_time = FlocId::ReadComputeTimeFromPrefs(prefs_);
 
   if (!last_compute_time.is_null()) {
-    first_floc_computed_ = true;
-
     base::TimeDelta time_since_last_compute =
         base::Time::Now() - last_compute_time;
     if (time_since_last_compute < GetFlocIdScheduledUpdateInterval()) {
@@ -103,8 +101,7 @@ std::string FlocIdProviderImpl::GetInterestCohortForJsApi(
   return floc_id_.ToStringForJsApi();
 }
 
-void FlocIdProviderImpl::OnComputeFlocCompleted(ComputeFlocTrigger trigger,
-                                                ComputeFlocResult result) {
+void FlocIdProviderImpl::OnComputeFlocCompleted(ComputeFlocResult result) {
   DCHECK(floc_computation_in_progress_);
   floc_computation_in_progress_ = false;
 
@@ -112,11 +109,11 @@ void FlocIdProviderImpl::OnComputeFlocCompleted(ComputeFlocTrigger trigger,
   // this computation completely and recompute.
   if (need_recompute_) {
     need_recompute_ = false;
-    ComputeFloc(trigger);
+    ComputeFloc();
     return;
   }
 
-  LogFlocComputedEvent(trigger, result);
+  LogFlocComputedEvent(result);
   floc_id_ = result.floc_id;
 
   floc_id_.SaveToPrefs(prefs_);
@@ -125,18 +122,10 @@ void FlocIdProviderImpl::OnComputeFlocCompleted(ComputeFlocTrigger trigger,
   ScheduleFlocComputation(GetFlocIdScheduledUpdateInterval());
 }
 
-void FlocIdProviderImpl::LogFlocComputedEvent(ComputeFlocTrigger trigger,
-                                              const ComputeFlocResult& result) {
+void FlocIdProviderImpl::LogFlocComputedEvent(const ComputeFlocResult& result) {
   if (!base::FeatureList::IsEnabled(features::kFlocIdComputedEventLogging))
     return;
 
-  // Don't log if it's the 1st computation and sim_hash is not computed. This
-  // is likely due to sync just gets enabled but some floc permission settings
-  // are disabled. We don't want to mess up with the initial user event
-  // messagings (and some sync integration tests would fail otherwise).
-  if (trigger == ComputeFlocTrigger::kFirstCompute && !result.sim_hash_computed)
-    return;
-
   auto specifics = std::make_unique<sync_pb::UserEventSpecifics>();
   specifics->set_event_time_usec(
       base::Time::Now().ToDeltaSinceWindowsEpoch().InMicroseconds());
@@ -144,24 +133,6 @@ void FlocIdProviderImpl::LogFlocComputedEvent(ComputeFlocTrigger trigger,
   sync_pb::UserEventSpecifics_FlocIdComputed* const floc_id_computed_event =
       specifics->mutable_floc_id_computed_event();
 
-  sync_pb::UserEventSpecifics_FlocIdComputed_EventTrigger event_trigger;
-  switch (trigger) {
-    case ComputeFlocTrigger::kFirstCompute:
-      event_trigger =
-          sync_pb::UserEventSpecifics_FlocIdComputed_EventTrigger_NEW;
-      break;
-    case ComputeFlocTrigger::kScheduledUpdate:
-      event_trigger =
-          sync_pb::UserEventSpecifics_FlocIdComputed_EventTrigger_REFRESHED;
-      break;
-    case ComputeFlocTrigger::kHistoryDelete:
-      event_trigger = sync_pb::
-          UserEventSpecifics_FlocIdComputed_EventTrigger_HISTORY_DELETE;
-      break;
-  }
-
-  floc_id_computed_event->set_event_trigger(event_trigger);
-
   if (result.sim_hash_computed)
     floc_id_computed_event->set_floc_id(result.sim_hash);
 
@@ -208,7 +179,7 @@ void FlocIdProviderImpl::OnURLsDeleted(
   // We log the invalidation event although it's technically not a recompute.
   // It'd give us a better idea how often the floc is invalidated due to
   // history-delete.
-  LogFlocComputedEvent(ComputeFlocTrigger::kHistoryDelete, ComputeFlocResult());
+  LogFlocComputedEvent(ComputeFlocResult());
   floc_id_ = FlocId();
   floc_id_.SaveToPrefs(prefs_);
 }
@@ -249,27 +220,17 @@ void FlocIdProviderImpl::MaybeTriggerImmediateComputation() {
   if (!first_sync_history_enabled_seen_ || !sorting_lsh_ready_or_not_required)
     return;
 
-  ComputeFlocTrigger trigger = first_floc_computed_
-                                   ? ComputeFlocTrigger::kScheduledUpdate
-                                   : ComputeFlocTrigger::kFirstCompute;
-  ComputeFloc(trigger);
+  ComputeFloc();
 }
 
-void FlocIdProviderImpl::OnComputeFlocScheduledUpdate() {
-  DCHECK(!floc_computation_in_progress_);
-  ComputeFloc(ComputeFlocTrigger::kScheduledUpdate);
-}
-
-void FlocIdProviderImpl::ComputeFloc(ComputeFlocTrigger trigger) {
-  DCHECK_NE(trigger, ComputeFlocTrigger::kHistoryDelete);
-
+void FlocIdProviderImpl::ComputeFloc() {
   DCHECK(!floc_computation_in_progress_);
 
   floc_computation_in_progress_ = true;
 
   auto compute_floc_completed_callback =
       base::BindOnce(&FlocIdProviderImpl::OnComputeFlocCompleted,
-                     weak_ptr_factory_.GetWeakPtr(), trigger);
+                     weak_ptr_factory_.GetWeakPtr());
 
   CheckCanComputeFloc(
       base::BindOnce(&FlocIdProviderImpl::OnCheckCanComputeFlocCompleted,
@@ -428,10 +389,9 @@ void FlocIdProviderImpl::DidApplySortingLshPostProcessing(
 }
 
 void FlocIdProviderImpl::ScheduleFlocComputation(base::TimeDelta delay) {
-  compute_floc_timer_.Start(
-      FROM_HERE, delay,
-      base::BindOnce(&FlocIdProviderImpl::OnComputeFlocScheduledUpdate,
-                     weak_ptr_factory_.GetWeakPtr()));
+  compute_floc_timer_.Start(FROM_HERE, delay,
+                            base::BindOnce(&FlocIdProviderImpl::ComputeFloc,
+                                           weak_ptr_factory_.GetWeakPtr()));
 }
 
 }  // namespace federated_learning

chrome/browser/federated_learning/floc_id_provider_impl.h

@@ -63,19 +63,6 @@ class FlocIdProviderImpl : public FlocIdProvider,
                            public history::HistoryServiceObserver,
                            public syncer::SyncServiceObserver {
  public:
-  // TODO(crbug/1148358): Consider removing this. For event logging, we are not
-  // really interested in the trigger.
-  enum class ComputeFlocTrigger {
-    // When the first browser session of a profile starts.
-    kFirstCompute,
-
-    // A long period of time has passed since the last computation.
-    kScheduledUpdate,
-
-    // History is deleted.
-    kHistoryDelete,
-  };
-
   struct ComputeFlocResult {
     ComputeFlocResult() = default;
 
@@ -117,10 +104,8 @@ class FlocIdProviderImpl : public FlocIdProvider,
 
  protected:
   // protected virtual for testing.
-  virtual void OnComputeFlocCompleted(ComputeFlocTrigger trigger,
-                                      ComputeFlocResult result);
-  virtual void LogFlocComputedEvent(ComputeFlocTrigger trigger,
-                                    const ComputeFlocResult& result);
+  virtual void OnComputeFlocCompleted(ComputeFlocResult result);
+  virtual void LogFlocComputedEvent(const ComputeFlocResult& result);
 
  private:
   friend class FlocIdProviderUnitTest;
@@ -150,9 +135,7 @@ class FlocIdProviderImpl : public FlocIdProvider,
   // browser session starts.
   void MaybeTriggerImmediateComputation();
 
-  void OnComputeFlocScheduledUpdate();
-
-  void ComputeFloc(ComputeFlocTrigger trigger);
+  void ComputeFloc();
 
   void CheckCanComputeFloc(CanComputeFlocCallback callback);
   void OnCheckCanComputeFlocCompleted(ComputeFlocCompletedCallback callback,
@@ -190,9 +173,6 @@ class FlocIdProviderImpl : public FlocIdProvider,
 
   bool floc_computation_in_progress_ = false;
 
-  // Whether the floc has ever been computed.
-  bool first_floc_computed_ = false;
-
   // True if history-delete occurs during an in-progress computation. When the
   // in-progress one finishes, we would disregard the result (i.e. no loggings
   // or floc update), and compute again. Potentially we could maintain extra

chrome/browser/sync/test/integration/profile_sync_service_harness.cc

@@ -16,6 +16,8 @@
 #include "base/test/bind.h"
 #include "build/build_config.h"
 #include "build/chromeos_buildflags.h"
+#include "chrome/browser/federated_learning/floc_remote_permission_service.h"
+#include "chrome/browser/federated_learning/floc_remote_permission_service_factory.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/signin/identity_manager_factory.h"
 #include "chrome/browser/sync/profile_sync_service_factory.h"
@@ -57,6 +59,15 @@ bool HasAuthError(ProfileSyncService* service) {
              GoogleServiceAuthError::REQUEST_CANCELED;
 }
 
+void FinishOutstandingFlocRemotePermissionQueries(Profile* profile) {
+  base::RunLoop run_loop;
+  FlocRemotePermissionServiceFactory::GetForProfile(profile)
+      ->QueryFlocPermission(
+          base::BindLambdaForTesting([&](bool success) { run_loop.Quit(); }),
+          PARTIAL_TRAFFIC_ANNOTATION_FOR_TESTS);
+  run_loop.Run();
+}
+
 class EngineInitializeChecker : public SingleClientStatusChangeChecker {
  public:
   explicit EngineInitializeChecker(ProfileSyncService* service)
@@ -457,6 +468,15 @@ bool ProfileSyncServiceHarness::AwaitSyncSetupCompletion() {
     return false;
   }
 
+  // Finish any outstanding floc permission network requests. Right after sync
+  // gets set up, the floc code may make a network call, and at response time
+  // a floc specific user event could be logged. We need this waiting to ensure
+  // that the behavior (i.e. either logged or not logged) are determnistic.
+  //
+  // TODO(yaoxia): The network call should be mocked out / intercepted and
+  // handled locally.
+  FinishOutstandingFlocRemotePermissionQueries(profile_);
+
   return true;
 }
 

chrome/browser/sync/test/integration/single_client_user_events_sync_test.cc

@@ -17,6 +17,7 @@
 #include "chrome/browser/sync/test/integration/sync_test.h"
 #include "chrome/browser/sync/test/integration/user_events_helper.h"
 #include "chrome/browser/sync/user_event_service_factory.h"
+#include "chrome/common/chrome_features.h"
 #include "components/sync/protocol/user_event_specifics.pb.h"
 #include "components/sync_user_events/user_event_service.h"
 #include "content/public/test/browser_test.h"
@@ -36,7 +37,14 @@ CommitResponse::ResponseType BounceType(
 
 class SingleClientUserEventsSyncTest : public SyncTest {
  public:
-  SingleClientUserEventsSyncTest() : SyncTest(SINGLE_CLIENT) {}
+  SingleClientUserEventsSyncTest() : SyncTest(SINGLE_CLIENT) {
+    // Disable the floc-event-logging for this test suite. This event could be
+    // logged right after sync gets enabled, and could mess up with the test
+    // expectations. The scenario when the floc-event-logging is enabled is
+    // tested separately.
+    feature_list_.InitAndDisableFeature(features::kFlocIdComputedEventLogging);
+  }
+
   ~SingleClientUserEventsSyncTest() override = default;
 
   bool ExpectUserEvents(std::vector<UserEventSpecifics> expected_specifics) {
@@ -230,4 +238,39 @@ IN_PROC_BROWSER_TEST_F(SingleClientUserEventsSyncTest,
   EXPECT_TRUE(ExpectUserEvents({}));
 }
 
+class SingleClientUserEventsSyncTestFlocEventLoggingEnabled
+    : public SingleClientUserEventsSyncTest {
+ public:
+  SingleClientUserEventsSyncTestFlocEventLoggingEnabled() {
+    feature_list_.Reset();
+    feature_list_.InitAndEnableFeature(features::kFlocIdComputedEventLogging);
+  }
+};
+
+IN_PROC_BROWSER_TEST_F(SingleClientUserEventsSyncTestFlocEventLoggingEnabled,
+                       FlocEventAfterSyncSetup) {
+  ASSERT_TRUE(SetupSync());
+
+  // When sync gets enabled, the floc calculation should start, and by this
+  // point it should have failed the remote permission check and logged an user
+  // event.
+  EXPECT_TRUE(ServerCountMatchStatusChecker(syncer::USER_EVENTS, 1).Wait());
+
+  UserEventSpecifics server_specifics =
+      GetFakeServer()
+          ->GetSyncEntitiesByModelType(syncer::USER_EVENTS)[0]
+          .specifics()
+          .user_event();
+
+  EXPECT_EQ(sync_pb::UserEventSpecifics::kFlocIdComputedEvent,
+            server_specifics.event_case());
+
+  const sync_pb::UserEventSpecifics_FlocIdComputed& event =
+      server_specifics.floc_id_computed_event();
+
+  // It shouldn't have the floc_id field, because remote permission check should
+  // have failed.
+  EXPECT_FALSE(event.has_floc_id());
+}
+
 }  // namespace

chrome/browser/sync/test/integration/two_client_user_events_sync_test.cc

@@ -11,6 +11,7 @@
 #include "chrome/browser/sync/test/integration/updated_progress_marker_checker.h"
 #include "chrome/browser/sync/test/integration/user_events_helper.h"
 #include "chrome/browser/sync/user_event_service_factory.h"
+#include "chrome/common/chrome_features.h"
 #include "components/sync/protocol/user_event_specifics.pb.h"
 #include "components/sync_user_events/user_event_service.h"
 #include "content/public/test/browser_test.h"
@@ -25,7 +26,14 @@ const int kDecryptingClientId = 1;
 
 class TwoClientUserEventsSyncTest : public SyncTest {
  public:
-  TwoClientUserEventsSyncTest() : SyncTest(TWO_CLIENT) {}
+  TwoClientUserEventsSyncTest() : SyncTest(TWO_CLIENT) {
+    // Disable the floc-event-logging for this test suite. This event could be
+    // logged right after sync gets enabled, and could mess up with the test
+    // expectations. The scenario when the floc-event-logging is enabled is
+    // tested separately.
+    feature_list_.InitAndDisableFeature(features::kFlocIdComputedEventLogging);
+  }
+
   ~TwoClientUserEventsSyncTest() override = default;
 
   bool UseVerifier() override {

components/sync/protocol/proto_visitors.h

@@ -962,7 +962,6 @@ VISIT_PROTO_FIELDS(
 }
 
 VISIT_PROTO_FIELDS(const sync_pb::UserEventSpecifics::FlocIdComputed& proto) {
-  VISIT_ENUM(event_trigger);
   VISIT(floc_id);
 }
 

components/sync/protocol/user_event_specifics.proto

@@ -190,7 +190,8 @@ message UserEventSpecifics {
       // Event added because the floc id is re-computed due to history deletion.
       HISTORY_DELETE = 3;
     }
-    optional EventTrigger event_trigger = 1;
+    reserved 1;
+    reserved "event_trigger";
 
     // If not set, it means that the floc is disabled.
     optional uint64 floc_id = 2;