Skip to content

Commit

Permalink
[Android][UPM] Log the latency and success metrics for PasswordStoreB…
Browse files Browse the repository at this point in the history
…uiltInBackend

MetricsRecorder is moved so it can be used by both
PasswordStoreAndroidBackend and PasswordStoreBuiltInBackend and it was
renamed to PasswordStoreBackendMetricsRecorder.
PasswordStoreBackendMetricsRecorder now needs a ClassInfix so we could
distinguish and compare the logs coming from the two different backends.
The plan is to add logging for the methods of PasswordStoreBuiltInBackend
that correspond to the methods of PasswordStoreAndroidBackend that log
their metrics. This will be done in a followup CL. For now, just
GetAllLoginsAsync is logged.

Bug: 1298467
Change-Id: I7bc93f4ef6d20d7ca0463fff3f946dc20c1ed0ed
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3535784
Reviewed-by: Friedrich Horschig <fhorschig@chromium.org>
Reviewed-by: Vasilii Sukhanov <vasilii@chromium.org>
Commit-Queue: Ivana Žužić <izuzic@google.com>
Cr-Commit-Position: refs/heads/main@{#987988}
  • Loading branch information
Ivana Žužić authored and Chromium LUCI CQ committed Apr 1, 2022
1 parent 727f58e commit 0a8dc25
Show file tree
Hide file tree
Showing 12 changed files with 323 additions and 119 deletions.
1 change: 1 addition & 0 deletions chrome/browser/password_manager/android/DEPS
Original file line number Diff line number Diff line change
Expand Up @@ -2,4 +2,5 @@ include_rules = [
"-content/public/android/java",
"+content/public/android/java/src/org/chromium/content_public",
"+components/device_reauth",
"+components/password_manager/core/browser",
]
Original file line number Diff line number Diff line change
Expand Up @@ -150,58 +150,10 @@ PasswordStoreAndroidBackendBridge::Account GetAccount(

} // namespace

PasswordStoreAndroidBackend::MetricsRecorder::MetricsRecorder() = default;

PasswordStoreAndroidBackend::MetricsRecorder::MetricsRecorder(
MetricInfix metric_infix)
: metric_infix_(std::move(metric_infix)) {}

PasswordStoreAndroidBackend::MetricsRecorder::MetricsRecorder(
MetricsRecorder&&) = default;

PasswordStoreAndroidBackend::MetricsRecorder&
PasswordStoreAndroidBackend::MetricsRecorder::MetricsRecorder::operator=(
MetricsRecorder&&) = default;

PasswordStoreAndroidBackend::MetricsRecorder::~MetricsRecorder() = default;

void PasswordStoreAndroidBackend::MetricsRecorder::RecordMetrics(
bool success,
absl::optional<AndroidBackendError> error) const {
auto BuildMetricName = [this](base::StringPiece suffix) {
return base::StrCat({"PasswordManager.PasswordStoreAndroidBackend.",
*metric_infix_, ".", suffix});
};
base::TimeDelta duration = base::Time::Now() - start_;
base::UmaHistogramMediumTimes(BuildMetricName("Latency"), duration);
base::UmaHistogramBoolean(BuildMetricName("Success"), success);
if (!error.has_value())
return;

DCHECK(!success);
// In case of AndroidBackend error, we report additional metrics.
base::UmaHistogramEnumeration(
"PasswordManager.PasswordStoreAndroidBackend.ErrorCode",
error.value().type);
base::UmaHistogramEnumeration(BuildMetricName("ErrorCode"),
error.value().type);
if (error.value().type == AndroidBackendErrorType::kExternalError) {
DCHECK(error.value().api_error_code.has_value());
base::HistogramBase* histogram = base::SparseHistogram::FactoryGet(
"PasswordManager.PasswordStoreAndroidBackend.APIError",
base::HistogramBase::kUmaTargetedHistogramFlag);
histogram->Add(error.value().api_error_code.value());
histogram = base::SparseHistogram::FactoryGet(
BuildMetricName("APIError"),
base::HistogramBase::kUmaTargetedHistogramFlag);
histogram->Add(error.value().api_error_code.value());
}
}

class PasswordStoreAndroidBackend::ClearAllLocalPasswordsMetricRecorder {
public:
explicit ClearAllLocalPasswordsMetricRecorder(
PasswordStoreAndroidBackend::MetricsRecorder metrics_recorder)
PasswordStoreBackendMetricsRecorder metrics_recorder)
: metrics_recorder_(std::move(metrics_recorder)) {}

void OnAllRemovalsFinished() {
Expand Down Expand Up @@ -229,20 +181,20 @@ class PasswordStoreAndroidBackend::ClearAllLocalPasswordsMetricRecorder {
private:
int total_count_ = 0;
int failure_count_ = 0;
MetricsRecorder metrics_recorder_;
PasswordStoreBackendMetricsRecorder metrics_recorder_;
};

PasswordStoreAndroidBackend::JobReturnHandler::JobReturnHandler() = default;

PasswordStoreAndroidBackend::JobReturnHandler::JobReturnHandler(
LoginsOrErrorReply callback,
MetricsRecorder metrics_recorder)
PasswordStoreBackendMetricsRecorder metrics_recorder)
: success_callback_(std::move(callback)),
metrics_recorder_(std::move(metrics_recorder)) {}

PasswordStoreAndroidBackend::JobReturnHandler::JobReturnHandler(
PasswordStoreChangeListReply callback,
MetricsRecorder metrics_recorder)
PasswordStoreBackendMetricsRecorder metrics_recorder)
: success_callback_(std::move(callback)),
metrics_recorder_(std::move(metrics_recorder)) {}

