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
2025-10-01 19:22:51 -07:00
parent fedffb8acd
commit a696995740
12 changed files with 103 additions and 49 deletions
@@ -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);
@@ -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) {
@@ -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",
@@ -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<SearchEngineSwitchP3A>(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<int>(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);
 }
@@ -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>;
  SearchEngineTrackerFactory();
@@ -92,9 +99,6 @@ class SearchEngineTrackerFactory : public BrowserContextKeyedServiceFactory {
  std::unique_ptr<KeyedService> 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<TemplateURLService, TemplateURLServiceObserver>
      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<PrefService> local_state_;
  raw_ptr<PrefService> profile_prefs_;
@@ -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)
@@ -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",
@@ -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},
@@ -26,6 +26,7 @@ enum class MetricAttribute {
  kCountryCode,
  kWoi,
  // Alternative attributes
+  kLocaleCountryCode,
  kGeneralPlatform,
  kRegion,
  kSubregion,
@@ -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}}},
@@ -166,10 +166,15 @@ std::vector<std::array<std::string, 2>> 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_;
@@ -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<base::Time> GetActivationDate(
      std::string_view histogram_name) const;