Allow chrome://histograms selecting histogram while monitoring

In crbug.com/40864008 it was discussed that selecting histogram while in
monitoring mode isn't really supported because as soon as new data comes
the selection gets invalidated. The only way to monitor single histogram
as to select them before getting into monitoring mode.

This attempts to fix that by doing the following:

1. Make `StartMonitoring` ignore the query taking the baseline of ALL
the histograms even if one is preselected. This is done to allow
selection change without changing a base point.

2. Make `GetDiff` not ignore the query returning only the histogram that
is actually requested by a frontend page.

3. Don't force `requestHistograms` when hash selection changes during
monitoring mode. This mixes monitoring data which are diff from baseline
with a global histogram data. With monitoring mode the updated data will
be pulled with the next fetch anyway (within 1s).

4. Link header to `#` allowing to reset selection (aka. `query`) without
refreshing the whole page and therefore the baseline. This allows
changing selection live and opens potential for selecting multiple
histograms.

There is a small risk of increased memory footprint for users doing a
lot of selective monitoring as each of them will now keep the whole
"baseline" set of histograms. However it seems like an extremely obscure
use case and the footprint is not enormous anyway O(kB).

Tested:

 * Updated and added new coverage to automated testing
 * Checked the functionality with local build on gLinux

Change-Id: I8fbf6e7fc138e2639e2d5ea5dff2d48a5666075a
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6175086
Reviewed-by: John Lee <johntlee@chromium.org>
Commit-Queue: Andrzej Fiedukowicz <afie@google.com>
Reviewed-by: Alexei Svitkine <asvitkine@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1409115}

content/browser/metrics/histograms_internals_ui.cc

@@ -136,7 +136,7 @@ void HistogramsMessageHandler::HandleStartMoninoring(
     const base::Value::List& args) {
   JsParams params = AllowJavascriptAndUnpackParams(args);
   ImportHistograms(params.include_subprocesses);
-  histogram_monitor_.StartMonitoring(params.query);
+  histogram_monitor_.StartMonitoring();
   ResolveJavascriptCallback(base::Value(params.callback_id),
                             base::Value("Success"));
 }
