Merge pull request #23274 from brave/news-p3a-bucket-adj

[Brave News] Adjust subscription metric buckets, fix channel subscriptions
brave · Apr 26, 2024 · a19c891 · a19c891
2 parents b4cb4e0 + c637f76
commit a19c891
Show file tree

Hide file tree

Showing 5 changed files with 33 additions and 23 deletions.
diff --git a/components/brave_news/browser/brave_news_p3a.cc b/components/brave_news/browser/brave_news_p3a.cc
@@ -27,7 +27,7 @@ constexpr int kCardViewBuckets[] = {0, 1, 10, 20, 40, 80, 100};
 constexpr int kCardClickBuckets[] = {2, 5, 10, 15, 20, 25};
 constexpr int kSidebarFilterUsageBuckets[] = {1, 4, 7, 10};
 constexpr int kClickCardDepthBuckets[] = {3, 6, 10, 15, 20};
-constexpr int kSubscriptionCountBuckets[] = {1, 4, 7, 10};
+constexpr int kSubscriptionCountBuckets[] = {0, 1, 4, 8, 13};
 constexpr size_t kCardClickDepthMetricThreshold = 5;
 constexpr base::TimeDelta kMonthlyUserTimeThreshold = base::Days(30);
 constexpr base::TimeDelta kReportInterval = base::Days(1);
@@ -91,12 +91,11 @@ void NewsMetrics::RecordDirectFeedsTotal() {
     return;
   }
 
-  constexpr int kBuckets[] = {0, 1, 2, 3, 4, 5, 10};
   std::size_t feed_count =
       pref_manager_->GetSubscriptions().direct_feeds().size();
   DVLOG(1) << "DirectFeedTotal: " << feed_count;
