[PageLoadMetrics] Keep track of Ad Sizes on Pages

chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.cc

Index: chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.cc
diff --git a/chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.cc b/chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.cc
new file mode 100644
index 0000000000000000000000000000000000000000..eae71da5ee3f67f144d62857ca4612dcbb013c7a
--- /dev/null
+++ b/chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.cc
@@ -0,0 +1,253 @@
+// 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.
+
+#include "chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.h"
+
+#include <string>
+
+#include "base/feature_list.h"
+#include "base/memory/ptr_util.h"
+#include "base/strings/string_util.h"
+#include "chrome/browser/page_load_metrics/page_load_metrics_util.h"
+#include "content/public/browser/navigation_handle.h"
+#include "content/public/browser/render_frame_host.h"
+#include "content/public/browser/web_contents.h"
+#include "url/gurl.h"
+
+namespace {
+
+const base::Feature kAdsFeature{"AdsMetrics", base::FEATURE_ENABLED_BY_DEFAULT};
+
+bool FrameIsAd(content::NavigationHandle* navigation_handle) {
+  int frame_tree_node_id = navigation_handle->GetFrameTreeNodeId();
+  content::RenderFrameHost* current_frame_host =
+      navigation_handle->GetWebContents()->FindFrameByFrameTreeNodeId(

[Bryan McQuade, 2017/04/14 19:19:23]

is it possible for FindFrameByFrameTreeNodeId to return nullptr? should we guard
against that case?

[Charlie Harrison, 2017/04/14 20:16:26]

I think we should be able to DCHECK this especially if we're only calling this
from DidFinishNavigation.

[Bryan McQuade, 2017/04/17 22:00:04]

actually, can we just call
navigation_handle->GetRenderFrameHost() instead?

ah, ok, i see jam's comment - seems like we can do this

[jkarlin, 2017/04/24 17:27:58]

Implemented jam's suggestion and dcheck the pointer.

+          frame_tree_node_id);

[jam, 2017/04/17 15:41:55]

these two lines can just be navigation_handle->GetRenderFrameHost which is
guaranteed to be null

[jkarlin, 2017/04/24 17:27:58]

Done.

+  const std::string& name = current_frame_host->GetFrameName();
+  const GURL& url = navigation_handle->GetURL();
+
+  // Because sub-resource filtering isn't always enabled, and doesn't work
+  // well in monitoring mode (no CSS enforcement), it's difficult to identify
+  // ads. Google ads are prevalent and easy to track, so we'll start by
+  // tracking those. Note that the frame name can be very large, so be careful
+  // to avoid full string searches if possible.
+  // TODO(jkarlin): Track other ad networks that are easy to identify.
+  return base::StartsWith(name, "google_ads_iframe",
+                          base::CompareCase::SENSITIVE) ||
+         base::StartsWith(name, "google_ads_frame",
+                          base::CompareCase::SENSITIVE) ||
+         (url.host_piece() == "tpc.googlesyndication.com" &&
+          base::StartsWith(url.path_piece(), "/safeframe",
+                           base::CompareCase::SENSITIVE));
+}
+
+}  // namespace
+
+// static
+std::unique_ptr<AdsPageLoadMetricsObserver>
+AdsPageLoadMetricsObserver::CreateIfNeeded() {
+  if (!base::FeatureList::IsEnabled(kAdsFeature))
+    return nullptr;
+  return base::MakeUnique<AdsPageLoadMetricsObserver>();
+}
+
+AdsPageLoadMetricsObserver::AdsPageLoadMetricsObserver() = default;
+AdsPageLoadMetricsObserver::~AdsPageLoadMetricsObserver() = default;
+
+page_load_metrics::PageLoadMetricsObserver::ObservePolicy
+AdsPageLoadMetricsObserver::OnCommit(
+    content::NavigationHandle* navigation_handle) {
+  DCHECK(ad_frames_data_.empty());

[Charlie Harrison, 2017/04/14 20:16:26]

nit: include base/logging for DCHECK

[jkarlin, 2017/04/24 17:27:58]

Done.

+
+  // The main frame is never considered an ad.
+  ad_frames_data_[navigation_handle->GetFrameTreeNodeId()] = nullptr;
+  ProcessOngoingNavigationResources(navigation_handle->GetFrameTreeNodeId());
+  return CONTINUE_OBSERVING;
+}
+
+page_load_metrics::PageLoadMetricsObserver::ObservePolicy
+AdsPageLoadMetricsObserver::OnCommitSubFrame(
+    content::NavigationHandle* navigation_handle) {
+  FrameTreeNodeId frame_tree_node_id = navigation_handle->GetFrameTreeNodeId();
+  content::RenderFrameHost* parent_frame_host =
+      navigation_handle->GetRenderFrameHost()->GetParent();
+  DCHECK(parent_frame_host);
+
+  bool top_level_subframe = !parent_frame_host->GetParent();
+
+  const auto& id_and_data = ad_frames_data_.find(frame_tree_node_id);
+  if (id_and_data != ad_frames_data_.end()) {
+    // An existing subframe is navigating again.
+    if (id_and_data->second) {
+      // The subframe was an ad to begin with, keep tracking it as an ad.
+      ProcessOngoingNavigationResources(frame_tree_node_id);
+      return CONTINUE_OBSERVING;
+    }
+    // This frame was previously not an ad, process it as usual. If it had

[Bryan McQuade, 2017/04/14 19:19:23]

do we expect repeat navs for ads? if not, it might be worth counting how common
it is for either a previously committed frame that wasn't considered an ad to
load an ad, or a previously committed frame that was an ad to commit. you could
add an enum counter histogram to understand the frequency here.

[Charlie Harrison, 2017/04/14 20:16:26]

+1

[jkarlin, 2017/04/24 17:27:58]

Certainly doable. What would we do with this information?

[Bryan McQuade, 2017/04/24 19:28:22]

I expect most people looking at the rest of the histograms this class produces
aren't thinking about these cases when they try to reason about what the data
means, so I think it'd be good to confirm that these cases are rare and thus
it's reasonable for us to effectively ignore them.

For instance if a frame starts as an ad and then becomes a non-ad but we
continue to attribute its bytes as ads, is that correct? Incorrect? If it's a
rare event it doesn't matter. If we don't know if it's rare then we are facing
an unknown in our data that could make it hard to reason about that data when
thinking about questions like this.

If it's hard to add these counters, no need, but if it's easy, I think it'd be
helpful to confirm that these cases are rare.

+    // any child frames that were ads, those will still be recorded.
+  } else if (top_level_subframe) {
+    top_level_subframe_count_ += 1;
+  }
+
+  // Determine who the parent frame's ad ancestor is.
+  const auto& parent_id_and_data =
+      ad_frames_data_.find(parent_frame_host->GetFrameTreeNodeId());
+  DCHECK(parent_id_and_data != ad_frames_data_.end());
+  AdFrameData* ad_data = parent_id_and_data->second;
+
+  if (!ad_data && FrameIsAd(navigation_handle)) {
+    // This frame is not nested within an ad frame but is itself an ad.
+    ad_frames_data_storage_.push_back(AdFrameData());

[Bryan McQuade, 2017/04/14 19:19:23]

i'm not deeply familiar with it, but this looks like a case where you could use
emplace_back() without any argument instead of push_back. take a look and see
what you think.

[jkarlin, 2017/04/24 17:27:59]

Done.

+    ad_data = &ad_frames_data_storage_.back();
+  }
+
+  ad_frames_data_[frame_tree_node_id] = ad_data;
+
+  if (top_level_subframe && ad_data)
+    top_level_ad_frame_count_ += 1;
+
+  ProcessOngoingNavigationResources(frame_tree_node_id);
+  return CONTINUE_OBSERVING;
+}
+
+page_load_metrics::PageLoadMetricsObserver::ObservePolicy
+AdsPageLoadMetricsObserver::FlushMetricsOnAppEnterBackground(
+    const page_load_metrics::PageLoadTiming& timing,
+    const page_load_metrics::PageLoadExtraInfo& extra_info) {
+  // The browser may come back, but there is no guarantee. To be safe, record
+  // what we have now and ignore future changes to this navigation.
+  if (extra_info.did_commit)
+    RecordHistograms();
+
+  return STOP_OBSERVING;
+}
+
+void AdsPageLoadMetricsObserver::OnLoadedResource(
+    const page_load_metrics::ExtraRequestInfo& extra_request_info) {
+  const auto& id_and_data =
+      ad_frames_data_.find(extra_request_info.frame_tree_node_id);
+  if (id_and_data == ad_frames_data_.end()) {
+    // This resouce is for a frame that hasn't yet committed. It must be the
+    // main document for the frame. Hold onto it and once it commits we'll run
+    // it in ProcessOngoingNavigationResources.
+    // TODO(jkarlin): Plumb the resource type through and DCHECK that the type
+    // is document.
+
+    // Because ExtraRequestInfo doesn't have a default constructor,  use insert
+    // to add the entry to the map.
+    auto it_and_success = ongoing_navigation_resources_.insert(std::make_pair(
+        extra_request_info.frame_tree_node_id, extra_request_info));
+    if (!it_and_success.second)

[Bryan McQuade, 2017/04/17 22:00:04]

should we dcheck that no entry already exists for the given frame?

could the following happen?

1. OnLoadedResource fires for a frame which has't yet committed, so we stuff the
resource into ongoing_navigation_resources_

2. the navigation in the frame fails to commit

3. a new navigation commits in the same frame

4. the OnLoadedResource for the nav that failed to commit gets associated with
the new nav

If so, is this undesirable behavior? Perhaps we should change OnCommitSubFrame
to DidFinishSubframeNavigation, and invoke it for both commits and non-commits,
and in step (2) above, clear the entry created in step (1)?

[jkarlin, 2017/04/24 17:27:59]

Yes, that can happen hence the lack of dcheck.
Good idea, done. Also added a new test.

+      it_and_success.first->second = extra_request_info;
+    return;
+  }
+
+  page_bytes_ += extra_request_info.raw_body_bytes;
+  if (!extra_request_info.was_cached)
+    uncached_page_bytes_ += extra_request_info.raw_body_bytes;
+
+  // Determine if the frame (or its ancestor) is an ad, if so attribute the
+  // bytes to the highest ad ancestor.
+  AdFrameData* ancestor_data = id_and_data->second;
+
+  if (ancestor_data) {
+    ancestor_data->frame_bytes += extra_request_info.raw_body_bytes;
+    if (!extra_request_info.was_cached) {
+      ancestor_data->frame_bytes_uncached += extra_request_info.raw_body_bytes;
+    }
+  }
+}
+
+void AdsPageLoadMetricsObserver::OnComplete(
+    const page_load_metrics::PageLoadTiming& timing,
+    const page_load_metrics::PageLoadExtraInfo& info) {
+  RecordHistograms();
+}
+
+void AdsPageLoadMetricsObserver::RecordHistograms() {
+  if (page_bytes_ == 0)
+    return;
+
+  size_t total_ad_frame_bytes = 0;
+  size_t uncached_ad_frame_bytes = 0;
+
+  for (const AdFrameData& ad_frame_data : ad_frames_data_storage_) {
+    total_ad_frame_bytes += ad_frame_data.frame_bytes;
+    uncached_ad_frame_bytes += ad_frame_data.frame_bytes_uncached;
+
+    PAGE_BYTES_HISTOGRAM("PageLoad.Clients.Ads.Google.Bytes.AdFrameBytes",

[Bryan McQuade, 2017/04/18 17:11:32]

wdyt about triyng to use a naming scheme more consistent with our existing bytes
histograms? we use '.Network', '.Cache', and '.Total'. could we use the same
suffixes in this class as well? e.g.
PageLoad.Clients.Ads.Google.Bytes.PerAdFrame.Total and
PageLoad.Clients.Ads.Google.Bytes.PerAdFrame.Network?

[jkarlin, 2017/04/24 17:27:59]

Good idea, done.

+                         ad_frame_data.frame_bytes);
+    PAGE_BYTES_HISTOGRAM(
+        "PageLoad.Clients.Ads.Google.Bytes.AdFrameBytesFromNetwork",
+        ad_frame_data.frame_bytes_uncached);
+    if (ad_frame_data.frame_bytes > 0) {
+      UMA_HISTOGRAM_PERCENTAGE(
+          "PageLoad.Clients.Ads.Google.Bytes.PercentAdFrameBytesFromNetwork",

[Bryan McQuade, 2017/04/18 17:11:33]

these percentage histograms have historically been confusing since pages w/
small and large number of bytes are mixed together. so if this says 90% for
example, does that mean there's a big opportunity to reduce network utilization,
or does it just mean that most frames only load a small number of bytes, mostly
from network? my experience is that these histos are not very actionable.

would it be possible to infer something as useful by just taking the ratio of
sums for network bytes and total bytes in aggregate when looking at the uma
dash?

i'm fine keeping this if you want it - just want to raise the potential concern
that these can be misleading early on.

[jkarlin, 2017/04/24 17:27:58]

I think it's useful to be able to make statements like, "the median page loads
less than 30% of its ad data from the network, and for 95% of page loads ads
make up less than 40% of bytes" etc. So I'd like to keep them.

+          ad_frame_data.frame_bytes_uncached * 100 / ad_frame_data.frame_bytes);
+    }
+  }
+
+  UMA_HISTOGRAM_BOOLEAN("PageLoad.Clients.Ads.Google.PageHasNoAds",

[Bryan McQuade, 2017/04/18 17:11:33]

could we instead have a single counter for number of ad frames per page, and the
%/count where count is zero can be used to compute this histogram?

[jkarlin, 2017/04/24 17:27:59]

Done.

+                        ad_frames_data_storage_.empty());
+
+  // Don't post UMA for pages that don't have ads.
+  if (ad_frames_data_storage_.empty())

[Bryan McQuade, 2017/04/18 17:11:32]

can we move the earlier histo that reports ad counts, as well as this return
statement, to the top of this function?

[jkarlin, 2017/04/24 17:27:58]

Done.

+    return;
+
+  UMA_HISTOGRAM_COUNTS_1000("PageLoad.Clients.Ads.Google.TopLevelSubFrameCount",
+                            top_level_subframe_count_);
+  UMA_HISTOGRAM_COUNTS_1000("PageLoad.Clients.Ads.Google.TopLevelAdFrameCount",

[Bryan McQuade, 2017/04/18 17:11:33]

since you have 'Ads.Google.AdFrameCount' below, could we suffix this, and make
it 'Ads.Google.AdFrameCount.TopLevel'? and maybe also SubFrameCount.TopLevel
above as well?

[jkarlin, 2017/04/24 17:27:59]

Done, though I didn't use proper suffixes in the histograms.xml as I'm not sure
it's worth the added complexity for reviewers.

+                            top_level_ad_frame_count_);
+
+  DCHECK_LT(0, top_level_subframe_count_);  // Because ad frames isn't empty.
+  UMA_HISTOGRAM_PERCENTAGE(
+      "PageLoad.Clients.Ads.Google.PercentTopLevelSubFramesAreAdFrames",
+      top_level_ad_frame_count_ * 100 / top_level_subframe_count_);
+
+  PAGE_BYTES_HISTOGRAM("PageLoad.Clients.Ads.Google.Bytes.AllAdFramesBytes",

[Bryan McQuade, 2017/04/18 17:11:32]

here too if we use the 'Bytes.PerAdFrame.Total' above, we can use
'Bytes.AllAdFrames.Total' here which is more consistent with naming elsewhere.
(could potentially shorten to Bytes.Ads.Total and Bytes.NonAds.Total, but that
may be slightly misleading in that it's only reporting ad frame bytes whereas
some ads are loaded in the top level page, so AllAdFrames is probably more
accurate naming)

[jkarlin, 2017/04/24 17:27:59]

Done.

+                       total_ad_frame_bytes);
+
+  PAGE_BYTES_HISTOGRAM(
+      "PageLoad.Clients.Ads.Google.Bytes.PageBytesSansAllAdFrames",

[Bryan McQuade, 2017/04/18 17:11:33]

Bytes.NonAdFrames.Total?

[jkarlin, 2017/04/24 17:27:59]

Done.

+      page_bytes_ - total_ad_frame_bytes);
+
+  PAGE_BYTES_HISTOGRAM("PageLoad.Clients.Ads.Google.Bytes.PageBytes",

[Bryan McQuade, 2017/04/18 17:11:32]

Bytes.Total?

[jkarlin, 2017/04/24 17:27:59]

Done.

+                       page_bytes_);
+  PAGE_BYTES_HISTOGRAM("PageLoad.Clients.Ads.Google.Bytes.PageBytesFromNetwork",

[Bryan McQuade, 2017/04/18 17:11:32]

Bytes.Network?

[jkarlin, 2017/04/24 17:27:58]

Done.

+                       uncached_page_bytes_);
+  if (page_bytes_) {
+    UMA_HISTOGRAM_PERCENTAGE(
+        "PageLoad.Clients.Ads.Google.Bytes.PercentPageBytesFromAllAdFrames",
+        total_ad_frame_bytes * 100 / page_bytes_);
+  }
+
+  UMA_HISTOGRAM_COUNTS_1000("PageLoad.Clients.Ads.Google.AdFrameCount",
+                            ad_frames_data_storage_.size());
+
+  PAGE_BYTES_HISTOGRAM(
+      "PageLoad.Clients.Ads.Google.Bytes.AllAdFramesBytesFromNetwork",

[Bryan McQuade, 2017/04/18 17:11:32]

Bytes.AllAdFrames.Total?

[jkarlin, 2017/04/24 17:27:58]

I think you mean Bytes.AllAdFrames.Network. Done.

+      uncached_ad_frame_bytes);
+
+  if (total_ad_frame_bytes) {
+    UMA_HISTOGRAM_PERCENTAGE(
+        "PageLoad.Clients.Ads.Google.Bytes.PercentAllAdFramesBytesFromNetwork",
+        uncached_ad_frame_bytes * 100 / total_ad_frame_bytes);
+  }
+  if (uncached_page_bytes_ > 0) {
+    UMA_HISTOGRAM_PERCENTAGE(
+        "PageLoad.Clients.Ads.Google.Bytes."
+        "PercentPageNetworkBytesFromAllAdFrames",
+        uncached_ad_frame_bytes * 100 / uncached_page_bytes_);
+  }
+}
+
+void AdsPageLoadMetricsObserver::ProcessOngoingNavigationResources(
+    FrameTreeNodeId frame_tree_node_id) {
+  const auto& frame_id_and_request =
+      ongoing_navigation_resources_.find(frame_tree_node_id);
+  if (frame_id_and_request == ongoing_navigation_resources_.end())
+    return;
+  OnLoadedResource(frame_id_and_request->second);

[Bryan McQuade, 2017/04/17 22:00:04]

can we instead create a method ProcessLoadedResource that does the processing
and invoke that here, and in OnLoadedResource in cases where the frame has
already committed? It's potentially surprising for a newcomer to the code that
this class invokes a method that's normally only invoked by PageLoadTracker.

[jkarlin, 2017/04/24 17:27:58]

Done.

+  ongoing_navigation_resources_.erase(frame_id_and_request);
+}