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

This reverts commit e16309d9.

Reason for revert: This landed in the same build batch where the TwoClientEventSyncTest broke. Looking at the CL description, this seems related. See https://logs.chromium.org/logs/chromium/buildbucket/cr-buildbucket.appspot.com/8861341071397738112/+/steps/sync_integration_tests_on__none__GPU_on_Mac_on_Mac-10.13.6/0/logs/Deterministic_failure:_TwoClientUserEventsSyncTest.SetPassphraseAndRecordEventAndThenSetupSync__status_FAILURE_/0

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}

TBR=treib@chromium.org,jkarlin@chromium.org,yaoxia@chromium.org,chromium-scoped@luci-project-accounts.iam.gserviceaccount.com

Change-Id: I1095984a9a38063e0d722d7c78fb9f83538977eb
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 1148358
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2582222Reviewed-by: Martin Šrámek <msramek@chromium.org>
Commit-Queue: Martin Šrámek <msramek@chromium.org>
Cr-Commit-Position: refs/heads/master@{#835585}

chrome/browser/federated_learning/floc_id_provider_browsertest.cc

@@ -481,6 +481,8 @@ 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());
 }
 
@@ -499,18 +501,10 @@ IN_PROC_BROWSER_TEST_F(FlocIdProviderWithCustomizedServicesBrowserTest,
 
   InitializeHistorySync();
 
-  // 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 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_FALSE(GetFlocId().IsValid());
 }
 
