[UKM] Modify the Blink.Update event recording

There are too many Blink.UpdateTime events, resulting in
significant "sampling" by the UKM system which in turn
leads to way low client counts.

Fix it by emitting two samples per page load: once before
First Contentful Paint and one after, chosen uniformly
at random within in period.

Modify the test to test the new approach as best can be done.

Design Doc (chromium.org):
https://docs.google.com/document/d/1P49GrDe3mDDV9Q1yUm27ioWE4m1_Zz1FwRwVwGQh-ZI/edit?usp=sharing

Bug: 1014195

First pass at updating sampling.

Change-Id: I4461015346d506adf5d01bc65e14538d05f2b798
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2083686Reviewed-by: vmpstr <vmpstr@chromium.org>
Commit-Queue: Stephen Chenney <schenney@chromium.org>
Cr-Commit-Position: refs/heads/master@{#746848}

third_party/blink/renderer/core/frame/local_frame_ukm_aggregator.h

@@ -35,7 +35,7 @@ namespace blink {
 // - recorder: UkmRecorder which will handle the events
 //
 // The aggregator manages all of the UKM and UMA names for LocalFrameView.
-// It constructs and takes ownership the UMA counters when constructed
+// It constructs and takes ownership of the UMA counters when constructed
 // itself. We do this to localize all UMA and UKM metrics in one place, so
 // that adding a metric is localized to the cc file of this class, protected
 // from errors that might arise when adding names in multiple places.
@@ -53,12 +53,11 @@ namespace blink {
 // primary time and computes metrics that depend on it. UMA metrics are updated
 // at this time.
 //
-// A UKM event is generated according to a sampling strategy. A record is always
-// generated on the first lifecycle update, and then additional samples are
-// taken at random frames simulating a poisson process with mean number of
-// frames between events of mean_frames_between_samples_. The first primary
-// metric recording after the frame count has passed will produce an event with
-// all the data for that frame (i.e. the period since the last BeginMainFrame).
+// A UKM event is generated according to a sampling strategy, with the goal
+// being to choose one frame to report before First Contentful Paint and
+// one frame to report during the subsequent document lifetime. We maintain
+// a copy of the current sample, and randomly choose to update it on each frame
+// such that any given frame is equally likely to be the final sample.
 //
 // Sample usage (see also SCOPED_UMA_AND_UKM_TIMER):
 //   std::unique_ptr<UkmHierarchicalTimeAggregator> aggregator(
@@ -224,7 +223,7 @@ class CORE_EXPORT LocalFrameUkmAggregator
   };
 
   LocalFrameUkmAggregator(int64_t source_id, ukm::UkmRecorder*);
-  ~LocalFrameUkmAggregator() = default;
+  ~LocalFrameUkmAggregator();
 
   // Create a scoped timer with the index of the metric. Note the index must
   // correspond to the matching index in metric_names.
@@ -259,8 +258,9 @@ class CORE_EXPORT LocalFrameUkmAggregator
   void BeginMainFrame();
 
   // Inform the aggregator that we have reached First Contentful Paint.
-  // The UKM event reports this and UMA for aggregated contributions to
-  // FCP are reported if are_painting_main_frame is true.
+  // The UKM event for the pre-FCP period will be recorded and UMA for
+  // aggregated contributions to FCP are reported if are_painting_main_frame
+  // is true.
   void DidReachFirstContentfulPaint(bool are_painting_main_frame);
 
   bool InMainFrameUpdate() { return in_main_frame_update_; }
@@ -296,22 +296,36 @@ class CORE_EXPORT LocalFrameUkmAggregator
     void reset() { interval_duration = base::TimeDelta(); }
   };
 
-  void UpdateEventTimeAndRecordEventIfNeeded(
+  struct SampleToRecord {
+    base::TimeDelta primary_metric_duration;
+    Vector<base::TimeDelta> sub_metrics_durations;
+    Vector<unsigned> sub_metric_percentages;
+    cc::ActiveFrameSequenceTrackers trackers;
+  };
+
+  void UpdateEventTimeAndUpdateSampleIfNeeded(
       cc::ActiveFrameSequenceTrackers trackers);
-  void RecordEvent(cc::ActiveFrameSequenceTrackers trackers);
+  void UpdateSample(cc::ActiveFrameSequenceTrackers trackers);
   void ResetAllMetrics();
-  unsigned SampleFramesToNextEvent();
+
+  // Reports the current sample to the UKM system. Called on the first main
+  // frame update after First Contentful Paint and at destruction. Also resets
+  // the frame count.
+  void ReportUpdateTimeEvent();
+
+  // Reports the Blink.PageLoad to the UKM system. Called on the first main
+  // frame after First Contentful Paint.
+  void ReportPreFCPEvent();
 
   // Implements throttling of the ForcedStyleAndLayoutUMA metric.
   void RecordForcedStyleLayoutUMA(base::TimeDelta& duration);
 
-  // To test event sampling. This and all future intervals will be the given
-  // frame count, until this is called again.
-  void FramesToNextEventForTest(unsigned num_frames) {
-    frames_to_next_event_for_test_ = num_frames;
-  }
+  // To test event sampling. Controls whether we update the current sample
+  // on the next frame, or do not. Values persist until explicitly changed.
+  void ChooseNextFrameForTest();
+  void DoNotChooseNextFrameForTest();
 
-  // Used to check that we only for the MainFrame of a document.
+  // Used to check that we record only for the MainFrame of a document.
   bool AllMetricsAreZero();
 
   // The caller is the owner of the |clock|. The |clock| must outlive the
@@ -329,32 +343,36 @@ class CORE_EXPORT LocalFrameUkmAggregator
   Vector<AbsoluteMetricRecord> absolute_metric_records_;
   Vector<MainFramePercentageRecord> main_frame_percentage_records_;
 
-  // Sampling control. We use a Poisson process with an exponential decay
-  // multiplier. The goal is to get many randomly distributed samples early
-  // during page load and initial interaction, then samples at an exponentially
-  // decreasing rate to effectively cap the number of samples. The particular
-  // parameters chosen here give roughly 5-10 samples in the first 100 frames,
-  // decaying to several hours between samples by the 40th sample. The
-  // multiplier value and sample_decay_rate_ should be tuned to achieve a total
-  // sample count that avoids throttling by the UKM system.
-  double sample_decay_rate_ = 1;
-  double sample_rate_multiplier_ = 2;
-  unsigned samples_so_far_ = 0;
-  unsigned frames_to_next_event_ = 0;
+  // The current sample to report. When RecordEvent() is called we
+  // check for uniform_random[0,1) < 1 / n where n is the number of frames
+  // we have seen (including this one). If true, we replace the sample with
+  // the current frame data. The result is a uniformly randomly chosen frame
+  // in the period between the frame counter being reset and the recording
+  // to the UKM system of the current sample.
+  // This process is designed to get maximum utility while only sending 2
+  // events per page load, which in turn maximizes client counts.
+  SampleToRecord current_sample_;
+  unsigned frames_since_last_report_ = 0;
 
   // Control for the ForcedStyleAndUpdate UMA metric sampling
   unsigned mean_calls_between_forced_style_layout_uma_ = 100;
   unsigned calls_to_next_forced_style_layout_uma_ = 0;
 
-  // Test data, used for SampleFramesToNextEvent if present
-  unsigned frames_to_next_event_for_test_ = 0;
-
   // Set by BeginMainFrame() and cleared in RecordMEndOfFrameMetrics.
   // Main frame metrics are only recorded if this is true.
   bool in_main_frame_update_ = false;
 
-  // Record whether or not it is before the First Contentful Paint.
-  bool is_before_fcp_ = true;
+  // A bitfield maintaining state for first contentful paint.
+  enum FCPState { kBeforeFCPSignal, kThisFrameReachedFCP, kHavePassedFCP };
+  FCPState fcp_state_ = kBeforeFCPSignal;
+
+  // A bitfield used to control updating the sample for tests.
+  enum SampleControlForTest {
+    kNoPreference,
+    kMustChooseNextFrame,
+    kMustNotChooseNextFrame
+  };
+  SampleControlForTest next_frame_sample_control_for_test_ = kNoPreference;
 
   DISALLOW_COPY_AND_ASSIGN(LocalFrameUkmAggregator);
 };

third_party/blink/renderer/core/page/scrolling/scrolling_test.cc

@@ -1441,7 +1441,10 @@ TEST_P(ScrollingTest, UpdateUMAMetricUpdated) {
 
   // After an initial compositing update, we should have one scrolling update
   // recorded as PreFCP.
+  GetWebView()->MainFrameWidget()->RecordStartOfFrameMetrics();
   ForceFullCompositingUpdate();
+  GetWebView()->MainFrameWidget()->RecordEndOfFrameMetrics(base::TimeTicks(),
+                                                           0);
   histogram_tester.ExpectTotalCount("Blink.ScrollingCoordinator.UpdateTime", 1);
   histogram_tester.ExpectTotalCount(
       "Blink.ScrollingCoordinator.UpdateTime.PreFCP", 1);
@@ -1451,7 +1454,10 @@ TEST_P(ScrollingTest, UpdateUMAMetricUpdated) {
       "Blink.ScrollingCoordinator.UpdateTime.AggregatedPreFCP", 0);
 
   // An update with no scrolling changes should not cause a scrolling update.
+  GetWebView()->MainFrameWidget()->RecordStartOfFrameMetrics();
   ForceFullCompositingUpdate();
+  GetWebView()->MainFrameWidget()->RecordEndOfFrameMetrics(base::TimeTicks(),
+                                                           0);
   histogram_tester.ExpectTotalCount("Blink.ScrollingCoordinator.UpdateTime", 1);
   histogram_tester.ExpectTotalCount(
       "Blink.ScrollingCoordinator.UpdateTime.PreFCP", 1);
@@ -1463,27 +1469,34 @@ TEST_P(ScrollingTest, UpdateUMAMetricUpdated) {
   // A change to background color does not need to cause a scrolling update but,
   // because hit test display items paint, we also cause a scrolling coordinator
   // update when the background paints. Also render some text to get past FCP.
+  // Note that this frame is still considered pre-FCP.
   auto* background = GetFrame()->GetDocument()->getElementById("bg");
   background->removeAttribute(html_names::kStyleAttr);
   background->SetInnerHTMLFromString("Some Text");
+  GetWebView()->MainFrameWidget()->RecordStartOfFrameMetrics();
   ForceFullCompositingUpdate();
+  GetWebView()->MainFrameWidget()->RecordEndOfFrameMetrics(base::TimeTicks(),
+                                                           0);
   histogram_tester.ExpectTotalCount("Blink.ScrollingCoordinator.UpdateTime", 2);
   histogram_tester.ExpectTotalCount(
-      "Blink.ScrollingCoordinator.UpdateTime.PreFCP", 1);
+      "Blink.ScrollingCoordinator.UpdateTime.PreFCP", 2);
   histogram_tester.ExpectTotalCount(
-      "Blink.ScrollingCoordinator.UpdateTime.PostFCP", 1);
+      "Blink.ScrollingCoordinator.UpdateTime.PostFCP", 0);
   histogram_tester.ExpectTotalCount(
       "Blink.ScrollingCoordinator.UpdateTime.AggregatedPreFCP", 1);
 
   // Removing a scrollable area should cause a scrolling update.
   auto* scroller = GetFrame()->GetDocument()->getElementById("scroller");
   scroller->removeAttribute(html_names::kStyleAttr);
+  GetWebView()->MainFrameWidget()->RecordStartOfFrameMetrics();
   ForceFullCompositingUpdate();
+  GetWebView()->MainFrameWidget()->RecordEndOfFrameMetrics(base::TimeTicks(),
+                                                           0);
   histogram_tester.ExpectTotalCount("Blink.ScrollingCoordinator.UpdateTime", 3);
   histogram_tester.ExpectTotalCount(
-      "Blink.ScrollingCoordinator.UpdateTime.PreFCP", 1);
+      "Blink.ScrollingCoordinator.UpdateTime.PreFCP", 2);
   histogram_tester.ExpectTotalCount(
-      "Blink.ScrollingCoordinator.UpdateTime.PostFCP", 2);
+      "Blink.ScrollingCoordinator.UpdateTime.PostFCP", 1);
   histogram_tester.ExpectTotalCount(
       "Blink.ScrollingCoordinator.UpdateTime.AggregatedPreFCP", 1);
 }