Add History.TopHostsVisitsByRank UMA metric.

components/history/core/browser/history_backend.cc

Index: components/history/core/browser/history_backend.cc
diff --git a/components/history/core/browser/history_backend.cc b/components/history/core/browser/history_backend.cc
index d04c038d5bef7fd7c6d97a6bd878e7288a5b94ef..1551714e08348cedde900231496aece05766eec5 100644
--- a/components/history/core/browser/history_backend.cc
+++ b/components/history/core/browser/history_backend.cc
@@ -37,6 +37,7 @@
 #include "components/history/core/browser/keyword_search_term.h"
 #include "components/history/core/browser/page_usage_data.h"
 #include "components/history/core/browser/typed_url_syncable_service.h"
+#include "components/history/core/browser/url_utils.h"
 #include "components/history/core/browser/visit_filter.h"
 #include "net/base/registry_controlled_domains/registry_controlled_domain.h"
 #include "sql/error_delegate_util.h"
@@ -89,6 +90,8 @@ const int kMaxRedirectCount = 32;
 // and is deleted.
 const int kExpireDaysThreshold = 90;
 
+const int kMaxTopHostsForMetrics = 50;

[bengr, 2015/06/13 00:21:14]

Is that big enough?

[twifkak, 2015/06/16 20:25:23]

Bigger would be better, since the median of History.MonthlyHostCount is 75ish
and the mode is 120ish. However, the default is 50 buckets, and I didn't want to
rock any boats. What are the trade-offs involved in picking more buckets?
Alternatively, I could choose a coarser granularity. Thoughts?

+
 // Converts from PageUsageData to MostVisitedURL. |redirects| is a
 // list of redirects for this URL. Empty list means no redirects.
 MostVisitedURL MakeMostVisitedURL(const PageUsageData& page_data,
@@ -416,7 +419,15 @@ TopHostsList HistoryBackend::TopHosts(int num_hosts) const {
   if (!db_)
     return TopHostsList();
 
-  return db_->TopHosts(num_hosts);
+  auto top_hosts = db_->TopHosts(num_hosts);
+
+  host_ranks_.clear();
+  int i = 1;
+  for (const auto& host_count : top_hosts) {

[bengr, 2015/06/13 00:21:14]

Not required, but you could make this slightly more compact:

int i = 0;
for (const auto& host_count : top_hosts)
  host_ranks[host_count.first] = ++i;

[twifkak, 2015/06/16 20:25:23]

Done. I chose the other way because strahinja doesn't like the use of ++i as an
expression, since it requires mentally tracking some state when reading. But I
have no strong opinion.

+    host_ranks_[host_count.first] = i;
+    ++i;
+  }
+  return top_hosts;
 }
 
 void HistoryBackend::AddPage(const HistoryAddPageArgs& request) {
@@ -725,6 +736,15 @@ void HistoryBackend::CloseAllDatabases() {
   }
 }
 
+void HistoryBackend::RecordTopHostsMetrics(const GURL& url) {
+  int host_rank = host_ranks_[HostForTopHosts(url)];

[sky, 2015/06/15 17:28:17]

This always results in inserting into host_ranks_, which I don't think you want.
Use find to determine if the value is present.

[twifkak, 2015/06/16 20:25:23]

Oof, fail. Done.

[Mark P, 2015/06/16 20:54:31]

This makes me think you should either have unit tests or at least test this
using about:histograms.

[twifkak, 2015/06/17 00:05:23]

A unit test (or about:histograms) wouldn't have caught this particular bug,
since 0 was the correct default value. The only effect would have been unbounded
memory growth.

That said, a unit test is a good idea. Added. Found a bug in this latest
find-based version and fixed it.

+  if (host_rank == 0 || host_rank > kMaxTopHostsForMetrics)
+    host_rank = kMaxTopHostsForMetrics + 1;
+
+  UMA_HISTOGRAM_ENUMERATION("History.TopHostsVisitsByRank", host_rank,

[sky, 2015/06/15 17:28:17]

Not being familiar with TopHosts, I'm not sure how often it is called. If I
leave the browser open for a long time won't host_ranks_ get stale? Seems to me
it would be better to periodically query for the data you want and not
internally cache host_ranks_.

[twifkak, 2015/06/16 20:25:23]

TopHosts will be computed approximately once or twice per day for users in the
Precache experiment.

I wanted to write a metric that didn't have a significant user impact, since
that was a prominent concern on the CL that I reference in the description. If I
periodically compute host_ranks_, I will block other history-needing features,
such as autocomplete, I will have an impact on IO usage, which may affect other
tasks, and I will impact battery usage. PrecacheLauncher solves this by querying
only when the device is on-power and not presently interactive -- something
which can not (and probably should not) be done from within HistoryBackend,
AFAIK.

[sky, 2015/06/30 19:17:26]

Sure, but then how realistic is your metric if it gets stale?

[twifkak, 2015/06/30 21:14:43]

My intended use for this metric is to gather data with which to tweak the
parameters for the Precache experiment. The Precache experiment prefetches
static resources for the top N hosts, as determined by calling TopHosts(). As
such, the metric is actually the most realistic reflection of what I want to
determine: an estimate of the experiment's impact for a given N.

As far as how realistic it is for uses other than my intended use, I guess I'd
need some ideas of what those uses might be in order to determine the level of
precision they require.

+                            kMaxTopHostsForMetrics + 2);
+}
+
 std::pair<URLID, VisitID> HistoryBackend::AddPageVisit(
     const GURL& url,
     Time time,
@@ -745,6 +765,11 @@ std::pair<URLID, VisitID> HistoryBackend::AddPageVisit(
        transition_type == ui::PAGE_TRANSITION_KEYWORD_GENERATED))
     typed_increment = 1;
 
+  if (!host_ranks_.empty() && visit_source == SOURCE_BROWSED &&
+      (transition & ui::PAGE_TRANSITION_CHAIN_END)) {
+    RecordTopHostsMetrics(url);
+  }
+
   // See if this URL is already in the DB.
   URLRow url_info(url);
   URLID url_id = db_->GetRowForURL(url, &url_info);