Skip to content

Commit

Permalink
Add a histogram for Advanced Protection enabled status
Browse files Browse the repository at this point in the history 
We presently don't have a good metric to understand the popularity
of AP users. This CL adds a new histogram to track the enabled/disabled
status for users.

Bug: 1414633
Change-Id: Ie2d5f6efdf1bfb10f8dda8904f0372061901a1da
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4237273
Reviewed-by: Daniel Rubery <drubery@chromium.org>
Commit-Queue: Mustafa Emre Acer <meacer@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1104149}
  • Loading branch information
@meacer
meacer authored and Chromium LUCI CQ committed Feb 11, 2023
1 parent be1aec6 commit 0c1024d
Show file tree
Hide file tree
Showing 5 changed files with 131 additions and 1 deletion.
15 changes: 14 additions & 1 deletion chrome/browser/safe_browsing/advanced_protection_status_manager.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
#include "base/command_line.h"
#include "base/feature_list.h"
#include "base/functional/bind.h"
#include "base/metrics/histogram_functions.h"
#include "base/metrics/histogram_macros.h"
#include "chrome/browser/profiles/profile.h"
#include "chrome/browser/safe_browsing/advanced_protection_status_manager_factory.h"
Expand Down Expand Up @@ -38,6 +39,11 @@ const base::TimeDelta kMinimumRefreshDelay = base::Minutes(1);
const char kForceTreatUserAsAdvancedProtection[] =
"safe-browsing-treat-user-as-advanced-protection";

void RecordUMA(AdvancedProtectionStatusManager::UmaEvent event) {
base::UmaHistogramEnumeration("SafeBrowsing.AdvancedProtection.Enabled",
event);
}

} // namespace