-  p3a_utils::RecordToHistogramBucket(kDirectFeedsTotalHistogramName, kBuckets,
-                                     feed_count);
+  p3a_utils::RecordToHistogramBucket(kDirectFeedsTotalHistogramName,
+                                     kSubscriptionCountBuckets, feed_count);
 }
 
 void NewsMetrics::RecordWeeklyAddedDirectFeedsCount(int change) {
@@ -333,6 +332,9 @@ void NewsMetrics::OnConfigChanged() {
   }
   was_enabled_ = pref_manager_->IsEnabled();
   RecordFeatureEnabledChange();
+
+  // Get initial publisher count, which should be 0.
+  OnPublishersChanged();
 }
 
 void NewsMetrics::OnPublishersChanged() {
@@ -354,14 +356,11 @@ void NewsMetrics::OnPublishersChanged() {
 
 void NewsMetrics::OnChannelsChanged() {
   DVLOG(1) << __FUNCTION__;
-  std::vector<std::string> subscribed_channels;
+  base::flat_set<std::string> distinct_channels;
   for (const auto& [locale, channels] :
        pref_manager_->GetSubscriptions().channels()) {
-    for (const auto& channel : channels) {
-      subscribed_channels.push_back(channel);
-    }
+    distinct_channels.insert(channels.begin(), channels.end());
   }
-  base::flat_set<std::string> distinct_channels(subscribed_channels);
   RecordTotalSubscribedCount(SubscribeType::kChannels,
                              distinct_channels.size());
 }

diff --git a/components/brave_news/browser/brave_news_p3a.h b/components/brave_news/browser/brave_news_p3a.h
@@ -28,7 +28,7 @@ inline constexpr char kTotalCardViewsHistogramName[] =
 inline constexpr char kWeeklyDisplayAdsViewedHistogramName[] =
     "Brave.Today.WeeklyDisplayAdsViewedCount";
 inline constexpr char kDirectFeedsTotalHistogramName[] =
-    "Brave.Today.DirectFeedsTotal.2";
+    "Brave.Today.DirectFeedsTotal.3";
 inline constexpr char kWeeklyAddedDirectFeedsHistogramName[] =
     "Brave.Today.WeeklyAddedDirectFeedsCount";
 inline constexpr char kLastUsageTimeHistogramName[] =
@@ -41,9 +41,10 @@ inline constexpr char kSidebarFilterUsageHistogramName[] =
     "Brave.Today.SidebarFilterUsages";
 inline constexpr char kClickCardDepthHistogramName[] =
     "Brave.Today.ClickCardDepth";
-inline constexpr char kChannelCountHistogramName[] = "Brave.Today.ChannelCount";
+inline constexpr char kChannelCountHistogramName[] =
+    "Brave.Today.ChannelCount.2";
 inline constexpr char kPublisherCountHistogramName[] =
-    "Brave.Today.PublisherCount";
+    "Brave.Today.PublisherCount.2";
 inline constexpr char kIsEnabledHistogramName[] = "Brave.Today.IsEnabled";
 inline constexpr char kUsageMonthlyHistogramName[] = "Brave.Today.UsageMonthly";
 inline constexpr char kUsageDailyHistogramName[] = "Brave.Today.UsageDaily";

diff --git a/components/brave_news/browser/brave_news_p3a_unittest.cc b/components/brave_news/browser/brave_news_p3a_unittest.cc
@@ -207,6 +207,7 @@ TEST_F(BraveNewsP3ATest, TestDirectFeedsTotal) {
   pref_manager_->AddDirectPublisher(GURL("https://bar.com"), "");
 
   histogram_tester_.ExpectTotalCount(kDirectFeedsTotalHistogramName, 3);
+  histogram_tester_.ExpectBucketCount(kDirectFeedsTotalHistogramName, 1, 1);
   histogram_tester_.ExpectBucketCount(kDirectFeedsTotalHistogramName, 2, 1);
 }
 

diff --git a/components/brave_news/browser/channels_controller.cc b/components/brave_news/browser/channels_controller.cc
@@ -45,6 +45,7 @@ Channels ChannelsController::GetChannelsFromPublishers(
     const BraveNewsSubscriptions& subscriptions) {
   Channels channels;
   for (const auto& it : publishers) {
+    // Collect all channels from all locales first...
     for (const auto& locale_info : it.second->locales) {
       for (const auto& channel_id : locale_info->channels) {
         auto migrated_channel_id = GetMigratedChannel(channel_id);
@@ -53,16 +54,24 @@ Channels ChannelsController::GetChannelsFromPublishers(
           channel->channel_name = channel_id;
           channels[migrated_channel_id] = std::move(channel);
         }
-
-        auto& channel = channels[migrated_channel_id];
-
+      }
+    }
+    // ...and then check subscription statuses.
+    // We iterate twice (once above, and once here),
+    // just in case the user is subscribed to a channel
+    // that does not have sources for their locale. In that case,
+    // the channel struct could be created via occurence in another
+    // locale, which would result in potential missing subscription statuses
+    // if we only iterated once.
+    for (const auto& locale_info : it.second->locales) {
+      for (auto& [channel_id, channel] : channels) {
         // We already know we're subscribed to this channel in this locale.
         if (base::Contains(channel->subscribed_locales, locale_info->locale)) {
           continue;
         }
 
-        auto subscribed_in_locale = subscriptions.GetChannelSubscribed(
-            locale_info->locale, migrated_channel_id);
+        auto subscribed_in_locale =
+            subscriptions.GetChannelSubscribed(locale_info->locale, channel_id);
         if (subscribed_in_locale) {
           channel->subscribed_locales.push_back(locale_info->locale);
         }

diff --git a/components/p3a/metric_names.h b/components/p3a/metric_names.h
@@ -257,9 +257,9 @@ inline constexpr auto kCollectedSlowHistograms =
     "Brave.Shields.ForgetFirstParty",
     "Brave.Sync.EnabledTypes",
     "Brave.Sync.SyncedObjectsCount.2",
-    "Brave.Today.ChannelCount",
-    "Brave.Today.DirectFeedsTotal.2",
-    "Brave.Today.PublisherCount",
+    "Brave.Today.ChannelCount.2",
+    "Brave.Today.DirectFeedsTotal.3",
+    "Brave.Today.PublisherCount.2",
     "Brave.Today.UsageMonthly",
     "Brave.Toolbar.ForwardNavigationAction",
     "Brave.Wallet.UsageMonthly",
@@ -320,11 +320,11 @@ inline constexpr auto kEphemeralHistograms =
     "Brave.Rewards.ToolbarButtonTrigger",
     "Brave.Search.QueriesBeforeChurn",
     "Brave.Shields.AllowScriptOnce",
-    "Brave.Today.ChannelCount",
+    "Brave.Today.ChannelCount.2",
     "Brave.Today.ClickCardDepth",
-    "Brave.Today.DirectFeedsTotal.2",
+    "Brave.Today.DirectFeedsTotal.3",
     "Brave.Today.IsEnabled",
-    "Brave.Today.PublisherCount",
+    "Brave.Today.PublisherCount.2",
     "Brave.Today.SidebarFilterUsages",
     "Brave.Today.UsageDaily",
     "Brave.Today.UsageMonthly",