Commit a510012a authored by Sophie Chang's avatar Sophie Chang Committed by Commit Bot

Move out request status histogram recording to HintsFetcher

This makes the HintsFetchedCallback more general and paves the way for
the async CanApplyOptimization where we need to associate each hints
fetching attempt with its URL.

Change-Id: I1c0e809dbcd4244ecc268ec77a107d115291bfd2
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2013841
Commit-Queue: Sophie Chang <sophiechang@chromium.org>
Reviewed-by: default avatarMichael Crouse <mcrouse@chromium.org>
Cr-Commit-Position: refs/heads/master@{#733833}
parent 00037259
......@@ -10,7 +10,6 @@
#include "base/bind.h"
#include "base/logging.h"
#include "base/metrics/histogram_functions.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"
......@@ -556,34 +555,10 @@ void OptimizationGuideHintsManager::FetchTopHostsHints() {
hints_fetcher_->FetchOptimizationGuideServiceHints(
top_hosts, std::vector<GURL>{}, registered_optimization_types_,
optimization_guide::proto::CONTEXT_BATCH_UPDATE,
base::BindOnce(&OptimizationGuideHintsManager::OnHintsFetched,
base::BindOnce(&OptimizationGuideHintsManager::OnTopHostsHintsFetched,
ui_weak_ptr_factory_.GetWeakPtr()));
}
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();
}
NOTREACHED();
}
void OptimizationGuideHintsManager::OnTopHostsHintsFetched(
base::Optional<std::unique_ptr<optimization_guide::proto::GetHintsResponse>>
get_hints_response) {
......@@ -751,8 +726,9 @@ void OptimizationGuideHintsManager::OnPredictionUpdated(
target_hosts_serialized, std::vector<GURL>{},
registered_optimization_types_,
optimization_guide::proto::CONTEXT_PAGE_NAVIGATION,
base::BindOnce(&OptimizationGuideHintsManager::OnHintsFetched,
ui_weak_ptr_factory_.GetWeakPtr()));
base::BindOnce(
&OptimizationGuideHintsManager::OnPageNavigationHintsFetched,
ui_weak_ptr_factory_.GetWeakPtr()));
for (const auto& host : target_hosts)
LoadHintForHost(host, base::DoNothing());
......@@ -1050,8 +1026,9 @@ void OptimizationGuideHintsManager::MaybeFetchHintsForNavigation(
hints_fetcher_->FetchOptimizationGuideServiceHints(
hosts, urls, registered_optimization_types_,
optimization_guide::proto::CONTEXT_PAGE_NAVIGATION,
base::BindOnce(&OptimizationGuideHintsManager::OnHintsFetched,
ui_weak_ptr_factory_.GetWeakPtr()));
base::BindOnce(
&OptimizationGuideHintsManager::OnPageNavigationHintsFetched,
ui_weak_ptr_factory_.GetWeakPtr()));
if (!hosts.empty() && !urls.empty()) {
race_navigation_recorder.set_race_attempt_status(
......
......@@ -207,16 +207,6 @@ class OptimizationGuideHintsManager
// Service. Used to fetch hints for origins frequently visited by the user.
void FetchTopHostsHints();
// Called when the hints have been fetched from the remote Optimization Guide
// 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);
// Called when the hints for the top hosts have been fetched from the remote
// Optimization Guide Service and are ready for parsing. This is used when
// fetching hints in batch mode.
......
......@@ -189,32 +189,22 @@ class TestHintsFetcher : public optimization_guide::HintsFetcher {
override {
switch (fetch_state_) {
case HintsFetcherEndState::kFetchFailed:
std::move(hints_fetched_callback)
.Run(request_context,
optimization_guide::HintsFetcherRequestStatus::kResponseError,
base::nullopt);
std::move(hints_fetched_callback).Run(base::nullopt);
return false;
case HintsFetcherEndState::kFetchSuccessWithHostHints:
hints_fetched_ = true;
std::move(hints_fetched_callback)
.Run(request_context,
optimization_guide::HintsFetcherRequestStatus::kSuccess,
BuildHintsResponse({"host.com"}, {}));
.Run(BuildHintsResponse({"host.com"}, {}));
return true;
case HintsFetcherEndState::kFetchSuccessWithURLHints:
hints_fetched_ = true;
std::move(hints_fetched_callback)
.Run(request_context,
optimization_guide::HintsFetcherRequestStatus::kSuccess,
BuildHintsResponse({},
.Run(BuildHintsResponse({},
{"https://somedomain.org/news/whatever"}));
return true;
case HintsFetcherEndState::kFetchSuccessWithNoHints:
hints_fetched_ = true;
std::move(hints_fetched_callback)
.Run(request_context,
optimization_guide::HintsFetcherRequestStatus::kSuccess,
BuildHintsResponse({}, {}));
std::move(hints_fetched_callback).Run(BuildHintsResponse({}, {}));
return true;
}
return true;
......
......@@ -67,6 +67,14 @@ std::vector<GURL> GetValidURLsForFetching(const std::vector<GURL>& urls) {
return valid_urls;
}
void RecordRequestStatusHistogram(proto::RequestContext request_context,
HintsFetcherRequestStatus status) {
base::UmaHistogramEnumeration(
"OptimizationGuide.HintsFetcher.GetHintsRequest.RequestStatus." +
GetStringNameForRequestContext(request_context),
status);
}
} // namespace
HintsFetcher::HintsFetcher(
......@@ -131,16 +139,16 @@ bool HintsFetcher::FetchOptimizationGuideServiceHints(
DCHECK_GT(optimization_types.size(), 0u);
if (content::GetNetworkConnectionTracker()->IsOffline()) {
std::move(hints_fetched_callback)
.Run(request_context, HintsFetcherRequestStatus::kNetworkOffline,
base::nullopt);
RecordRequestStatusHistogram(request_context,
HintsFetcherRequestStatus::kNetworkOffline);
std::move(hints_fetched_callback).Run(base::nullopt);
return false;
}
if (active_url_loader_) {
std::move(hints_fetched_callback)
.Run(request_context, HintsFetcherRequestStatus::kFetcherBusy,
base::nullopt);
RecordRequestStatusHistogram(request_context,
HintsFetcherRequestStatus::kFetcherBusy);
std::move(hints_fetched_callback).Run(base::nullopt);
return false;
}
......@@ -148,9 +156,9 @@ bool HintsFetcher::FetchOptimizationGuideServiceHints(
GetSizeLimitedHostsDueForHintsRefresh(hosts);
std::vector<GURL> valid_urls = GetValidURLsForFetching(urls);
if (filtered_hosts.empty() && valid_urls.empty()) {
std::move(hints_fetched_callback)
.Run(request_context, HintsFetcherRequestStatus::kNoHostsOrURLsToFetch,
base::nullopt);
RecordRequestStatusHistogram(
request_context, HintsFetcherRequestStatus::kNoHostsOrURLsToFetch);
std::move(hints_fetched_callback).Run(base::nullopt);
return false;
}
......@@ -158,33 +166,33 @@ bool HintsFetcher::FetchOptimizationGuideServiceHints(
filtered_hosts.size());
if (optimization_types.empty()) {
std::move(hints_fetched_callback)
.Run(request_context,
HintsFetcherRequestStatus::kNoSupportedOptimizationTypes,
base::nullopt);
RecordRequestStatusHistogram(
request_context,
HintsFetcherRequestStatus::kNoSupportedOptimizationTypes);
std::move(hints_fetched_callback).Run(base::nullopt);
return false;
}
hints_fetch_start_time_ = base::TimeTicks::Now();
request_context_ = request_context;
get_hints_request_ = std::make_unique<proto::GetHintsRequest>();
proto::GetHintsRequest get_hints_request;
for (const auto& optimization_type : optimization_types)
get_hints_request_->add_supported_optimizations(optimization_type);
get_hints_request.add_supported_optimizations(optimization_type);
get_hints_request_->set_context(request_context_);
get_hints_request.set_context(request_context_);
for (const auto& url : valid_urls)
get_hints_request_->add_urls()->set_url(url.spec());
get_hints_request.add_urls()->set_url(url.spec());
for (const auto& host : filtered_hosts) {
proto::HostInfo* host_info = get_hints_request_->add_hosts();
proto::HostInfo* host_info = get_hints_request.add_hosts();
host_info->set_host(host);
}
std::string serialized_request;
get_hints_request_->SerializeToString(&serialized_request);
get_hints_request.SerializeToString(&serialized_request);
net::NetworkTrafficAnnotationTag traffic_annotation =
net::DefineNetworkTrafficAnnotation("hintsfetcher_gethintsrequest", R"(
......@@ -274,13 +282,13 @@ void HintsFetcher::HandleResponse(const std::string& get_hints_response_data,
GetStringNameForRequestContext(request_context_),
fetch_latency);
UpdateHostsSuccessfullyFetched();
std::move(hints_fetched_callback_)
.Run(request_context_, HintsFetcherRequestStatus::kSuccess,
std::move(get_hints_response));
RecordRequestStatusHistogram(request_context_,
HintsFetcherRequestStatus::kSuccess);
std::move(hints_fetched_callback_).Run(std::move(get_hints_response));
} else {
std::move(hints_fetched_callback_)
.Run(request_context_, HintsFetcherRequestStatus::kResponseError,
base::nullopt);
RecordRequestStatusHistogram(request_context_,
HintsFetcherRequestStatus::kResponseError);
std::move(hints_fetched_callback_).Run(base::nullopt);
}
}
......
......@@ -58,8 +58,6 @@ enum class HintsFetcherRequestStatus {
// 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
......@@ -135,10 +133,6 @@ class HintsFetcher {
std::vector<std::string> GetSizeLimitedHostsDueForHintsRefresh(
const std::vector<std::string>& hosts) const;
// Used to hold the GetHintsRequest being constructed and sent as a remote
// request.
std::unique_ptr<proto::GetHintsRequest> get_hints_request_;
// Used to hold the callback while the SimpleURLLoader performs the request
// asynchronously.
HintsFetchedCallback hints_fetched_callback_;
......
......@@ -55,21 +55,14 @@ class HintsFetcherTest : public testing::Test {
hints_fetcher_->SetTimeClockForTesting(task_environment_.GetMockClock());
}
~HintsFetcherTest() override {}
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;
~HintsFetcherTest() override = default;
void OnHintsFetched(base::Optional<std::unique_ptr<proto::GetHintsResponse>>
get_hints_response) {
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() {
......@@ -150,8 +143,6 @@ 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_;
......@@ -172,8 +163,6 @@ TEST_F(HintsFetcherTest, FetchOptimizationGuideServiceHints) {
EXPECT_TRUE(FetchHints({"foo.com"}, {} /* urls */));
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(
......@@ -181,6 +170,10 @@ TEST_F(HintsFetcherTest, FetchOptimizationGuideServiceHints) {
histogram_tester.ExpectTotalCount(
"OptimizationGuide.HintsFetcher.GetHintsRequest.FetchLatency.BatchUpdate",
1);
histogram_tester.ExpectUniqueSample(
"OptimizationGuide.HintsFetcher.GetHintsRequest.RequestStatus."
"BatchUpdate",
HintsFetcherRequestStatus::kSuccess, 1);
}
// Tests to ensure that multiple hint fetches by the same object cannot be in
......@@ -189,19 +182,29 @@ TEST_F(HintsFetcherTest, FetchInProgress) {
base::SimpleTestClock test_clock;
SetTimeClockForTesting(&test_clock);
std::string response_content;
// Fetch back to back without waiting for Fetch to complete,
// |fetch_in_progress_| should cause early exit.
EXPECT_TRUE(FetchHints({"foo.com"}, {} /* urls */));
EXPECT_FALSE(FetchHints({"bar.com"}, {} /* urls */));
EXPECT_EQ(optimization_guide::HintsFetcherRequestStatus::kFetcherBusy,
fetcher_request_status());
{
base::HistogramTester histogram_tester;
EXPECT_TRUE(FetchHints({"foo.com"}, {} /* urls */));
EXPECT_FALSE(FetchHints({"bar.com"}, {} /* urls */));
histogram_tester.ExpectUniqueSample(
"OptimizationGuide.HintsFetcher.GetHintsRequest.RequestStatus."
"BatchUpdate",
HintsFetcherRequestStatus::kFetcherBusy, 1);
}
// Once response arrives, check to make sure a new fetch can start.
SimulateResponse(response_content, net::HTTP_OK);
EXPECT_TRUE(FetchHints({"bar.com"}, {} /* urls */));
EXPECT_EQ(optimization_guide::HintsFetcherRequestStatus::kSuccess,
fetcher_request_status());
{
base::HistogramTester histogram_tester;
std::string response_content;
SimulateResponse(response_content, net::HTTP_OK);
EXPECT_TRUE(FetchHints({"bar.com"}, {} /* urls */));
histogram_tester.ExpectUniqueSample(
"OptimizationGuide.HintsFetcher.GetHintsRequest.RequestStatus."
"BatchUpdate",
HintsFetcherRequestStatus::kSuccess, 1);
}
}
// Tests that the hints are refreshed again for hosts for whom hints were
......@@ -269,12 +272,14 @@ 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.
// Make sure histograms are recorded correctly on bad response.
histogram_tester.ExpectTotalCount(
"OptimizationGuide.HintsFetcher.GetHintsRequest.FetchLatency", 0);
histogram_tester.ExpectUniqueSample(
"OptimizationGuide.HintsFetcher.GetHintsRequest.RequestStatus."
"BatchUpdate",
HintsFetcherRequestStatus::kResponseError, 1);
}
TEST_F(HintsFetcherTest, FetchReturnBadResponse) {
......@@ -285,12 +290,14 @@ 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.
// Make sure histograms are recorded correctly on bad response.
histogram_tester.ExpectTotalCount(
"OptimizationGuide.HintsFetcher.GetHintsRequest.FetchLatency", 0);
histogram_tester.ExpectUniqueSample(
"OptimizationGuide.HintsFetcher.GetHintsRequest.RequestStatus."
"BatchUpdate",
HintsFetcherRequestStatus::kResponseError, 1);
}
TEST_F(HintsFetcherTest, FetchAttemptWhenNetworkOffline) {
......@@ -300,12 +307,14 @@ TEST_F(HintsFetcherTest, FetchAttemptWhenNetworkOffline) {
std::string response_content;
EXPECT_FALSE(FetchHints({"foo.com"}, {} /* urls */));
EXPECT_FALSE(hints_fetched());
EXPECT_EQ(optimization_guide::HintsFetcherRequestStatus::kNetworkOffline,
fetcher_request_status());
// Make sure histogram not recorded on bad response.
// Make sure histograms are recorded correctly on bad response.
histogram_tester.ExpectTotalCount(
"OptimizationGuide.HintsFetcher.GetHintsRequest.FetchLatency", 0);
histogram_tester.ExpectUniqueSample(
"OptimizationGuide.HintsFetcher.GetHintsRequest.RequestStatus."
"BatchUpdate",
HintsFetcherRequestStatus::kNetworkOffline, 1);
SetConnectionOnline();
EXPECT_TRUE(FetchHints({"foo.com"}, {} /* urls */));
......@@ -385,7 +394,6 @@ TEST_F(HintsFetcherTest, HintsFetchClearHostsSuccessfullyFetched) {
}
TEST_F(HintsFetcherTest, HintsFetcherHostsCovered) {
base::HistogramTester histogram_tester;
std::vector<std::string> hosts{"host1.com", "host2.com"};
base::Time host_invalid_time =
base::Time::Now() + base::TimeDelta().FromHours(1);
......@@ -397,7 +405,6 @@ TEST_F(HintsFetcherTest, HintsFetcherHostsCovered) {
}
TEST_F(HintsFetcherTest, HintsFetcherCoveredHostExpired) {
base::HistogramTester histogram_tester;
std::string response_content;
std::vector<std::string> hosts{"host1.com", "host2.com"};
base::Time host_invalid_time =
......@@ -430,7 +437,6 @@ TEST_F(HintsFetcherTest, HintsFetcherCoveredHostExpired) {
}
TEST_F(HintsFetcherTest, HintsFetcherHostNotCovered) {
base::HistogramTester histogram_tester;
std::vector<std::string> hosts{"host1.com", "host2.com"};
base::Time host_invalid_time =
base::Time::Now() + base::TimeDelta().FromHours(1);
......@@ -446,7 +452,6 @@ TEST_F(HintsFetcherTest, HintsFetcherHostNotCovered) {
}
TEST_F(HintsFetcherTest, HintsFetcherRemoveExpiredOnSuccessfullyFetched) {
base::HistogramTester histogram_tester;
std::string response_content;
std::vector<std::string> hosts_expired{"host1.com", "host2.com"};
base::Time host_invalid_time =
......@@ -475,7 +480,6 @@ TEST_F(HintsFetcherTest, HintsFetcherRemoveExpiredOnSuccessfullyFetched) {
}
TEST_F(HintsFetcherTest, HintsFetcherSuccessfullyFetchedHostsFull) {
base::HistogramTester histogram_tester;
std::string response_content;
std::vector<std::string> hosts;
size_t max_hosts =
......@@ -505,7 +509,6 @@ TEST_F(HintsFetcherTest, HintsFetcherSuccessfullyFetchedHostsFull) {
}
TEST_F(HintsFetcherTest, MaxHostsForOptimizationGuideServiceHintsFetch) {
base::HistogramTester histogram_tester;
std::string response_content;
std::vector<std::string> all_hosts;
......@@ -546,8 +549,6 @@ TEST_F(HintsFetcherTest, OnlyURLsToFetch) {
EXPECT_TRUE(FetchHints({}, {GURL("https://baz.com/r/werd")}));
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(
......@@ -555,6 +556,10 @@ TEST_F(HintsFetcherTest, OnlyURLsToFetch) {
histogram_tester.ExpectTotalCount(
"OptimizationGuide.HintsFetcher.GetHintsRequest.FetchLatency.BatchUpdate",
1);
histogram_tester.ExpectUniqueSample(
"OptimizationGuide.HintsFetcher.GetHintsRequest.RequestStatus."
"BatchUpdate",
static_cast<int>(HintsFetcherRequestStatus::kSuccess), 1);
}
TEST_F(HintsFetcherTest, NoHostsOrURLsToFetch) {
......@@ -564,9 +569,10 @@ TEST_F(HintsFetcherTest, NoHostsOrURLsToFetch) {
EXPECT_FALSE(FetchHints({} /* hosts */, {} /* urls */
));
EXPECT_FALSE(hints_fetched());
EXPECT_EQ(
optimization_guide::HintsFetcherRequestStatus::kNoHostsOrURLsToFetch,
fetcher_request_status());
histogram_tester.ExpectUniqueSample(
"OptimizationGuide.HintsFetcher.GetHintsRequest.RequestStatus."
"BatchUpdate",
static_cast<int>(HintsFetcherRequestStatus::kNoHostsOrURLsToFetch), 1);
}
} // namespace optimization_guide
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment