Reland: [GRC] Add heuristics UKM and refactor MetricsCollector.

This patch also disables MetricsCollectorTest on Windows because it failed on Win 7 dbg. This is a reland of f28e176 Original change's description: > Reland "Reland "[GRC] Add heuristics UKM and refactor MetricsCollector."" > > This is a reland of 6bc2eb1 > Original change's description: > > Reland "[GRC] Add heuristics UKM and refactor MetricsCollector." > > > > This is a reland of e31b0b4 > > Original change's description: > > > > TBR=chrisha@chromium.org,dcheng@chromium.org,holte@chromium.org > > > [GRC] Add heuristics UKM and refactor MetricsCollector. > > > > > > This patch: > > > 1. adds heuristics UKM; > > > 2. rafactors MetricsCollector to unify background tabs metrics report; > > > 3. moved metrics report 5-minutes timeout logic to MetricsCollector; > > > 4. adds more unit tests. > > > > > > BUG=731270, 753486 > > > > > > Change-Id: Ica08674bf6b95d88f8de571ef025a7eaf58515d7 > > > Reviewed-on: https://chromium-review.googlesource.com/627594 > > > Reviewed-by: Chris Hamilton <chrisha@chromium.org> > > > Reviewed-by: Daniel Cheng <dcheng@chromium.org> > > > Reviewed-by: Steven Holte <holte@chromium.org> > > > Commit-Queue: lpy <lpy@chromium.org> > > > Cr-Commit-Position: refs/heads/master@{#497900} > > > > Bug: 731270, 753486 > > Change-Id: I43c99c0cdc7c7d3431f08fae86cb10d6d87d99fa > > Reviewed-on: https://chromium-review.googlesource.com/640010 > > Reviewed-by: lpy <lpy@chromium.org> > > Commit-Queue: lpy <lpy@chromium.org> > > Cr-Commit-Position: refs/heads/master@{#498395} > > TBR=chrisha@chromium.org,dcheng@chromium.org,holte@chromium.org > > Bug: 731270, 753486 > Change-Id: I65f2a5bc86e8ddd4af4b1739c306b6d1bc70b4b1 > Reviewed-on: https://chromium-review.googlesource.com/643568 > Commit-Queue: lpy <lpy@chromium.org> > Reviewed-by: lpy <lpy@chromium.org> > Cr-Commit-Position: refs/heads/master@{#498965} TBR=chrisha@chromium.org,dcheng@chromium.org,holte@chromium.org Bug: 731270, 753486 Change-Id: I10d8a16821848c8896e054dad0185b368905dc6e Reviewed-on: https://chromium-review.googlesource.com/646533 Reviewed-by: lpy <lpy@chromium.org> Reviewed-by: Daniel Cheng <dcheng@chromium.org> Commit-Queue: lpy <lpy@chromium.org> Cr-Commit-Position: refs/heads/master@{#499148}
nwjs · Sep 1, 2017 · 85b2a0f · 85b2a0f
1 parent ff8a5a5
commit 85b2a0f
Show file tree

Hide file tree

Showing 11 changed files with 572 additions and 202 deletions.
diff --git a/chrome/browser/resource_coordinator/resource_coordinator_web_contents_observer.cc b/chrome/browser/resource_coordinator/resource_coordinator_web_contents_observer.cc
@@ -28,10 +28,6 @@
 
 DEFINE_WEB_CONTENTS_USER_DATA_KEY(ResourceCoordinatorWebContentsObserver);
 
-// We delay the sending of favicon/title update signal to GRC for 5 minutes
-// after the main frame navigation is committed.
-const base::TimeDelta kFaviconAndTitleUpdateIgnoredTimeout =
-    base::TimeDelta::FromMinutes(5);
 bool ResourceCoordinatorWebContentsObserver::ukm_recorder_initialized = false;
 
 ResourceCoordinatorWebContentsObserver::ResourceCoordinatorWebContentsObserver(
@@ -149,7 +145,8 @@ void ResourceCoordinatorWebContentsObserver::DidFinishNavigation(
   if (navigation_handle->IsInMainFrame()) {
     UpdateUkmRecorder(navigation_handle);
     ResetFlag();
-    navigation_finished_time_ = base::TimeTicks::Now();
+    tab_resource_coordinator_->SendEvent(
+        resource_coordinator::mojom::Event::kNavigationCommitted);
   }
 
   content::RenderFrameHost* render_frame_host =
@@ -171,13 +168,6 @@ void ResourceCoordinatorWebContentsObserver::TitleWasSet(
     first_time_title_set_ = true;
     return;
   }
-  // Ignore update when the tab is in foreground or the update happens within 5
-  // minutes after the main frame is committed.
-  if (web_contents()->IsVisible() ||
-      base::TimeTicks::Now() - navigation_finished_time_ <
-          kFaviconAndTitleUpdateIgnoredTimeout) {
-    return;
-  }
   tab_resource_coordinator_->SendEvent(
       resource_coordinator::mojom::Event::kTitleUpdated);
 }
@@ -188,13 +178,6 @@ void ResourceCoordinatorWebContentsObserver::DidUpdateFaviconURL(
     first_time_favicon_set_ = true;
     return;
   }
-  // Ignore update when the tab is in foreground or the update happens within 5
-  // minutes after the main frame is committed.
-  if (web_contents()->IsVisible() ||
-      base::TimeTicks::Now() - navigation_finished_time_ <
-          kFaviconAndTitleUpdateIgnoredTimeout) {
-    return;
-  }
   tab_resource_coordinator_->SendEvent(
       resource_coordinator::mojom::Event::kFaviconUpdated);
 }

