Log hint cache UMA in OGKS path

The refactor of the hints manager unittest and metrics recording is
not strictly required but done in the below CL:
https://chromium-review.googlesource.com/c/chromium/src/+/1762982

This is mostly to reduce the amount of merge conflicts I will get

Bug: 969558
Change-Id: I49d7afc1a977bc2111dedc48fdd5cf5e3c022184
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1772476
Commit-Queue: Sophie Chang <sophiechang@chromium.org>
Reviewed-by: Doug Arnett <dougarnett@chromium.org>
Cr-Commit-Position: refs/heads/master@{#690925}

chrome/browser/optimization_guide/optimization_guide_hints_manager.cc

@@ -100,6 +100,21 @@ bool CanProcessComponentVersion(PrefService* pref_service,
   return true;
 }
 
+// Returns the OptimizationGuideNavigationData for |navigation_handle| if the
+// OptimizationGuideWebContentsObserver is registered.
+OptimizationGuideNavigationData* GetNavigationDataForNavigationHandle(
+    content::NavigationHandle* navigation_handle) {
+  OptimizationGuideWebContentsObserver*
+      optimization_guide_web_contents_observer =
+          OptimizationGuideWebContentsObserver::FromWebContents(
+              navigation_handle->GetWebContents());
+  if (!optimization_guide_web_contents_observer)
+    return nullptr;
+
+  return optimization_guide_web_contents_observer
+      ->GetOrCreateOptimizationGuideNavigationData(navigation_handle);
+}
+
 }  // namespace
 
 OptimizationGuideHintsManager::OptimizationGuideHintsManager(
@@ -482,6 +497,17 @@ void OptimizationGuideHintsManager::LoadHintForNavigation(
     return;
   }
 
+  OptimizationGuideNavigationData* navigation_data =
+      GetNavigationDataForNavigationHandle(navigation_handle);
+  if (navigation_data) {
+    bool has_hint = hint_cache_->HasHint(url.host());
+    if (navigation_handle->HasCommitted()) {
+      navigation_data->set_has_hint_after_commit(has_hint);
+    } else {
+      navigation_data->set_has_hint_before_commit(has_hint);
+    }
+  }
+
   hint_cache_->LoadHint(
       url.host(),
       base::BindOnce(&OptimizationGuideHintsManager::OnHintLoaded,
@@ -591,24 +617,23 @@ void OptimizationGuideHintsManager::CanApplyOptimization(
   // Check if we have a hint already loaded for this navigation.
   const optimization_guide::proto::Hint* loaded_hint =
       hint_cache_->GetHintIfLoaded(host);
-  if (loaded_hint) {
-    OptimizationGuideNavigationData* navigation_data = nullptr;
-    OptimizationGuideWebContentsObserver*
-        optimization_guide_web_contents_observer =
-            OptimizationGuideWebContentsObserver::FromWebContents(
-                navigation_handle->GetWebContents());
-    if (optimization_guide_web_contents_observer) {
-      navigation_data =
-          optimization_guide_web_contents_observer
-              ->GetOrCreateOptimizationGuideNavigationData(navigation_handle);
+  bool has_hint_in_cache = hint_cache_->HasHint(host);
+  const optimization_guide::proto::PageHint* matched_page_hint =
+      loaded_hint ? optimization_guide::FindPageHintForURL(url, loaded_hint)
+                  : nullptr;
+
+  // Populate navigation data with hint information.
+  OptimizationGuideNavigationData* navigation_data =
+      GetNavigationDataForNavigationHandle(navigation_handle);
+  if (navigation_data) {
+    navigation_data->set_has_hint_after_commit(has_hint_in_cache);
+    navigation_data->set_has_page_hint(matched_page_hint);
+
+    if (loaded_hint)
       navigation_data->set_serialized_hint_version_string(
           loaded_hint->version());
-    }
   }
 
-  const optimization_guide::proto::PageHint* matched_page_hint =
-      loaded_hint ? optimization_guide::FindPageHintForURL(url, loaded_hint)
-                  : nullptr;
   if (matched_page_hint && matched_page_hint->has_max_ect_trigger()) {
     max_ect_trigger = optimization_guide::ConvertProtoEffectiveConnectionType(
         matched_page_hint->max_ect_trigger());
@@ -659,7 +684,7 @@ void OptimizationGuideHintsManager::CanApplyOptimization(
     // cache, we do not know what to do with the URL so just return.
     // Otherwise, we do have information, but we just do not know it yet.
     *optimization_type_decision =
-        hint_cache_->HasHint(host)
+        has_hint_in_cache
             ? optimization_guide::OptimizationTypeDecision::
                   kHadHintButNotLoadedInTime
             : optimization_guide::OptimizationTypeDecision::kNoHintAvailable;

chrome/browser/optimization_guide/optimization_guide_keyed_service_browsertest.cc

@@ -26,6 +26,7 @@
 #include "components/optimization_guide/proto/hints.pb.h"
 #include "components/optimization_guide/test_hints_component_creator.h"
 #include "components/prefs/pref_service.h"
+#include "components/previews/core/previews_features.h"
 #include "components/previews/core/previews_switches.h"
 #include "components/ukm/test_ukm_recorder.h"
 #include "content/public/test/browser_test_utils.h"
@@ -136,7 +137,11 @@ class OptimizationGuideKeyedServiceBrowserTest
     scoped_feature_list_.InitWithFeatures(
         {optimization_guide::features::kOptimizationHints,
          optimization_guide::features::kOptimizationGuideKeyedService},
-        {});
+        // It is possible for the same things to get recorded using Previews -
+        // disable it for these tests.
+        // TODO(crbug/969558): Remove this feature disable when the switch to
+        // only instantiate the hint cache logic in one place has landed.
+        {previews::features::kPreviews});
 
     OptimizationGuideKeyedServiceDisabledBrowserTest::SetUp();
   }
@@ -281,6 +286,15 @@ IN_PROC_BROWSER_TEST_F(OptimizationGuideKeyedServiceBrowserTest,
   // load a hint that succeeds.
   histogram_tester.ExpectUniqueSample("OptimizationGuide.LoadedHint.Result",
                                       true, 1);
+  // Make sure hint cache match UMA was logged.
+  histogram_tester.ExpectUniqueSample(
+      "OptimizationGuide.HintCache.HasHint.BeforeCommit", true, 1);
+  histogram_tester.ExpectUniqueSample(
+      "OptimizationGuide.HintCache.HasHint.AtCommit", true, 1);
+  histogram_tester.ExpectUniqueSample(
+      "OptimizationGuide.HintCache.HostMatch.AtCommit", true, 1);
+  histogram_tester.ExpectUniqueSample(
+      "OptimizationGuide.HintCache.PageMatch.AtCommit", true, 1);
 
   // We had a hint but it wasn't loaded and it was painful enough.
   EXPECT_EQ(optimization_guide::OptimizationGuideDecision::kTrue,
@@ -370,6 +384,15 @@ IN_PROC_BROWSER_TEST_F(
   // Hint is still applicable so we expect it to be allowed to be applied.
   EXPECT_EQ(optimization_guide::OptimizationGuideDecision::kTrue,
             last_consumer_decision());
+  // Make sure hint cache match UMA was logged.
+  histogram_tester.ExpectUniqueSample(
+      "OptimizationGuide.HintCache.HasHint.BeforeCommit", true, 1);
+  histogram_tester.ExpectUniqueSample(
+      "OptimizationGuide.HintCache.HasHint.AtCommit", true, 1);
+  histogram_tester.ExpectUniqueSample(
+      "OptimizationGuide.HintCache.HostMatch.AtCommit", true, 1);
+  histogram_tester.ExpectUniqueSample(
+      "OptimizationGuide.HintCache.PageMatch.AtCommit", true, 1);
   // Expect that the optimization guide UKM was recorded.
   auto entries = ukm_recorder.GetEntriesByName(
       ukm::builders::OptimizationGuide::kEntryName);
@@ -413,6 +436,15 @@ IN_PROC_BROWSER_TEST_F(OptimizationGuideKeyedServiceBrowserTest,
       static_cast<int>(
           optimization_guide::OptimizationTargetDecision::kPageLoadMatches),
       1);
+  // Make sure hint cache match UMA was logged.
+  histogram_tester.ExpectUniqueSample(
+      "OptimizationGuide.HintCache.HasHint.BeforeCommit", false, 1);
+  histogram_tester.ExpectUniqueSample(
+      "OptimizationGuide.HintCache.HasHint.AtCommit", false, 1);
+  histogram_tester.ExpectTotalCount(
+      "OptimizationGuide.HintCache.HostMatch.AtCommit", 0);
+  histogram_tester.ExpectTotalCount(
+      "OptimizationGuide.HintCache.PageMatch.AtCommit", 0);
   // Should expect that no hints were loaded and so we don't have a hint
   // version recorded.
   auto entries = ukm_recorder.GetEntriesByName(
@@ -448,6 +480,15 @@ IN_PROC_BROWSER_TEST_F(OptimizationGuideKeyedServiceBrowserTest,
       static_cast<int>(
           optimization_guide::OptimizationTargetDecision::kPageLoadMatches),
       1);
+  // Make sure hint cache match UMA was logged.
+  histogram_tester.ExpectUniqueSample(
+      "OptimizationGuide.HintCache.HasHint.BeforeCommit", true, 1);
+  histogram_tester.ExpectUniqueSample(
+      "OptimizationGuide.HintCache.HasHint.AtCommit", true, 1);
+  histogram_tester.ExpectUniqueSample(
+      "OptimizationGuide.HintCache.HostMatch.AtCommit", true, 1);
+  histogram_tester.ExpectUniqueSample(
+      "OptimizationGuide.HintCache.PageMatch.AtCommit", true, 1);
   // Should expect that UKM was not recorded since it did not have a version.
   auto entries = ukm_recorder.GetEntriesByName(
       ukm::builders::OptimizationGuide::kEntryName);
@@ -482,6 +523,15 @@ IN_PROC_BROWSER_TEST_F(OptimizationGuideKeyedServiceBrowserTest,
       static_cast<int>(
           optimization_guide::OptimizationTargetDecision::kPageLoadMatches),
       1);
+  // Make sure hint cache match UMA was logged.
+  histogram_tester.ExpectUniqueSample(
+      "OptimizationGuide.HintCache.HasHint.BeforeCommit", true, 1);
+  histogram_tester.ExpectUniqueSample(
+      "OptimizationGuide.HintCache.HasHint.AtCommit", true, 1);
+  histogram_tester.ExpectUniqueSample(
+      "OptimizationGuide.HintCache.HostMatch.AtCommit", true, 1);
+  histogram_tester.ExpectUniqueSample(
+      "OptimizationGuide.HintCache.PageMatch.AtCommit", true, 1);
   // Should expect that UKM was not recorded since it had a bad version string.
   auto entries = ukm_recorder.GetEntriesByName(
       ukm::builders::OptimizationGuide::kEntryName);

chrome/browser/optimization_guide/optimization_guide_navigation_data.cc

@@ -6,6 +6,7 @@
 
 #include "base/base64.h"
 #include "base/metrics/histogram_functions.h"
+#include "base/metrics/histogram_macros.h"
 #include "base/strings/stringprintf.h"
 #include "components/optimization_guide/hints_processing_util.h"
 #include "services/metrics/public/cpp/ukm_builders.h"
@@ -41,7 +42,42 @@ OptimizationGuideNavigationData::~OptimizationGuideNavigationData() = default;
 OptimizationGuideNavigationData::OptimizationGuideNavigationData(
     const OptimizationGuideNavigationData& other) = default;
 
-void OptimizationGuideNavigationData::RecordMetrics() const {
+void OptimizationGuideNavigationData::RecordMetrics(bool has_committed) const {
+  RecordHintCacheMatch(has_committed);
+  RecordOptimizationTypeAndTargetDecisions();
+  RecordOptimizationGuideUKM();
+}
+
+void OptimizationGuideNavigationData::RecordHintCacheMatch(
+    bool has_committed) const {
+  if (has_hint_before_commit_.has_value()) {
+    UMA_HISTOGRAM_BOOLEAN("OptimizationGuide.HintCache.HasHint.BeforeCommit",
+                          has_hint_before_commit_.value());
+  }
+  // If the navigation didn't commit, then don't proceed to record any of the
+  // remaining metrics.
+  if (!has_committed || !has_hint_after_commit_.has_value())
+    return;
+
+  UMA_HISTOGRAM_BOOLEAN("OptimizationGuide.HintCache.HasHint.AtCommit",
+                        has_hint_after_commit_.value());
+  // The remaining metrics rely on having a hint, so do not record them if we
+  // did not have a hint for the navigation.
+  if (!has_hint_after_commit_.value())
+    return;
+
+  bool had_hint_loaded = serialized_hint_version_string_.has_value();
+  UMA_HISTOGRAM_BOOLEAN("OptimizationGuide.HintCache.HostMatch.AtCommit",
+                        had_hint_loaded);
+  if (had_hint_loaded) {
+    UMA_HISTOGRAM_BOOLEAN("OptimizationGuide.HintCache.PageMatch.AtCommit",
+                          has_page_hint_.has_value() && has_page_hint_.value());
+  }
+}
+
+void OptimizationGuideNavigationData::RecordOptimizationTypeAndTargetDecisions()
+    const {
+  // Record optimization type decisions.
   for (const auto& optimization_type_decision : optimization_type_decisions_) {
     optimization_guide::proto::OptimizationType optimization_type =
         optimization_type_decision.first;
@@ -57,6 +93,7 @@ void OptimizationGuideNavigationData::RecordMetrics() const {
             optimization_guide::OptimizationTypeDecision::kMaxValue));
   }
 
+  // Record optimization target decisions.
   for (const auto& optimization_target_decision :
        optimization_target_decisions_) {
     optimization_guide::OptimizationTarget optimization_target =
@@ -71,7 +108,9 @@ void OptimizationGuideNavigationData::RecordMetrics() const {
         static_cast<int>(
             optimization_guide::OptimizationTargetDecision::kMaxValue));
   }
+}
 
+void OptimizationGuideNavigationData::RecordOptimizationGuideUKM() const {
   if (!serialized_hint_version_string_.has_value() ||
       serialized_hint_version_string_.value().empty())
     return;

chrome/browser/optimization_guide/optimization_guide_navigation_data.h

@@ -15,6 +15,7 @@
 #include "components/optimization_guide/proto/hints.pb.h"
 
 // A representation of optimization guide information related to a navigation.
+// This also includes methods for recording metrics based on this data.
 class OptimizationGuideNavigationData {
  public:
   explicit OptimizationGuideNavigationData(int64_t navigation_id);
@@ -22,8 +23,9 @@ class OptimizationGuideNavigationData {
 
   OptimizationGuideNavigationData(const OptimizationGuideNavigationData& other);
 
-  // Records metrics based on data currently held in |this|.
-  void RecordMetrics() const;
+  // Records metrics based on data currently held in |this|. |has_committed|
+  // indicates whether commit-time metrics should be recorded.
+  void RecordMetrics(bool has_committed) const;
 
   // The navigation ID of the navigation handle that this data is associated
   // with.
@@ -56,7 +58,39 @@ class OptimizationGuideNavigationData {
       optimization_guide::OptimizationTarget optimization_target,
       optimization_guide::OptimizationTargetDecision decision);
 
+  // Whether the hint cache had a hint for the navigation before commit.
+  base::Optional<bool> has_hint_before_commit() const {
+    return has_hint_before_commit_;
+  }
+  void set_has_hint_before_commit(bool has_hint_before_commit) {
+    has_hint_before_commit_ = has_hint_before_commit;
+  }
+
+  // Whether the hint cache had a hint after commit.
+  base::Optional<bool> has_hint_after_commit() const {
+    return has_hint_after_commit_;
+  }
+  void set_has_hint_after_commit(bool has_hint_after_commit) {
+    has_hint_after_commit_ = has_hint_after_commit;
+  }
+
+  // Whether the hint cache had a page hint for the navigation.
+  base::Optional<bool> has_page_hint() const { return has_page_hint_; }
+  void set_has_page_hint(bool has_page_hint) { has_page_hint_ = has_page_hint; }
+
  private:
+  // Records hint cache histograms based on data currently held in |this|.
+  void RecordHintCacheMatch(bool has_committed) const;
+
+  // Records histograms for the decisions made for each optimization target and
+  // type that was queried for the navigation based on data currently held in
+  // |this|.
+  void RecordOptimizationTypeAndTargetDecisions() const;
+
+  // Records the OptimizationGuide UKM event based on data currently held in
+  // |this|.
+  void RecordOptimizationGuideUKM() const;
+
   // The navigation ID of the navigation handle that this data is associated
   // with.
   const int64_t navigation_id_;
@@ -74,6 +108,15 @@ class OptimizationGuideNavigationData {
                      optimization_guide::OptimizationTargetDecision>
       optimization_target_decisions_;
 
+  // Whether the hint cache had a hint for the navigation before commit.
+  base::Optional<bool> has_hint_before_commit_ = base::nullopt;
+
+  // Whether the hint cache had a hint for the navigation after commit.
+  base::Optional<bool> has_hint_after_commit_ = base::nullopt;
+
+  // Whether there was a page hint for the navigation.
+  base::Optional<bool> has_page_hint_ = base::nullopt;
+
   DISALLOW_ASSIGN(OptimizationGuideNavigationData);
 };
 

chrome/browser/optimization_guide/optimization_guide_web_contents_observer.cc

@@ -14,10 +14,6 @@
 #include "components/optimization_guide/proto/hints.pb.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/navigation_handle.h"
-#include "services/metrics/public/cpp/ukm_builders.h"
-#include "services/metrics/public/cpp/ukm_recorder.h"
-#include "services/metrics/public/cpp/ukm_source.h"
-#include "services/metrics/public/cpp/ukm_source_id.h"
 
 namespace {
 
@@ -124,18 +120,19 @@ void OptimizationGuideWebContentsObserver::DidFinishNavigation(
       base::BindOnce(&OptimizationGuideWebContentsObserver::
                          FlushMetricsAndRemoveOptimizationGuideNavigationData,
                      weak_factory_.GetWeakPtr(),
-                     navigation_handle->GetNavigationId()));
+                     navigation_handle->GetNavigationId(),
+                     navigation_handle->HasCommitted()));
 }
 
 void OptimizationGuideWebContentsObserver::
-    FlushMetricsAndRemoveOptimizationGuideNavigationData(
-        int64_t navigation_id) {
+    FlushMetricsAndRemoveOptimizationGuideNavigationData(int64_t navigation_id,
+                                                         bool has_committed) {
   auto nav_data_iter =
       inflight_optimization_guide_navigation_datas_.find(navigation_id);
   if (nav_data_iter == inflight_optimization_guide_navigation_datas_.end())
     return;
 
-  (nav_data_iter->second).RecordMetrics();
+  (nav_data_iter->second).RecordMetrics(has_committed);
 
   inflight_optimization_guide_navigation_datas_.erase(navigation_id);
 }

chrome/browser/optimization_guide/optimization_guide_web_contents_observer.h

@@ -54,7 +54,8 @@ class OptimizationGuideWebContentsObserver
   // |navigation_id| and then removes any data associated with it, including its
   // entry in |inflight_optimization_guide_navigation_datas_|.
   void FlushMetricsAndRemoveOptimizationGuideNavigationData(
-      int64_t navigation_id);
+      int64_t navigation_id,
+      bool has_committed);
 
   // The data related to a given navigation ID.
   std::unordered_map<int64_t, OptimizationGuideNavigationData>

chrome/browser/previews/previews_content_util.cc

@@ -241,9 +241,6 @@ content::PreviewsState DetermineAllowedClientPreviewsState(
     return content::LITE_PAGE_REDIRECT_ON;
   }
 
-  // Record whether the hint cache has a matching entry for this pre-commit URL.
-  previews_decider->LogHintCacheMatch(url, false /* is_committed */);
-
   if (!previews::params::ArePreviewsAllowed()) {
     return previews_state;
   }
@@ -255,6 +252,9 @@ content::PreviewsState DetermineAllowedClientPreviewsState(
   if (!is_data_saver_user)
     return previews_state;
 
+  // Record whether the hint cache has a matching entry for this pre-commit URL.
+  previews_decider->LogHintCacheMatch(url, false /* is_committed */);
+
   auto* previews_service =
       navigation_handle && navigation_handle->GetWebContents()
           ? PreviewsServiceFactory::GetForProfile(Profile::FromBrowserContext(