[HintsFetcher] Adds status enum to callback to allow retry/queuing po…

…licy

This is preparation for being able to implement a queuing policy in the
hints manager. Basically, HintsFetcher needs to surface different
cases as to why hints weren't fetched in order for caller to know if
should try again soon or try again later. This change also lands UMA
for these status values to help us understand more about what queueing
policy might be useful/needed.

Bug: 1021310
Change-Id: I27d5cd72c597344db6844f5833c674edbc350e02
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1918145
Commit-Queue: Doug Arnett <dougarnett@chromium.org>
Reviewed-by: Michael Crouse <mcrouse@chromium.org>
Reviewed-by: Tarun Bansal <tbansal@chromium.org>
Cr-Commit-Position: refs/heads/master@{#716403}

chrome/browser/optimization_guide/optimization_guide_hints_manager.cc

@@ -9,6 +9,7 @@
 
 #include "base/bind.h"
 #include "base/logging.h"
+#include "base/metrics/histogram_macros.h"
 #include "base/metrics/histogram_macros_local.h"
 #include "base/rand_util.h"
 #include "base/sequenced_task_runner.h"
@@ -26,7 +27,6 @@
 #include "components/optimization_guide/bloom_filter.h"
 #include "components/optimization_guide/hint_cache.h"
 #include "components/optimization_guide/hints_component_util.h"
-#include "components/optimization_guide/hints_fetcher.h"
 #include "components/optimization_guide/hints_processing_util.h"
 #include "components/optimization_guide/optimization_filter.h"
 #include "components/optimization_guide/optimization_guide_constants.h"
@@ -482,14 +482,21 @@ void OptimizationGuideHintsManager::FetchTopHostsHints() {
 
 void OptimizationGuideHintsManager::OnHintsFetched(
     optimization_guide::proto::RequestContext request_context,
+    optimization_guide::HintsFetcherRequestStatus fetch_status,
     base::Optional<std::unique_ptr<optimization_guide::proto::GetHintsResponse>>
         get_hints_response) {
   switch (request_context) {
     case optimization_guide::proto::CONTEXT_BATCH_UPDATE:
       OnTopHostsHintsFetched(std::move(get_hints_response));
+      UMA_HISTOGRAM_ENUMERATION(
+          "OptimizationGuide.HintsFetcher.RequestStatus.BatchUpdate",
+          fetch_status);
       return;
     case optimization_guide::proto::CONTEXT_PAGE_NAVIGATION:
       OnPageNavigationHintsFetched(std::move(get_hints_response));
+      UMA_HISTOGRAM_ENUMERATION(
+          "OptimizationGuide.HintsFetcher.RequestStatus.PageNavigation",
+          fetch_status);
       return;
     case optimization_guide::proto::CONTEXT_UNSPECIFIED:
       NOTREACHED();

chrome/browser/optimization_guide/optimization_guide_hints_manager.h

@@ -20,6 +20,7 @@
 #include "base/timer/timer.h"
 #include "chrome/browser/navigation_predictor/navigation_predictor_keyed_service.h"
 #include "components/optimization_guide/hints_component_info.h"
+#include "components/optimization_guide/hints_fetcher.h"
 #include "components/optimization_guide/optimization_guide_service_observer.h"
 #include "components/optimization_guide/proto/hints.pb.h"
 #include "components/optimization_guide/proto/models.pb.h"
@@ -44,7 +45,6 @@ class SharedURLLoaderFactory;
 
 namespace optimization_guide {
 class HintCache;
-class HintsFetcher;
 enum class OptimizationGuideDecision;
 class OptimizationFilter;
 struct OptimizationMetadata;
@@ -203,9 +203,11 @@ class OptimizationGuideHintsManager
   void FetchTopHostsHints();
 
   // Called when the hints have been fetched from the remote Optimization Guide
-  // Service and are ready for parsing.
+  // Service and are ready for parsing or when the fetch was not able to be
+  // completed.
   void OnHintsFetched(
       optimization_guide::proto::RequestContext request_context,
+      optimization_guide::HintsFetcherRequestStatus fetch_status,
       base::Optional<
           std::unique_ptr<optimization_guide::proto::GetHintsResponse>>
           get_hints_response);

chrome/browser/optimization_guide/optimization_guide_hints_manager_unittest.cc

@@ -171,17 +171,24 @@ class TestHintsFetcher : public optimization_guide::HintsFetcher {
       override {
     switch (fetch_state_) {
       case HintsFetcherEndState::kFetchFailed:
-        std::move(hints_fetched_callback).Run(request_context, base::nullopt);
+        std::move(hints_fetched_callback)
+            .Run(request_context,
+                 optimization_guide::HintsFetcherRequestStatus::kResponseError,
+                 base::nullopt);
         return false;
       case HintsFetcherEndState::kFetchSuccessWithHints:
         hints_fetched_ = true;
         std::move(hints_fetched_callback)
-            .Run(request_context, BuildHintsResponse({"host.com"}));
+            .Run(request_context,
+                 optimization_guide::HintsFetcherRequestStatus::kSuccess,
+                 BuildHintsResponse({"host.com"}));
         return true;
       case HintsFetcherEndState::kFetchSuccessWithNoHints:
         hints_fetched_ = true;
         std::move(hints_fetched_callback)
-            .Run(request_context, BuildHintsResponse({}));
+            .Run(request_context,
+                 optimization_guide::HintsFetcherRequestStatus::kSuccess,
+                 BuildHintsResponse({}));
         return true;
     }
     return true;

components/optimization_guide/hints_fetcher.cc

@@ -96,17 +96,28 @@ bool HintsFetcher::FetchOptimizationGuideServiceHints(
   SEQUENCE_CHECKER(sequence_checker_);
 
   if (content::GetNetworkConnectionTracker()->IsOffline()) {
-    std::move(hints_fetched_callback).Run(request_context, base::nullopt);
+    std::move(hints_fetched_callback)
+        .Run(request_context, HintsFetcherRequestStatus::kNetworkOffline,
+             base::nullopt);
     return false;
   }
 
-  if (url_loader_)
+  if (active_url_loader_) {
+    std::move(hints_fetched_callback)
+        .Run(request_context, HintsFetcherRequestStatus::kFetcherBusy,
+             base::nullopt);
     return false;
+  }
 
   std::vector<std::string> filtered_hosts =
       GetSizeLimitedHostsDueForHintsRefresh(hosts);
-  if (filtered_hosts.empty())
+  if (filtered_hosts.empty()) {
+    std::move(hints_fetched_callback)
+        .Run(request_context, HintsFetcherRequestStatus::kNoHostsToFetch,
+             base::nullopt);
     return false;
+  }
+
   DCHECK_GE(features::MaxHostsForOptimizationGuideServiceHintsFetch(),
             filtered_hosts.size());
 
@@ -163,24 +174,25 @@ bool HintsFetcher::FetchOptimizationGuideServiceHints(
   resource_request->method = "POST";
   resource_request->credentials_mode = network::mojom::CredentialsMode::kOmit;
 
-  url_loader_ = network::SimpleURLLoader::Create(std::move(resource_request),
-                                                 traffic_annotation);
+  active_url_loader_ = network::SimpleURLLoader::Create(
+      std::move(resource_request), traffic_annotation);
 
-  url_loader_->AttachStringForUpload(serialized_request,
-                                     "application/x-protobuf");
+  active_url_loader_->AttachStringForUpload(serialized_request,
+                                            "application/x-protobuf");
 
   UMA_HISTOGRAM_COUNTS_100(
       "OptimizationGuide.HintsFetcher.GetHintsRequest.HostCount",
       filtered_hosts.size());
 
-  // |url_loader_| should not retry on 5xx errors since the server may already
-  // be overloaded.  |url_loader_| should retry on network changes since the
-  // network stack may receive the connection change event later than |this|.
+  // |active_url_loader_| should not retry on 5xx errors since the server may
+  // already be overloaded. |active_url_loader_| should retry on network changes
+  // since the network stack may receive the connection change event later than
+  // |this|.
   static const int kMaxRetries = 1;
-  url_loader_->SetRetryOptions(
+  active_url_loader_->SetRetryOptions(
       kMaxRetries, network::SimpleURLLoader::RETRY_ON_NETWORK_CHANGE);
 
-  url_loader_->DownloadToStringOfUnboundedSizeUntilCrashAndDie(
+  active_url_loader_->DownloadToStringOfUnboundedSizeUntilCrashAndDie(
       url_loader_factory_.get(),
       base::BindOnce(&HintsFetcher::OnURLLoadComplete, base::Unretained(this)));
 
@@ -216,9 +228,12 @@ void HintsFetcher::HandleResponse(const std::string& get_hints_response_data,
         base::TimeTicks::Now() - hints_fetch_start_time_);
     UpdateHostsSuccessfullyFetched();
     std::move(hints_fetched_callback_)
-        .Run(request_context_, std::move(get_hints_response));
+        .Run(request_context_, HintsFetcherRequestStatus::kSuccess,
+             std::move(get_hints_response));
   } else {
-    std::move(hints_fetched_callback_).Run(request_context_, base::nullopt);
+    std::move(hints_fetched_callback_)
+        .Run(request_context_, HintsFetcherRequestStatus::kResponseError,
+             base::nullopt);
   }
 }
 
@@ -277,12 +292,14 @@ void HintsFetcher::OnURLLoadComplete(
   SEQUENCE_CHECKER(sequence_checker_);
 
   int response_code = -1;
-  if (url_loader_->ResponseInfo() && url_loader_->ResponseInfo()->headers) {
-    response_code = url_loader_->ResponseInfo()->headers->response_code();
+  if (active_url_loader_->ResponseInfo() &&
+      active_url_loader_->ResponseInfo()->headers) {
+    response_code =
+        active_url_loader_->ResponseInfo()->headers->response_code();
   }
-  HandleResponse(response_body ? *response_body : "", url_loader_->NetError(),
-                 response_code);
-  url_loader_.reset();
+  HandleResponse(response_body ? *response_body : "",
+                 active_url_loader_->NetError(), response_code);
+  active_url_loader_.reset();
 }
 
 std::vector<std::string> HintsFetcher::GetSizeLimitedHostsDueForHintsRefresh(

components/optimization_guide/hints_fetcher.h

@@ -28,11 +28,34 @@ class SimpleURLLoader;
 
 namespace optimization_guide {
 
+// Status of a request to fetch hints.
+// This enum must remain synchronized with the enum
+// |OptimizationGuideHintsFetcherRequestStatus| in
+// tools/metrics/histograms/enums.xml.
+enum class HintsFetcherRequestStatus {
+  // No fetch status known. Used in testing.
+  kUnknown,
+  // Fetch request was sent and a response received.
+  kSuccess,
+  // Fetch request was sent but no response received.
+  kResponseError,
+  // Fetch request not sent because of offline network status.
+  kNetworkOffline,
+  // Fetch request not sent because fetcher was busy with another request.
+  kFetcherBusy,
+  // Fetch request not sent because the host list was empty.
+  kNoHostsToFetch,
+
+  // Insert new values before this line.
+  kMaxValue = kNoHostsToFetch
+};
+
 // Callback to inform the caller that the remote hints have been fetched and
 // to pass back the fetched hints response from the remote Optimization Guide
 // Service.
 using HintsFetchedCallback = base::OnceCallback<void(
     optimization_guide::proto::RequestContext request_context,
+    HintsFetcherRequestStatus fetch_status,
     base::Optional<std::unique_ptr<proto::GetHintsResponse>>)>;
 
 // A class to handle requests for optimization hints from a remote Optimization
@@ -51,11 +74,11 @@ class HintsFetcher {
 
   // Requests hints from the Optimization Guide Service if a request for them is
   // not already in progress. Returns whether a new request was issued.
-  // |hints_fetched_callback| is run, passing a GetHintsResponse object, if a
-  // fetch was successful or passes nullopt if the fetch fails. Virtualized for
-  // testing. Hints fetcher may fetch hints for only a subset of the provided
-  // |hosts|. |hosts| should be an ordered list in descending order of
-  // probability that the hints are needed for that host.
+  // |hints_fetched_callback| is run once when the outcome of this request is
+  // determined (whether a request was actually sent or not).
+  // Virtualized for testing. Hints fetcher may fetch hints for only a subset
+  // of the provided |hosts|. |hosts| should be an ordered list in descending
+  // order of probability that the hints are needed for that host.
   virtual bool FetchOptimizationGuideServiceHints(
       const std::vector<std::string>& hosts,
       optimization_guide::proto::RequestContext request_context,
@@ -113,7 +136,7 @@ class HintsFetcher {
   const GURL optimization_guide_service_url_;
 
   // Holds the |URLLoader| for an active hints request.
-  std::unique_ptr<network::SimpleURLLoader> url_loader_;
+  std::unique_ptr<network::SimpleURLLoader> active_url_loader_;
 
   // Context of the fetch request. Opaque field that's returned back in the
   // callback and is also included in the requests to the hints server.
@@ -128,7 +151,7 @@ class HintsFetcher {
   // Clock used for recording time that the hints fetch occurred.
   const base::Clock* time_clock_;
 
-  // Used for creating a |url_loader_| when needed for request hints.
+  // Used for creating an |active_url_loader_| when needed for request hints.
   scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory_;
 
   // The start time of the current hints fetch, used to determine the latency in

components/optimization_guide/hints_fetcher_unittest.cc

@@ -57,13 +57,19 @@ class HintsFetcherTest : public testing::Test {
 
   ~HintsFetcherTest() override {}
 
-  void OnHintsFetched(optimization_guide::proto::RequestContext request_context,
-                      base::Optional<std::unique_ptr<proto::GetHintsResponse>>
-                          get_hints_response) {
+  void OnHintsFetched(
+      optimization_guide::proto::RequestContext request_context,
+      optimization_guide::HintsFetcherRequestStatus fetcher_request_status,
+      base::Optional<std::unique_ptr<proto::GetHintsResponse>>
+          get_hints_response) {
+    fetcher_request_status_ = fetcher_request_status;
     if (get_hints_response)
       hints_fetched_ = true;
   }
 
+  optimization_guide::HintsFetcherRequestStatus fetcher_request_status() {
+    return fetcher_request_status_;
+  }
   bool hints_fetched() { return hints_fetched_; }
 
   void SetConnectionOffline() {
@@ -142,6 +148,8 @@ class HintsFetcherTest : public testing::Test {
     base::RunLoop().RunUntilIdle();
   }
 
+  optimization_guide::HintsFetcherRequestStatus fetcher_request_status_ =
+      optimization_guide::HintsFetcherRequestStatus::kUnknown;
   bool hints_fetched_ = false;
   base::test::TaskEnvironment task_environment_;
 
@@ -162,6 +170,8 @@ TEST_F(HintsFetcherTest, FetchOptimizationGuideServiceHints) {
   EXPECT_TRUE(FetchHints(std::vector<std::string>{"foo.com"}));
   VerifyHasPendingFetchRequests();
   EXPECT_TRUE(SimulateResponse(response_content, net::HTTP_OK));
+  EXPECT_EQ(optimization_guide::HintsFetcherRequestStatus::kSuccess,
+            fetcher_request_status());
   EXPECT_TRUE(hints_fetched());
 
   histogram_tester.ExpectTotalCount(
@@ -179,10 +189,14 @@ TEST_F(HintsFetcherTest, FetchInProgress) {
   // |fetch_in_progress_| should cause early exit.
   EXPECT_TRUE(FetchHints(std::vector<std::string>{"foo.com"}));
   EXPECT_FALSE(FetchHints(std::vector<std::string>{"bar.com"}));
+  EXPECT_EQ(optimization_guide::HintsFetcherRequestStatus::kFetcherBusy,
+            fetcher_request_status());
 
   // Once response arrives, check to make sure a new fetch can start.
   SimulateResponse(response_content, net::HTTP_OK);
   EXPECT_TRUE(FetchHints(std::vector<std::string>{"bar.com"}));
+  EXPECT_EQ(optimization_guide::HintsFetcherRequestStatus::kSuccess,
+            fetcher_request_status());
 }
 
 // Tests that the hints are refreshed again for hosts for whom hints were
@@ -250,6 +264,8 @@ TEST_F(HintsFetcherTest, FetchReturned404) {
   // Send a 404 to HintsFetcher.
   SimulateResponse(response_content, net::HTTP_NOT_FOUND);
   EXPECT_FALSE(hints_fetched());
+  EXPECT_EQ(optimization_guide::HintsFetcherRequestStatus::kResponseError,
+            fetcher_request_status());
 
   // Make sure histogram not recorded on bad response.
   histogram_tester.ExpectTotalCount(
@@ -264,6 +280,8 @@ TEST_F(HintsFetcherTest, FetchReturnBadResponse) {
   VerifyHasPendingFetchRequests();
   EXPECT_TRUE(SimulateResponse(response_content, net::HTTP_OK));
   EXPECT_FALSE(hints_fetched());
+  EXPECT_EQ(optimization_guide::HintsFetcherRequestStatus::kResponseError,
+            fetcher_request_status());
 
   // Make sure histogram not recorded on bad response.
   histogram_tester.ExpectTotalCount(
@@ -277,6 +295,8 @@ TEST_F(HintsFetcherTest, FetchAttemptWhenNetworkOffline) {
   std::string response_content;
   EXPECT_FALSE(FetchHints(std::vector<std::string>{"foo.com"}));
   EXPECT_FALSE(hints_fetched());
+  EXPECT_EQ(optimization_guide::HintsFetcherRequestStatus::kNetworkOffline,
+            fetcher_request_status());
 
   // Make sure histogram not recorded on bad response.
   histogram_tester.ExpectTotalCount(

components/previews/content/previews_optimization_guide_impl.cc

@@ -18,7 +18,6 @@
 #include "components/data_reduction_proxy/core/browser/data_reduction_proxy_settings.h"
 #include "components/optimization_guide/hints_component_info.h"
 #include "components/optimization_guide/hints_component_util.h"
-#include "components/optimization_guide/hints_fetcher.h"
 #include "components/optimization_guide/optimization_guide_constants.h"
 #include "components/optimization_guide/optimization_guide_features.h"
 #include "components/optimization_guide/optimization_guide_prefs.h"
@@ -288,6 +287,7 @@ void PreviewsOptimizationGuideImpl::FetchHints() {
 
 void PreviewsOptimizationGuideImpl::OnHintsFetched(
     optimization_guide::proto::RequestContext request_context,
+    optimization_guide::HintsFetcherRequestStatus fetch_status,
     base::Optional<std::unique_ptr<optimization_guide::proto::GetHintsResponse>>
         get_hints_response) {
   // TODO(mcrouse): this will be dropped into a backgroundtask as it will likely

components/previews/content/previews_optimization_guide_impl.h

@@ -19,6 +19,7 @@
 #include "base/time/clock.h"
 #include "base/timer/timer.h"
 #include "components/optimization_guide/hint_cache.h"
+#include "components/optimization_guide/hints_fetcher.h"
 #include "components/optimization_guide/optimization_guide_service_observer.h"
 #include "components/previews/content/previews_optimization_guide.h"
 #include "components/previews/core/previews_experiments.h"
@@ -38,7 +39,6 @@ class SharedURLLoaderFactory;
 }  // namespace network
 namespace optimization_guide {
 struct HintsComponentInfo;
-class HintsFetcher;
 class OptimizationGuideService;
 class TopHostProvider;
 namespace proto {
@@ -126,6 +126,7 @@ class PreviewsOptimizationGuideImpl
   // testing.
   virtual void OnHintsFetched(
       optimization_guide::proto::RequestContext request_context,
+      optimization_guide::HintsFetcherRequestStatus fetch_status,
       base::Optional<
           std::unique_ptr<optimization_guide::proto::GetHintsResponse>>
           get_hints_response);