diff --git a/chrome/browser/resource_coordinator/resource_coordinator_web_contents_observer.h b/chrome/browser/resource_coordinator/resource_coordinator_web_contents_observer.h
@@ -61,12 +61,6 @@ class ResourceCoordinatorWebContentsObserver
       tab_resource_coordinator_;
   ukm::SourceId ukm_source_id_;
 
-  // We only want to send signals to GRC 5 minutes after the main frame is
-  // committed. Thus we record the time when navigation of main frame is
-  // finished, when title/favicon is changed, we check whether it's already more
-  // than 5 minutes from the time navigation of main frame is finished.
-  base::TimeTicks navigation_finished_time_;
-
   // Favicon and title are set when a page is loaded, we only want to send
   // signals to GRC about title and favicon update from the previous title and
   // favicon, thus we want to ignore the very first update since it is always

diff --git a/services/resource_coordinator/BUILD.gn b/services/resource_coordinator/BUILD.gn
@@ -12,6 +12,7 @@ import("//services/service_manager/public/tools/test/service_test.gni")
 
 source_set("lib") {
   sources = [
+    "coordination_unit/background_metrics_reporter.h",
     "coordination_unit/coordination_unit_graph_observer.cc",
     "coordination_unit/coordination_unit_graph_observer.h",
     "coordination_unit/coordination_unit_impl.cc",

diff --git a/services/resource_coordinator/coordination_unit/background_metrics_reporter.h b/services/resource_coordinator/coordination_unit/background_metrics_reporter.h
@@ -0,0 +1,136 @@
+// Copyright 2017 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef SERVICES_RESOURCE_COORDINATOR_COORDINATION_UNIT_BACKGROUND_METRICS_REPORTER_H_
+#define SERVICES_RESOURCE_COORDINATOR_COORDINATION_UNIT_BACKGROUND_METRICS_REPORTER_H_
+
+#include "base/macros.h"
+#include "base/metrics/histogram_macros.h"
+#include "base/time/time.h"
+#include "services/metrics/public/cpp/ukm_builders.h"
+
+// Tabs can be kept in the background for a long time, metrics show 75th
+// percentile of time spent in background is 2.5 hours, and the 95th is 24 hour.
+// In order to guide the selection of an appropriate observation window we are
+// proposing using a CUSTOM_TIMES histogram from 1s to 48h, with 100 buckets.
+#define HEURISTICS_HISTOGRAM(name, sample)                                  \
+  UMA_HISTOGRAM_CUSTOM_TIMES(name, sample, base::TimeDelta::FromSeconds(1), \
+                             base::TimeDelta::FromHours(48), 100)
+
+namespace ukm {
+class MojoUkmRecorder;
+}  // namespace ukm
+
+namespace resource_coordinator {
+
+namespace internal {
+
+enum UKMFrameReportType : uint8_t {
+  kMainFrameOnly = 0,
+  kMainFrameAndChildFrame
+};
+
+template <class UKMBuilderClass,
+          internal::UKMFrameReportType kShouldReportChildFrameUkm>
+class UKMReportDelegate {};
+
+template <class UKMBuilderClass>
+class UKMReportDelegate<UKMBuilderClass, internal::kMainFrameOnly> {
+ public:
+  void ReportUKM(int64_t ukm_source_id,
+                 bool is_main_frame,
+                 int64_t duration_in_ms,
+                 ukm::MojoUkmRecorder* ukm_recorder) {
+    UKMBuilderClass ukm_builder(ukm_source_id);
+    ukm_builder.SetTimeFromBackgrounded(duration_in_ms).Record(ukm_recorder);
+  }
+};
+
+template <class UKMBuilderClass>
+class UKMReportDelegate<UKMBuilderClass, internal::kMainFrameAndChildFrame> {
+ public:
+  void ReportUKM(int64_t ukm_source_id,
+                 bool is_main_frame,
+                 int64_t duration_in_ms,
+                 ukm::MojoUkmRecorder* ukm_recorder) {
+    UKMBuilderClass ukm_builder(ukm_source_id);
+    ukm_builder.SetIsMainFrame(is_main_frame)
+        .SetTimeFromBackgrounded(duration_in_ms)
+        .Record(ukm_recorder);
+  }
+};
+
+}  // namespace internal
+
+template <class UKMBuilderClass,
+          const char* kMetricName,
+          internal::UKMFrameReportType kShouldReportChildFrameUkm>
+class BackgroundMetricsReporter {
+ public:
+  BackgroundMetricsReporter()
+      : ukm_source_id_(-1),
+        uma_reported_(false),
+        ukm_reported_(false),
+        child_frame_ukm_reported_(false) {}
+
+  void Reset() {
+    uma_reported_ = false;
+    ukm_reported_ = false;
+    child_frame_ukm_reported_ = false;
+  }
+
+  void SetUKMSourceID(int64_t ukm_source_id) { ukm_source_id_ = ukm_source_id; }
+
+  void OnSignalReceived(bool is_main_frame,
+                        base::TimeDelta duration,
+                        ukm::MojoUkmRecorder* ukm_recorder) {
+    if (!uma_reported_) {
+      uma_reported_ = true;
+      HEURISTICS_HISTOGRAM(kMetricName, duration);
+    }
+
+    ReportUKMIfNeeded(is_main_frame, duration, ukm_recorder);
+  }
+
+ private:
+  void ReportUKMIfNeeded(bool is_main_frame,
+                         base::TimeDelta duration,
+                         ukm::MojoUkmRecorder* ukm_recorder) {
+    if (ukm_source_id_ == -1 ||
+        (!kShouldReportChildFrameUkm && ukm_reported_) ||
+        (kShouldReportChildFrameUkm &&
+         !ShouldReportMainFrameUKM(is_main_frame) &&
+         !ShouldReportChildFrameUKM(is_main_frame))) {
+      return;
+    }
+
+    ukm_reporter_.ReportUKM(ukm_source_id_, is_main_frame,
+                            duration.InMilliseconds(), ukm_recorder);
+
+    if (is_main_frame) {
+      ukm_reported_ = true;
+    } else {
+      child_frame_ukm_reported_ = true;
+    }
+  }
+
+  bool ShouldReportMainFrameUKM(bool is_main_frame) const {
+    return is_main_frame && !ukm_reported_;
+  }
+
+  bool ShouldReportChildFrameUKM(bool is_main_frame) const {
+    return !is_main_frame && !child_frame_ukm_reported_;
+  }
+
+  int64_t ukm_source_id_;
+  bool uma_reported_;
+  bool ukm_reported_;
+  bool child_frame_ukm_reported_;
+  internal::UKMReportDelegate<UKMBuilderClass, kShouldReportChildFrameUkm>
+      ukm_reporter_;
+};
+
+}  // namespace resource_coordinator
+
+#endif  // SERVICES_RESOURCE_COORDINATOR_COORDINATION_UNIT_BACKGROUND_METRICS_REPORTER_H_
diff --git a/services/resource_coordinator/coordination_unit/coordination_unit_impl.cc b/services/resource_coordinator/coordination_unit/coordination_unit_impl.cc
@@ -43,6 +43,14 @@ CoordinationUnitImpl::ToWebContentsCoordinationUnit(
   return static_cast<WebContentsCoordinationUnitImpl*>(coordination_unit);
 }
 
+// static
+const WebContentsCoordinationUnitImpl*
+CoordinationUnitImpl::ToWebContentsCoordinationUnit(
+    const CoordinationUnitImpl* cu) {
+  DCHECK(cu->id().type == CoordinationUnitType::kWebContents);
+  return static_cast<const WebContentsCoordinationUnitImpl*>(cu);
+}
+
 // static
 std::vector<CoordinationUnitImpl*>
 CoordinationUnitImpl::GetCoordinationUnitsOfType(CoordinationUnitType type) {

diff --git a/services/resource_coordinator/coordination_unit/coordination_unit_impl.h b/services/resource_coordinator/coordination_unit/coordination_unit_impl.h
@@ -38,6 +38,8 @@ class CoordinationUnitImpl : public mojom::CoordinationUnit {
       const CoordinationUnitImpl* coordination_unit);
   static WebContentsCoordinationUnitImpl* ToWebContentsCoordinationUnit(
       CoordinationUnitImpl* coordination_unit);
+  static const WebContentsCoordinationUnitImpl* ToWebContentsCoordinationUnit(
+      const CoordinationUnitImpl* coordination_unit);
 
   static std::vector<CoordinationUnitImpl*> GetCoordinationUnitsOfType(
       CoordinationUnitType type);