From a69699574094b8a62c2e339985311245e0dfb6d0 Mon Sep 17 00:00:00 2001 From: Darnell Andries Date: Wed, 1 Oct 2025 19:22:51 -0700 Subject: [PATCH] Default engine/switch engine metric modifications (#31419) * Add locale-based country code P3A attribute, use for the default engine/engine switch metrics * Change format and reporting cadence for default engine switch metric --- browser/brave_local_state_prefs.cc | 1 + browser/brave_profile_prefs.cc | 4 + browser/search_engines/DEPS | 1 - .../search_engines/search_engine_tracker.cc | 76 +++++++++++++------ .../search_engines/search_engine_tracker.h | 21 +++-- .../search_engine_tracker_browsertest.cc | 15 ++-- browser/search_engines/sources.gni | 2 - components/p3a/metric_config.cc | 1 + components/p3a/metric_config.h | 1 + components/p3a/metric_names.h | 6 +- components/p3a/p3a_message.cc | 21 ++++- components/p3a/p3a_message.h | 3 +- 12 files changed, 103 insertions(+), 49 deletions(-) diff --git a/browser/brave_local_state_prefs.cc b/browser/brave_local_state_prefs.cc index 93ddc2924a7..c011fe5af57 100644 --- a/browser/brave_local_state_prefs.cc +++ b/browser/brave_local_state_prefs.cc @@ -174,6 +174,7 @@ void RegisterLocalStatePrefs(PrefRegistrySimple* registry) { RegisterLocalStatePrefsForMigration(registry); brave_search_conversion::p3a::RegisterLocalStatePrefs(registry); + SearchEngineTrackerFactory::RegisterLocalStatePrefs(registry); #if BUILDFLAG(ENABLE_BRAVE_VPN) brave_vpn::RegisterLocalStatePrefs(registry); diff --git a/browser/brave_profile_prefs.cc b/browser/brave_profile_prefs.cc index 6d52780340f..9dcdb6c084e 100644 --- a/browser/brave_profile_prefs.cc +++ b/browser/brave_profile_prefs.cc @@ -10,6 +10,7 @@ #include "base/feature_list.h" #include "brave/browser/brave_shields/brave_shields_web_contents_observer.h" #include "brave/browser/new_tab/new_tab_shows_options.h" +#include "brave/browser/search_engines/search_engine_tracker.h" #include "brave/browser/themes/brave_dark_mode_utils.h" #include "brave/browser/translate/brave_translate_prefs_migration.h" #include "brave/browser/ui/bookmark/brave_bookmark_prefs.h" @@ -338,6 +339,9 @@ void RegisterProfilePrefsForMigration( #if BUILDFLAG(ENABLE_SPEEDREADER) speedreader::RegisterProfilePrefsForMigration(registry); #endif + + // Added 2025-09 + SearchEngineTrackerFactory::RegisterProfilePrefsForMigration(registry); } void RegisterProfilePrefs(user_prefs::PrefRegistrySyncable* registry) { diff --git a/browser/search_engines/DEPS b/browser/search_engines/DEPS index 8637bd3aa00..ff433f36de0 100644 --- a/browser/search_engines/DEPS +++ b/browser/search_engines/DEPS @@ -1,5 +1,4 @@ include_rules = [ - "+brave/components/time_period_storage", "+brave/components/web_discovery/buildflags", "+brave/android/java/org/chromium/chrome/browser/search_engines/jni_headers", "+brave/components/l10n/common/locale_util.h", diff --git a/browser/search_engines/search_engine_tracker.cc b/browser/search_engines/search_engine_tracker.cc index e663cb55e6e..0704243a470 100644 --- a/browser/search_engines/search_engine_tracker.cc +++ b/browser/search_engines/search_engine_tracker.cc @@ -12,8 +12,8 @@ #include "base/metrics/histogram_macros.h" #include "base/no_destructor.h" #include "base/strings/string_util.h" +#include "base/time/time.h" #include "brave/components/brave_ads/core/public/prefs/pref_names.h" -#include "brave/components/brave_search_conversion/features.h" #include "brave/components/brave_search_conversion/p3a.h" #include "brave/components/brave_search_conversion/utils.h" #include "brave/components/constants/pref_names.h" @@ -25,9 +25,16 @@ namespace { -// Preference name switch events are stored under. +// Preference name for last switch report timestamp (new location in local +// state). +constexpr char kLastSwitchReportPref[] = "brave.search.last_switch_report"; +// Old preference name for switch events (for migration from profile prefs). constexpr char kSwitchSearchEngineP3AStorage[] = "brave.search.p3a_default_switch"; +// Report interval for P3A switch metrics (1 hour) +constexpr base::TimeDelta kReportInterval = base::Hours(1); +// Minimum time between reports (1 day) +constexpr base::TimeDelta kMinReportInterval = base::Days(1); constexpr char kBraveDomain[] = "brave.com"; constexpr char kGoogleDomain[] = "google.com"; constexpr char kDDGDomain[] = "duckduckgo.com"; @@ -144,8 +151,14 @@ bool SearchEngineTrackerFactory::ServiceIsCreatedWithBrowserContext() const { return true; } -void SearchEngineTrackerFactory::RegisterProfilePrefs( +void SearchEngineTrackerFactory::RegisterLocalStatePrefs( + PrefRegistrySimple* registry) { + registry->RegisterTimePref(kLastSwitchReportPref, {}); +} + +void SearchEngineTrackerFactory::RegisterProfilePrefsForMigration( user_prefs::PrefRegistrySyncable* registry) { + // Register old pref for migration registry->RegisterListPref(kSwitchSearchEngineP3AStorage); } @@ -153,8 +166,7 @@ SearchEngineTracker::SearchEngineTracker( TemplateURLService* template_url_service, PrefService* profile_prefs, PrefService* local_state) - : switch_record_(profile_prefs, kSwitchSearchEngineP3AStorage), - local_state_(local_state), + : local_state_(local_state), profile_prefs_(profile_prefs), template_url_service_(template_url_service) { DCHECK(template_url_service); @@ -162,6 +174,10 @@ SearchEngineTracker::SearchEngineTracker( DCHECK(local_state); observer_.Observe(template_url_service_); + + // Migrate any old prefs + MigrateObsoletePrefs(); + const TemplateURL* template_url = template_url_service_->GetDefaultSearchProvider(); @@ -255,33 +271,45 @@ void SearchEngineTracker::RecordWebDiscoveryEnabledP3A() { #endif void SearchEngineTracker::RecordSwitchP3A(const GURL& url) { - // Default to the last recorded switch so when we're called - // at start-up we initialize the histogram with whatever we - // remember from the previous run. - auto answer = SearchEngineSwitchP3A::kNoSwitch; - auto last = switch_record_.GetLatest(); - if (last) { - answer = static_cast(last.value()); - DCHECK(answer <= SearchEngineSwitchP3A::kMaxValue); - } + const base::Time now = base::Time::Now(); + const base::Time last_report = local_state_->GetTime(kLastSwitchReportPref); - if (url.is_valid() && url != previous_search_url_) { - // The default url has been switched, record that instead. + // Determine the appropriate "no switch" value based on current search engine + auto answer = url.is_valid() && url.DomainIs(kBraveDomain) + ? SearchEngineSwitchP3A::kNoSwitchBrave + : SearchEngineSwitchP3A::kNoSwitchNonBrave; + bool should_report = false; + + if (previous_search_url_.is_valid() && url.is_valid() && + url != previous_search_url_) { + // The default url has been switched, record that. answer = SearchEngineSwitchP3AMapAnswer(url, previous_search_url_); previous_search_url_ = url; - switch_record_.Add(static_cast(answer)); + should_report = true; if (url.DomainIs(kBraveDomain)) { brave_search_conversion::p3a::RecordDefaultEngineConversion(local_state_); } + } else if (last_report.is_null() || + (now - last_report) >= kMinReportInterval) { + // Report if we haven't reported before or it's been >= 1 day + should_report = true; } - if (brave_search_conversion::IsBraveSearchConversionFeatureEnabled() || - base::FeatureList::IsEnabled(brave_search_conversion::features::kNTP)) { - // Do not report if search conversion promo is enabled, to prevent metric - // overlap with conversion metrics. - UMA_HISTOGRAM_EXACT_LINEAR(kSwitchSearchEngineMetric, INT_MAX - 1, 8); - return; + // Set up timer for next report regardless of whether we report now + switch_report_timer_.Start( + FROM_HERE, now + kReportInterval, + base::BindOnce(&SearchEngineTracker::RecordSwitchP3A, + base::Unretained(this), url)); + + if (should_report) { + // Update the last report timestamp + local_state_->SetTime(kLastSwitchReportPref, now); + + UMA_HISTOGRAM_ENUMERATION(kSwitchSearchEngineMetric, answer); } - UMA_HISTOGRAM_ENUMERATION(kSwitchSearchEngineMetric, answer); +} + +void SearchEngineTracker::MigrateObsoletePrefs() { + profile_prefs_->ClearPref(kSwitchSearchEngineP3AStorage); } diff --git a/browser/search_engines/search_engine_tracker.h b/browser/search_engines/search_engine_tracker.h index 9b15f319602..09eafd739f8 100644 --- a/browser/search_engines/search_engine_tracker.h +++ b/browser/search_engines/search_engine_tracker.h @@ -10,10 +10,11 @@ #include "base/memory/raw_ptr.h" #include "base/scoped_observation.h" -#include "brave/components/time_period_storage/weekly_event_storage.h" +#include "base/timer/wall_clock_timer.h" #include "brave/components/web_discovery/buildflags/buildflags.h" #include "components/keyed_service/content/browser_context_keyed_service_factory.h" #include "components/keyed_service/core/keyed_service.h" +#include "components/pref_registry/pref_registry_syncable.h" #include "components/prefs/pref_change_registrar.h" #include "components/prefs/pref_member.h" #include "components/prefs/pref_registry_simple.h" @@ -30,7 +31,8 @@ class NoDestructor; // Exposed for tests. inline constexpr char kDefaultSearchEngineMetric[] = "Brave.Search.DefaultEngine.4"; -inline constexpr char kSwitchSearchEngineMetric[] = "Brave.Search.SwitchEngine"; +inline constexpr char kSwitchSearchEngineMetric[] = + "Brave.Search.SwitchEngine.2"; inline constexpr char kWebDiscoveryEnabledMetric[] = "Brave.Search.WebDiscoveryEnabled"; inline constexpr char kWebDiscoveryAndAdsMetric[] = @@ -59,7 +61,8 @@ enum class SearchEngineP3A { // Note: append-only enumeration! Never remove any existing values, as this enum // is used to bucket a UMA histogram, and removing values breaks that. enum class SearchEngineSwitchP3A { - kNoSwitch, + kNoSwitchBrave, // No switch, currently using Brave Search + kNoSwitchNonBrave, // No switch, currently using non-Brave search engine kBraveToGoogle, kBraveToDDG, kBraveToOther, @@ -79,6 +82,10 @@ class SearchEngineTrackerFactory : public BrowserContextKeyedServiceFactory { static SearchEngineTracker* GetForBrowserContext( content::BrowserContext* context); + static void RegisterLocalStatePrefs(PrefRegistrySimple* registry); + static void RegisterProfilePrefsForMigration( + user_prefs::PrefRegistrySyncable* registry); + private: friend base::NoDestructor; SearchEngineTrackerFactory(); @@ -92,9 +99,6 @@ class SearchEngineTrackerFactory : public BrowserContextKeyedServiceFactory { std::unique_ptr BuildServiceInstanceForBrowserContext( content::BrowserContext* context) const override; bool ServiceIsCreatedWithBrowserContext() const override; - - void RegisterProfilePrefs( - user_prefs::PrefRegistrySyncable* registry) override; }; // Records P3A metrics when default search engine changes, @@ -120,6 +124,8 @@ class SearchEngineTracker : public KeyedService, void RecordWebDiscoveryEnabledP3A(); #endif + void MigrateObsoletePrefs(); + base::ScopedObservation observer_{this}; @@ -129,7 +135,8 @@ class SearchEngineTracker : public KeyedService, GURL default_search_url_; GURL previous_search_url_; SearchEngineP3A current_default_engine_ = SearchEngineP3A::kOther; - WeeklyEventStorage switch_record_; + + base::WallClockTimer switch_report_timer_; raw_ptr local_state_; raw_ptr profile_prefs_; diff --git a/browser/search_engines/search_engine_tracker_browsertest.cc b/browser/search_engines/search_engine_tracker_browsertest.cc index 2a5f2ebb798..053eed3165f 100644 --- a/browser/search_engines/search_engine_tracker_browsertest.cc +++ b/browser/search_engines/search_engine_tracker_browsertest.cc @@ -22,7 +22,6 @@ #include "chrome/test/base/search_test_utils.h" #include "components/country_codes/country_codes.h" #include "components/keyed_service/content/browser_context_dependency_manager.h" -#include "components/regional_capabilities/regional_capabilities_country_id.h" #include "components/regional_capabilities/regional_capabilities_prefs.h" #include "components/regional_capabilities/regional_capabilities_service.h" #include "components/search_engines/template_url_prepopulate_data.h" @@ -102,11 +101,11 @@ IN_PROC_BROWSER_TEST_F(SearchEngineProviderP3ATest, DefaultSearchEngineP3A) { IN_PROC_BROWSER_TEST_F(SearchEngineProviderP3ATest, SwitchSearchEngineP3A) { // Check that the metric is reported on startup. - // For some reason we can record kNoSwitch twice, even though - // kDefaultSearchEngineMetric is only updated once at this point. - auto start_count = histogram_tester_->GetBucketCount( - kSwitchSearchEngineMetric, SearchEngineSwitchP3A::kNoSwitch); - EXPECT_GT(start_count, 0); + // Since we override the region to US, Brave Search should be the default + auto start_count_brave = histogram_tester_->GetBucketCount( + kSwitchSearchEngineMetric, SearchEngineSwitchP3A::kNoSwitchBrave); + // We should see kNoSwitchBrave since Brave is default in US region + EXPECT_GT(start_count_brave, 0); // Load service for switching the default search engine. auto* service = @@ -154,13 +153,13 @@ IN_PROC_BROWSER_TEST_F(SearchEngineProviderP3ATest, SwitchSearchEngineP3A) { SearchEngineSwitchP3A::kOtherToBrave, 1); // Check that incognito or TOR profiles do not emit the metric. - histogram_tester_->ExpectTotalCount(kSwitchSearchEngineMetric, 8); + histogram_tester_->ExpectTotalCount(kSwitchSearchEngineMetric, 5); CreateIncognitoBrowser(); #if BUILDFLAG(ENABLE_TOR) brave::NewOffTheRecordWindowTor(browser()); #endif - histogram_tester_->ExpectTotalCount(kSwitchSearchEngineMetric, 8); + histogram_tester_->ExpectTotalCount(kSwitchSearchEngineMetric, 5); } #if BUILDFLAG(ENABLE_EXTENSIONS) || BUILDFLAG(ENABLE_WEB_DISCOVERY_NATIVE) diff --git a/browser/search_engines/sources.gni b/browser/search_engines/sources.gni index 44a951b67b2..1ff8a18673e 100644 --- a/browser/search_engines/sources.gni +++ b/browser/search_engines/sources.gni @@ -15,7 +15,6 @@ brave_browser_search_engines_sources = [ brave_browser_search_engines_deps = [ "//base", "//brave/components/brave_search_conversion", - "//brave/components/time_period_storage", "//chrome/browser/profiles:profile", "//components/keyed_service/content", "//components/keyed_service/core", @@ -57,7 +56,6 @@ if (is_android) { brave_browser_search_engines_deps += [ "//brave/browser/profiles:util", "//brave/components/search_engines", - "//brave/components/time_period_storage", "//chrome/browser/profiles:profile", "//components/keyed_service/content", "//components/pref_registry", diff --git a/components/p3a/metric_config.cc b/components/p3a/metric_config.cc index 1a52dd153fb..8565a8e1206 100644 --- a/components/p3a/metric_config.cc +++ b/components/p3a/metric_config.cc @@ -20,6 +20,7 @@ constexpr auto kMetricAttributeMap = {"channel", MetricAttribute::kChannel}, {"platform", MetricAttribute::kPlatform}, {"country_code", MetricAttribute::kCountryCode}, + {"locale_country_code", MetricAttribute::kLocaleCountryCode}, {"woi", MetricAttribute::kWoi}, {"general_platform", MetricAttribute::kGeneralPlatform}, {"region", MetricAttribute::kRegion}, diff --git a/components/p3a/metric_config.h b/components/p3a/metric_config.h index 866d15bc7f0..b085483cd40 100644 --- a/components/p3a/metric_config.h +++ b/components/p3a/metric_config.h @@ -26,6 +26,7 @@ enum class MetricAttribute { kCountryCode, kWoi, // Alternative attributes + kLocaleCountryCode, kGeneralPlatform, kRegion, kSubregion, diff --git a/components/p3a/metric_names.h b/components/p3a/metric_names.h index 6bf8d811eaa..62d170aa963 100644 --- a/components/p3a/metric_names.h +++ b/components/p3a/metric_names.h @@ -117,7 +117,6 @@ inline constexpr auto kCollectedTypicalHistograms = {"Brave.Search.Promo.DDGBannerD", {}}, {"Brave.Search.Promo.NewTabPage", {}}, {"Brave.Search.QueriesBeforeChurn", MetricConfig{.ephemeral = true}}, - {"Brave.Search.SwitchEngine", {}}, {"Brave.Search.WebDiscoveryAndAds", {}}, {"Brave.Search.WebDiscoveryDefaultEngine", {}}, {"Brave.Search.WidgetDefault", {}}, @@ -218,7 +217,10 @@ inline constexpr auto kCollectedExpressHistograms = }}, {"Brave.Search.BraveDaily", MetricConfig{.ephemeral = true}}, {"Brave.Search.DefaultEngine.4", MetricConfig{ - .attributes = MetricAttributes{MetricAttribute::kAnswerIndex, MetricAttribute::kChannel, MetricAttribute::kPlatform, MetricAttribute::kDateOfInstall, MetricAttribute::kVersion, MetricAttribute::kCountryCode}, + .attributes = MetricAttributes{MetricAttribute::kAnswerIndex, MetricAttribute::kChannel, MetricAttribute::kPlatform, MetricAttribute::kDateOfInstall, MetricAttribute::kVersion, MetricAttribute::kLocaleCountryCode}, + }}, + {"Brave.Search.SwitchEngine.2", MetricConfig{ + .attributes = MetricAttributes{MetricAttribute::kAnswerIndex, MetricAttribute::kChannel, MetricAttribute::kPlatform, MetricAttribute::kDateOfInstall, MetricAttribute::kVersion, MetricAttribute::kLocaleCountryCode}, }}, {"Brave.Search.WebDiscoveryEnabled", {}}, {"Brave.Today.EnabledSetting", MetricConfig{.attributes = MetricAttributes{MetricAttribute::kAnswerIndex, MetricAttribute::kDateOfActivation, MetricAttribute::kDateOfInstall, MetricAttribute::kVersion, MetricAttribute::kChannel, MetricAttribute::kPlatform, MetricAttribute::kCountryCode}}}, diff --git a/components/p3a/p3a_message.cc b/components/p3a/p3a_message.cc index 20774d35a8f..c3d23f45f65 100644 --- a/components/p3a/p3a_message.cc +++ b/components/p3a/p3a_message.cc @@ -166,10 +166,15 @@ std::vector> PopulateConstellationAttributes( attribute_value = meta.country_code_from_locale_raw(); } else { attribute_value = meta.GetCountryCodeForNormalMetrics( - metric_config && metric_config->disable_country_strip); + metric_config && metric_config->disable_country_strip, false); } attributes.push_back({kCountryCodeAttributeName, attribute_value}); break; + case MetricAttribute::kLocaleCountryCode: + attribute_value = meta.GetCountryCodeForNormalMetrics( + metric_config && metric_config->disable_country_strip, true); + attributes.push_back({kCountryCodeAttributeName, attribute_value}); + break; case MetricAttribute::kWoi: if (is_creative) { continue; @@ -264,7 +269,7 @@ base::Value::Dict GenerateP3AMessageDict(std::string_view metric_name, // Fill meta. result.Set(kCountryCodeAttributeName, - meta.GetCountryCodeForNormalMetrics(false)); + meta.GetCountryCodeForNormalMetrics(false, false)); result.Set(kVersionAttributeName, meta.version()); result.Set(kWoiAttributeName, meta.woi()); @@ -352,7 +357,7 @@ void MessageMetainfo::Init(PrefService* local_state, country_code_from_locale_ = country_code_from_locale_raw_; region_identifiers_ = - GetRegionIdentifiers(GetCountryCodeForNormalMetrics(true)); + GetRegionIdentifiers(GetCountryCodeForNormalMetrics(true, false)); MaybeStripCountry(); @@ -419,7 +424,15 @@ void MessageMetainfo::MaybeStripCountry() { } const std::string& MessageMetainfo::GetCountryCodeForNormalMetrics( - bool raw) const { + bool raw, + bool is_locale) const { + if (is_locale) { + if (raw) { + return country_code_from_locale_raw_; + } + return country_code_from_locale_; + } + #if BUILDFLAG(IS_IOS) if (raw) { return country_code_from_locale_raw_; diff --git a/components/p3a/p3a_message.h b/components/p3a/p3a_message.h index 7cd29127625..08f82317e9a 100644 --- a/components/p3a/p3a_message.h +++ b/components/p3a/p3a_message.h @@ -37,7 +37,8 @@ class MessageMetainfo { void Update(); - const std::string& GetCountryCodeForNormalMetrics(bool raw) const; + const std::string& GetCountryCodeForNormalMetrics(bool raw, + bool is_locale) const; std::optional GetActivationDate( std::string_view histogram_name) const;