[local-search-service] Add search metrics reporter

Add search metrics reporter for the new sandboxed Index. The logic is
similar to what we have done with IndexSync.

Design doc: go/lss-sandboxing
Implementation plan: go/lss-sandboxing-impl

Bug: 1137560
Change-Id: I4f54cf8eaa0f0f54db0e64b71749ea87c860d26f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2569563Reviewed-by: Jia Meng <jiameng@chromium.org>
Commit-Queue: Thanh Nguyen <thanhdng@chromium.org>
Cr-Commit-Position: refs/heads/master@{#833221}

chromeos/components/local_search_service/index.cc

@@ -4,12 +4,40 @@
 
 #include "chromeos/components/local_search_service/index.h"
 
+#include "base/metrics/histogram_functions.h"
 #include "base/optional.h"
 
 namespace chromeos {
 namespace local_search_service {
 
-Index::Index(IndexId index_id, Backend backend) {}
+namespace {
+
+void LogIndexIdAndBackendType(const std::string& histogram_prefix,
+                              Backend backend) {
+  base::UmaHistogramEnumeration(histogram_prefix + ".Backend", backend);
+}
+
+std::string IndexIdBasedHistogramPrefix(IndexId index_id) {
+  const std::string prefix = "LocalSearchService.";
+  switch (index_id) {
+    case IndexId::kCrosSettings:
+      return prefix + "CrosSettings";
+    case IndexId::kHelpApp:
+      return prefix + "HelpApp";
+  }
+}
+
+void OnSearchPerformedDone() {
+  // TODO(thanhdng): add a histogram to log this.
+}
+
+}  // namespace
+
+Index::Index(IndexId index_id, Backend backend) : index_id_(index_id) {
+  histogram_prefix_ = IndexIdBasedHistogramPrefix(index_id);
+  DCHECK(!histogram_prefix_.empty());
+  LogIndexIdAndBackendType(histogram_prefix_, backend);
+}
 
 Index::~Index() = default;
 
@@ -17,5 +45,34 @@ void Index::BindReceiver(mojo::PendingReceiver<mojom::Index> receiver) {
   receivers_.Add(this, std::move(receiver));
 }
 
+void Index::SetReporterRemote(
+    mojo::PendingRemote<mojom::SearchMetricsReporter> reporter_remote) {
+  DCHECK(!reporter_remote_.is_bound());
+  reporter_remote_.Bind(std::move(reporter_remote));
+}
+
+void Index::MaybeLogSearchResultsStats(ResponseStatus status,
+                                       size_t num_results,
+                                       base::TimeDelta latency) {
+  if (reporter_remote_.is_bound())
+    reporter_remote_->OnSearchPerformed(index_id_,
+                                        base::BindOnce(&OnSearchPerformedDone));
+
+  base::UmaHistogramEnumeration(histogram_prefix_ + ".ResponseStatus", status);
+  if (status == ResponseStatus::kSuccess) {
+    // Only logs number of results and latency if search is a success.
+    base::UmaHistogramCounts100(histogram_prefix_ + ".NumberResults",
+                                num_results);
+    base::UmaHistogramTimes(histogram_prefix_ + ".SearchLatency", latency);
+  }
+}
+
+void Index::MaybeLogIndexSize(uint64_t index_size) {
+  if (index_size != 0u) {
+    base::UmaHistogramCounts10000(histogram_prefix_ + ".NumberDocuments",
+                                  index_size);
+  }
+}
+
 }  // namespace local_search_service
 }  // namespace chromeos

chromeos/components/local_search_service/index.h

@@ -9,6 +9,7 @@
 
 #include "base/strings/string16.h"
 #include "chromeos/components/local_search_service/public/mojom/index.mojom.h"
+#include "chromeos/components/local_search_service/public/mojom/local_search_service.mojom.h"
 #include "mojo/public/cpp/bindings/pending_receiver.h"
 #include "mojo/public/cpp/bindings/receiver_set.h"
 
@@ -22,7 +23,25 @@ class Index : public mojom::Index {
 
   void BindReceiver(mojo::PendingReceiver<mojom::Index> receiver);
 
+  // Call once to set the SearchMetricsReporter remote.
+  void SetReporterRemote(
+      mojo::PendingRemote<mojom::SearchMetricsReporter> reporter_remote);
+
+ protected:
+  // Logs daily search metrics if |reporter_remote_| is bound. Also logs
+  // other UMA metrics (number results and search latency).
+  void MaybeLogSearchResultsStats(ResponseStatus status,
+                                  size_t num_results,
+                                  base::TimeDelta latency);
+
+  // Logs number of documents in the index.
+  void MaybeLogIndexSize(uint64_t index_size);
+
+  IndexId index_id_;
+
  private:
+  std::string histogram_prefix_;
+  mojo::Remote<mojom::SearchMetricsReporter> reporter_remote_;
   mojo::ReceiverSet<mojom::Index> receivers_;
 };
 

chromeos/components/local_search_service/index_sync.cc

@@ -49,9 +49,9 @@ IndexSync::IndexSync(IndexId index_id,
 
 IndexSync::~IndexSync() = default;
 
-void IndexSync::MaybeLogSearchResultsStats(ResponseStatus status,
-                                           size_t num_results,
-                                           base::TimeDelta latency) {
+void IndexSync::MaybeLogSearchResultsStatsSync(ResponseStatus status,
+                                               size_t num_results,
+                                               base::TimeDelta latency) {
   if (reporter_)
     reporter_->OnSearchPerformed();
 
@@ -64,7 +64,7 @@ void IndexSync::MaybeLogSearchResultsStats(ResponseStatus status,
   }
 }
 
-void IndexSync::MaybeLogIndexSize() {
+void IndexSync::MaybeLogIndexSizeSync() {
   const uint64_t index_size = GetSizeSync();
   if (index_size != 0u) {
     base::UmaHistogramCounts10000(histogram_prefix_ + ".NumberDocuments",

chromeos/components/local_search_service/index_sync.h

@@ -61,14 +61,14 @@ class IndexSync {
   // UMA metrics (number results and search latency).
   // Each implementation of this class should call this method at the end of
   // Find.
-  void MaybeLogSearchResultsStats(ResponseStatus status,
-                                  size_t num_results,
-                                  base::TimeDelta latency);
+  void MaybeLogSearchResultsStatsSync(ResponseStatus status,
+                                      size_t num_results,
+                                      base::TimeDelta latency);
 
   // Logs number of documents in the index if the index is not empty.
   // Each implementation of this class should call this method at the end of
   // Find.
-  void MaybeLogIndexSize();
+  void MaybeLogIndexSizeSync();
 
   void SetSearchParams(const SearchParams& search_params);
   SearchParams GetSearchParamsForTesting();

chromeos/components/local_search_service/inverted_index_search.cc

@@ -145,12 +145,12 @@ ResponseStatus InvertedIndexSearch::FindSync(const base::string16& query,
   results->clear();
   if (query.empty()) {
     const ResponseStatus status = ResponseStatus::kEmptyQuery;
-    MaybeLogSearchResultsStats(status, 0u, base::TimeDelta());
+    MaybeLogSearchResultsStatsSync(status, 0u, base::TimeDelta());
     return status;
   }
   if (GetSizeSync() == 0u) {
     const ResponseStatus status = ResponseStatus::kEmptyIndex;
-    MaybeLogSearchResultsStats(status, 0u, base::TimeDelta());
+    MaybeLogSearchResultsStatsSync(status, 0u, base::TimeDelta());
     return status;
   }
 
@@ -163,7 +163,7 @@ ResponseStatus InvertedIndexSearch::FindSync(const base::string16& query,
 
   const base::TimeTicks end = base::TimeTicks::Now();
   const ResponseStatus status = ResponseStatus::kSuccess;
-  MaybeLogSearchResultsStats(status, results->size(), end - start);
+  MaybeLogSearchResultsStatsSync(status, results->size(), end - start);
   return status;
 }
 
@@ -208,12 +208,17 @@ void InvertedIndexSearch::Find(const base::string16& query,
                                uint32_t max_results,
                                FindCallback callback) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+  const base::TimeTicks start = base::TimeTicks::Now();
   if (query.empty()) {
-    std::move(callback).Run(ResponseStatus::kEmptyQuery, base::nullopt);
+    const ResponseStatus status = ResponseStatus::kEmptyQuery;
+    MaybeLogSearchResultsStats(status, 0u, base::TimeDelta());
+    std::move(callback).Run(status, base::nullopt);
     return;
   }
   if (inverted_index_->NumberDocuments() == 0u) {
-    std::move(callback).Run(ResponseStatus::kEmptyIndex, base::nullopt);
+    const ResponseStatus status = ResponseStatus::kEmptyIndex;
+    MaybeLogSearchResultsStats(status, 0u, base::TimeDelta());
+    std::move(callback).Run(status, base::nullopt);
     return;
   }
 
@@ -224,7 +229,11 @@ void InvertedIndexSearch::Find(const base::string16& query,
 
   if (results.size() > max_results && max_results > 0u)
     results.resize(max_results);
-  std::move(callback).Run(ResponseStatus::kSuccess, results);
+
+  const ResponseStatus status = ResponseStatus::kSuccess;
+  const base::TimeTicks end = base::TimeTicks::Now();
+  MaybeLogSearchResultsStats(status, results.size(), end - start);
+  std::move(callback).Run(status, results);
 }
 
 void InvertedIndexSearch::ClearIndex(ClearIndexCallback callback) {

chromeos/components/local_search_service/linear_map_search.cc

@@ -99,7 +99,7 @@ void LinearMapSearch::AddOrUpdateSync(
     UpdateData(id, item.contents, &data_);
   }
 
-  MaybeLogIndexSize();
+  MaybeLogIndexSizeSync();
 }
 
 uint32_t LinearMapSearch::DeleteSync(const std::vector<std::string>& ids) {
@@ -109,7 +109,7 @@ uint32_t LinearMapSearch::DeleteSync(const std::vector<std::string>& ids) {
     num_deleted += data_.erase(id);
   }
 
-  MaybeLogIndexSize();
+  MaybeLogIndexSizeSync();
   return num_deleted;
 }
 
@@ -126,13 +126,13 @@ ResponseStatus LinearMapSearch::FindSync(const base::string16& query,
 
   if (query.empty()) {
     const ResponseStatus status = ResponseStatus::kEmptyQuery;
-    MaybeLogSearchResultsStats(status, 0u, base::TimeDelta());
+    MaybeLogSearchResultsStatsSync(status, 0u, base::TimeDelta());
     return status;
   }
 
   if (data_.empty()) {
     const ResponseStatus status = ResponseStatus::kEmptyIndex;
-    MaybeLogSearchResultsStats(status, 0u, base::TimeDelta());
+    MaybeLogSearchResultsStatsSync(status, 0u, base::TimeDelta());
     return status;
   }
 
@@ -140,7 +140,7 @@ ResponseStatus LinearMapSearch::FindSync(const base::string16& query,
 
   const base::TimeTicks end = base::TimeTicks::Now();
   const ResponseStatus status = ResponseStatus::kSuccess;
-  MaybeLogSearchResultsStats(status, results->size(), end - start);
+  MaybeLogSearchResultsStatsSync(status, results->size(), end - start);
   return status;
 }
 
@@ -150,13 +150,13 @@ void LinearMapSearch::GetSize(GetSizeCallback callback) {
 
 void LinearMapSearch::AddOrUpdate(const std::vector<Data>& data,
                                   AddOrUpdateCallback callback) {
-  // TODO(thanhdng): Add logging to this function once we have the metrics
-  // reporter available.
   for (const auto& item : data) {
     const auto& id = item.id;
     DCHECK(!id.empty());
     UpdateData(id, item.contents, &data_);
   }
+
+  MaybeLogIndexSize(data_.size());
   std::move(callback).Run();
 }
 
@@ -168,6 +168,7 @@ void LinearMapSearch::Delete(const std::vector<std::string>& ids,
     num_deleted += data_.erase(id);
   }
 
+  MaybeLogIndexSize(data_.size());
   std::move(callback).Run(num_deleted);
 }
 
@@ -185,24 +186,35 @@ void LinearMapSearch::UpdateDocuments(const std::vector<Data>& data,
     }
   }
 
+  MaybeLogIndexSize(data_.size());
   std::move(callback).Run(num_deleted);
 }
 
 void LinearMapSearch::Find(const base::string16& query,
                            uint32_t max_results,
                            FindCallback callback) {
+  const base::TimeTicks start = base::TimeTicks::Now();
   if (query.empty()) {
-    std::move(callback).Run(ResponseStatus::kEmptyQuery, base::nullopt);
+    const ResponseStatus status = ResponseStatus::kEmptyQuery;
+    MaybeLogSearchResultsStats(status, 0u, base::TimeDelta());
+    std::move(callback).Run(status, base::nullopt);
     return;
   }
 
   if (data_.empty()) {
-    std::move(callback).Run(ResponseStatus::kEmptyIndex, base::nullopt);
+    const ResponseStatus status = ResponseStatus::kEmptyIndex;
+    MaybeLogSearchResultsStats(status, 0u, base::TimeDelta());
+    std::move(callback).Run(status, base::nullopt);
     return;
   }
 
   std::vector<Result> results = GetSearchResults(query, max_results);
-  std::move(callback).Run(ResponseStatus::kSuccess, std::move(results));
+
+  const ResponseStatus status = ResponseStatus::kSuccess;
+  const base::TimeTicks end = base::TimeTicks::Now();
+  MaybeLogSearchResultsStats(status, results.size(), end - start);
+
+  std::move(callback).Run(status, std::move(results));
 }
 
 void LinearMapSearch::ClearIndex(ClearIndexCallback callback) {

chromeos/components/local_search_service/local_search_service.cc

@@ -47,9 +47,9 @@ void LocalSearchService::BindIndex(
     return;
   }
 
-  if (reporter_remote)
-    reporter_remote_set_.Add(std::move(reporter_remote));
   it->second->BindReceiver(std::move(index_receiver));
+  if (reporter_remote)
+    it->second->SetReporterRemote(std::move(reporter_remote));
   std::move(callback).Run(base::nullopt);
 }
 

chromeos/components/local_search_service/local_search_service.h

@@ -31,7 +31,6 @@ class LocalSearchService : public mojom::LocalSearchService {
 
  private:
   mojo::Receiver<mojom::LocalSearchService> receiver_;
-  mojo::RemoteSet<mojom::SearchMetricsReporter> reporter_remote_set_;
   std::map<IndexId, std::unique_ptr<Index>> indices_;
 };