Commit baaf1937 authored by Nate Fischer's avatar Nate Fischer Committed by Commit Bot

AW UMA: refactor package name sampling

This moves the package name sampling result into a boolean member
variable (which slightly improves performance, and is more consistent
with is_in_sample_), and rephrases a comment.

This fixes some tests to mock the IsInPackageNameSample() value prior to
MaybeStartMetrics, rather than after, since the value is cached during
that method.

This is a follow up to feedback on https://crrev.com/c/1898711.

Test: run_android_webview_unittests --gtest_filter=AwMetricsServiceClientTest.*
Change-Id: I824540aa7362ab34f9cb4981b88cc446338bdff3
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1937704Reviewed-by: default avatarTao Bai <michaelbai@chromium.org>
Commit-Queue: Nate Fischer <ntfschr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#719429}
parent d9059b9e
...@@ -59,7 +59,7 @@ const double kStableSampledInRate = 0.02; ...@@ -59,7 +59,7 @@ const double kStableSampledInRate = 0.02;
const double kBetaDevCanarySampledInRate = 0.99; const double kBetaDevCanarySampledInRate = 0.99;
// As a mitigation to preserve use privacy, the privacy team has asked that we // As a mitigation to preserve use privacy, the privacy team has asked that we
// upload package name with no more than 10% of UMA records. This is to mitigate // upload package name with no more than 10% of UMA clients. This is to mitigate
// fingerprinting for users on low-usage applications (if an app only has a // fingerprinting for users on low-usage applications (if an app only has a
// a small handful of users, there's a very good chance many of them won't be // a small handful of users, there's a very good chance many of them won't be
// uploading UMA records due to sampling). Do not change this constant without // uploading UMA records due to sampling). Do not change this constant without
...@@ -190,6 +190,7 @@ void AwMetricsServiceClient::MaybeStartMetrics() { ...@@ -190,6 +190,7 @@ void AwMetricsServiceClient::MaybeStartMetrics() {
RegisterForNotifications(); RegisterForNotifications();
metrics_state_manager_->ForceClientIdCreation(); metrics_state_manager_->ForceClientIdCreation();
is_in_sample_ = IsInSample(); is_in_sample_ = IsInSample();
is_in_package_name_sample_ = IsInPackageNameSample();
if (IsReportingEnabled()) { if (IsReportingEnabled()) {
// WebView has no shutdown sequence, so there's no need for a matching // WebView has no shutdown sequence, so there's no need for a matching
// Stop() call. // Stop() call.
...@@ -327,7 +328,7 @@ bool AwMetricsServiceClient::ShouldStartUpFastForTesting() const { ...@@ -327,7 +328,7 @@ bool AwMetricsServiceClient::ShouldStartUpFastForTesting() const {
} }
std::string AwMetricsServiceClient::GetAppPackageName() { std::string AwMetricsServiceClient::GetAppPackageName() {
if (IsInPackageNameSample() && CanRecordPackageNameForAppType()) { if (is_in_package_name_sample_ && CanRecordPackageNameForAppType()) {
JNIEnv* env = base::android::AttachCurrentThread(); JNIEnv* env = base::android::AttachCurrentThread();
base::android::ScopedJavaLocalRef<jstring> j_app_name = base::android::ScopedJavaLocalRef<jstring> j_app_name =
Java_AwMetricsServiceClient_getAppPackageName(env); Java_AwMetricsServiceClient_getAppPackageName(env);
......
...@@ -185,6 +185,7 @@ class AwMetricsServiceClient : public metrics::MetricsServiceClient, ...@@ -185,6 +185,7 @@ class AwMetricsServiceClient : public metrics::MetricsServiceClient,
bool user_consent_ = false; bool user_consent_ = false;
bool app_consent_ = false; bool app_consent_ = false;
bool is_in_sample_ = false; bool is_in_sample_ = false;
bool is_in_package_name_sample_ = false;
bool fast_startup_for_testing_ = false; bool fast_startup_for_testing_ = false;
// When non-zero, this overrides the default value in // When non-zero, this overrides the default value in
......
...@@ -179,9 +179,9 @@ TEST_F(AwMetricsServiceClientTest, TestShouldNotUploadPackageName_AppType) { ...@@ -179,9 +179,9 @@ TEST_F(AwMetricsServiceClientTest, TestShouldNotUploadPackageName_AppType) {
auto prefs = CreateTestPrefs(); auto prefs = CreateTestPrefs();
prefs->SetString(metrics::prefs::kMetricsClientID, kTestClientId); prefs->SetString(metrics::prefs::kMetricsClientID, kTestClientId);
auto client = CreateAndInitTestClient(prefs.get()); auto client = CreateAndInitTestClient(prefs.get());
client->SetHaveMetricsConsent(true, true);
client->SetRecordPackageNameForAppType(false); client->SetRecordPackageNameForAppType(false);
client->SetInPackageNameSample(true); client->SetInPackageNameSample(true);
client->SetHaveMetricsConsent(true, true);
std::string package_name = client->GetAppPackageName(); std::string package_name = client->GetAppPackageName();
EXPECT_TRUE(package_name.empty()); EXPECT_TRUE(package_name.empty());
} }
...@@ -190,9 +190,9 @@ TEST_F(AwMetricsServiceClientTest, TestShouldNotUploadPackageName_SampledOut) { ...@@ -190,9 +190,9 @@ TEST_F(AwMetricsServiceClientTest, TestShouldNotUploadPackageName_SampledOut) {
auto prefs = CreateTestPrefs(); auto prefs = CreateTestPrefs();
prefs->SetString(metrics::prefs::kMetricsClientID, kTestClientId); prefs->SetString(metrics::prefs::kMetricsClientID, kTestClientId);
auto client = CreateAndInitTestClient(prefs.get()); auto client = CreateAndInitTestClient(prefs.get());
client->SetHaveMetricsConsent(true, true);
client->SetRecordPackageNameForAppType(true); client->SetRecordPackageNameForAppType(true);
client->SetInPackageNameSample(false); client->SetInPackageNameSample(false);
client->SetHaveMetricsConsent(true, true);
std::string package_name = client->GetAppPackageName(); std::string package_name = client->GetAppPackageName();
EXPECT_TRUE(package_name.empty()); EXPECT_TRUE(package_name.empty());
} }
...@@ -201,9 +201,9 @@ TEST_F(AwMetricsServiceClientTest, TestCanUploadPackageName) { ...@@ -201,9 +201,9 @@ TEST_F(AwMetricsServiceClientTest, TestCanUploadPackageName) {
auto prefs = CreateTestPrefs(); auto prefs = CreateTestPrefs();
prefs->SetString(metrics::prefs::kMetricsClientID, kTestClientId); prefs->SetString(metrics::prefs::kMetricsClientID, kTestClientId);
auto client = CreateAndInitTestClient(prefs.get()); auto client = CreateAndInitTestClient(prefs.get());
client->SetHaveMetricsConsent(true, true);
client->SetRecordPackageNameForAppType(true); client->SetRecordPackageNameForAppType(true);
client->SetInPackageNameSample(true); client->SetInPackageNameSample(true);
client->SetHaveMetricsConsent(true, true);
std::string package_name = client->GetAppPackageName(); std::string package_name = client->GetAppPackageName();
EXPECT_FALSE(package_name.empty()); EXPECT_FALSE(package_name.empty());
} }
......
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