Add UKM Metrics to NavigationPredictor Renderer Warmup

Moves the feature logic to a check-everything paradigm instead of doing
early returns, for the sake of easy to understand metrics.

privacy doc: http://shortn/_99lQLYJfL1

Bug: 1115259
Change-Id: I036a412104d9ccce885789637461314d50f1e967
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2388270Reviewed-by: Alexei Svitkine <asvitkine@chromium.org>
Reviewed-by: Brian White <bcwhite@chromium.org>
Reviewed-by: Ryan Sturm <ryansturm@chromium.org>
Commit-Queue: Robert Ogden <robertogden@chromium.org>
Cr-Commit-Position: refs/heads/master@{#806701}

chrome/browser/navigation_predictor/navigation_predictor_keyed_service.cc

@@ -15,7 +15,7 @@
 #include "content/public/browser/web_contents.h"
 
 NavigationPredictorKeyedService::Prediction::Prediction(
-    const content::WebContents* web_contents,
+    content::WebContents* web_contents,
     const base::Optional<GURL>& source_document_url,
     const base::Optional<std::vector<std::string>>& external_app_packages_name,
     PredictionSource prediction_source,
@@ -112,7 +112,7 @@ NavigationPredictorKeyedService::Prediction::sorted_predicted_urls() const {
   return sorted_predicted_urls_;
 }
 
-const content::WebContents*
+content::WebContents*
 NavigationPredictorKeyedService::Prediction::web_contents() const {
   DCHECK_EQ(PredictionSource::kAnchorElementsParsedFromWebPage,
             prediction_source_);
@@ -141,7 +141,7 @@ NavigationPredictorKeyedService::~NavigationPredictorKeyedService() {
 }
 
 void NavigationPredictorKeyedService::OnPredictionUpdated(
-    const content::WebContents* web_contents,
+    content::WebContents* web_contents,
     const GURL& document_url,
     PredictionSource prediction_source,
     const std::vector<GURL>& sorted_predicted_urls) {

chrome/browser/navigation_predictor/navigation_predictor_keyed_service.h

@@ -40,7 +40,7 @@ class NavigationPredictorKeyedService : public KeyedService {
   // Stores the next set of URLs that the user is expected to navigate to.
   class Prediction {
    public:
-    Prediction(const content::WebContents* web_contents,
+    Prediction(content::WebContents* web_contents,
                const base::Optional<GURL>& source_document_url,
                const base::Optional<std::vector<std::string>>&
                    external_app_packages_name,
@@ -56,13 +56,13 @@ class NavigationPredictorKeyedService : public KeyedService {
     const std::vector<GURL>& sorted_predicted_urls() const;
 
     // Null if the prediction source is kExternalAndroidApp.
-    const content::WebContents* web_contents() const;
+    content::WebContents* web_contents() const;
 
    private:
     // The WebContents from where the navigation may happen. Do not use this
     // pointer outside the observer's call stack unless its destruction is also
     // observed.
-    const content::WebContents* web_contents_;
+    content::WebContents* web_contents_;
 
     // Current URL of the document from where the navigtion may happen.
     base::Optional<GURL> source_document_url_;
@@ -113,7 +113,7 @@ class NavigationPredictorKeyedService : public KeyedService {
   SearchEnginePreconnector* search_engine_preconnector();
 
   // |document_url| may be invalid. Called by navigation predictor.
-  void OnPredictionUpdated(const content::WebContents* web_contents,
+  void OnPredictionUpdated(content::WebContents* web_contents,
                            const GURL& document_url,
                            PredictionSource prediction_source,
                            const std::vector<GURL>& sorted_predicted_urls);

chrome/browser/navigation_predictor/navigation_predictor_renderer_warmup_client.cc

@@ -18,7 +18,13 @@
 #include "components/search_engines/template_url_service.h"
 #include "content/public/browser/browser_task_traits.h"
 #include "content/public/browser/browser_thread.h"
+#include "content/public/browser/render_frame_host.h"
 #include "content/public/browser/render_process_host.h"
+#include "content/public/browser/web_contents.h"
+#include "services/metrics/public/cpp/metrics_utils.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 "url/gurl.h"
 #include "url/origin.h"
 
@@ -27,6 +33,11 @@ const base::Feature kNavigationPredictorRendererWarmup{
     "NavigationPredictorRendererWarmup", base::FEATURE_DISABLED_BY_DEFAULT};
 }
 
+NavigationPredictorRendererWarmupClient::PredictionMetrics::
+    PredictionMetrics() = default;
+NavigationPredictorRendererWarmupClient::PredictionMetrics::
+    ~PredictionMetrics() = default;
+
 NavigationPredictorRendererWarmupClient::
     ~NavigationPredictorRendererWarmupClient() = default;
 NavigationPredictorRendererWarmupClient::
@@ -78,6 +89,8 @@ NavigationPredictorRendererWarmupClient::
 void NavigationPredictorRendererWarmupClient::OnPredictionUpdated(
     const base::Optional<NavigationPredictorKeyedService::Prediction>
         prediction) {
+  DCHECK(!metrics_);
+
   if (!prediction) {
     return;
   }
@@ -88,6 +101,10 @@ void NavigationPredictorRendererWarmupClient::OnPredictionUpdated(
     return;
   }
 
+  if (!prediction->web_contents()) {
+    return;
+  }
+
   if (!prediction->source_document_url()) {
     return;
   }
@@ -96,14 +113,19 @@ void NavigationPredictorRendererWarmupClient::OnPredictionUpdated(
     return;
   }
 
-  if (!IsEligibleForWarmupOnCommonCriteria()) {
+  if (!base::FeatureList::IsEnabled(kNavigationPredictorRendererWarmup)) {
     return;
   }
 
-  if (IsEligibleForCrossNavigationWarmup(*prediction) ||
-      IsEligibleForDSEWarmup(*prediction)) {
-    RecordMetricsAndMaybeDoWarmup();
-  }
+  metrics_ = std::make_unique<PredictionMetrics>();
+
+  // Each of these methods will set some state in |metrics_| which is used in
+  // |RecordMetricsAndMaybeDoWarmup|.
+  CheckIsEligibleForWarmupOnCommonCriteria();
+  CheckIsEligibleForCrossNavigationWarmup(*prediction);
+  CheckIsEligibleForDSEWarmup(*prediction);
+
+  RecordMetricsAndMaybeDoWarmup(prediction->web_contents());
 }
 
 void NavigationPredictorRendererWarmupClient::DoRendererWarmpup() {
@@ -121,36 +143,26 @@ bool NavigationPredictorRendererWarmupClient::BrowserHasSpareRenderer() const {
   return false;
 }
 
-bool NavigationPredictorRendererWarmupClient::
-    IsEligibleForWarmupOnCommonCriteria() const {
-  if (!base::FeatureList::IsEnabled(kNavigationPredictorRendererWarmup)) {
-    return false;
-  }
-
+void NavigationPredictorRendererWarmupClient::
+    CheckIsEligibleForWarmupOnCommonCriteria() {
   base::TimeDelta duration_since_last_warmup =
       tick_clock_->NowTicks() - last_warmup_time_;
   if (cooldown_duration_ >= duration_since_last_warmup) {
-    return false;
-  }
-
-  if (mem_threshold_mb_ >= base::SysInfo::AmountOfPhysicalMemoryMB()) {
-    return false;
+    metrics_->page_independent_status |= 1 << 0;
   }
 
   if (BrowserHasSpareRenderer()) {
-    return false;
+    metrics_->page_independent_status |= 1 << 1;
   }
 
-  return true;
-}
-
-bool NavigationPredictorRendererWarmupClient::
-    IsEligibleForCrossNavigationWarmup(
-        const NavigationPredictorKeyedService::Prediction& prediction) const {
-  if (!use_navigation_predictions_) {
-    return false;
+  if (mem_threshold_mb_ >= base::SysInfo::AmountOfPhysicalMemoryMB()) {
+    metrics_->page_independent_status |= 1 << 2;
   }
+}
 
+void NavigationPredictorRendererWarmupClient::
+    CheckIsEligibleForCrossNavigationWarmup(
+        const NavigationPredictorKeyedService::Prediction& prediction) {
   url::Origin src_origin =
       url::Origin::Create(prediction.source_document_url().value());
 
@@ -159,7 +171,7 @@ bool NavigationPredictorRendererWarmupClient::
   size_t examine_n_urls =
       std::min(urls.size(), static_cast<size_t>(examine_top_n_predictions_));
   if (examine_n_urls == 0) {
-    return false;
+    return;
   }
 
   int cross_origin_count = 0;
@@ -180,26 +192,54 @@ bool NavigationPredictorRendererWarmupClient::
     }
   }
 
-  // Just in case there's very few links on a page, check against the threshold
-  // as a ratio. This may be helpful on redirector sites, like Cloudflare's DDoS
-  // checker.
-  double cross_origin_ratio = static_cast<double>(cross_origin_count) /
-                              static_cast<double>(examine_n_urls);
-  return cross_origin_ratio >= prediction_crosss_origin_threshold_;
+  // Just in case there's very few links on a page, use a ratio. This may be
+  // helpful on redirector sites, like Cloudflare's DDoS checker.
+  metrics_->cross_origin_links_ratio = static_cast<double>(cross_origin_count) /
+                                       static_cast<double>(examine_n_urls);
+}
+
+void NavigationPredictorRendererWarmupClient::CheckIsEligibleForDSEWarmup(
+    const NavigationPredictorKeyedService::Prediction& prediction) {
+  metrics_->was_dse_srp = TemplateURLServiceFactory::GetForProfile(profile_)
+                              ->IsSearchResultsPageFromDefaultSearchProvider(
+                                  prediction.source_document_url().value());
 }
 
-bool NavigationPredictorRendererWarmupClient::IsEligibleForDSEWarmup(
-    const NavigationPredictorKeyedService::Prediction& prediction) const {
-  if (!warmup_on_dse_) {
-    return false;
+void NavigationPredictorRendererWarmupClient::RecordMetricsAndMaybeDoWarmup(
+    content::WebContents* web_contents) {
+  bool eligible_on_common_criteria = metrics_->page_independent_status == 0;
+
+  bool eligible_for_cross_origin_warmup =
+      use_navigation_predictions_ &&
+      metrics_->cross_origin_links_ratio.has_value() &&
+      metrics_->cross_origin_links_ratio.value() >=
+          prediction_crosss_origin_threshold_;
+
+  bool eligible_for_dse_warmup = warmup_on_dse_ && metrics_->was_dse_srp;
+
+  bool do_warmup =
+      eligible_on_common_criteria &&
+      (eligible_for_cross_origin_warmup || eligible_for_dse_warmup);
+  metrics_->did_warmup = do_warmup;
+
+  // Record metrics in UKM.
+  ukm::builders::NavigationPredictorRendererWarmup builder(
+      web_contents->GetMainFrame()->GetPageUkmSourceId());
+  if (metrics_->cross_origin_links_ratio) {
+    builder.SetCrossOriginLinksRatio(
+        static_cast<int>(100.0 * metrics_->cross_origin_links_ratio.value()));
   }
+  builder.SetDidWarmup(metrics_->did_warmup);
+  builder.SetPageIndependentStatusBitMask(metrics_->page_independent_status);
+  builder.SetWasDSESRP(metrics_->was_dse_srp);
+  builder.Record(ukm::UkmRecorder::Get());
 
-  return TemplateURLServiceFactory::GetForProfile(profile_)
-      ->IsSearchResultsPageFromDefaultSearchProvider(
-          prediction.source_document_url().value());
-}
+  metrics_.reset();
+
+  if (!do_warmup) {
+    return;
+  }
 
-void NavigationPredictorRendererWarmupClient::RecordMetricsAndMaybeDoWarmup() {
   last_warmup_time_ = tick_clock_->NowTicks();
 
   if (counterfactual_) {

chrome/browser/navigation_predictor/navigation_predictor_renderer_warmup_client.h

@@ -5,6 +5,8 @@
 #ifndef CHROME_BROWSER_NAVIGATION_PREDICTOR_NAVIGATION_PREDICTOR_RENDERER_WARMUP_CLIENT_H_
 #define CHROME_BROWSER_NAVIGATION_PREDICTOR_NAVIGATION_PREDICTOR_RENDERER_WARMUP_CLIENT_H_
 
+#include <memory>
+
 #include "base/macros.h"
 #include "base/memory/weak_ptr.h"
 #include "base/optional.h"
@@ -14,6 +16,10 @@
 
 class Profile;
 
+namespace content {
+class WebContents;
+}
+
 // A client of Navigation Predictor that uses predictions to initiate a renderer
 // warmup (in the form of starting a spare renderer process) when it is likely
 // the user will soon do a cross-origin navigation.
@@ -40,23 +46,49 @@ class NavigationPredictorRendererWarmupClient
   virtual bool BrowserHasSpareRenderer() const;
 
  private:
+  // Metrics per-prediction about how this class acts, for recording in UKM.
+  struct PredictionMetrics {
+    PredictionMetrics();
+    ~PredictionMetrics();
+
+    // A bitmask that records common criteria.
+    //   1 << 0 = Set if the feature is in a cooldown period following the last
+    //            renderer warmup.
+    //   1 << 1 = Set if the browser already had a spare renderer.
+    //   1 << 2 = Set if the device memory is below the experiment threshold.
+    size_t page_independent_status = 0;
+
+    // Represents the ratio of the links on a page that are cross-origin to the
+    // source document. Set when there are 1 or more links on a page.
+    base::Optional<double> cross_origin_links_ratio;
+
+    // Whether the source document was a search result page for the default
+    // search engine.
+    bool was_dse_srp = false;
+
+    // Set when all eligibility criteria are met, regardless of
+    // |counterfactual_|.
+    bool did_warmup = false;
+  };
+  std::unique_ptr<PredictionMetrics> metrics_;
+
   // Checks if there is already a spare renderer or we requested a spare
   // renderer too recently.
-  bool IsEligibleForWarmupOnCommonCriteria() const;
+  void CheckIsEligibleForWarmupOnCommonCriteria();
 
   // Checks if the |prediction| is eligible to trigger a renderer warmup based
   // on the number of predicted origins.
-  bool IsEligibleForCrossNavigationWarmup(
-      const NavigationPredictorKeyedService::Prediction& prediction) const;
+  void CheckIsEligibleForCrossNavigationWarmup(
+      const NavigationPredictorKeyedService::Prediction& prediction);
 
   // Checks if the |prediction| is eligible to trigger a renderer warmup based
   // on the current page being search results for the default search engine.
-  bool IsEligibleForDSEWarmup(
-      const NavigationPredictorKeyedService::Prediction& prediction) const;
+  void CheckIsEligibleForDSEWarmup(
+      const NavigationPredictorKeyedService::Prediction& prediction);
 
   // Records class state and metrics before checking |counterfactual_| and then
   // calling |DoRendererWarmpup| if |counterfactual_| is false.
-  void RecordMetricsAndMaybeDoWarmup();
+  void RecordMetricsAndMaybeDoWarmup(content::WebContents* web_contents);
 
   Profile* profile_;
 

chrome/browser/navigation_predictor/navigation_predictor_renderer_warmup_client_browsertest.cc

@@ -12,11 +12,16 @@
 #include "chrome/browser/ui/tabs/tab_strip_model.h"
 #include "chrome/test/base/in_process_browser_test.h"
 #include "chrome/test/base/ui_test_utils.h"
+#include "components/ukm/test_ukm_recorder.h"
 #include "content/public/browser/render_process_host.h"
 #include "content/public/test/browser_test.h"
 #include "content/public/test/browser_test_base.h"
 #include "content/public/test/browser_test_utils.h"
 #include "content/public/test/test_utils.h"
+#include "services/metrics/public/cpp/metrics_utils.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 "testing/gtest/include/gtest/gtest.h"
 #include "url/gurl.h"
 
@@ -44,6 +49,11 @@ class NavigationPredictorRendererWarmupClientBrowserTest
     InProcessBrowserTest::SetUp();
   }
 
+  void SetUpOnMainThread() override {
+    InProcessBrowserTest::SetUpOnMainThread();
+    ukm_recorder_ = std::make_unique<ukm::TestAutoSetUkmRecorder>();
+  }
+
   size_t SpareRendererCount() const {
     size_t count = 0;
     for (content::RenderProcessHost::iterator iter(
@@ -74,8 +84,18 @@ class NavigationPredictorRendererWarmupClientBrowserTest
             {});
   }
 
+  void VerifyHasUKMEntry(const GURL& url) {
+    auto entries = ukm_recorder_->GetEntriesByName(
+        ukm::builders::NavigationPredictorRendererWarmup::kEntryName);
+    ASSERT_EQ(1U, entries.size());
+
+    const auto* entry = entries.front();
+    ukm_recorder_->ExpectEntrySourceHasUrl(entry, url);
+  }
+
  private:
   base::test::ScopedFeatureList scoped_feature_list_;
+  std::unique_ptr<ukm::TestAutoSetUkmRecorder> ukm_recorder_;
 };
 
 IN_PROC_BROWSER_TEST_F(NavigationPredictorRendererWarmupClientBrowserTest,
@@ -83,13 +103,15 @@ IN_PROC_BROWSER_TEST_F(NavigationPredictorRendererWarmupClientBrowserTest,
   // Navigate to a site so that the default renderer is used.
   embedded_test_server()->ServeFilesFromSourceDirectory("chrome/test/data");
   ASSERT_TRUE(embedded_test_server()->Start());
-  ui_test_utils::NavigateToURL(browser(),
-                               embedded_test_server()->GetURL("/simple.html"));
 
+  GURL url = embedded_test_server()->GetURL("/simple.html");
+
+  ui_test_utils::NavigateToURL(browser(), url);
   MakeEligibleNavigationPrediction();
   base::RunLoop().RunUntilIdle();
 
   EXPECT_EQ(SpareRendererCount(), 1U);
+  VerifyHasUKMEntry(url);
 }
 
 IN_PROC_BROWSER_TEST_F(NavigationPredictorRendererWarmupClientBrowserTest,
@@ -97,8 +119,10 @@ IN_PROC_BROWSER_TEST_F(NavigationPredictorRendererWarmupClientBrowserTest,
   // Navigate to a site so that the default renderer is used.
   embedded_test_server()->ServeFilesFromSourceDirectory("chrome/test/data");
   ASSERT_TRUE(embedded_test_server()->Start());
-  ui_test_utils::NavigateToURL(browser(),
-                               embedded_test_server()->GetURL("/simple.html"));
+
+  GURL url = embedded_test_server()->GetURL("/simple.html");
+
+  ui_test_utils::NavigateToURL(browser(), url);
 
   MakeSpareRenderer();
   base::RunLoop().RunUntilIdle();
@@ -108,4 +132,5 @@ IN_PROC_BROWSER_TEST_F(NavigationPredictorRendererWarmupClientBrowserTest,
   base::RunLoop().RunUntilIdle();
 
   EXPECT_EQ(SpareRendererCount(), 1U);
+  VerifyHasUKMEntry(url);
 }

chrome/browser/prerender/isolated/isolated_prerender_tab_helper_unittest.cc

@@ -149,7 +149,7 @@ class IsolatedPrerenderTabHelperTest : public ChromeRenderViewHostTestHarness {
         SetDataSaverEnabledForTesting(profile()->GetPrefs(), enabled);
   }
 
-  void MakeNavigationPrediction(const content::WebContents* web_contents,
+  void MakeNavigationPrediction(content::WebContents* web_contents,
                                 const GURL& doc_url,
                                 const std::vector<GURL>& predicted_urls) {
     NavigationPredictorKeyedServiceFactory::GetForProfile(profile())

tools/metrics/ukm/ukm.xml

@@ -7679,6 +7679,44 @@ be describing additional metrics about the same event.
   </metric>
 </event>
 
+<event name="NavigationPredictorRendererWarmup">
+  <owner>robertogden@chromium.org</owner>
+  <owner>tbansal@chromium.org</owner>
+  <summary>
+    Internal feature metrics for a feature that decides whether to start a spare
+    renderer on pages where Chrome heuristically determines that a cross-origin
+    page load is likely to occur based on the anchor elements on the page being
+    cross-origin and/or whether the current page is a search engine result page.
+  </summary>
+  <metric name="CrossOriginLinksRatio">
+    <summary>
+      An int in the range [0, 100] to represent a percentage of the ratio of the
+      links on a page that are cross-origin to the source document. Recorded
+      when there are 1 or more links on a page.
+    </summary>
+  </metric>
+  <metric name="DidWarmup" enum="Boolean">
+    <summary>
+      A bool that is set if a spare renderer was started. Also set on
+      counter-factual arms when the same criteria were met.
+    </summary>
+  </metric>
+  <metric name="PageIndependentStatusBitMask">
+    <summary>
+      A bitmask, recorded on every page when the feature flag is enabled. 0b0001
+      - Set if the feature is in a cooldown period following the last renderer
+      warmup. 0b0010 - Set if the browser already had a spare renderer. 0b0100 -
+      Set if the device memory is below the experiment threshold.
+    </summary>
+  </metric>
+  <metric name="WasDSESRP" enum="Boolean">
+    <summary>
+      A bool that is set if the page is a search result page for the browser's
+      default search engine
+    </summary>
+  </metric>
+</event>
+
 <event name="NavigationTiming" singular="True">
   <owner>nhiroki@chromium.org</owner>
   <owner>chrome-loading@google.com</owner>