[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/+/2572733Reviewed-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}

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/single_client_user_events_sync_test.cc

@@ -7,6 +7,8 @@
 #include "base/test/bind.h"
 #include "base/time/time.h"
 #include "build/build_config.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/sync/test/integration/bookmarks_helper.h"
 #include "chrome/browser/sync/test/integration/encryption_helper.h"
 #include "chrome/browser/sync/test/integration/profile_sync_service_harness.h"
@@ -17,9 +19,11 @@
 #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"
+#include "net/traffic_annotation/network_traffic_annotation_test_helper.h"
 
 using sync_pb::CommitResponse;
 using sync_pb::SyncEntity;
@@ -36,7 +40,13 @@ 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 +240,50 @@ IN_PROC_BROWSER_TEST_F(SingleClientUserEventsSyncTest,
   EXPECT_TRUE(ExpectUserEvents({}));
 }
 
+class SingleClientUserEventsSyncTestFlocEventLoggingEnabled
+    : public SingleClientUserEventsSyncTest {
+ public:
+  SingleClientUserEventsSyncTestFlocEventLoggingEnabled() {
+    feature_list_.Reset();
+    feature_list_.InitAndEnableFeature(features::kFlocIdComputedEventLogging);
+  }
+
+  void FinishOutstandingFlocRemotePermissionQueries() {
+    base::RunLoop run_loop;
+    FlocRemotePermissionServiceFactory::GetForProfile(GetProfile(0))
+        ->QueryFlocPermission(
+            base::BindLambdaForTesting([&](bool success) { run_loop.Quit(); }),
+            PARTIAL_TRAFFIC_ANNOTATION_FOR_TESTS);
+    run_loop.Run();
+  }
+};
+
+IN_PROC_BROWSER_TEST_F(SingleClientUserEventsSyncTestFlocEventLoggingEnabled,
+                       FlocEventAfterSyncSetup) {
+  ASSERT_TRUE(SetupSync());
+
+  FinishOutstandingFlocRemotePermissionQueries();
+
+  // 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

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;