////////////////////////////////////////////////////////////////////////////////
Expand All @@ -62,6 +68,8 @@ void AdvancedProtectionStatusManager::MaybeRefreshOnStartUp() {
return;

is_under_advanced_protection_ = core_info.is_under_advanced_protection;
RecordUMA(is_under_advanced_protection_ ? UmaEvent::kEnabled
: UmaEvent::kDisabled);

if (pref_service_->HasPrefPath(prefs::kAdvancedProtectionLastRefreshInUs)) {
last_refreshed_ = base::Time::FromDeltaSinceWindowsEpoch(base::Microseconds(
Expand Down Expand Up @@ -118,7 +126,6 @@ void AdvancedProtectionStatusManager::OnExtendedAccountInfoRemoved(
GetUnconsentedPrimaryAccountId();
if (!unconsented_primary_account_id.empty() &&
unconsented_primary_account_id == info.account_id) {
is_under_advanced_protection_ = false;
OnAdvancedProtectionDisabled();
}
}
Expand All @@ -144,12 +151,18 @@ void AdvancedProtectionStatusManager::OnPrimaryAccountChanged(
}

void AdvancedProtectionStatusManager::OnAdvancedProtectionEnabled() {
if (!is_under_advanced_protection_) {
RecordUMA(UmaEvent::kEnabledAfterDisabled);
}
is_under_advanced_protection_ = true;
UpdateLastRefreshTime();
ScheduleNextRefresh();
}

void AdvancedProtectionStatusManager::OnAdvancedProtectionDisabled() {
if (is_under_advanced_protection_) {
RecordUMA(UmaEvent::kDisabledAfterEnabled);
}
is_under_advanced_protection_ = false;
UpdateLastRefreshTime();
CancelFutureRefresh();
Expand Down
22 changes: 22 additions & 0 deletions chrome/browser/safe_browsing/advanced_protection_status_manager.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,28 @@ class AdvancedProtectionStatusManager
: public KeyedService,
public signin::IdentityManager::Observer {
public:
// Tracks Advanced Protection status. Might be recorded multiple times for
// the same user. Recorded in histograms, do not reorder or delete items.
enum class UmaEvent {
kNone = 0,
// Advanced Protection is disabled. Either the user hasn't signed in or
// the signed-in user isn't under AP.
kDisabled = 1,
// Advanced Protection is enabled for the signed-in user.
kEnabled = 2,
// Advanced Protection was enabled for the user, but is now disabled.
// This is only recorded within a browser session and is only a rough count.
// If the user unenrolls from AP and closes Chrome before the next refresh,
// we'll record kDisabled on startup instead of kDisabledAfterEnabled.
kDisabledAfterEnabled = 3,
// Advanced Protection was disabled for the user, but is now enabled.
// Also a rough count. If the user enrolls to AP and closes Chrome before
// the next refresh, we'll record kEnabled on startup instead of
// kEnabledAfterDisabled.
kEnabledAfterDisabled = 4,
kMaxValue = kEnabledAfterDisabled,
};

AdvancedProtectionStatusManager(PrefService* pref_service,
signin::IdentityManager* identity_manager);

Expand Down
72 changes: 72 additions & 0 deletions chrome/browser/safe_browsing/advanced_protection_status_manager_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@
#include "testing/gmock/include/gmock/gmock.h"
#include "testing/gtest/include/gtest/gtest.h"

using UmaEvent = safe_browsing::AdvancedProtectionStatusManager::UmaEvent;

namespace safe_browsing {
namespace {

Expand All @@ -31,6 +33,8 @@ static const char* kIdTokenAdvancedProtectionDisabled =
"eyAic2VydmljZXMiOiBbXSB9" // payload: { "services": [] }
".dummy-signature";

static const char* kAPEnabledMetric = "SafeBrowsing.AdvancedProtection.Enabled";

// Helper class that ensure RegisterProfilePrefs() is called on the test
// PrefService's registry before the IdentityTestEnvironment constructor
// is invoked.
Expand Down Expand Up @@ -86,6 +90,8 @@ class AdvancedProtectionStatusManagerTest : public TestWithPrefService {
} // namespace

TEST_F(AdvancedProtectionStatusManagerTest, NotSignedInOnStartUp) {
base::HistogramTester histograms;

ASSERT_FALSE(
pref_service_.HasPrefPath(prefs::kAdvancedProtectionLastRefreshInUs));
AdvancedProtectionStatusManager aps_manager(
Expand All @@ -99,6 +105,8 @@ TEST_F(AdvancedProtectionStatusManagerTest, NotSignedInOnStartUp) {
EXPECT_FALSE(
pref_service_.HasPrefPath(prefs::kAdvancedProtectionLastRefreshInUs));
aps_manager.UnsubscribeFromSigninEvents();

EXPECT_THAT(histograms.GetAllSamples(kAPEnabledMetric), testing::IsEmpty());
}

TEST_F(AdvancedProtectionStatusManagerTest,
Expand Down Expand Up @@ -128,6 +136,9 @@ TEST_F(AdvancedProtectionStatusManagerTest,
EXPECT_FALSE(
pref_service_.HasPrefPath(prefs::kAdvancedProtectionLastRefreshInUs));
aps_manager.UnsubscribeFromSigninEvents();

EXPECT_THAT(histograms.GetAllSamples(kAPEnabledMetric),
testing::ElementsAre(base::Bucket(UmaEvent::kDisabled, 1)));
}

TEST_F(AdvancedProtectionStatusManagerTest,
Expand All @@ -152,6 +163,9 @@ TEST_F(AdvancedProtectionStatusManagerTest,
// No retry should be scheduled.
EXPECT_FALSE(aps_manager.IsRefreshScheduled());
aps_manager.UnsubscribeFromSigninEvents();

EXPECT_THAT(histograms.GetAllSamples(kAPEnabledMetric),
testing::ElementsAre(base::Bucket(UmaEvent::kDisabled, 1)));
}

TEST_F(AdvancedProtectionStatusManagerTest, SignedInLongTimeAgoNotUnderAP) {
Expand All @@ -178,9 +192,14 @@ TEST_F(AdvancedProtectionStatusManagerTest, SignedInLongTimeAgoNotUnderAP) {
pref_service_.HasPrefPath(prefs::kAdvancedProtectionLastRefreshInUs));

aps_manager.UnsubscribeFromSigninEvents();

EXPECT_THAT(histograms.GetAllSamples(kAPEnabledMetric),
testing::ElementsAre(base::Bucket(UmaEvent::kDisabled, 1)));
}

TEST_F(AdvancedProtectionStatusManagerTest, SignedInLongTimeAgoUnderAP) {
base::HistogramTester histograms;

// Simulates the situation where user signed in long time ago, thus
// has no advanced protection status yet.
CoreAccountId account_id = SignIn("test@test.com",
Expand All @@ -199,9 +218,16 @@ TEST_F(AdvancedProtectionStatusManagerTest, SignedInLongTimeAgoUnderAP) {
EXPECT_TRUE(
pref_service_.HasPrefPath(prefs::kAdvancedProtectionLastRefreshInUs));
aps_manager.UnsubscribeFromSigninEvents();

EXPECT_THAT(
histograms.GetAllSamples(kAPEnabledMetric),
testing::ElementsAre(base::Bucket(UmaEvent::kDisabled, 1),
base::Bucket(UmaEvent::kEnabledAfterDisabled, 1)));
}

TEST_F(AdvancedProtectionStatusManagerTest, AlreadySignedInAndUnderAP) {
base::HistogramTester histograms;

pref_service_.SetInt64(
prefs::kAdvancedProtectionLastRefreshInUs,
base::Time::Now().ToDeltaSinceWindowsEpoch().InMicroseconds());
Expand All @@ -219,9 +245,14 @@ TEST_F(AdvancedProtectionStatusManagerTest, AlreadySignedInAndUnderAP) {
// A refresh is scheduled in the future.
EXPECT_TRUE(aps_manager.IsRefreshScheduled());
aps_manager.UnsubscribeFromSigninEvents();

EXPECT_THAT(histograms.GetAllSamples(kAPEnabledMetric),
testing::ElementsAre(base::Bucket(UmaEvent::kEnabled, 1)));
}

TEST_F(AdvancedProtectionStatusManagerTest, AlreadySignedInAndNotUnderAP) {
base::HistogramTester histograms;

pref_service_.SetInt64(
prefs::kAdvancedProtectionLastRefreshInUs,
base::Time::Now().ToDeltaSinceWindowsEpoch().InMicroseconds());
Expand All @@ -238,9 +269,14 @@ TEST_F(AdvancedProtectionStatusManagerTest, AlreadySignedInAndNotUnderAP) {
// original profile.
EXPECT_FALSE(aps_manager.IsUnderAdvancedProtection());
aps_manager.UnsubscribeFromSigninEvents();

EXPECT_THAT(histograms.GetAllSamples(kAPEnabledMetric),
testing::ElementsAre(base::Bucket(UmaEvent::kDisabled, 1)));
}

TEST_F(AdvancedProtectionStatusManagerTest, StayInAdvancedProtection) {
base::HistogramTester histograms;

base::Time last_update = base::Time::Now();
pref_service_.SetInt64(
prefs::kAdvancedProtectionLastRefreshInUs,
Expand All @@ -262,11 +298,16 @@ TEST_F(AdvancedProtectionStatusManagerTest, StayInAdvancedProtection) {
last_update);
EXPECT_TRUE(aps_manager.IsRefreshScheduled());
aps_manager.UnsubscribeFromSigninEvents();

EXPECT_THAT(histograms.GetAllSamples(kAPEnabledMetric),
testing::ElementsAre(base::Bucket(UmaEvent::kEnabled, 1)));
}

#if !BUILDFLAG(IS_CHROMEOS_ASH)
// Not applicable to Chrome OS.
TEST_F(AdvancedProtectionStatusManagerTest, SignInAndSignOutEvent) {
base::HistogramTester histograms;

AdvancedProtectionStatusManager aps_manager(
&pref_service_, identity_test_env_.identity_manager(),
base::TimeDelta() /*no min delay*/);
Expand All @@ -284,10 +325,17 @@ TEST_F(AdvancedProtectionStatusManagerTest, SignInAndSignOutEvent) {
pref_service_.HasPrefPath(prefs::kAdvancedProtectionLastRefreshInUs));
EXPECT_FALSE(aps_manager.IsRefreshScheduled());
aps_manager.UnsubscribeFromSigninEvents();

EXPECT_THAT(
histograms.GetAllSamples(kAPEnabledMetric),
testing::ElementsAre(base::Bucket(UmaEvent::kDisabledAfterEnabled, 1),
base::Bucket(UmaEvent::kEnabledAfterDisabled, 1)));
}
#endif

TEST_F(AdvancedProtectionStatusManagerTest, AccountRemoval) {
base::HistogramTester histograms;

AdvancedProtectionStatusManager aps_manager(
&pref_service_, identity_test_env_.identity_manager(),
base::TimeDelta() /*no min delay*/);
Expand Down Expand Up @@ -318,10 +366,17 @@ TEST_F(AdvancedProtectionStatusManagerTest, AccountRemoval) {
pref_service_.HasPrefPath(prefs::kAdvancedProtectionLastRefreshInUs));
EXPECT_FALSE(aps_manager.IsRefreshScheduled());
aps_manager.UnsubscribeFromSigninEvents();

EXPECT_THAT(
histograms.GetAllSamples(kAPEnabledMetric),
testing::ElementsAre(base::Bucket(UmaEvent::kDisabledAfterEnabled, 1),
base::Bucket(UmaEvent::kEnabledAfterDisabled, 1)));
}

TEST_F(AdvancedProtectionStatusManagerTest,
AdvancedProtectionDisabledAfterSignin) {
base::HistogramTester histograms;

AdvancedProtectionStatusManager aps_manager(
&pref_service_, identity_test_env_.identity_manager(),
base::TimeDelta() /*no min delay*/);
Expand All @@ -345,10 +400,16 @@ TEST_F(AdvancedProtectionStatusManagerTest,
EXPECT_FALSE(aps_manager.IsRefreshScheduled());

aps_manager.UnsubscribeFromSigninEvents();

EXPECT_THAT(
histograms.GetAllSamples(kAPEnabledMetric),
testing::ElementsAre(base::Bucket(UmaEvent::kDisabledAfterEnabled, 1),
base::Bucket(UmaEvent::kEnabledAfterDisabled, 1)));
}

TEST_F(AdvancedProtectionStatusManagerTest,
StartupAfterLongWaitRefreshesImmediately) {
base::HistogramTester histograms;
CoreAccountId account_id = SignIn("test@test.com",
/* is_under_advanced_protection = */ true);
base::RunLoop().RunUntilIdle();
Expand All @@ -372,12 +433,19 @@ TEST_F(AdvancedProtectionStatusManagerTest,
EXPECT_FALSE(aps_manager.IsRefreshScheduled());

aps_manager.UnsubscribeFromSigninEvents();

EXPECT_THAT(
histograms.GetAllSamples(kAPEnabledMetric),
testing::ElementsAre(base::Bucket(UmaEvent::kEnabled, 1),
base::Bucket(UmaEvent::kDisabledAfterEnabled, 1)));
}

// On ChromeOS, there is no unconsented primary account. We can only track the
// primary account.
#if !BUILDFLAG(IS_CHROMEOS_ASH)
TEST_F(AdvancedProtectionStatusManagerTest, TracksUnconsentedPrimaryAccount) {
base::HistogramTester histograms;

AdvancedProtectionStatusManager aps_manager(
&pref_service_, identity_test_env_.identity_manager(),
base::TimeDelta() /*no min delay*/);
Expand All @@ -394,6 +462,10 @@ TEST_F(AdvancedProtectionStatusManagerTest, TracksUnconsentedPrimaryAccount) {
EXPECT_TRUE(aps_manager.IsRefreshScheduled());

aps_manager.UnsubscribeFromSigninEvents();

EXPECT_THAT(
histograms.GetAllSamples(kAPEnabledMetric),
testing::ElementsAre(base::Bucket(UmaEvent::kEnabledAfterDisabled, 1)));
}
#endif

Expand Down
11 changes: 11 additions & 0 deletions tools/metrics/histograms/enums.xml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1641,6 +1641,17 @@ Unknown properties are collapsed to zero. -->
<int value="1" label="Received activation"/>
</enum>

<enum name="AdvancedProtectionEnabledStatus">
<int value="0" label="Advanced Protection enabled status unknown"/>
<int value="1" label="User not signed-in or isn't under Advanced Protection"/>
<int value="2" label="User is signed in and under Advanced Protection"/>
<int value="3"
label="User was under Advanced Protection but disabled it or signed out"/>
<int value="4"
label="User was signed out or wasn't under Advanced Protection but now
is"/>
</enum>

<enum name="AffiliationFetchResult">
<int value="0" label="Success"/>
<int value="1" label="Network/server error"/>
Expand Down
12 changes: 12 additions & 0 deletions tools/metrics/histograms/metadata/safe_browsing/histograms.xml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -134,6 +134,18 @@ chromium-metrics-reviews@google.com.
summary="url allowlist on client side phishing detection"/>
</variants>

<histogram name="SafeBrowsing.AdvancedProtection.Enabled"
enum="AdvancedProtectionEnabledStatus" expires_after="2023-12-01">
<owner>meacer@chromium.org</owner>
<owner>trusty-transport@chromium.org</owner>
<summary>
Records whether the user has Advanced Protection enabled. Logged on startup
when the enabled status changes. Can be logged multiple times for a user. In
practice, this counts the number of Chrome startups with Advanced Protection
enabled/disabled, rather than the actual user count.
</summary>
</histogram>

<histogram
name="SafeBrowsing.Android.RealTimeAllowlist.InstallerLoadFromDiskPbFileEmpty"
units="bool" expires_after="2023-10-26">
Expand Down

0 comments on commit 0c1024d

Please sign in to comment.