Expand Down Expand Up @@ -458,7 +410,8 @@ void PasswordStoreAndroidBackend::DisableAutoSignInForOriginsAsync(
// this callback more gracefully when it's implemented.
PasswordStoreChangeListReply record_metrics_and_run_completion =
base::BindOnce(
[](MetricsRecorder metrics_recorder, base::OnceClosure completion,
[](PasswordStoreBackendMetricsRecorder metrics_recorder,
base::OnceClosure completion,
absl::optional<PasswordStoreChangeList> changes) {
// Errors are not recorded at the moment.
// TODO(https://crbug.com/1278807): Implement error handling, when
Expand All @@ -467,7 +420,9 @@ void PasswordStoreAndroidBackend::DisableAutoSignInForOriginsAsync(
/*error=*/absl::nullopt);
std::move(completion).Run();
},
MetricsRecorder(MetricInfix("DisableAutoSignInForOriginsAsync")),
PasswordStoreBackendMetricsRecorder(
ClassInfix("PasswordStoreAndroidBackend"),
MetricInfix("DisableAutoSignInForOriginsAsync")),
std::move(completion));

GetAllLoginsAsync(
Expand All @@ -492,7 +447,7 @@ PasswordStoreAndroidBackend::CreateSyncControllerDelegate() {
void PasswordStoreAndroidBackend::ClearAllLocalPasswords() {
LoginsOrErrorReply cleaning_callback = base::BindOnce(
[](base::WeakPtr<PasswordStoreAndroidBackend> weak_self,
MetricsRecorder metrics_recorder,
PasswordStoreBackendMetricsRecorder metrics_recorder,
LoginsResultOrError logins_or_error) {
if (!weak_self || absl::holds_alternative<PasswordStoreBackendError>(
logins_or_error)) {
Expand Down Expand Up @@ -529,7 +484,9 @@ void PasswordStoreAndroidBackend::ClearAllLocalPasswords() {
std::move(callbacks_chain).Run();
},
weak_ptr_factory_.GetWeakPtr(),
MetricsRecorder(MetricInfix("ClearAllLocalPasswords")));
PasswordStoreBackendMetricsRecorder(
ClassInfix("PasswordStoreAndroidBackend"),
MetricInfix("ClearAllLocalPasswords")));

GetAllLoginsForAccount(PasswordStoreOperationTarget::kLocalStorage,
std::move(cleaning_callback));
Expand Down Expand Up @@ -592,7 +549,9 @@ void PasswordStoreAndroidBackend::QueueNewJob(JobId job_id,
DCHECK_CALLED_ON_VALID_SEQUENCE(main_sequence_checker_);
request_for_job_.emplace(
job_id, JobReturnHandler(std::move(callback),
MetricsRecorder(std::move(metric_infix))));
PasswordStoreBackendMetricsRecorder(
ClassInfix("PasswordStoreAndroidBackend"),
std::move(metric_infix))));
}

PasswordStoreAndroidBackend::JobReturnHandler
Expand Down Expand Up @@ -660,16 +619,18 @@ PasswordStoreAndroidBackend::ReportMetricsAndInvokeCallbackForLoginsRetrieval(
// TODO(https://crbug.com/1229655) Switch to using base::PassThrough to handle
// this callback more gracefully when it's implemented.
return base::BindOnce(
[](MetricsRecorder metrics_recorder, LoginsReply callback,
LoginsResultOrError results) {
[](PasswordStoreBackendMetricsRecorder metrics_recorder,
LoginsReply callback, LoginsResultOrError results) {
metrics_recorder.RecordMetrics(
/*success=*/!(
absl::holds_alternative<PasswordStoreBackendError>(results)),
/*error=*/absl::nullopt);
std::move(callback).Run(
GetLoginsOrEmptyListOnFailure(std::move(results)));
},
MetricsRecorder(metric_infix), std::move(callback));
PasswordStoreBackendMetricsRecorder(
ClassInfix("PasswordStoreAndroidBackend"), metric_infix),
std::move(callback));
}

// static
Expand All @@ -680,7 +641,7 @@ PasswordStoreChangeListReply PasswordStoreAndroidBackend::
// TODO(https://crbug.com/1229655) Switch to using base::PassThrough to handle
// this callback more gracefully when it's implemented.
return base::BindOnce(
[](MetricsRecorder metrics_recorder,
[](PasswordStoreBackendMetricsRecorder metrics_recorder,
PasswordStoreChangeListReply callback,
absl::optional<PasswordStoreChangeList> results) {
// Errors are not recorded at the moment.
Expand All @@ -690,7 +651,9 @@ PasswordStoreChangeListReply PasswordStoreAndroidBackend::
/*error=*/absl::nullopt);
std::move(callback).Run(std::move(results));
},
MetricsRecorder(metric_infix), std::move(callback));
PasswordStoreBackendMetricsRecorder(
ClassInfix("PasswordStoreAndroidBackend"), metric_infix),
std::move(callback));
}

void PasswordStoreAndroidBackend::GetAllLoginsForAccount(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@
#include "chrome/browser/password_manager/android/password_store_android_backend_bridge.h"
#include "chrome/browser/password_manager/android/password_sync_controller_delegate_android.h"
#include "components/password_manager/core/browser/password_store_backend.h"
#include "components/password_manager/core/browser/password_store_backend_metrics_recorder.h"
#include "components/password_manager/core/browser/password_store_util.h"
#include "third_party/abseil-cpp/absl/types/optional.h"
#include "third_party/abseil-cpp/absl/types/variant.h"

Expand Down Expand Up @@ -49,36 +51,6 @@ class PasswordStoreAndroidBackend
private:
SEQUENCE_CHECKER(main_sequence_checker_);

using MetricInfix = base::StrongAlias<struct MetricNameTag, std::string>;

// Records metrics for an asynchronous job or a series of jobs. The job is
// expected to have started when the MetricsRecorder instance is created.
// Latency is reported in RecordMetrics() under that assumption.
class MetricsRecorder {
public:
MetricsRecorder();
explicit MetricsRecorder(MetricInfix metric_name);
MetricsRecorder(MetricsRecorder&&);
MetricsRecorder& operator=(MetricsRecorder&&);
~MetricsRecorder();

// Records the following metrics:
// - "PasswordManager.PasswordStoreAndroidBackend.<metric_infix_>.Latency"
// - "PasswordManager.PasswordStoreAndroidBackend.<metric_infix_>.Success"
// When |error| is specified, the following metrcis are recorded in
// addition:
// - "PasswordManager.PasswordStoreAndroidBackend.APIError"
// - "PasswordManager.PasswordStoreAndroidBackend.ErrorCode"
// - "PasswordManager.PasswordStoreAndroidBackend.<metric_infix_>.APIError"
// - "PasswordManager.PasswordStoreAndroidBackend.<metric_infix_>.ErrorCode"
void RecordMetrics(bool success,
absl::optional<AndroidBackendError> error) const;

private:
MetricInfix metric_infix_;
base::Time start_ = base::Time::Now();
};

class ClearAllLocalPasswordsMetricRecorder;

// Wraps the handler for an asynchronous job (if successful) and invokes the
Expand All @@ -91,9 +63,9 @@ class PasswordStoreAndroidBackend

JobReturnHandler();
JobReturnHandler(LoginsOrErrorReply callback,
MetricsRecorder metrics_recorder);
PasswordStoreBackendMetricsRecorder metrics_recorder);
JobReturnHandler(PasswordStoreChangeListReply callback,
MetricsRecorder metrics_recorder);
PasswordStoreBackendMetricsRecorder metrics_recorder);
JobReturnHandler(JobReturnHandler&&);
JobReturnHandler& operator=(JobReturnHandler&&);
~JobReturnHandler();
Expand All @@ -113,7 +85,7 @@ class PasswordStoreAndroidBackend
private:
absl::variant<LoginsOrErrorReply, PasswordStoreChangeListReply>
success_callback_;
MetricsRecorder metrics_recorder_;
PasswordStoreBackendMetricsRecorder metrics_recorder_;
};

