Merge pull request #4301 from brave/pr3931_ie_p3a_fix_1.4.x

Improve some P3A metrics (uplift to 1.4.x)

components/brave_ads/browser/ads_service_factory.cc

@@ -84,7 +84,8 @@ void AdsServiceFactory::RegisterProfilePrefs(
   registry->RegisterUint64Pref(prefs::kAdsPerDay, 20);
 
   registry->RegisterIntegerPref(prefs::kIdleThreshold, 15);
-  registry->RegisterBooleanPref(prefs::kBraveAdsWereDisabled, false);
+  registry->RegisterBooleanPref(prefs::kAdsWereDisabled, false);
+  registry->RegisterBooleanPref(prefs::kHasAdsP3AState, false);
 
   registry->RegisterBooleanPref(prefs::kShouldShowMyFirstAdNotification, true);
 

components/brave_ads/common/pref_names.cc

@@ -13,7 +13,11 @@ namespace prefs {
 const char kEnabled[] = "brave.brave_ads.enabled";
 
 // Stores whether ads were disabled at least once
-const char kBraveAdsWereDisabled[] = "brave.brave_ads.were_disabled";
+const char kAdsWereDisabled[] = "brave.brave_ads.were_disabled";
+
+// Indicates whether we have any initial state of the ads status metric, besides
+// "No Wallet".
+const char kHasAdsP3AState[] = "brave.brave_ads.has_p3a_state";
 
 // Stores the maximum amount of ads per hour
 const char kAdsPerHour[] = "brave.brave_ads.ads_per_hour";

components/brave_ads/common/pref_names.h

@@ -14,7 +14,8 @@ extern const char kEnabled[];
 
 extern const char kAdsPerHour[];
 extern const char kAdsPerDay[];
-extern const char kBraveAdsWereDisabled[];
+extern const char kAdsWereDisabled[];
+extern const char kHasAdsP3AState[];
 
 extern const char kIdleThreshold[];
 

components/brave_rewards/browser/rewards_p3a.cc

@@ -88,17 +88,17 @@ void UpdateAdsP3AOnPreferenceChange(PrefService *prefs,
   if (pref == brave_ads::prefs::kEnabled) {
     if (prefs->GetBoolean(brave_ads::prefs::kEnabled)) {
       brave_rewards::RecordAdsState(AdsP3AState::kAdsEnabled);
-      prefs->SetBoolean(brave_ads::prefs::kBraveAdsWereDisabled, false);
+      prefs->SetBoolean(brave_ads::prefs::kAdsWereDisabled, false);
     } else {
       // Apparently, the pref was disabled.
       brave_rewards::RecordAdsState(
           rewards_enabled ? AdsP3AState::kAdsEnabledThenDisabledRewardsOn :
                             AdsP3AState::kAdsEnabledThenDisabledRewardsOff);
-      prefs->SetBoolean(brave_ads::prefs::kBraveAdsWereDisabled, true);
+      prefs->SetBoolean(brave_ads::prefs::kAdsWereDisabled, true);
     }
   } else if (pref == brave_rewards::prefs::kBraveRewardsEnabled) {
     // Rewards pref was changed
-    if (prefs->GetBoolean(brave_ads::prefs::kBraveAdsWereDisabled)) {
+    if (prefs->GetBoolean(brave_ads::prefs::kAdsWereDisabled)) {
       brave_rewards::RecordAdsState(
           rewards_enabled ? AdsP3AState::kAdsEnabledThenDisabledRewardsOn :
                             AdsP3AState::kAdsEnabledThenDisabledRewardsOff);
@@ -107,6 +107,15 @@ void UpdateAdsP3AOnPreferenceChange(PrefService *prefs,
   }
 }
 
+void MaybeRecordInitialAdsP3AState(PrefService* prefs) {
+  if (!prefs->GetBoolean(brave_ads::prefs::kHasAdsP3AState)) {
+    const bool ads_state = prefs->GetBoolean(brave_ads::prefs::kEnabled);
+    RecordAdsState(ads_state ? AdsP3AState::kAdsEnabled
+                             : AdsP3AState::kAdsDisabled);
+    prefs->SetBoolean(brave_ads::prefs::kHasAdsP3AState, true);
+  }
+}
+
 void RecordNoWalletCreatedForAllMetrics() {
   RecordWalletBalanceP3A(false, 0);
   RecordAutoContributionsState(AutoContributionsP3AState::kNoWallet, 0);

components/brave_rewards/browser/rewards_p3a.h

@@ -50,6 +50,10 @@ void RecordAdsState(AdsP3AState state);
 void UpdateAdsP3AOnPreferenceChange(PrefService* prefs,
                                     const std::string& pref);
 
+// Records an initial metric state ("disabled" or "enabled") if it was not done
+// before. Intended to be called if the user has already created a wallet.
+void MaybeRecordInitialAdsP3AState(PrefService* local_state);
+
 void RecordNoWalletCreatedForAllMetrics();
 
 double CalcWalletBalanceForP3A(base::flat_map<std::string, double> wallets,

components/brave_rewards/browser/rewards_service_impl.cc

@@ -927,7 +927,7 @@ void RewardsServiceImpl::OnWalletInitialized(ledger::Result result) {
     RecordWalletBalanceP3A(true, 0);
 #if BUILDFLAG(BRAVE_ADS_ENABLED)
     const bool ads_enabled =
-      profile_->GetPrefs()->GetBoolean(brave_ads::prefs::kEnabled);
+        profile_->GetPrefs()->GetBoolean(brave_ads::prefs::kEnabled);
     RecordAdsState(ads_enabled ? AdsP3AState::kAdsEnabled
                                : AdsP3AState::kAdsDisabled);
 #endif
@@ -1069,6 +1069,7 @@ void RewardsServiceImpl::OnLedgerStateLoaded(
     }
     // Record stats.
     RecordBackendP3AStats();
+    MaybeRecordInitialAdsP3AState(profile_->GetPrefs());
   }
   if (state.first.empty()) {
     RecordNoWalletCreatedForAllMetrics();

components/p3a/brave_histogram_rewrite.cc

@@ -16,11 +16,13 @@ namespace {
 // Please keep this list sorted and synced with |DoHistogramBravezation|.
 constexpr const char* kBravezationHistograms[] = {
     "Bookmarks.Count.OnProfileLoad",
+    "DefaultBrowser.State",
     "Extensions.LoadExtension",
     "Tabs.TabCount",
     "Tabs.WindowCount",
 };
 
+// TODO(iefremov): Replace a bunch of 'if's with something more elegant.
 // Records the given sample using the proper Brave way.
 void DoHistogramBravezation(base::StringPiece histogram_name,
                             base::HistogramBase::Sample sample) {
@@ -40,6 +42,25 @@ void DoHistogramBravezation(base::StringPiece histogram_name,
     return;
   }
 
+  if ("DefaultBrowser.State" == histogram_name) {
+    int answer = 0;
+    switch (sample) {
+      case 0:  // Not default.
+      case 1:  // Default.
+        answer = sample;
+        break;
+      case 2:  // Unknown, merging to "Not default".
+        answer = 0;
+        break;
+      case 3:  // Other mode is default, merging to "Default".
+        answer = 1;
+        break;
+    default:
+      NOTREACHED();
+    }
+    UMA_HISTOGRAM_BOOLEAN("Brave.Core.IsDefault", answer);
+  }
+
   if ("Extensions.LoadExtension" == histogram_name) {
     int answer = 0;
     if (sample == 1)

components/p3a/brave_p3a_log_store.cc

@@ -34,10 +34,10 @@ void RecordP3A(uint64_t answers_count) {
 
 }  // namespace
 
-BraveP3ALogStore::BraveP3ALogStore(LogSerializer* serializer,
+BraveP3ALogStore::BraveP3ALogStore(Delegate* delegate,
                                    PrefService* local_state)
-    : serializer_(serializer), local_state_(local_state) {
-  DCHECK(serializer_);
+    : delegate_(delegate), local_state_(local_state) {
+  DCHECK(delegate_);
   DCHECK(local_state);
 }
 
@@ -123,7 +123,8 @@ void BraveP3ALogStore::StageNextLog() {
   DCHECK(!log_.find(staged_entry_key_)->second.sent);
 
   uint64_t staged_entry_value = log_[staged_entry_key_].value;
-  staged_log_ = serializer_->Serialize(staged_entry_key_, staged_entry_value);
+  staged_log_ = delegate_->Serialize(staged_entry_key_, staged_entry_value);
+
   VLOG(2) << "BraveP3ALogStore::StageNextLog: staged " << staged_entry_key_;
 }
 
@@ -161,10 +162,17 @@ void BraveP3ALogStore::LoadPersistedUnsentLogs() {
   DCHECK(log_.empty());
   DCHECK(unsent_entries_.empty());
 
-  const base::DictionaryValue* list = local_state_->GetDictionary(kPrefName);
+  DictionaryPrefUpdate update(local_state_, kPrefName);
+  base::DictionaryValue* list = update.Get();
   for (auto dict_item : list->DictItems()) {
     LogEntry entry;
-    std::string name = dict_item.first;
+    const std::string name = dict_item.first;
+    // Check if the metric is obsolete.
+    if (!delegate_->IsActualMetric(name)) {
+      // Drop it from the local state.
+      list->RemoveKey(name);
+      continue;
+    }
     const base::Value& dict = dict_item.second;
     // Value.
     if (const base::Value* v =

components/p3a/brave_p3a_log_store.h

@@ -25,15 +25,17 @@ namespace brave {
 // for now persisted entries never expire.
 class BraveP3ALogStore : public metrics::LogStore {
  public:
-  // Prepares a string representaion of an entry.
-  class LogSerializer {
+  class Delegate {
    public:
+    // Prepares a string representaion of an entry.
     virtual std::string Serialize(base::StringPiece histogram_name,
                                   uint64_t value) const = 0;
-    virtual ~LogSerializer() {}
+    // Returns false if the metric is obsolete and should be cleaned up.
+    virtual bool IsActualMetric(base::StringPiece histogram_name) const = 0;
+    virtual ~Delegate() {}
   };
 
-  explicit BraveP3ALogStore(LogSerializer* serializer,
+  explicit BraveP3ALogStore(Delegate* delegate,
                             PrefService* local_state);
 
   // TODO(iefremov): Make parent destructor virtual?
@@ -78,7 +80,7 @@ class BraveP3ALogStore : public metrics::LogStore {
     base::Time sent_timestamp;  // At the moment only for debugging purposes.
   };
 
-  const LogSerializer* const serializer_ = nullptr;  // Weak.
+  const Delegate* const delegate_ = nullptr;  // Weak.
   PrefService* const local_state_ = nullptr;
 
   // TODO(iefremov): Try to replace with base::StringPiece?

components/p3a/brave_p3a_service.cc

@@ -51,11 +51,13 @@ constexpr uint64_t kDefaultUploadIntervalSeconds = 60 * 60;  // 1 hour.
 constexpr const char* kCollectedHistograms[] = {
     "Brave.P3A.SentAnswersCount",
     "Brave.Sync.Status",
-    "DefaultBrowser.State",
+    // Deprecated:
+    // "DefaultBrowser.State",
     "Brave.Importer.ImporterSource",
     "Brave.Shields.UsageStatus",
     // Do not gather detailed info regarding TOR usage for now.
     // "Brave.Core.LastTimeTorUsed",
+    "Brave.Core.IsDefault",
     "Brave.Core.TorEverUsed",
     "Brave.Core.LastTimeIncognitoUsed",
     "Brave.Core.NumberOfExtensions",
@@ -205,6 +207,14 @@ std::string BraveP3AService::Serialize(base::StringPiece histogram_name,
   return message.SerializeAsString();
 }
 
+bool
+BraveP3AService::IsActualMetric(base::StringPiece histogram_name) const {
+  static const base::NoDestructor<base::flat_set<base::StringPiece>>
+      metric_names {std::begin(kCollectedHistograms),
+                    std::end(kCollectedHistograms)};
+  return metric_names->contains(histogram_name);
+}
+
 void BraveP3AService::MaybeOverrideSettingsFromCommandLine() {
   base::CommandLine* cmdline = base::CommandLine::ForCurrentProcess();
 

components/p3a/brave_p3a_service.h

@@ -30,7 +30,7 @@ class BraveP3AUploader;
 // on any thread.
 // TODO(iefremov): It should be possible to get rid of refcounted here.
 class BraveP3AService : public base::RefCountedThreadSafe<BraveP3AService>,
-                        public BraveP3ALogStore::LogSerializer {
+                        public BraveP3ALogStore::Delegate {
  public:
   explicit BraveP3AService(PrefService* local_state);
 
@@ -43,10 +43,12 @@ class BraveP3AService : public base::RefCountedThreadSafe<BraveP3AService>,
   // Needs a living browser process to complete the initialization.
   void Init();
 
-  // BraveP3ALogStore::LogSerializer
+  // BraveP3ALogStore::Delegate
   std::string Serialize(base::StringPiece histogram_name,
                         uint64_t value) const override;
 
+  bool IsActualMetric(base::StringPiece histogram_name) const override;
+
  private:
   friend class base::RefCountedThreadSafe<BraveP3AService>;
   ~BraveP3AService() override;