@@ -545,6 +539,8 @@ 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,6 +58,8 @@ 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()) {
@@ -101,7 +103,8 @@ std::string FlocIdProviderImpl::GetInterestCohortForJsApi(
   return floc_id_.ToStringForJsApi();
 }
 
-void FlocIdProviderImpl::OnComputeFlocCompleted(ComputeFlocResult result) {
+void FlocIdProviderImpl::OnComputeFlocCompleted(ComputeFlocTrigger trigger,
+                                                ComputeFlocResult result) {
   DCHECK(floc_computation_in_progress_);
   floc_computation_in_progress_ = false;
 
@@ -109,11 +112,11 @@ void FlocIdProviderImpl::OnComputeFlocCompleted(ComputeFlocResult result) {
   // this computation completely and recompute.
   if (need_recompute_) {
     need_recompute_ = false;
-    ComputeFloc();
+    ComputeFloc(trigger);
     return;
   }
 
-  LogFlocComputedEvent(result);
+  LogFlocComputedEvent(trigger, result);
   floc_id_ = result.floc_id;
 
   floc_id_.SaveToPrefs(prefs_);
@@ -122,10 +125,18 @@ void FlocIdProviderImpl::OnComputeFlocCompleted(ComputeFlocResult result) {
   ScheduleFlocComputation(GetFlocIdScheduledUpdateInterval());
 }
 
-void FlocIdProviderImpl::LogFlocComputedEvent(const ComputeFlocResult& result) {
+void FlocIdProviderImpl::LogFlocComputedEvent(ComputeFlocTrigger trigger,
+                                              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());
@@ -133,6 +144,24 @@ void FlocIdProviderImpl::LogFlocComputedEvent(const ComputeFlocResult& result) {
   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);
 
@@ -179,7 +208,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(ComputeFlocResult());
+  LogFlocComputedEvent(ComputeFlocTrigger::kHistoryDelete, ComputeFlocResult());
   floc_id_ = FlocId();
   floc_id_.SaveToPrefs(prefs_);
 }
@@ -220,17 +249,27 @@ void FlocIdProviderImpl::MaybeTriggerImmediateComputation() {
   if (!first_sync_history_enabled_seen_ || !sorting_lsh_ready_or_not_required)
     return;
 
-  ComputeFloc();
+  ComputeFlocTrigger trigger = first_floc_computed_
+                                   ? ComputeFlocTrigger::kScheduledUpdate
+                                   : ComputeFlocTrigger::kFirstCompute;
+  ComputeFloc(trigger);
 }
 
-void FlocIdProviderImpl::ComputeFloc() {
+void FlocIdProviderImpl::OnComputeFlocScheduledUpdate() {
+  DCHECK(!floc_computation_in_progress_);
+  ComputeFloc(ComputeFlocTrigger::kScheduledUpdate);
+}
+
+void FlocIdProviderImpl::ComputeFloc(ComputeFlocTrigger trigger) {
+  DCHECK_NE(trigger, ComputeFlocTrigger::kHistoryDelete);
+
   DCHECK(!floc_computation_in_progress_);
 
   floc_computation_in_progress_ = true;
 
   auto compute_floc_completed_callback =
       base::BindOnce(&FlocIdProviderImpl::OnComputeFlocCompleted,
-                     weak_ptr_factory_.GetWeakPtr());
+                     weak_ptr_factory_.GetWeakPtr(), trigger);
 
   CheckCanComputeFloc(
       base::BindOnce(&FlocIdProviderImpl::OnCheckCanComputeFlocCompleted,
@@ -389,9 +428,10 @@ void FlocIdProviderImpl::DidApplySortingLshPostProcessing(
 }
 
 void FlocIdProviderImpl::ScheduleFlocComputation(base::TimeDelta delay) {
-  compute_floc_timer_.Start(FROM_HERE, delay,
-                            base::BindOnce(&FlocIdProviderImpl::ComputeFloc,
-                                           weak_ptr_factory_.GetWeakPtr()));
+  compute_floc_timer_.Start(
+      FROM_HERE, delay,
+      base::BindOnce(&FlocIdProviderImpl::OnComputeFlocScheduledUpdate,
+                     weak_ptr_factory_.GetWeakPtr()));
 }
 
 }  // namespace federated_learning

chrome/browser/federated_learning/floc_id_provider_impl.h

@@ -63,6 +63,19 @@ 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;
 
@@ -104,8 +117,10 @@ class FlocIdProviderImpl : public FlocIdProvider,
 
  protected:
   // protected virtual for testing.
-  virtual void OnComputeFlocCompleted(ComputeFlocResult result);
-  virtual void LogFlocComputedEvent(const ComputeFlocResult& result);
+  virtual void OnComputeFlocCompleted(ComputeFlocTrigger trigger,
+                                      ComputeFlocResult result);
+  virtual void LogFlocComputedEvent(ComputeFlocTrigger trigger,
+                                    const ComputeFlocResult& result);
 
  private:
   friend class FlocIdProviderUnitTest;
@@ -135,7 +150,9 @@ class FlocIdProviderImpl : public FlocIdProvider,
   // browser session starts.
   void MaybeTriggerImmediateComputation();
 
-  void ComputeFloc();
+  void OnComputeFlocScheduledUpdate();
+
+  void ComputeFloc(ComputeFlocTrigger trigger);
 
   void CheckCanComputeFloc(CanComputeFlocCallback callback);
   void OnCheckCanComputeFlocCompleted(ComputeFlocCompletedCallback callback,
@@ -173,6 +190,9 @@ 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,8 +7,6 @@
 #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"
@@ -19,11 +17,9 @@
 #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;
@@ -40,13 +36,7 @@ CommitResponse::ResponseType BounceType(
 
 class SingleClientUserEventsSyncTest : public SyncTest {
  public:
-  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() : SyncTest(SINGLE_CLIENT) {}
   ~SingleClientUserEventsSyncTest() override = default;
 
   bool ExpectUserEvents(std::vector<UserEventSpecifics> expected_specifics) {
@@ -240,50 +230,4 @@ 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,6 +962,7 @@ 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,8 +190,7 @@ message UserEventSpecifics {
       // Event added because the floc id is re-computed due to history deletion.
       HISTORY_DELETE = 3;
     }
-    reserved 1;
-    reserved "event_trigger";
+    optional EventTrigger event_trigger = 1;
 
     // If not set, it means that the floc is disabled.
     optional uint64 floc_id = 2;