using JobId = PasswordStoreAndroidBackendBridge::JobId;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
#include "base/android/jni_array.h"
#include "base/android/jni_string.h"
#include "chrome/browser/password_manager/android/jni_headers/PasswordStoreAndroidBackendBridgeImpl_jni.h"
#include "components/password_manager/core/browser/android_backend_error.h"
#include "components/password_manager/core/browser/password_form.h"
#include "components/password_manager/core/browser/sync/password_proto_utils.h"
#include "components/sync/protocol/list_passwords_result.pb.h"
Expand Down
3 changes: 3 additions & 0 deletions components/password_manager/core/browser/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -189,6 +189,8 @@ static_library("browser") {
"password_store.cc",
"password_store.h",
"password_store_backend.h",
"password_store_backend_metrics_recorder.cc",
"password_store_backend_metrics_recorder.h",
"password_store_built_in_backend.cc",
"password_store_built_in_backend.h",
"password_store_change.cc",
Expand Down Expand Up @@ -736,6 +738,7 @@ source_set("unit_tests") {
"built_in_backend_to_android_backend_migrator_unittest.cc",
"capabilities_service_impl_unittest.cc",
"password_scripts_fetcher_impl_unittests.cc",
"password_store_backend_metrics_recorder_unittest.cc",
"password_store_backend_migration_decorator_unittest.cc",
"password_store_proxy_backend_unittest.cc",
"saved_passwords_capabilities_fetcher_unittest.cc",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -74,22 +74,34 @@ bool LoginDatabaseAsyncHelper::Initialize(
return success;
}

LoginsResult LoginDatabaseAsyncHelper::GetAllLogins() {
LoginsResult LoginDatabaseAsyncHelper::GetAllLogins(
PasswordStoreBackendMetricsRecorder metrics_recorder) {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
PrimaryKeyToFormMap key_to_form_map;

if (!login_db_)
if (!login_db_) {
metrics_recorder.RecordMetrics(
/*success=*/false,
/*error=*/absl::optional<ErrorFromPasswordStoreOrAndroidBackend>(
PasswordStoreBackendError::kUnrecoverable));
return {};
}
FormRetrievalResult result = login_db_->GetAllLogins(&key_to_form_map);
if (result != FormRetrievalResult::kSuccess &&
result != FormRetrievalResult::kEncryptionServiceFailureWithPartialData)
result != FormRetrievalResult::kEncryptionServiceFailureWithPartialData) {
metrics_recorder.RecordMetrics(
/*success=*/false,
/*error=*/absl::optional<ErrorFromPasswordStoreOrAndroidBackend>(
PasswordStoreBackendError::kUnrecoverable));
return {};
}

std::vector<std::unique_ptr<PasswordForm>> obtained_forms;
obtained_forms.reserve(key_to_form_map.size());
for (auto& pair : key_to_form_map) {
obtained_forms.push_back(std::move(pair.second));
}
metrics_recorder.RecordMetrics(/*success=*/true, /*error=*/absl::nullopt);
return obtained_forms;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
#include "base/memory/scoped_refptr.h"
#include "base/memory/weak_ptr.h"
#include "components/password_manager/core/browser/password_store_backend.h"
#include "components/password_manager/core/browser/password_store_backend_metrics_recorder.h"
#include "components/password_manager/core/browser/password_store_sync.h"

namespace syncer {
Expand Down Expand Up @@ -41,7 +42,8 @@ class LoginDatabaseAsyncHelper : private PasswordStoreSync {
base::RepeatingClosure sync_enabled_or_disabled_cb);

// Synchronous implementation of PasswordStoreBackend interface.
LoginsResult GetAllLogins();
LoginsResult GetAllLogins(
PasswordStoreBackendMetricsRecorder metrics_recorder);
LoginsResult GetAutofillableLogins();
LoginsResult FillMatchingLogins(const std::vector<PasswordFormDigest>& forms,
bool include_psl);
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,66 @@
// Copyright 2022 The Chromium Authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.

#include "components/password_manager/core/browser/password_store_backend_metrics_recorder.h"

namespace password_manager {

PasswordStoreBackendMetricsRecorder::PasswordStoreBackendMetricsRecorder() =
default;

PasswordStoreBackendMetricsRecorder::PasswordStoreBackendMetricsRecorder(
ClassInfix class_infix,
MetricInfix metric_infix)
: class_infix_(std::move(class_infix)),
metric_infix_(std::move(metric_infix)) {}

PasswordStoreBackendMetricsRecorder::PasswordStoreBackendMetricsRecorder(
PasswordStoreBackendMetricsRecorder&&) = default;

PasswordStoreBackendMetricsRecorder& PasswordStoreBackendMetricsRecorder::
PasswordStoreBackendMetricsRecorder::operator=(
PasswordStoreBackendMetricsRecorder&&) = default;

PasswordStoreBackendMetricsRecorder::~PasswordStoreBackendMetricsRecorder() =
default;

void PasswordStoreBackendMetricsRecorder::RecordMetrics(
bool success,
absl::optional<ErrorFromPasswordStoreOrAndroidBackend> error) const {
auto BuildMetricName = [this](base::StringPiece suffix) {
return base::StrCat(
{"PasswordManager.", *class_infix_, ".", *metric_infix_, ".", suffix});
};
base::TimeDelta duration = base::Time::Now() - start_;
base::UmaHistogramMediumTimes(BuildMetricName("Latency"), duration);
base::UmaHistogramBoolean(BuildMetricName("Success"), success);
if (!error.has_value())
return;

DCHECK(!success);
ErrorFromPasswordStoreOrAndroidBackend error_variant =
std::move(error.value());
// In case of AndroidBackend error, we report additional metrics.
if (absl::holds_alternative<AndroidBackendError>(error_variant)) {
AndroidBackendError backend_error = std::move(absl::get<1>(error_variant));
base::UmaHistogramEnumeration(
"PasswordManager.PasswordStoreAndroidBackend.ErrorCode",
backend_error.type);
base::UmaHistogramEnumeration(BuildMetricName("ErrorCode"),
backend_error.type);
if (backend_error.type == AndroidBackendErrorType::kExternalError) {
DCHECK(backend_error.api_error_code.has_value());
base::HistogramBase* histogram = base::SparseHistogram::FactoryGet(
"PasswordManager.PasswordStoreAndroidBackend.APIError",
base::HistogramBase::kUmaTargetedHistogramFlag);
histogram->Add(backend_error.api_error_code.value());
histogram = base::SparseHistogram::FactoryGet(
BuildMetricName("APIError"),
base::HistogramBase::kUmaTargetedHistogramFlag);
histogram->Add(backend_error.api_error_code.value());
}
}
}

} // namespace password_manager

0 comments on commit 0a8dc25

Please sign in to comment.