Commit 42ea8421 authored by Nela Kaczmarek's avatar Nela Kaczmarek Committed by Commit Bot

[Passwords] Add metrics for change password affiliation info.

This change records GetChangePasswordURLMetric in AffiliationService and FetchTimeMetric in AffiliationFetcher.

Bug: 1108279
Change-Id: Ibbeb4fc29e30c8b28dd98e8312608f64ed9173d1
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2385360
Commit-Queue: Nela Kaczmarek <nelakaczmarek@google.com>
Reviewed-by: default avatarVasilii Sukhanov <vasilii@chromium.org>
Reviewed-by: default avatarJan Wilken Dörrie <jdoerrie@chromium.org>
Cr-Commit-Position: refs/heads/master@{#804800}
parent fc28a126
......@@ -77,6 +77,7 @@ void AffiliationFetcher::SetFactoryForTesting(
void AffiliationFetcher::StartRequest(const std::vector<FacetURI>& facet_uris,
RequestInfo request_info) {
DCHECK(!simple_url_loader_);
fetch_timer_ = base::ElapsedTimer();
requested_facet_uris_ = facet_uris;
net::NetworkTrafficAnnotationTag traffic_annotation =
......@@ -188,6 +189,8 @@ bool AffiliationFetcher::ParseResponse(
void AffiliationFetcher::OnSimpleLoaderComplete(
std::unique_ptr<std::string> response_body) {
base::UmaHistogramTimes("PasswordManager.AffiliationFetcher.FetchTime",
fetch_timer_.Elapsed());
// Note that invoking the |delegate_| may destroy |this| synchronously, so the
// invocation must happen last.
std::unique_ptr<AffiliationFetcherDelegate::Result> result_data(
......
......@@ -11,6 +11,7 @@
#include "base/macros.h"
#include "base/memory/ref_counted.h"
#include "base/timer/elapsed_timer.h"
#include "components/password_manager/core/browser/android_affiliation/affiliation_fetcher_interface.h"
class GURL;
......@@ -92,6 +93,8 @@ class AffiliationFetcher : public AffiliationFetcherInterface {
scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory_;
std::vector<FacetURI> requested_facet_uris_;
AffiliationFetcherDelegate* const delegate_;
// Timer to track the response time of the request.
base::ElapsedTimer fetch_timer_;
std::unique_ptr<network::SimpleURLLoader> simple_url_loader_;
......
......@@ -10,6 +10,7 @@
#include "base/macros.h"
#include "base/test/bind_test_util.h"
#include "base/test/metrics/histogram_tester.h"
#include "base/test/null_task_runner.h"
#include "base/test/task_environment.h"
#include "components/password_manager/core/browser/android_affiliation/affiliation_api.pb.h"
......@@ -449,4 +450,22 @@ TEST_F(AffiliationFetcherTest, FailOnNetworkError) {
WaitForResponse();
}
TEST_F(AffiliationFetcherTest, FetchTimeMetric) {
base::HistogramTester histogram_tester;
std::vector<FacetURI> requested_uris = {
FacetURI::FromCanonicalSpec(kExampleWebFacet1URI)};
SetupSuccessfulResponse(
affiliation_pb::LookupAffiliationResponse().SerializeAsString());
MockAffiliationFetcherDelegate mock_delegate;
EXPECT_CALL(mock_delegate, OnFetchSucceededProxy());
auto fetcher =
AffiliationFetcher::Create(test_shared_loader_factory(), &mock_delegate);
fetcher->StartRequest(requested_uris, {});
WaitForResponse();
histogram_tester.ExpectTotalCount(
"PasswordManager.AffiliationFetcher.FetchTime", 1);
}
} // namespace password_manager
......@@ -10,6 +10,7 @@
#include "base/json/json_reader.h"
#include "base/metrics/histogram_functions.h"
#include "base/timer/elapsed_timer.h"
#include "components/password_manager/core/browser/password_manager_metrics_util.h"
#include "components/password_manager/core/common/password_manager_pref_names.h"
#include "components/prefs/pref_service.h"
#include "net/base/registry_controlled_domains/registry_controlled_domain.h"
......@@ -123,8 +124,9 @@ GURL ChangePasswordUrlServiceImpl::GetChangePasswordUrl(const GURL& url) {
// ongoing or if it succeeded. Network errors are logged in the response
// callback.
if (state_ == FetchState::kIsLoading) {
base::UmaHistogramEnumeration(kGetChangePasswordUrlMetricName,
GetChangePasswordUrlMetric::kNotFetchedYet);
base::UmaHistogramEnumeration(
kGetChangePasswordUrlMetricName,
metrics_util::GetChangePasswordUrlMetric::kNotFetchedYet);
} else if (state_ == FetchState::kFetchSucceeded) {
std::string domain_and_registry =
net::registry_controlled_domains::GetDomainAndRegistry(
......@@ -133,12 +135,12 @@ GURL ChangePasswordUrlServiceImpl::GetChangePasswordUrl(const GURL& url) {
if (it != change_password_url_map_.end()) {
base::UmaHistogramEnumeration(
kGetChangePasswordUrlMetricName,
GetChangePasswordUrlMetric::kUrlOverrideUsed);
metrics_util::GetChangePasswordUrlMetric::kUrlOverrideUsed);
return it->second;
} else {
base::UmaHistogramEnumeration(
kGetChangePasswordUrlMetricName,
GetChangePasswordUrlMetric::kNoUrlOverrideAvailable);
metrics_util::GetChangePasswordUrlMetric::kNoUrlOverrideAvailable);
}
}
// Fallback if no valid change-password url available no response available.
......
......@@ -32,20 +32,6 @@ extern const char kGstaticFetchErrorCodeMetricName[];
extern const char kGstaticFetchHttpResponseCodeMetricName[];
extern const char kGstaticFetchTimeMetricName[];
// Used to record metrics for the usage and timing of the GetChangePasswordUrl
// call. These values are persisted to logs. Entries should not be renumbered
// and numeric values should never be reused.
enum class GetChangePasswordUrlMetric {
// Used when GetChangePasswordUrl is called before the gstatic response
// arrives.
kNotFetchedYet = 0,
// Used when a url was in the gsatic file.
kUrlOverrideUsed = 1,
// Used when no override url was available
kNoUrlOverrideAvailable = 2,
kMaxValue = kNoUrlOverrideAvailable,
};
// Used to log the response of the request to the gstatic file. These values are
// persisted to logs. Entries should not be renumbered and numeric values should
// never be reused.
......
......@@ -7,6 +7,7 @@
#include "base/test/metrics/histogram_tester.h"
#include "base/test/mock_callback.h"
#include "base/test/task_environment.h"
#include "components/password_manager/core/browser/password_manager_metrics_util.h"
#include "components/password_manager/core/common/password_manager_pref_names.h"
#include "components/prefs/pref_registry_simple.h"
#include "components/prefs/testing_pref_service.h"
......@@ -141,7 +142,7 @@ TEST_F(ChangePasswordUrlServiceTest,
EXPECT_EQ(GetChangePasswordUrl(GURL("https://google.com/foo")), GURL());
histogram_tester().ExpectUniqueSample(
kGetChangePasswordUrlMetricName,
GetChangePasswordUrlMetric::kNotFetchedYet, 1);
metrics_util::GetChangePasswordUrlMetric::kNotFetchedYet, 1);
}
TEST_F(ChangePasswordUrlServiceTest,
......@@ -152,7 +153,7 @@ TEST_F(ChangePasswordUrlServiceTest,
GURL("https://google.com/change-password"));
histogram_tester().ExpectUniqueSample(
kGetChangePasswordUrlMetricName,
GetChangePasswordUrlMetric::kUrlOverrideUsed, 1);
metrics_util::GetChangePasswordUrlMetric::kUrlOverrideUsed, 1);
}
TEST_F(ChangePasswordUrlServiceTest,
......@@ -162,7 +163,7 @@ TEST_F(ChangePasswordUrlServiceTest,
EXPECT_EQ(GetChangePasswordUrl(GURL("https://netflix.com")), GURL());
histogram_tester().ExpectUniqueSample(
kGetChangePasswordUrlMetricName,
GetChangePasswordUrlMetric::kNoUrlOverrideAvailable, 1);
metrics_util::GetChangePasswordUrlMetric::kNoUrlOverrideAvailable, 1);
}
TEST_F(ChangePasswordUrlServiceTest, NetworkMetrics_Failed) {
......
......@@ -456,6 +456,23 @@ enum class MoveToAccountStoreTrigger {
kMaxValue = kExplicitlyTriggeredInSettings,
};
// Used to record metrics for the usage and timing of the GetChangePasswordUrl
// call. These values are persisted to logs. Entries should not be renumbered
// and numeric values should never be reused.
enum class GetChangePasswordUrlMetric {
// Used when GetChangePasswordUrl is called before the response
// arrives.
kNotFetchedYet = 0,
// Used when a url was used, which corresponds to the requested site.
kUrlOverrideUsed = 1,
// Used when no override url was available.
kNoUrlOverrideAvailable = 2,
// Used when a url was used, which corresponds to a site from within same
// FacetGroup.
kGroupUrlOverrideUsed = 3,
kMaxValue = kGroupUrlOverrideUsed,
};
std::string GetPasswordAccountStorageUserStateHistogramSuffix(
PasswordAccountStorageUserState user_state);
......
......@@ -4,6 +4,7 @@
#include "components/password_manager/core/browser/site_affiliation/affiliation_service_impl.h"
#include "base/metrics/histogram_functions.h"
#include "base/util/ranges/algorithm.h"
#include "components/password_manager/core/browser/android_affiliation/affiliation_fetcher.h"
#include "components/password_manager/core/browser/password_store_factory_util.h"
......@@ -21,9 +22,10 @@ namespace {
// paired with another facet's URL, which belongs to the same group. In case
// none of the group's facets have change password URLs then those facets are
// not inserted to the map.
std::map<FacetURI, GURL> CreateFacetUriToChangePasswordUrlMap(
std::map<FacetURI, AffiliationServiceImpl::ChangePasswordUrlMatch>
CreateFacetUriToChangePasswordUrlMap(
const std::vector<GroupedFacets>& groupings) {
std::map<FacetURI, GURL> uri_to_url;
std::map<FacetURI, AffiliationServiceImpl::ChangePasswordUrlMatch> uri_to_url;
for (const auto& grouped_facets : groupings) {
std::vector<FacetURI> uris_without_urls;
GURL fallback_url;
......@@ -32,12 +34,16 @@ std::map<FacetURI, GURL> CreateFacetUriToChangePasswordUrlMap(
uris_without_urls.push_back(facet.uri);
continue;
}
uri_to_url[facet.uri] = facet.change_password_url;
uri_to_url[facet.uri] = AffiliationServiceImpl::ChangePasswordUrlMatch{
.change_password_url = facet.change_password_url,
.group_url_override = false};
fallback_url = facet.change_password_url;
}
if (fallback_url.is_valid()) {
for (const auto& uri : uris_without_urls)
uri_to_url[uri] = fallback_url;
for (const auto& uri : uris_without_urls) {
uri_to_url[uri] = AffiliationServiceImpl::ChangePasswordUrlMatch{
.change_password_url = fallback_url, .group_url_override = true};
}
}
}
return uri_to_url;
......@@ -45,6 +51,9 @@ std::map<FacetURI, GURL> CreateFacetUriToChangePasswordUrlMap(
} // namespace
const char kGetChangePasswordURLMetricName[] =
"PasswordManager.AffiliationService.GetChangePasswordUsage";
AffiliationServiceImpl::AffiliationServiceImpl(
syncer::SyncService* sync_service,
scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory)
......@@ -68,20 +77,41 @@ void AffiliationServiceImpl::Clear() {
GURL AffiliationServiceImpl::GetChangePasswordURL(const GURL& url) const {
auto it = change_password_urls_.find(url::SchemeHostPort(url));
return it != change_password_urls_.end() ? it->second : GURL();
if (it != change_password_urls_.end()) {
if (it->second.group_url_override) {
base::UmaHistogramEnumeration(
kGetChangePasswordURLMetricName,
metrics_util::GetChangePasswordUrlMetric::kGroupUrlOverrideUsed);
} else {
base::UmaHistogramEnumeration(
kGetChangePasswordURLMetricName,
metrics_util::GetChangePasswordUrlMetric::kUrlOverrideUsed);
}
return it->second.change_password_url;
}
if (base::Contains(requested_tuple_origins_, url::SchemeHostPort(url))) {
base::UmaHistogramEnumeration(
kGetChangePasswordURLMetricName,
metrics_util::GetChangePasswordUrlMetric::kNotFetchedYet);
} else {
base::UmaHistogramEnumeration(
kGetChangePasswordURLMetricName,
metrics_util::GetChangePasswordUrlMetric::kNoUrlOverrideAvailable);
}
return GURL();
}
void AffiliationServiceImpl::OnFetchSucceeded(
std::unique_ptr<AffiliationFetcherDelegate::Result> result) {
fetcher_.reset();
std::map<FacetURI, GURL> uri_to_url =
CreateFacetUriToChangePasswordUrlMap(result->groupings);
std::map<FacetURI, AffiliationServiceImpl::ChangePasswordUrlMatch>
uri_to_url = CreateFacetUriToChangePasswordUrlMap(result->groupings);
for (const url::SchemeHostPort& requested_tuple : requested_tuple_origins_) {
auto it = uri_to_url.find(
FacetURI::FromPotentiallyInvalidSpec(requested_tuple.Serialize()));
if (it != uri_to_url.end())
if (it != uri_to_url.end()) {
change_password_urls_[requested_tuple] = it->second;
}
}
requested_tuple_origins_.clear();
......
......@@ -14,6 +14,7 @@
#include "base/memory/scoped_refptr.h"
#include "components/password_manager/core/browser/android_affiliation/affiliation_fetcher_delegate.h"
#include "components/password_manager/core/browser/android_affiliation/affiliation_fetcher_interface.h"
#include "components/password_manager/core/browser/password_manager_metrics_util.h"
namespace network {
class SharedURLLoaderFactory;
......@@ -29,9 +30,16 @@ class SchemeHostPort;
namespace password_manager {
extern const char kGetChangePasswordURLMetricName[];
class AffiliationServiceImpl : public AffiliationService,
public AffiliationFetcherDelegate {
public:
struct ChangePasswordUrlMatch {
GURL change_password_url;
bool group_url_override;
};
explicit AffiliationServiceImpl(
syncer::SyncService* sync_service,
scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory);
......@@ -80,7 +88,7 @@ class AffiliationServiceImpl : public AffiliationService,
syncer::SyncService* sync_service_;
scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory_;
std::vector<url::SchemeHostPort> requested_tuple_origins_;
std::map<url::SchemeHostPort, GURL> change_password_urls_;
std::map<url::SchemeHostPort, ChangePasswordUrlMatch> change_password_urls_;
// TODO(crbug.com/1117045): A vector of pending fetchers to be created.
std::unique_ptr<AffiliationFetcherInterface> fetcher_;
};
......
......@@ -5,6 +5,7 @@
#include <memory>
#include <vector>
#include "base/test/metrics/histogram_tester.h"
#include "base/test/task_environment.h"
#include "components/password_manager/core/browser/android_affiliation/affiliation_fetcher.h"
#include "components/password_manager/core/browser/android_affiliation/mock_affiliation_fetcher.h"
......@@ -23,7 +24,6 @@ namespace password_manager {
namespace {
constexpr char kEmptyURL[] = "";
constexpr char k1ExampleURL[] = "https://1.example.com";
constexpr char k1ExampleChangePasswordURL[] =
"https://1.example.com/.well-known/change-password";
......@@ -72,6 +72,7 @@ class AffiliationServiceImplTest : public testing::Test {
AffiliationFetcher::SetFactoryForTesting(nullptr);
}
base::HistogramTester& histogram_tester() { return histogram_tester_; }
syncer::TestSyncService* sync_service() { return sync_service_.get(); }
AffiliationServiceImpl* service() { return &service_; }
MockAffiliationFetcherFactory* mock_fetcher_factory() {
......@@ -86,6 +87,7 @@ class AffiliationServiceImplTest : public testing::Test {
private:
base::test::TaskEnvironment task_env_;
base::HistogramTester histogram_tester_;
std::unique_ptr<syncer::TestSyncService> sync_service_;
AffiliationServiceImpl service_;
MockAffiliationFetcherFactory mock_fetcher_factory_;
......@@ -96,15 +98,15 @@ TEST_F(AffiliationServiceImplTest, GetChangePasswordURLReturnsEmpty) {
}
TEST_F(AffiliationServiceImplTest, ClearStopsOngoingAffiliationFetcherRequest) {
const std::vector<GURL> origins = {GURL(k1ExampleURL), GURL(k2ExampleURL)};
auto* mock_fetcher = new MockAffiliationFetcher();
EXPECT_CALL(*mock_fetcher_factory(), CreateInstance)
.WillOnce(Return(mock_fetcher));
const std::vector<GURL> origins = {GURL(k1ExampleURL), GURL(k2ExampleURL)};
EXPECT_CALL(*mock_fetcher,
StartRequest(ToFacetsURIs(origins),
AffiliationFetcherInterface::RequestInfo{
.change_password_info = true}));
EXPECT_CALL(*mock_fetcher_factory(), CreateInstance)
.WillOnce(Return(mock_fetcher));
service()->PrefetchChangePasswordURLs(origins);
EXPECT_NE(nullptr, service()->GetFetcherForTesting());
......@@ -131,7 +133,7 @@ TEST_F(AffiliationServiceImplTest,
{.uri = FacetURI::FromPotentiallyInvalidSpec(k1ExampleURL),
.change_password_url = GURL(k1ExampleChangePasswordURL)},
{.uri = FacetURI::FromPotentiallyInvalidSpec(kM1ExampleURL),
.change_password_url = GURL(kEmptyURL)},
.change_password_url = GURL()},
{.uri = FacetURI::FromPotentiallyInvalidSpec(kOneExampleURL),
.change_password_url = GURL(kOneExampleChangePasswordURL)}};
auto test_result = std::make_unique<AffiliationFetcherDelegate::Result>();
......@@ -162,7 +164,7 @@ TEST_F(AffiliationServiceImplTest,
{.uri = FacetURI::FromPotentiallyInvalidSpec(k1ExampleURL),
.change_password_url = GURL(k1ExampleChangePasswordURL)},
{.uri = FacetURI::FromPotentiallyInvalidSpec(kM1ExampleURL),
.change_password_url = GURL(kEmptyURL)}};
.change_password_url = GURL()}};
auto test_result = std::make_unique<AffiliationFetcherDelegate::Result>();
test_result->groupings.push_back(group);
AffiliationFetcherDelegate* service_delegate = service();
......@@ -306,4 +308,94 @@ TEST_F(AffiliationServiceImplTest, SetupNotCompletedPreventsFetch) {
{GURL(k1ExampleURL), GURL(k2ExampleURL)});
}
// Below are the tests veryfing recorded metrics for
// PasswordManager.AffiliationService.GetChangePasswordUsage.
TEST_F(AffiliationServiceImplTest, NotFetchedYetMetricIfWaitingForResponse) {
const GURL origin(k1ExampleURL);
auto* mock_fetcher = new MockAffiliationFetcher();
EXPECT_CALL(*mock_fetcher,
StartRequest(ToFacetsURIs({origin}),
AffiliationFetcherInterface::RequestInfo{
.change_password_info = true}));
EXPECT_CALL(*mock_fetcher_factory(), CreateInstance)
.WillOnce(Return(mock_fetcher));
service()->PrefetchChangePasswordURLs({origin});
service()->GetChangePasswordURL(origin);
histogram_tester().ExpectUniqueSample(
kGetChangePasswordURLMetricName,
metrics_util::GetChangePasswordUrlMetric::kNotFetchedYet, 1);
}
TEST_F(AffiliationServiceImplTest, NoUrlOverrideAvailableMetric) {
service()->GetChangePasswordURL(GURL(k1ExampleURL));
histogram_tester().ExpectUniqueSample(
kGetChangePasswordURLMetricName,
metrics_util::GetChangePasswordUrlMetric::kNoUrlOverrideAvailable, 1);
}
TEST_F(AffiliationServiceImplTest, FoundForRequestedFacetMetric) {
const GURL origin(k1ExampleURL);
auto* mock_fetcher = new MockAffiliationFetcher();
EXPECT_CALL(*mock_fetcher,
StartRequest(ToFacetsURIs({origin}),
AffiliationFetcherInterface::RequestInfo{
.change_password_info = true}));
EXPECT_CALL(*mock_fetcher_factory(), CreateInstance)
.WillOnce(Return(mock_fetcher));
service()->PrefetchChangePasswordURLs({origin});
const GroupedFacets group = {
{.uri = FacetURI::FromPotentiallyInvalidSpec(k1ExampleURL),
.change_password_url = GURL(k1ExampleChangePasswordURL)},
{.uri = FacetURI::FromPotentiallyInvalidSpec(kOneExampleURL),
.change_password_url = GURL(kOneExampleChangePasswordURL)}};
auto test_result = std::make_unique<AffiliationFetcherDelegate::Result>();
test_result->groupings.push_back(group);
AffiliationFetcherDelegate* service_delegate = service();
service_delegate->OnFetchSucceeded(std::move(test_result));
service()->GetChangePasswordURL(origin);
histogram_tester().ExpectUniqueSample(
kGetChangePasswordURLMetricName,
metrics_util::GetChangePasswordUrlMetric::kUrlOverrideUsed, 1);
}
TEST_F(AffiliationServiceImplTest, FoundForGroupedFacetMetric) {
const GURL origin(kM1ExampleURL);
auto* mock_fetcher = new MockAffiliationFetcher();
EXPECT_CALL(*mock_fetcher,
StartRequest(ToFacetsURIs({origin}),
AffiliationFetcherInterface::RequestInfo{
.change_password_info = true}));
EXPECT_CALL(*mock_fetcher_factory(), CreateInstance)
.WillOnce(Return(mock_fetcher));
service()->PrefetchChangePasswordURLs({origin});
const GroupedFacets group = {
{.uri = FacetURI::FromPotentiallyInvalidSpec(k1ExampleURL),
.change_password_url = GURL(k1ExampleChangePasswordURL)},
{.uri = FacetURI::FromPotentiallyInvalidSpec(kM1ExampleURL),
.change_password_url = GURL()}};
auto test_result = std::make_unique<AffiliationFetcherDelegate::Result>();
test_result->groupings.push_back(group);
AffiliationFetcherDelegate* service_delegate = service();
service_delegate->OnFetchSucceeded(std::move(test_result));
service()->GetChangePasswordURL(origin);
histogram_tester().ExpectUniqueSample(
kGetChangePasswordURLMetricName,
metrics_util::GetChangePasswordUrlMetric::kGroupUrlOverrideUsed, 1);
}
} // namespace password_manager
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