TabDiscarder: Move the logging from TabManager to TabActivityWatcher.

Currently TabDiscarder logging happens inside TabManager::DiscardTab
which is not ideal.

This CL moves the logging to TabActivityWatcher::SortLifecycleUnitWithTabRanker
so that we can log exact the queried tabs.

Bug: 968366
Change-Id: I1ca457be59c548a3eb88aa209125d48707147a18
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1633894
Commit-Queue: Charles . <charleszhao@chromium.org>
Reviewed-by: François Doray <fdoray@chromium.org>
Cr-Commit-Position: refs/heads/master@{#665009}

chrome/browser/resource_coordinator/tab_activity_watcher.cc

@@ -42,23 +42,7 @@
                              base::TimeDelta::FromDays(1), 50)
 
 namespace resource_coordinator {
-namespace {
 
-// Returns an int64_t number as label_id or query_id. The number is generated
-// incrementally from 1.
-int64_t NewInt64ForLabelIdOrQueryId() {
-  static int64_t id = 0;
-  // The id is shifted 13 bits so that the lower bits are reserved for counting
-  // multiple queries.
-  // We choose 13 so that the lower bits for counting multiple queries and
-  // higher bits for labeling queries are both unlikely to overflow. (lower bits
-  // only overflows when we have more than 8192 queries without labeling events;
-  // higher bits only overflow when we have more than 100 billion discards.
-  constexpr int kIdShiftBits = 13;
-  return (++id) << kIdShiftBits;
-}
-
-}  // namespace
 
 // Per-WebContents helper class that observes its WebContents, notifying
 // TabActivityWatcher when interesting events occur. Also provides
@@ -73,7 +57,7 @@ class TabActivityWatcher::WebContentsData
   // Calculates the tab reactivation score for a background tab. Returns nullopt
   // if the score could not be calculated, e.g. because the tab is in the
   // foreground.
-  base::Optional<float> CalculateReactivationScore() {
+  base::Optional<float> CalculateReactivationScore(bool log_this_query) {
     if (web_contents()->IsBeingDestroyed() || backgrounded_time_.is_null())
       return base::nullopt;
 
@@ -83,6 +67,19 @@ class TabActivityWatcher::WebContentsData
     if (!tab.has_value())
       return base::nullopt;
 
+    if (log_this_query) {
+      // Update label_id_: a new label_id is generated for this query if the
+      // label_id_ is 0; otherwise the old label_id_ is incremented. This allows
+      // us to better pairing TabMetrics with ForegroundedOrClosed events
+      // offline. The same label_id_ will be logged with ForegroundedOrClosed
+      // event later on so that TabFeatures can be paired with
+      // ForegroundedOrClosed.
+      label_id_ = label_id_ ? label_id_ + 1 : NewInt64ForLabelIdOrQueryId();
+
+      TabActivityWatcher::GetInstance()->tab_metrics_logger_->LogTabMetrics(
+          ukm_source_id_, tab.value(), web_contents(), label_id_);
+    }
+
     float score = 0.0f;
     const tab_ranker::TabRankerResult result =
         TabActivityWatcher::GetInstance()->predictor_.ScoreTab(tab.value(),
@@ -240,7 +237,6 @@ class TabActivityWatcher::WebContentsData
 
     backgrounded_time_ = base::TimeTicks();
     foregrounded_time_ = NowTicks();
-    creation_time_ = NowTicks();
 
     page_metrics_.num_reactivations++;
   }
@@ -453,6 +449,11 @@ class TabActivityWatcher::WebContentsData
     label_id_ = 0;
   }
 
+  // Helper function for label_id and query_id.
+  inline int64_t NewInt64ForLabelIdOrQueryId() {
+    return TabActivityWatcher::GetInstance()->NewInt64ForLabelIdOrQueryId();
+  }
+
   // Updated when a navigation is finished.
   ukm::SourceId ukm_source_id_ = 0;
 
@@ -512,12 +513,13 @@ TabActivityWatcher::TabActivityWatcher()
 TabActivityWatcher::~TabActivityWatcher() = default;
 
 base::Optional<float> TabActivityWatcher::CalculateReactivationScore(
-    content::WebContents* web_contents) {
+    content::WebContents* web_contents,
+    bool log_this_query) {
   WebContentsData* web_contents_data =
       WebContentsData::FromWebContents(web_contents);
   if (!web_contents_data)
     return base::nullopt;
-  return web_contents_data->CalculateReactivationScore();
+  return web_contents_data->CalculateReactivationScore(log_this_query);
 }
 
 void TabActivityWatcher::LogOldestNTabFeatures() {
@@ -547,10 +549,13 @@ void TabActivityWatcher::SortLifecycleUnitWithTabRanker(
     std::vector<LifecycleUnit*>* tabs) {
   std::map<int32_t, float> reactivation_scores;
 
+  // Set query_id so that all TabFeatures logged in this query can be joined.
+  tab_metrics_logger_->set_query_id(NewInt64ForLabelIdOrQueryId());
+
   for (auto* lifecycle_unit : *tabs) {
     content::WebContents* web_content =
         lifecycle_unit->AsTabLifecycleUnitExternal()->GetWebContents();
-    base::Optional<float> score = CalculateReactivationScore(web_content);
+    base::Optional<float> score = CalculateReactivationScore(web_content, true);
     reactivation_scores[lifecycle_unit->GetID()] =
         score.has_value() ? score.value() : std::numeric_limits<float>::max();
   }
@@ -563,6 +568,17 @@ void TabActivityWatcher::SortLifecycleUnitWithTabRanker(
             });
 }
 
+int64_t TabActivityWatcher::NewInt64ForLabelIdOrQueryId() {
+  // The id is shifted 13 bits so that the lower bits are reserved for counting
+  // multiple queries.
+  // We choose 13 so that the lower bits for counting multiple queries and
+  // higher bits for labeling queries are both unlikely to overflow. (lower bits
+  // only overflows when we have more than 8192 queries without labeling events;
+  // higher bits only overflow when we have more than 100 billion discards.
+  constexpr int kIdShiftBits = 13;
+  return (++internal_id_for_logging_) << kIdShiftBits;
+}
+
 void TabActivityWatcher::OnBrowserSetLastActive(Browser* browser) {
   if (browser->tab_strip_model()->closing_all())
     return;
@@ -637,6 +653,7 @@ bool TabActivityWatcher::ShouldTrackBrowser(Browser* browser) {
 
 void TabActivityWatcher::ResetForTesting() {
   tab_metrics_logger_ = std::make_unique<TabMetricsLogger>();
+  internal_id_for_logging_ = 0;
 }
 
 // static

chrome/browser/resource_coordinator/tab_activity_watcher.h

@@ -36,7 +36,8 @@ class TabActivityWatcher : public BrowserListObserver,
   // value indicates a higher likelihood of being reactivated.
   // Returns the score if the tab could be scored.
   base::Optional<float> CalculateReactivationScore(
-      content::WebContents* web_contents);
+      content::WebContents* web_contents,
+      bool log_this_query = false);
 
   // Log TabFeatures for oldest n tabs.
   void LogOldestNTabFeatures();
@@ -45,6 +46,9 @@ class TabActivityWatcher : public BrowserListObserver,
   // the first candidate that will be discarded.
   void SortLifecycleUnitWithTabRanker(std::vector<LifecycleUnit*>* tabs);
 
+  // Returns an int64_t number as label_id or query_id.
+  int64_t NewInt64ForLabelIdOrQueryId();
+
   // Returns the single instance, creating it if necessary.
   static TabActivityWatcher* GetInstance();
 
@@ -95,6 +99,9 @@ class TabActivityWatcher : public BrowserListObserver,
   // All WebContentsData of the browser that is currently in closing_all mode.
   base::flat_set<WebContentsData*> all_closing_tabs_;
 
+  // Used for generating label_ids and query_ids.
+  int64_t internal_id_for_logging_ = 0;
+
   DISALLOW_COPY_AND_ASSIGN(TabActivityWatcher);
 };
 

chrome/browser/resource_coordinator/tab_activity_watcher_unittest.cc

@@ -44,6 +44,10 @@ using ForegroundedOrClosed =
 namespace resource_coordinator {
 namespace {
 
+const char* kTabMetricsEntryName = TabManager_TabMetrics::kEntryName;
+
+const int64_t kIdShift = 1 << 13;
+
 // Test URLs need to be from different origins to test site engagement score.
 const GURL kTestUrls[] = {
     GURL("https://test1.example.com"), GURL("https://test3.example.com"),
@@ -138,6 +142,94 @@ TEST_F(TabActivityWatcherTest, SortLifecycleUnitWithTabRanker) {
   tab_strip_model->CloseAllTabs();
 }
 
+// Test that lifecycleunits are correctly logged inside
+// SortLifecycleUnitWithTabRanker.
+TEST_F(TabActivityWatcherTest, LogInsideSortLifecycleUnitWithTabRanker) {
+  base::test::ScopedFeatureList feature_list_overrides;
+  feature_list_overrides.InitAndEnableFeatureWithParameters(
+      features::kTabRanker,
+      {{"disable_background_log_with_TabRanker", "true"}});
+  Browser::CreateParams params(profile(), true);
+  std::unique_ptr<Browser> browser =
+      CreateBrowserWithTestWindowForParams(&params);
+  TabStripModel* tab_strip_model = browser->tab_strip_model();
+
+  // Create lifecycleunits.
+  LifecycleUnit* tab0 = AddNewTab(tab_strip_model, 0);
+  LifecycleUnit* tab1 = AddNewTab(tab_strip_model, 1);
+  LifecycleUnit* tab2 = AddNewTab(tab_strip_model, 2);
+  std::vector<LifecycleUnit*> lifecycleunits = {tab0};
+
+  // Call SortLifecycleUnitWithTabRanker on tab0 should log the TabMetrics for
+  // tab0.
+  TabActivityWatcher::GetInstance()->SortLifecycleUnitWithTabRanker(
+      &lifecycleunits);
+  {
+    SCOPED_TRACE("");
+    ukm_entry_checker_.ExpectNewEntry(
+        kTabMetricsEntryName, kTestUrls[0],
+        {
+            {TabManager_TabMetrics::kQueryIdName, 1 * kIdShift},
+            {TabManager_TabMetrics::kLabelIdName, 2 * kIdShift},
+        });
+  }
+
+  // Call SortLifecycleUnitWithTabRanker on tab0 should log the TabMetrics for
+  // tab0.
+  TabActivityWatcher::GetInstance()->SortLifecycleUnitWithTabRanker(
+      &lifecycleunits);
+  {
+    SCOPED_TRACE("");
+    ukm_entry_checker_.ExpectNewEntry(
+        kTabMetricsEntryName, kTestUrls[0],
+        {
+            {TabManager_TabMetrics::kQueryIdName, 3 * kIdShift},
+            {TabManager_TabMetrics::kLabelIdName, 2 * kIdShift + 1},
+        });
+  }
+
+  // Call SortLifecycleUnitWithTabRanker on tab2 should not log the TabMetrics
+  // for tab2 because it is foregrounded.
+  lifecycleunits = {tab2};
+  TabActivityWatcher::GetInstance()->SortLifecycleUnitWithTabRanker(
+      &lifecycleunits);
+  {
+    SCOPED_TRACE("");
+    EXPECT_EQ(ukm_entry_checker_.NumNewEntriesRecorded(kTabMetricsEntryName),
+              0);
+  }
+
+  // Call SortLifecycleUnitWithTabRanker on all three tabs should log two
+  // TabMetrics events for tab0 and tab1.
+  lifecycleunits = {tab0, tab1, tab2};
+  TabActivityWatcher::GetInstance()->SortLifecycleUnitWithTabRanker(
+      &lifecycleunits);
+  {
+    SCOPED_TRACE("");
+    EXPECT_EQ(ukm_entry_checker_.NumNewEntriesRecorded(kTabMetricsEntryName),
+              2);
+
+    ukm_entry_checker_.ExpectNewEntry(
+        kTabMetricsEntryName, kTestUrls[0],
+        {
+            {TabManager_TabMetrics::kQueryIdName, 5 * kIdShift},
+            {TabManager_TabMetrics::kLabelIdName, 2 * kIdShift + 2},
+        });
+
+    ukm_entry_checker_.ExpectNewEntry(
+        kTabMetricsEntryName, kTestUrls[1],
+        {
+            {TabManager_TabMetrics::kQueryIdName, 5 * kIdShift},
+            {TabManager_TabMetrics::kLabelIdName, 6 * kIdShift},
+        });
+  }
+
+  // Closing the tabs destroys the WebContentses but should not trigger logging.
+  // The TestWebContentsObserver simulates hiding these tabs as they are closed;
+  // we verify in TearDown() that no logging occurred.
+  tab_strip_model->CloseAllTabs();
+}
+
 // Tests TabManager.TabMetrics UKM entries generated when tabs are backgrounded.
 class TabMetricsTest : public TabActivityWatcherTest {
  public:

chrome/browser/resource_coordinator/tab_manager.cc

@@ -35,7 +35,6 @@
 #include "chrome/browser/performance_manager/performance_manager.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/resource_coordinator/background_tab_navigation_throttle.h"
-#include "chrome/browser/resource_coordinator/tab_activity_watcher.h"
 #include "chrome/browser/resource_coordinator/tab_lifecycle_unit_external.h"
 #include "chrome/browser/resource_coordinator/tab_manager.h"
 #include "chrome/browser/resource_coordinator/tab_manager_features.h"
@@ -240,8 +239,6 @@ void TabManager::DiscardTab(LifecycleUnitDiscardReason reason,
                             TabDiscardDoneCB tab_discard_done) {
   if (reason == LifecycleUnitDiscardReason::URGENT) {
     stats_collector_->RecordWillDiscardUrgently(GetNumAliveTabs());
-    resource_coordinator::TabActivityWatcher::GetInstance()
-        ->LogOldestNTabFeatures();
   }
 
 #if defined(OS_CHROMEOS)