@@ -144,7 +144,7 @@ void HistogramsMessageHandler::HandleStartMoninoring(
 void HistogramsMessageHandler::HandleFetchDiff(const base::Value::List& args) {
   JsParams params = AllowJavascriptAndUnpackParams(args);
   ImportHistograms(params.include_subprocesses);
-  base::Value::List histograms_list = histogram_monitor_.GetDiff();
+  base::Value::List histograms_list = histogram_monitor_.GetDiff(params.query);
   ResolveJavascriptCallback(base::Value(params.callback_id),
                             std::move(histograms_list));
 }

content/browser/metrics/histograms_monitor.cc

@@ -13,22 +13,19 @@ HistogramsMonitor::HistogramsMonitor() = default;
 
 HistogramsMonitor::~HistogramsMonitor() = default;
 
-void HistogramsMonitor::StartMonitoring(std::string_view query) {
-  query_ = query;
+void HistogramsMonitor::StartMonitoring() {
   histograms_snapshot_.clear();
   // Save a snapshot of all current histograms that will be used as a baseline.
-  for (const auto* const histogram : base::StatisticsRecorder::WithName(
-           base::StatisticsRecorder::GetHistograms(), query_,
-           /*case_sensitive=*/false)) {
+  for (const auto* histogram : base::StatisticsRecorder::GetHistograms()) {
     histograms_snapshot_[histogram->histogram_name()] =
         histogram->SnapshotSamples();
   }
 }
 
-base::Value::List HistogramsMonitor::GetDiff() {
+base::Value::List HistogramsMonitor::GetDiff(const std::string& query) {
   base::StatisticsRecorder::Histograms histograms =
       base::StatisticsRecorder::Sort(base::StatisticsRecorder::WithName(
-          base::StatisticsRecorder::GetHistograms(), query_,
+          base::StatisticsRecorder::GetHistograms(), query,
           /*case_sensitive=*/false));
   return GetDiffInternal(histograms);
 }

content/browser/metrics/histograms_monitor.h

@@ -32,11 +32,12 @@ class CONTENT_EXPORT HistogramsMonitor {
 
   // Fetches and records a snapshot of the current histograms, as the baseline
   // to compare against in subsequent calls to GetDiff().
-  void StartMonitoring(std::string_view query);
+  void StartMonitoring();
 
   // Gets the histogram diffs between the current histograms and the snapshot
-  // recorded in StartMonitoring().
-  base::Value::List GetDiff();
+  // recorded in StartMonitoring(). Only returns the histograms that match
+  // the query string.
+  base::Value::List GetDiff(const std::string& query);
 
  private:
   // Gets the difference between the histograms argument and the stored snapshot
@@ -44,7 +45,6 @@ class CONTENT_EXPORT HistogramsMonitor {
   base::Value::List GetDiffInternal(
       const base::StatisticsRecorder::Histograms& histograms);
 
-  std::string query_;
   std::map<std::string, std::unique_ptr<base::HistogramSamples>>
       histograms_snapshot_;
 };

content/browser/metrics/histograms_monitor_unittest.cc

@@ -12,6 +12,12 @@
 
 namespace content {
 
+namespace {
+
+constexpr std::string kEmptyFilter = "";
+
+}  // namespace
+
 class HistogramsMonitorTest : public testing::Test {
  public:
   HistogramsMonitorTest()
@@ -26,17 +32,17 @@ TEST_F(HistogramsMonitorTest, StartMonitoringThenGetDiff) {
       "MonitorHistogram1", 1, 1000, 10, base::HistogramBase::kNoFlags);
   histogram1->Add(30);
   HistogramsMonitor monitor;
-  monitor.StartMonitoring(/*query=*/"");
+  monitor.StartMonitoring();
 
   // Get diff immediately should return nothing.
-  base::Value::List diff = monitor.GetDiff();
+  base::Value::List diff = monitor.GetDiff(kEmptyFilter);
   ASSERT_EQ(diff.size(), 0ull);
 
   // Add more data to histogram.
   histogram1->Add(30);
   histogram1->Add(40);
 
-  diff = monitor.GetDiff();
+  diff = monitor.GetDiff(kEmptyFilter);
   ASSERT_EQ(diff.size(), 1ull);
   std::string* header1 = diff[0].GetDict().FindString("header");
   EXPECT_EQ(*header1,
@@ -46,7 +52,7 @@ TEST_F(HistogramsMonitorTest, StartMonitoringThenGetDiff) {
   base::HistogramBase* histogram2 = base::Histogram::FactoryGet(
       "MonitorHistogram2", 1, 1000, 10, base::HistogramBase::kNoFlags);
   histogram2->Add(50);
-  diff = monitor.GetDiff();
+  diff = monitor.GetDiff(kEmptyFilter);
   ASSERT_EQ(diff.size(), 2ull);
   std::string* header2 = diff[1].GetDict().FindString("header");
   EXPECT_EQ(*header2,
@@ -60,12 +66,12 @@ TEST_F(HistogramsMonitorTest, StartMonitoringWithQueryThenGetDiff) {
       "MonitorHistogram1", 1, 1000, 10, base::HistogramBase::kNoFlags);
   histogram1->Add(30);
   HistogramsMonitor monitor;
-  monitor.StartMonitoring(/*query=*/"MonitorHistogram1");
+  monitor.StartMonitoring();
 
   base::HistogramBase* histogram2 = base::Histogram::FactoryGet(
       "MonitorHistogram2", 1, 1000, 10, base::HistogramBase::kNoFlags);
   histogram2->Add(50);
-  base::Value::List diff = monitor.GetDiff();
+  base::Value::List diff = monitor.GetDiff("MonitorHistogram1");
   ASSERT_EQ(diff.size(), 0ull);
 }
 
@@ -78,24 +84,83 @@ TEST_F(HistogramsMonitorTest, CaseInsensitiveQuery) {
   histogram1->Add(30);
 
   HistogramsMonitor monitor;
-  monitor.StartMonitoring(/*query=*/"histogram1");
+  monitor.StartMonitoring();
 
   base::HistogramBase* histogram2 = base::Histogram::FactoryGet(
       "MonitorHistogram2", 1, 1000, 10, base::HistogramBase::kNoFlags);
   histogram2->Add(50);
 
   // The query shouldn't match "MonitorHistogram2".
-  base::Value::List diff = monitor.GetDiff();
+  base::Value::List diff = monitor.GetDiff("histogram1");
   ASSERT_EQ(diff.size(), 0ull);
 
   histogram1->Add(10);
 
   // The query should match "MonitorHistogram1", since it's case insensitive.
-  diff = monitor.GetDiff();
+  diff = monitor.GetDiff("histogram1");
   ASSERT_EQ(diff.size(), 1ull);
   std::string* header2 = diff[0].GetDict().FindString("header");
   EXPECT_EQ(*header2,
             "Histogram: MonitorHistogram1 recorded 1 samples, mean = 10.0");
 }
 
+TEST_F(HistogramsMonitorTest, MonitoringBaselineDoesntChangeWithFilter) {
+  base::StatisticsRecorder::ForgetHistogramForTesting("MonitorHistogram1");
+  base::StatisticsRecorder::ForgetHistogramForTesting("MonitorHistogram2");
+  base::HistogramBase* histogram1 = base::Histogram::FactoryGet(
+      "MonitorHistogram1", 1, 1000, 10, base::HistogramBase::kNoFlags);
+  histogram1->Add(30);
+
+  base::HistogramBase* histogram2 = base::Histogram::FactoryGet(
+      "MonitorHistogram2", 1, 1000, 10, base::HistogramBase::kNoFlags);
+  histogram2->Add(50);
+  HistogramsMonitor monitor;
+
+  monitor.StartMonitoring();
+
+  // Get diff immediately should return nothing.
+  base::Value::List diff = monitor.GetDiff(kEmptyFilter);
+  ASSERT_EQ(diff.size(), 0ull);
+
+  // Add more data to histogram.
+  histogram1->Add(30);
+  histogram1->Add(40);
+
+  histogram2->Add(20);
+
+  // Filter the query to only return "MonitorHistogram1", the baseline should
+  // not change.
+  diff = monitor.GetDiff("MonitorHistogram1");
+  ASSERT_EQ(diff.size(), 1ull);
+  std::string* header1 = diff[0].GetDict().FindString("header");
+  EXPECT_EQ(*header1,
+            "Histogram: MonitorHistogram1 recorded 2 samples, mean = 35.0");
+
+
+  // Add more data to histogram2 and expect both samples after baseline to be
+  // returned.
+  histogram2->Add(10);
+  diff = monitor.GetDiff("MonitorHistogram2");
+  ASSERT_EQ(diff.size(), 1ull);
+  std::string* header2 = diff[0].GetDict().FindString("header");
+  EXPECT_EQ(*header2,
+            "Histogram: MonitorHistogram2 recorded 2 samples, mean = 15.0");
+
+  // Add more data to histogram1 and expect that both histograms are still
+  // returned for empty filter as they changed compared to the baseline.
+  histogram1->Add(50);
+  diff = monitor.GetDiff(kEmptyFilter);
+  ASSERT_EQ(diff.size(), 2ull);
+
+  std::vector<std::string> headers;
+  headers.push_back(*diff[0].GetDict().FindString("header"));
+  headers.push_back(*diff[1].GetDict().FindString("header"));
+  EXPECT_THAT(headers,
+              testing::UnorderedElementsAre(
+                  "Histogram: MonitorHistogram1 recorded 3 samples, "
+                  "mean = 40.0",
+                  "Histogram: MonitorHistogram2 recorded 2 samples, "
+                  "mean = 15.0"));
+}
+
 }  // namespace content

content/browser/resources/histograms/histograms_internals.html

@@ -12,7 +12,7 @@ found in the LICENSE file.
   <script type="module" src="histograms_internals.js"></script>
   <title>Histograms</title>
 </head>
-<h1>Histograms</h1>
+<h1><a href="#">Histograms</a></h1>
 <div id="accumulating_section">
   <button id="enable_monitoring">Switch to Monitoring Mode</button>
   <p>Stats accumulated from browser startup to previous page load;

content/browser/resources/histograms/histograms_internals.ts

@@ -249,6 +249,14 @@ function downloadHistograms() {
   }
 }
 
+function handleHashChange() {
+  // In monitoring mode the updated histograms will be fetched by fetchDiff()
+  // within 1s of changing URL and the proper filter will be applied.
+  if (!inMonitoringMode) {
+    requestHistograms();
+  }
+}
+
 /**
  * Load the initial list of histograms.
  */
@@ -266,4 +274,4 @@ document.addEventListener('DOMContentLoaded', function() {
 /**
  * Reload histograms when the "#abc" in "chrome://histograms/#abc" changes.
  */
-window.onhashchange = requestHistograms;
+window.onhashchange = handleHashChange;