Login: Introduce MetricsReporter to centralize UMA tracking

In the current login flow, reporting metrics are sparsely located. We would like to centralize the metrics reporting to make changes in metrics implementation simpler in the future. Bug: 1306417 Change-Id: I1a491108f0795b32b6ceae34b96b2355f93352c1 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3528787 Reviewed-by: Denis Kuznetsov <antrim@chromium.org> Reviewed-by: Ben Franz <bfranz@chromium.org> Commit-Queue: Sherri Lin <sherrilin@google.com> Cr-Commit-Position: refs/heads/main@{#988418}
chromium · Apr 4, 2022 · a593268 · a593268
1 parent 9b067ff
commit a593268
Show file tree

Hide file tree

Showing 12 changed files with 124 additions and 24 deletions.
diff --git a/ash/components/login/auth/BUILD.gn b/ash/components/login/auth/BUILD.gn
@@ -69,6 +69,8 @@ component("auth") {
     "key.h",
     "login_performer.cc",
     "login_performer.h",
+    "metrics_recorder.cc",
+    "metrics_recorder.h",
     "password_visibility_utils.cc",
     "password_visibility_utils.h",
     "safe_mode_delegate.h",

diff --git a/ash/components/login/auth/login_performer.cc b/ash/components/login/auth/login_performer.cc
@@ -4,12 +4,10 @@
 
 #include "ash/components/login/auth/login_performer.h"
 
+#include "ash/components/login/auth/metrics_recorder.h"
 #include "base/bind.h"
 #include "base/location.h"
 #include "base/logging.h"
-#include "base/metrics/histogram_macros.h"
-#include "base/metrics/user_metrics.h"
-#include "base/metrics/user_metrics_action.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/threading/thread_restrictions.h"
 #include "base/threading/thread_task_runner_handle.h"
@@ -20,13 +18,15 @@
 #include "components/user_manager/user_names.h"
 #include "google_apis/gaia/gaia_auth_util.h"
 
-using base::UserMetricsAction;
-
 namespace ash {
 
-LoginPerformer::LoginPerformer(Delegate* delegate)
+LoginPerformer::LoginPerformer(Delegate* delegate,
+                               MetricsRecorder* metrics_recorder)
     : delegate_(delegate),
-      last_login_failure_(AuthFailure::AuthFailureNone()) {}
+      metrics_recorder_(metrics_recorder),
+      last_login_failure_(AuthFailure::AuthFailureNone()) {
+  DCHECK(metrics_recorder_);
+}
 
 LoginPerformer::~LoginPerformer() {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
@@ -40,11 +40,8 @@ LoginPerformer::~LoginPerformer() {
 
 void LoginPerformer::OnAuthFailure(const AuthFailure& failure) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-  base::RecordAction(UserMetricsAction("Login_Failure"));
 
-  UMA_HISTOGRAM_ENUMERATION("Login.FailureReason",
-                            failure.reason(),
-                            AuthFailure::NUM_FAILURE_REASONS);
+  metrics_recorder_->OnAuthFailure(failure.reason());
 
   LOG(ERROR) << "Login failure, reason=" << failure.reason()
              << ", error.state=" << failure.error().state();
@@ -57,11 +54,9 @@ void LoginPerformer::OnAuthFailure(const AuthFailure& failure) {
 
 void LoginPerformer::OnAuthSuccess(const UserContext& user_context) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-  base::RecordAction(UserMetricsAction("Login_Success"));
 
   // Do not distinguish between offline and online success.
-  UMA_HISTOGRAM_ENUMERATION("Login.SuccessReason", OFFLINE_AND_ONLINE,
-                            NUM_SUCCESS_REASONS);
+  metrics_recorder_->OnLoginSuccess(OFFLINE_AND_ONLINE);
 
   VLOG(1) << "LoginSuccess hash: " << user_context.GetUserIDHash();
   base::SequencedTaskRunnerHandle::Get()->PostTask(
@@ -71,7 +66,7 @@ void LoginPerformer::OnAuthSuccess(const UserContext& user_context) {
 
 void LoginPerformer::OnOffTheRecordAuthSuccess() {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-  base::RecordAction(UserMetricsAction("Login_GuestLoginSuccess"));
+  metrics_recorder_->OnGuestLoignSuccess();
 
   base::SequencedTaskRunnerHandle::Get()->PostTask(
       FROM_HERE, base::BindOnce(&LoginPerformer::NotifyOffTheRecordAuthSuccess,

diff --git a/ash/components/login/auth/login_performer.h b/ash/components/login/auth/login_performer.h
@@ -11,9 +11,11 @@
 #include "ash/components/login/auth/auth_status_consumer.h"
 #include "ash/components/login/auth/authenticator.h"
 #include "ash/components/login/auth/extended_authenticator.h"
+#include "ash/components/login/auth/metrics_recorder.h"
 #include "ash/components/login/auth/user_context.h"
 #include "base/callback.h"
 #include "base/component_export.h"
+#include "base/memory/raw_ptr.h"
 #include "base/memory/weak_ptr.h"
 #include "base/sequence_checker.h"
 #include "components/user_manager/user_type.h"
@@ -55,7 +57,8 @@ class COMPONENT_EXPORT(ASH_LOGIN_AUTH) LoginPerformer
     virtual void PolicyLoadFailed() = 0;
   };
 
-  explicit LoginPerformer(Delegate* delegate);
+  explicit LoginPerformer(Delegate* delegate,
+                          MetricsRecorder* metrics_recorder);
 
   LoginPerformer(const LoginPerformer&) = delete;
   LoginPerformer& operator=(const LoginPerformer&) = delete;
@@ -191,6 +194,9 @@ class COMPONENT_EXPORT(ASH_LOGIN_AUTH) LoginPerformer
   // Used for logging in.
   scoped_refptr<Authenticator> authenticator_;
 
+  // Used for metric reporting.
+  const raw_ptr<MetricsRecorder> metrics_recorder_;
+
   // Represents last login failure that was encountered when communicating to
   // sign-in server. AuthFailure.LoginFailureNone() by default.
   AuthFailure last_login_failure_;

diff --git a/ash/components/login/auth/metrics_recorder.cc b/ash/components/login/auth/metrics_recorder.cc
@@ -0,0 +1,43 @@
+// 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 "ash/components/login/auth/metrics_recorder.h"
+
+#include "ash/components/login/auth/auth_status_consumer.h"
+#include "base/metrics/histogram_macros.h"
+#include "base/metrics/user_metrics.h"
+#include "base/metrics/user_metrics_action.h"
+
+namespace ash {
+namespace {
+
+// Histogram for tracking the reason of auth failure
+constexpr char kFailureReasonHistogramName[] = "Login.FailureReason";
+
+// Histogram for tracking the reason of login success
+constexpr char kSuccessReasonHistogramName[] = "Login.SuccessReason";
+
+}  // namespace
+
+MetricsRecorder::MetricsRecorder() {}
+
+MetricsRecorder::~MetricsRecorder() = default;
+
+void MetricsRecorder::OnAuthFailure(const AuthFailure::FailureReason& reason) {
+  base::RecordAction(base::UserMetricsAction("Login_Failure"));
+  UMA_HISTOGRAM_ENUMERATION(kFailureReasonHistogramName, reason,
+                            AuthFailure::NUM_FAILURE_REASONS);
+}
+
+void MetricsRecorder::OnLoginSuccess(const SuccessReason& reason) {
+  base::RecordAction(base::UserMetricsAction("Login_Success"));
+  UMA_HISTOGRAM_ENUMERATION(kSuccessReasonHistogramName, reason,
+                            SuccessReason::NUM_SUCCESS_REASONS);
+}
+
+void MetricsRecorder::OnGuestLoignSuccess() {
+  base::RecordAction(base::UserMetricsAction("Login_GuestLoginSuccess"));
+}
+
+}  // namespace ash
diff --git a/ash/components/login/auth/metrics_recorder.h b/ash/components/login/auth/metrics_recorder.h
@@ -0,0 +1,37 @@
+// 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.
+
+#ifndef ASH_COMPONENTS_LOGIN_AUTH_METRICS_RECORDER_H_
+#define ASH_COMPONENTS_LOGIN_AUTH_METRICS_RECORDER_H_
+
+#include "ash/components/login/auth/auth_status_consumer.h"
+
+namespace ash {
+
+// This class encapsulates metrics reporting. User actions and behaviors are
+// reported in multiple stages of the login flow. This metrics reporter would
+// centralize the tracking and reporting.
+class COMPONENT_EXPORT(ASH_LOGIN_AUTH) MetricsRecorder {
+ public:
+  // Reports various metrics during the login flow.
+  MetricsRecorder();
+  MetricsRecorder(const MetricsRecorder&) = delete;
+  MetricsRecorder& operator=(const MetricsRecorder&) = delete;
+  MetricsRecorder(MetricsRecorder&&) = delete;
+  MetricsRecorder& operator=(MetricsRecorder&&) = delete;
+  ~MetricsRecorder();
+
+  // Logs the auth failure action and reason.
+  void OnAuthFailure(const AuthFailure::FailureReason& failure_reason);
+
+  // Logs the login success action and reason.
+  void OnLoginSuccess(const SuccessReason& reason);
+
+  // Logs the guest login success action.
+  void OnGuestLoignSuccess();
+};
+
+}  // namespace ash
+
+#endif  // ASH_COMPONENTS_LOGIN_AUTH_METRICS_RECORDER_H_
diff --git a/chrome/browser/ash/app_mode/kiosk_profile_loader.cc b/chrome/browser/ash/app_mode/kiosk_profile_loader.cc
@@ -21,6 +21,7 @@
 #include "chrome/browser/ash/app_mode/kiosk_app_manager.h"
 #include "chrome/browser/ash/app_mode/kiosk_app_types.h"
 #include "chrome/browser/ash/login/auth/chrome_login_performer.h"
+#include "chrome/browser/ash/login/ui/login_display_host.h"
 #include "chrome/browser/lifetime/application_lifetime.h"
 #include "chromeos/dbus/dbus_thread_manager.h"
 #include "chromeos/dbus/userdataauth/userdataauth_client.h"
@@ -154,7 +155,8 @@ void KioskProfileLoader::Start() {
 }
 
 void KioskProfileLoader::LoginAsKioskAccount() {
-  login_performer_ = std::make_unique<ChromeLoginPerformer>(this);
+  login_performer_ = std::make_unique<ChromeLoginPerformer>(
+      this, LoginDisplayHost::default_host()->metrics_recorder());
   switch (app_type_) {
     case KioskAppType::kArcApp:
       login_performer_->LoginAsArcKioskAccount(account_id_);

diff --git a/chrome/browser/ash/login/auth/chrome_login_performer.cc b/chrome/browser/ash/login/auth/chrome_login_performer.cc
@@ -22,8 +22,9 @@
 
 namespace ash {
 
-ChromeLoginPerformer::ChromeLoginPerformer(Delegate* delegate)
-    : LoginPerformer(delegate) {}
+ChromeLoginPerformer::ChromeLoginPerformer(Delegate* delegate,
+                                           MetricsRecorder* metrics_recorder)
+    : LoginPerformer(delegate, metrics_recorder) {}
 
 ChromeLoginPerformer::~ChromeLoginPerformer() {}
 

diff --git a/chrome/browser/ash/login/auth/chrome_login_performer.h b/chrome/browser/ash/login/auth/chrome_login_performer.h
@@ -12,6 +12,7 @@
 #include "ash/components/login/auth/authenticator.h"
 #include "ash/components/login/auth/extended_authenticator.h"
 #include "ash/components/login/auth/login_performer.h"
+#include "ash/components/login/auth/metrics_recorder.h"
 #include "ash/components/login/auth/user_context.h"
 #include "base/memory/weak_ptr.h"
 #include "chrome/browser/ash/policy/login/wildcard_login_checker.h"
@@ -33,7 +34,8 @@ namespace ash {
 
 class ChromeLoginPerformer : public LoginPerformer {
  public:
-  explicit ChromeLoginPerformer(Delegate* delegate);
+  explicit ChromeLoginPerformer(Delegate* delegate,
+                                MetricsRecorder* metrics_recorder);
 
   ChromeLoginPerformer(const ChromeLoginPerformer&) = delete;
   ChromeLoginPerformer& operator=(const ChromeLoginPerformer&) = delete;

diff --git a/chrome/browser/ash/login/existing_user_controller.cc b/chrome/browser/ash/login/existing_user_controller.cc
@@ -598,7 +598,8 @@ void ExistingUserController::PerformLogin(
   if (!login_performer_.get() || num_login_attempts_ <= 1) {
     // Only one instance of LoginPerformer should exist at a time.
     login_performer_.reset(nullptr);
-    login_performer_ = std::make_unique<ChromeLoginPerformer>(this);
+    login_performer_ = std::make_unique<ChromeLoginPerformer>(
+        this, GetLoginDisplayHost()->metrics_recorder());
   }
   if (IsActiveDirectoryManaged() &&
       user_context.GetUserType() != user_manager::USER_TYPE_ACTIVE_DIRECTORY) {
@@ -1227,7 +1228,8 @@ void ExistingUserController::LoginAsGuest() {
 
   // Only one instance of LoginPerformer should exist at a time.
   login_performer_.reset(nullptr);
-  login_performer_ = std::make_unique<ChromeLoginPerformer>(this);
+  login_performer_ = std::make_unique<ChromeLoginPerformer>(
+      this, GetLoginDisplayHost()->metrics_recorder());
   login_performer_->LoginOffTheRecord();
   SendAccessibilityAlert(
       l10n_util::GetStringUTF8(IDS_CHROMEOS_ACC_LOGIN_SIGNIN_OFFRECORD));
@@ -1521,7 +1523,8 @@ void ExistingUserController::LoginAsPublicSessionInternal(
   VLOG(2) << "LoginAsPublicSessionInternal for user: "
           << user_context.GetAccountId();
   login_performer_.reset(nullptr);
-  login_performer_ = std::make_unique<ChromeLoginPerformer>(this);
+  login_performer_ = std::make_unique<ChromeLoginPerformer>(
+      this, GetLoginDisplayHost()->metrics_recorder());
   login_performer_->LoginAsPublicSession(user_context);
   SendAccessibilityAlert(
       l10n_util::GetStringUTF8(IDS_CHROMEOS_ACC_LOGIN_SIGNIN_PUBLIC_ACCOUNT));

diff --git a/chrome/browser/ash/login/screens/error_screen.cc b/chrome/browser/ash/login/screens/error_screen.cc
@@ -413,7 +413,8 @@ void ErrorScreen::StartGuestSessionAfterOwnershipCheck(
   if (guest_login_performer_)
     return;
 
-  guest_login_performer_ = std::make_unique<ChromeLoginPerformer>(this);
+  guest_login_performer_ = std::make_unique<ChromeLoginPerformer>(
+      this, LoginDisplayHost::default_host()->metrics_recorder());
   guest_login_performer_->LoginOffTheRecord();
 }
 

diff --git a/chrome/browser/ash/login/ui/login_display_host.cc b/chrome/browser/ash/login/ui/login_display_host.cc
@@ -14,6 +14,7 @@ LoginDisplayHost* LoginDisplayHost::default_host_ = nullptr;
 LoginDisplayHost::LoginDisplayHost() {
   DCHECK(default_host() == nullptr);
   default_host_ = this;
+  metrics_recorder_ = std::make_unique<MetricsRecorder>();
 }
 
 LoginDisplayHost::~LoginDisplayHost() {

diff --git a/chrome/browser/ash/login/ui/login_display_host.h b/chrome/browser/ash/login/ui/login_display_host.h
@@ -38,6 +38,7 @@ class Rect;
 namespace ash {
 class KioskAppId;
 class KioskLaunchController;
+class MetricsRecorder;
 class WebUILoginView;
 class WizardController;
 enum class OobeDialogState;
@@ -75,6 +76,9 @@ class LoginDisplayHost {
   // Returns the default LoginDisplayHost instance if it has been created.
   static LoginDisplayHost* default_host() { return default_host_; }
 
+  // Returns an owned pointer to the MetricsRecorder instance.
+  MetricsRecorder* metrics_recorder() { return metrics_recorder_.get(); }
+
   // Returns an unowned pointer to the LoginDisplay instance.
   virtual LoginDisplay* GetLoginDisplay() = 0;
 
@@ -267,6 +271,9 @@ class LoginDisplayHost {
   // Global LoginDisplayHost instance.
   static LoginDisplayHost* default_host_;
 
+  // Owned pointer to MetricsRecorder instance.
+  std::unique_ptr<MetricsRecorder> metrics_recorder_;
+
   // Callback to be executed when WebUI is started.
   base::RepeatingClosure on_wizard_controller_created_for_tests_;
 };