[Hashed logging] Refactor API and internal logic.

This is the first change to convert the existing hashed logging code
into a general framework. This CL mostly removes code that was specific
to the search ranking use of hashed logging, and also changes the
API for logging.

Note that all hashed logging has been disabled for this refactor.

We need to delete this code now because there's a chain of
dependencies between the refactoring CLs. Roughly:

 1. Code referencing the current proto used to report logs must be
    deleted.

 2. The proto itself can then be changed internally, and re-exported
    to chromium. This has to be done as a unitary CL, with no other
    changes.

 3. Then, code can be re-added that references the new version of the
    proto.

Detailed changes:

 1. AppListLaunchRecorder::Record has become ::Log, which now takes
    lists of hashed and unhashed data. This is a tentative first try
    at the new API.

 2. The core hashed logic in AppListLaunchMetricsProvider has been
    deleted, because it will need to be generalized.

 3. The unit tests have all been disabled and the core EXPECT logic
    removed. This too must be rewritten for the generalized version.

Bug: 1016655
Change-Id: Iccc98771003c602b1a8375b4b2898e077f28def6
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1874411Reviewed-by: Thanh Nguyen <thanhdng@chromium.org>
Commit-Queue: Tony Yeoman <tby@chromium.org>
Cr-Commit-Position: refs/heads/master@{#709329}

chrome/browser/ui/app_list/search/search_result_ranker/app_list_launch_metrics_provider.cc

@@ -30,8 +30,6 @@
 namespace app_list {
 namespace {
 
-using LaunchInfo = AppListLaunchRecorder::LaunchInfo;
-
 using ::metrics::ChromeOSAppListLaunchEventProto;
 using ::metrics::ChromeUserMetricsExtension;
 using ::metrics::MetricsLog;
@@ -250,7 +248,7 @@ void AppListLaunchMetricsProvider::ProvideCurrentSessionData(
 }
 
 void AppListLaunchMetricsProvider::OnAppListLaunch(
-    const LaunchInfo& launch_info) {
+    const AppListLaunchRecorder::LaunchInfo& launch_info) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   if (init_state_ == InitState::UNINITIALIZED) {
     init_state_ = InitState::INIT_STARTED;
@@ -261,8 +259,7 @@ void AppListLaunchMetricsProvider::OnAppListLaunch(
       launch_info_cache_.size() >= static_cast<size_t>(kMaxEventsPerUpload))
     return;
 
-  if (launch_info.launch_type ==
-      ChromeOSAppListLaunchEventProto::LAUNCH_TYPE_UNSPECIFIED) {
+  if (launch_info.client == AppListLaunchRecorder::Client::kUnspecified) {
     LogMetricsProviderError(MetricsProviderError::kLaunchTypeUnspecified);
     return;
   }
@@ -277,7 +274,7 @@ void AppListLaunchMetricsProvider::OnAppListLaunch(
 }
 
 void AppListLaunchMetricsProvider::CreateLaunchEvent(
-    const LaunchInfo& launch_info,
+    const AppListLaunchRecorder::LaunchInfo& launch_info,
     ChromeOSAppListLaunchEventProto* event) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   DCHECK(secret_ && user_id_);
@@ -285,18 +282,11 @@ void AppListLaunchMetricsProvider::CreateLaunchEvent(
   base::Time::Exploded now;
   base::Time::Now().LocalExplode(&now);
 
-  // Unhashed data.
-  event->set_recurrence_ranker_user_id(user_id_.value());
-  event->set_hour(now.hour);
-  event->set_search_query_length(launch_info.query.size());
-  event->set_launch_type(launch_info.launch_type);
-  event->set_search_provider_type(launch_info.search_provider_type);
-
-  // Hashed data.
-  event->set_hashed_target(HashWithSecret(launch_info.target, secret_.value()));
-  event->set_hashed_query(HashWithSecret(launch_info.query, secret_.value()));
-  event->set_hashed_domain(HashWithSecret(launch_info.domain, secret_.value()));
-  event->set_hashed_app(HashWithSecret(launch_info.app, secret_.value()));
+  // TODO(crbug.com/1016655): set hashed and unhashed data after proto has been
+  // updated.
+
+  // Dummy call so that HashWithSecret compiles.
+  LOG(ERROR) << HashWithSecret("dummy hash", secret_.value());
 }
 
 }  // namespace app_list

chrome/browser/ui/app_list/search/search_result_ranker/app_list_launch_metrics_provider_unittest.cc

@@ -39,10 +39,6 @@ namespace app_list {
 
 namespace {
 
-using LaunchType = ::metrics::ChromeOSAppListLaunchEventProto::LaunchType;
-using SearchProviderType =
-    ::metrics::ChromeOSAppListLaunchEventProto::SearchProviderType;
-
 using ::chromeos::ProfileHelper;
 using ::metrics::ChromeOSAppListLaunchEventProto;
 
@@ -64,27 +60,23 @@ constexpr char kValue2Hash[] = "506ECDDC0BA3C341";
 constexpr char kValue3Hash[] = "1E0CDC361557A12F";
 constexpr char kValue4Hash[] = "4875030730DE902A";
 
+enum class TestEvent {
+  kValueA = 0,
+  kValueB = 1,
+  kValueC = 2,
+};
+
 std::string HashToHex(uint64_t hash) {
   return base::HexEncode(&hash, sizeof(uint64_t));
 }
 
 void ExpectLoggingEventEquals(ChromeOSAppListLaunchEventProto proto,
-                              const char* target_hash,
-                              const char* query_hash,
-                              const char* domain_hash,
-                              const char* app_hash,
-                              int search_query_length) {
-  EXPECT_EQ(ChromeOSAppListLaunchEventProto::APP_TILES, proto.launch_type());
-  EXPECT_EQ(ChromeOSAppListLaunchEventProto::ZERO_STATE_FILE,
-            proto.search_provider_type());
-  // Hour field is untested.
-  EXPECT_EQ(kUserId, proto.recurrence_ranker_user_id());
-  EXPECT_EQ(search_query_length, proto.search_query_length());
-
-  EXPECT_EQ(target_hash, HashToHex(proto.hashed_target()));
-  EXPECT_EQ(query_hash, HashToHex(proto.hashed_query()));
-  EXPECT_EQ(domain_hash, HashToHex(proto.hashed_domain()));
-  EXPECT_EQ(app_hash, HashToHex(proto.hashed_app()));
+                              std::string value_a,
+                              std::string value_b,
+                              int value_c) {
+  // TODO(crbug.com/1016655): reimplement once proto has been refactored.
+  // Dummy call so HashToHex compiles.
+  LOG(ERROR) << HashToHex(0u);
 }
 
 }  // namespace
@@ -104,7 +96,7 @@ class AppListLaunchMetricsProviderTest : public testing::Test {
   }
 
   void InitProvider() {
-    AddLog("", "", "", "");
+    AddLog("", "", 0);
     Wait();
   }
 
@@ -187,16 +179,14 @@ class AppListLaunchMetricsProviderTest : public testing::Test {
                false);
   }
 
-  void AddLog(
-      const std::string& target,
-      const std::string& query,
-      const std::string& domain,
-      const std::string& app,
-      LaunchType launch_type = ChromeOSAppListLaunchEventProto::APP_TILES,
-      SearchProviderType search_provider_type =
-          ChromeOSAppListLaunchEventProto::ZERO_STATE_FILE) {
-    provider_->OnAppListLaunch(
-        {launch_type, search_provider_type, target, query, domain, app});
+  void AddLog(std::string value_a, std::string value_b, int value_c) {
+    AppListLaunchRecorder::LaunchInfo event;
+    event.client = AppListLaunchRecorder::Client::kTesting;
+    event.hashed = {{static_cast<int>(TestEvent::kValueA), value_a},
+                    {static_cast<int>(TestEvent::kValueB), value_b}};
+    event.unhashed = {{static_cast<int>(TestEvent::kValueC), value_c}};
+
+    provider_->OnAppListLaunch(event);
   }
 
   google::protobuf::RepeatedPtrField<ChromeOSAppListLaunchEventProto>
@@ -219,7 +209,8 @@ class AppListLaunchMetricsProviderTest : public testing::Test {
   std::unique_ptr<AppListLaunchMetricsProvider> provider_;
 };
 
-TEST_F(AppListLaunchMetricsProviderTest, ProvidesNothingWhenUninitialized) {
+TEST_F(AppListLaunchMetricsProviderTest,
+       DISABLED_ProvidesNothingWhenUninitialized) {
   MakeProvider();
 
   ExpectUninitialized();
@@ -227,7 +218,7 @@ TEST_F(AppListLaunchMetricsProviderTest, ProvidesNothingWhenUninitialized) {
   ExpectNoErrors();
 }
 
-TEST_F(AppListLaunchMetricsProviderTest, SucceedsGeneratingNewSecret) {
+TEST_F(AppListLaunchMetricsProviderTest, DISABLED_SucceedsGeneratingNewSecret) {
   MakeProvider();
   InitProvider();
 
@@ -244,7 +235,8 @@ TEST_F(AppListLaunchMetricsProviderTest, SucceedsGeneratingNewSecret) {
                                        MetricsProviderError::kNoStateProto, 1);
 }
 
-TEST_F(AppListLaunchMetricsProviderTest, SucceedsLoadingExistingSecret) {
+TEST_F(AppListLaunchMetricsProviderTest,
+       DISABLED_SucceedsLoadingExistingSecret) {
   WriteStateProto(kSecret);
 
   MakeProvider();
@@ -255,7 +247,7 @@ TEST_F(AppListLaunchMetricsProviderTest, SucceedsLoadingExistingSecret) {
   ExpectNoErrors();
 }
 
-TEST_F(AppListLaunchMetricsProviderTest, DisableOnInvalidSecret) {
+TEST_F(AppListLaunchMetricsProviderTest, DISABLED_DisableOnInvalidSecret) {
   WriteStateProto("wrong length");
 
   MakeProvider();
@@ -268,47 +260,42 @@ TEST_F(AppListLaunchMetricsProviderTest, DisableOnInvalidSecret) {
 
 // Tests that a call to ProvideCurrentSessionData populates protos for each log,
 // and that those protos contain the right values.
-TEST_F(AppListLaunchMetricsProviderTest, CorrectHashedValues) {
+TEST_F(AppListLaunchMetricsProviderTest, DISABLED_CorrectHashedValues) {
   WriteStateProto(kSecret);
   MakeProvider();
   InitProvider();
 
-  AddLog(kValue1, kValue2, kValue3, kValue4);
-  AddLog(kValue2, kValue3, kValue4, kValue1);
-  AddLog(kValue3, kValue4, kValue1, kValue2);
+  AddLog(kValue1, kValue2, 5);
+  AddLog(kValue2, kValue3, 7);
+  AddLog(kValue3, kValue4, 9);
 
   const auto& events = GetLogs();
   // events[0] is a dummy log created in |InitProvider|. We don't test for its
   // contents below.
   ASSERT_EQ(events.size(), 4);
 
-  ExpectLoggingEventEquals(events[1], kValue1Hash, kValue2Hash, kValue3Hash,
-                           kValue4Hash, std::string(kValue2).size());
-  ExpectLoggingEventEquals(events[2], kValue2Hash, kValue3Hash, kValue4Hash,
-                           kValue1Hash, std::string(kValue3).size());
-  ExpectLoggingEventEquals(events[3], kValue3Hash, kValue4Hash, kValue1Hash,
-                           kValue2Hash, std::string(kValue4).size());
+  ExpectLoggingEventEquals(events[1], kValue1Hash, kValue2Hash, 5);
+  ExpectLoggingEventEquals(events[2], kValue2Hash, kValue3Hash, 7);
+  ExpectLoggingEventEquals(events[3], kValue3Hash, kValue4Hash, 9);
   ExpectNoErrors();
 }
 
 // Tests that the logs reported in one call to ProvideCurrentSessionData do no
 // appear in the next.
-TEST_F(AppListLaunchMetricsProviderTest, EventsNotDuplicated) {
+TEST_F(AppListLaunchMetricsProviderTest, DISABLED_EventsNotDuplicated) {
   WriteStateProto(kSecret);
   MakeProvider();
   InitProvider();
 
-  AddLog(kValue1, kValue2, kValue3, kValue4);
+  AddLog(kValue1, kValue2, 44);
   auto events = GetLogs();
   ASSERT_EQ(events.size(), 2);
-  ExpectLoggingEventEquals(events[1], kValue1Hash, kValue2Hash, kValue3Hash,
-                           kValue4Hash, std::string(kValue2).size());
+  ExpectLoggingEventEquals(events[1], kValue1Hash, kValue2Hash, 44);
 
-  AddLog(kValue2, kValue3, kValue4, kValue1);
+  AddLog(kValue1, kValue4, 22);
   events = GetLogs();
   ASSERT_EQ(events.size(), 1);
-  ExpectLoggingEventEquals(events[0], kValue2Hash, kValue3Hash, kValue4Hash,
-                           kValue1Hash, std::string(kValue3).size());
+  ExpectLoggingEventEquals(events[0], kValue3Hash, kValue4Hash, 22);
 
   EXPECT_TRUE(GetLogs().empty());
   ExpectNoErrors();
@@ -316,7 +303,7 @@ TEST_F(AppListLaunchMetricsProviderTest, EventsNotDuplicated) {
 
 // Tests that logging events are dropped after an unreasonably large number of
 // them are made between uploads.
-TEST_F(AppListLaunchMetricsProviderTest, EventsAreCapped) {
+TEST_F(AppListLaunchMetricsProviderTest, DISABLED_EventsAreCapped) {
   MakeProvider();
   InitProvider();
 
@@ -324,13 +311,13 @@ TEST_F(AppListLaunchMetricsProviderTest, EventsAreCapped) {
 
   // Not enough events to hit the cap.
   for (int i = 0; i < max_events / 2; ++i)
-    AddLog(kValue1, kValue2, kValue3, kValue4);
+    AddLog(kValue1, kValue2, 1);
   // One event from init, kMaxEventsPerUpload/2 from the loop.
   EXPECT_EQ(1 + max_events / 2, GetLogs().size());
 
   // Enough events to hit the cap.
   for (int i = 0; i < 2 * max_events; ++i)
-    AddLog(kValue1, kValue2, kValue3, kValue4);
+    AddLog(kValue1, kValue2, 1);
   EXPECT_EQ(max_events, GetLogs().size());
 
   histogram_tester_.ExpectBucketCount(
@@ -341,27 +328,24 @@ TEST_F(AppListLaunchMetricsProviderTest, EventsAreCapped) {
 // Tests that logging events that occur before the provider is initialized are
 // still correctly logged after initialization.
 TEST_F(AppListLaunchMetricsProviderTest,
-       LaunchEventsBeforeInitializationAreRecorded) {
+       DISABLED_LaunchEventsBeforeInitializationAreRecorded) {
   WriteStateProto(kSecret);
   MakeProvider();
 
   // To begin with, the provider is uninitialized and has no logs at all.
   EXPECT_TRUE(GetLogs().empty());
   // These logs are added before the provider has finished initialising.
-  AddLog(kValue1, kValue2, kValue3, kValue4);
-  AddLog(kValue2, kValue3, kValue4, kValue1);
-  AddLog(kValue3, kValue4, kValue1, kValue2);
+  AddLog(kValue1, kValue2, 1);
+  AddLog(kValue2, kValue3, 2);
+  AddLog(kValue3, kValue4, 3);
   // Initialisation is finished here.
   Wait();
 
   const auto& events = GetLogs();
   ASSERT_EQ(events.size(), 3);
-  ExpectLoggingEventEquals(events[0], kValue1Hash, kValue2Hash, kValue3Hash,
-                           kValue4Hash, std::string(kValue2).size());
-  ExpectLoggingEventEquals(events[1], kValue2Hash, kValue3Hash, kValue4Hash,
-                           kValue1Hash, std::string(kValue3).size());
-  ExpectLoggingEventEquals(events[2], kValue3Hash, kValue4Hash, kValue1Hash,
-                           kValue2Hash, std::string(kValue4).size());
+  ExpectLoggingEventEquals(events[0], kValue1Hash, kValue2Hash, 1);
+  ExpectLoggingEventEquals(events[1], kValue2Hash, kValue3Hash, 2);
+  ExpectLoggingEventEquals(events[2], kValue3Hash, kValue4Hash, 3);
 
   EXPECT_TRUE(GetLogs().empty());
   ExpectNoErrors();
@@ -370,7 +354,7 @@ TEST_F(AppListLaunchMetricsProviderTest,
 // Without an existing saved state, instantiating a metrics provider should save
 // an almost certainly unique user ID and secret. Test this by creating a few
 // blank-slate metrics providers.
-TEST_F(AppListLaunchMetricsProviderTest, UserIDsAndSecretsAreUnique) {
+TEST_F(AppListLaunchMetricsProviderTest, DISABLED_UserIDsAndSecretsAreUnique) {
   const int num_runs = 10;
 
   std::set<uint64_t> user_ids;

chrome/browser/ui/app_list/search/search_result_ranker/app_list_launch_recorder.cc

@@ -10,26 +10,9 @@
 
 namespace app_list {
 
-using LaunchInfo = AppListLaunchRecorder::LaunchInfo;
-
-AppListLaunchRecorder::LaunchInfo::LaunchInfo(
-    metrics::ChromeOSAppListLaunchEventProto::LaunchType launch_type,
-    metrics::ChromeOSAppListLaunchEventProto::SearchProviderType
-        search_provider_type,
-    const std::string& target,
-    const std::string& query,
-    const std::string& domain,
-    const std::string& app)
-    : launch_type(launch_type),
-      search_provider_type(search_provider_type),
-      target(target),
-      query(query),
-      domain(domain),
-      app(app) {}
-
+AppListLaunchRecorder::LaunchInfo::LaunchInfo() = default;
 AppListLaunchRecorder::LaunchInfo::LaunchInfo(const LaunchInfo& other) =
     default;
-
 AppListLaunchRecorder::LaunchInfo::~LaunchInfo() = default;
 
 AppListLaunchRecorder* AppListLaunchRecorder::GetInstance() {
@@ -46,12 +29,4 @@ AppListLaunchRecorder::RegisterCallback(const LaunchEventCallback& callback) {
   return callback_list_.Add(callback);
 }
 
-// Notifies all observers of the given |launch_info|. If the
-// AppListMetricsProvider has been constructed, this will queue an
-// AppListLaunchEvent to be provided to the metrics service.
-void AppListLaunchRecorder::Record(const LaunchInfo& launch_info) {
-  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-  callback_list_.Notify(launch_info);
-}
-
 }  // namespace app_list

chrome/browser/ui/app_list/search/search_result_ranker/app_list_launch_recorder.h

@@ -7,8 +7,11 @@
 
 #include <memory>
 #include <string>
+#include <utility>
+#include <vector>
 
 #include "base/callback_list.h"
+#include "base/containers/flat_map.h"
 #include "base/macros.h"
 #include "base/no_destructor.h"
 #include "base/observer_list.h"
@@ -17,72 +20,71 @@
 
 namespace app_list {
 
-class SearchController;
+class AppListLaunchMetricsProvider;
 
-// AppListLaunchRecorder is a singleton that can be called to send hashed
-// logging events to UMA. Typical usage is:
-//
-//   AppListLaunchRecorder::GetInstance()->Record(my_data);
-//
-// This should only be called from the browser UI thread.
+// TODO(crbug.com/1016655): add comments and documentation once the API has been
+// finalized.
 class AppListLaunchRecorder {
  public:
-  // Stores information about a single launch event to be logged by a call to
-  // Record.
+  // Lists all clients using thie logging system. Each project should have an
+  // entry here. These values are persisted to logs. Entries should not be
+  // renumbered and numeric values should never be reused.
+  // TODO(crbug.com/1016655): add additional explanation for what it means to
+  // add a separate project (eg. different IDs) once the design has been
+  // finalized.
+  enum class Client {
+    kUnspecified = 0,
+    kTesting = 1,
+    kLauncher = 2,
+  };
+
   struct LaunchInfo {
-    LaunchInfo(metrics::ChromeOSAppListLaunchEventProto::LaunchType launch_type,
-               metrics::ChromeOSAppListLaunchEventProto::SearchProviderType
-                   search_provider_type,
-               const std::string& target,
-               const std::string& query,
-               const std::string& domain,
-               const std::string& app);
+    LaunchInfo();
     LaunchInfo(const LaunchInfo& other);
     ~LaunchInfo();
 
-    // Specifies which UI component this event was launched from.
-    metrics::ChromeOSAppListLaunchEventProto::LaunchType launch_type;
-    // Specifies which search provider created this event's result.
-    metrics::ChromeOSAppListLaunchEventProto::SearchProviderType
-        search_provider_type;
-    // A string identifier of the item being launched, eg. an app ID or
-    // filepath.
-    std::string target;
-    // The search query at the time of launch. If this is a zero-state launch
-    // (eg. from the suggested chips), this should be the empty string.
-    std::string query;
-    // The last-visited domain at the time of launch.
-    std::string domain;
-    // The app ID of the last-opened app at the time of launch.
-    std::string app;
+    AppListLaunchRecorder::Client client;
+    std::vector<std::pair<int, std::string>> hashed;
+    std::vector<std::pair<int, int>> unhashed;
   };
 
-  using LaunchEventCallback =
-      base::RepeatingCallback<void(const AppListLaunchRecorder::LaunchInfo&)>;
-  using LaunchEventSubscription = base::CallbackList<void(
-      const AppListLaunchRecorder::LaunchInfo&)>::Subscription;
-
-  // Returns the instance of AppListLaunchRecorder.
   static AppListLaunchRecorder* GetInstance();
 
-  // Registers a callback to be invoked on a call to Record().
-  std::unique_ptr<LaunchEventSubscription> RegisterCallback(
-      const LaunchEventCallback& callback);
-
  private:
   friend class base::NoDestructor<AppListLaunchRecorder>;
+  friend class app_list::AppListLaunchMetricsProvider;
 
-  // These are the clients of hashed logging:
-  friend class SearchController;
-
-  // Adds |launch_info| to the cache of launches to be hashed and provided to
-  // the metrics service on a call to ProvideCurrentSessionData.
-  void Record(const LaunchInfo& launch_info);
+  using EventFn = void(const LaunchInfo&);
+  using LaunchEventCallback = base::RepeatingCallback<EventFn>;
+  using LaunchEventCallbackList = base::CallbackList<EventFn>;
+  using LaunchEventSubscription = LaunchEventCallbackList::Subscription;
 
   AppListLaunchRecorder();
   ~AppListLaunchRecorder();
 
-  base::CallbackList<void(const LaunchInfo&)> callback_list_;
+  template <typename T>
+  void Log(Client client,
+           const std::vector<std::pair<T, std::string>>& hashed,
+           const std::vector<std::pair<T, int>>& unhashed) {
+    DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+    static_assert(std::is_enum<T>::value,
+                  "Non enum passed to AppListLaunchRecorder::Log");
+
+    LaunchInfo event;
+    event.client = client;
+    for (const auto& pair : hashed)
+      event.hashed.push_back({static_cast<int>(pair.first), pair.second});
+    for (const auto& pair : unhashed)
+      event.unhashed.push_back({static_cast<int>(pair.first), pair.second});
+
+    callback_list_.Notify(event);
+  }
+
+  // Registers a callback to be invoked on a call to Log().
+  std::unique_ptr<LaunchEventSubscription> RegisterCallback(
+      const LaunchEventCallback& callback);
+
+  LaunchEventCallbackList callback_list_;
 
   SEQUENCE_CHECKER(sequence_checker_);