Add Precache.Fetch.MinWeight UMA.

components/precache/core/precache_fetcher.cc

Index: components/precache/core/precache_fetcher.cc
diff --git a/components/precache/core/precache_fetcher.cc b/components/precache/core/precache_fetcher.cc
index 0bc2e1b2b40cb1e2c17a5bfdf84000c00aec1a08..e13d85649ca576239dc99d4707687f1d53b5e614 100644
--- a/components/precache/core/precache_fetcher.cc
+++ b/components/precache/core/precache_fetcher.cc
@@ -392,7 +392,8 @@ void PrecacheFetcher::Fetcher::OnURLFetchComplete(
 void PrecacheFetcher::RecordCompletionStatistics(
     const PrecacheUnfinishedWork& unfinished_work,
     size_t remaining_manifest_urls_to_fetch,
-    size_t remaining_resource_urls_to_fetch) {
+    size_t remaining_resource_urls_to_fetch,
+    base::Optional<double> min_weight_fetched) {
   // These may be unset in tests.
   if (!unfinished_work.has_start_time())
     return;
@@ -419,6 +420,11 @@ void PrecacheFetcher::RecordCompletionStatistics(
   UMA_HISTOGRAM_CUSTOM_COUNTS("Precache.Fetch.ResponseBytes.Network",
                               unfinished_work.network_bytes(), 1,
                               kMaxResponseBytes, 100);
+
+  if (min_weight_fetched.has_value()) {
+    UMA_HISTOGRAM_COUNTS_1000("Precache.Fetch.MinWeight",
+                              min_weight_fetched.value() * 1000);

[twifkak, 2017/02/22 18:20:00]

BTW, the bucket boundaries will look approximately like this:

irb(main):005:0> (0..50).map {|i| (Math.exp(Math.log(1) + i * (Math.log(1000) -
Math.log(1)) / 50.0) / 1000.0).round(4) }
=> [0.001, 0.0011, 0.0013, 0.0015, 0.0017, 0.002, 0.0023, 0.0026, 0.003, 0.0035,
0.004, 0.0046, 0.0052, 0.006, 0.0069, 0.0079, 0.0091, 0.0105, 0.012, 0.0138,
0.0158, 0.0182, 0.0209, 0.024, 0.0275, 0.0316, 0.0363, 0.0417, 0.0479, 0.055,
0.0631, 0.0724, 0.0832, 0.0955, 0.1096, 0.1259, 0.1445, 0.166, 0.1905, 0.2188,
0.2512, 0.2884, 0.3311, 0.3802, 0.4365, 0.5012, 0.5754, 0.6607, 0.7586, 0.871,
1.0]

+  }
 }
 
 // static
@@ -561,12 +567,12 @@ void PrecacheFetcher::StartNextManifestFetches() {
   }
 }
 
-void PrecacheFetcher::NotifyDone(
-    size_t remaining_manifest_urls_to_fetch,
-    size_t remaining_resource_urls_to_fetch) {
-  RecordCompletionStatistics(*unfinished_work_,
-                             remaining_manifest_urls_to_fetch,
-                             remaining_resource_urls_to_fetch);
+void PrecacheFetcher::NotifyDone(size_t remaining_manifest_urls_to_fetch,
+                                 size_t remaining_resource_urls_to_fetch,
+                                 base::Optional<double> min_weight_fetched) {
+  RecordCompletionStatistics(
+      *unfinished_work_, remaining_manifest_urls_to_fetch,
+      remaining_resource_urls_to_fetch, min_weight_fetched);
   precache_delegate_->OnDone();
 }
 
@@ -578,8 +584,13 @@ void PrecacheFetcher::StartNextFetch() {
        unfinished_work_->config_settings().max_bytes_total()) ||
       quota_.remaining() == 0) {
     pool_.DeleteAll();
+    base::Optional<double> min_weight;
+    if (unfinished_work_->config_settings().global_ranking() &&
+        !resources_to_fetch_.empty())

[jamartin, 2017/02/22 22:54:07]

If resources_to_fetch_.empty(), shouldn't you report 0?

[twifkak, 2017/02/23 23:28:41]

Yes. This is a bit of an edge case, I think. If the precache completes without
going over the byte cap, then this won't be run and instead the call at line 605
will. The edge case will happen, though, if the byte cap is hit on the very last
resource.

+      min_weight.emplace(resources_to_fetch_.front().weight);
     NotifyDone(top_hosts_to_fetch_.size() + top_hosts_fetching_.size(),
-               resources_to_fetch_.size() + resources_fetching_.size());
+               resources_to_fetch_.size() + resources_fetching_.size(),
+               min_weight);

[jamartin, 2017/02/22 22:54:07]

I wonder if we may want to report also the rank distribution that was precached
across hosts. I.e., what is the min, max, avg rank that we have precached across
all top hosts.

The reason is that, since we don't know the user's top host frequency, which is
used to compare the weights across hosts, we are missing part of the picture by
only having min_weight and not the rank distribution.

For instance, it can happen that the min_weight is .007 (in host a) yet we left
out a resource with a weight of .42 (in host b) because of a high difference in
host frequency. The min_weight tells us something, but the rank tells us
something else which includes additional data unseen by min_weight.

[twifkak, 2017/02/23 23:28:41]

The problem is that rank 23 for host A may mean something completely different
from host B, if they have different distributions of weight_ratio. (Say host A
has 50 common JS files, and host B has only 5, and then another 50 rare ones.)

I think we can combine History.MonthlyVisitCount and
History.TopHostsVisitsByRank [1] to get an approximate top host frequency for a
site at each position in top hosts, and then divide MinWeight by that to get a
minimum weight_ratio as a function of where the site sits in the user's top
hosts ranking. But perhaps that's too much calculation on top of UMA.

I could return the minimum weight_ratio rather than the minimum weight. Given
that the fetches are only partially ordered by weight_ratio, this seems weird.
But I don't have anything against it other than that. If you're looking to come
up with a conservative server-side threshold, would this work?

[1]
https://uma.googleplex.com/p/chrome/histograms/?endDate=20170221&dayCount=1&h...

     return;
   }
 
@@ -588,7 +599,10 @@ void PrecacheFetcher::StartNextFetch() {
   if (top_hosts_to_fetch_.empty() && resources_to_fetch_.empty() &&
       pool_.IsEmpty()) {
     // There are no more URLs to fetch, so end the precache cycle.
-    NotifyDone(0, 0);
+    base::Optional<double> min_weight;
+    if (unfinished_work_->config_settings().global_ranking())
+      min_weight.emplace(0);

[jamartin, 2017/02/22 22:54:07]

(nit/opt) I prefer min_weight = 0.

[twifkak, 2017/02/23 23:28:42]

Me too. Somehow I missed operator= when scanning the include file.

+    NotifyDone(0, 0, min_weight);
     // OnDone may have deleted this PrecacheFetcher, so don't do anything after
     // it is called.
   }
@@ -660,7 +674,7 @@ void PrecacheFetcher::OnManifestInfoRetrieved(
     // is invalid.
     top_hosts_to_fetch_.clear();
     unfinished_work_->set_num_manifest_urls(manifests_info.size());
-    NotifyDone(manifests_info.size(), resources_to_rank_.size());
+    NotifyDone(manifests_info.size(), resources_to_rank_.size(), base::nullopt);
     return;
   }