Add History.TopHostsVisitsByRank UMA metric.

components/history/core/browser/history_backend.cc

 // Copyright (c) 2012 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 "components/history/core/browser/history_backend.h"
 
 #include <algorithm>
 #include <functional>
 #include <list>
 #include <map>
@@ skipping 19 matching lines @@
 #include "components/history/core/browser/history_backend_observer.h"
 #include "components/history/core/browser/history_client.h"
 #include "components/history/core/browser/history_constants.h"
 #include "components/history/core/browser/history_database.h"
 #include "components/history/core/browser/history_database_params.h"
 #include "components/history/core/browser/history_db_task.h"
 #include "components/history/core/browser/in_memory_history_backend.h"
 #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"
 #include "third_party/skia/include/core/SkBitmap.h"
 #include "ui/gfx/codec/png_codec.h"
 #include "url/gurl.h"
 #include "url/url_constants.h"
 
 #if defined(OS_IOS)
 #include "base/ios/scoped_critical_action.h"
@@ skipping 32 matching lines @@
 const int kFaviconRefetchDays = 7;
 
 // The maximum number of items we'll allow in the redirect list before
 // deleting some.
 const int kMaxRedirectCount = 32;
 
 // The number of days old a history entry can be before it is considered "old"
 // 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,
                                   const RedirectList& redirects) {
   MostVisitedURL mv;
   mv.url = page_data.GetURL();
   mv.title = page_data.GetTitle();
   if (redirects.empty()) {
     // Redirects must contain at least the target url.
     mv.redirects.push_back(mv.url);
@@ skipping 307 matching lines @@
                                    ? end_ts - visit_row.visit_time
                                    : TimeDelta::FromMicroseconds(0);
     db_->UpdateVisitRow(visit_row);
   }
 }
 
 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) {
   if (!db_)
     return;
 
   // Will be filled with the URL ID and the visit ID of the last addition.
   std::pair<URLID, VisitID> last_ids(
       0, tracker_.GetLastVisit(request.context_id, request.nav_entry_id,
                                request.referrer));
@@ skipping 288 matching lines @@
     db_.reset();
     // Forget the first recorded time since the database is closed.
     first_recorded_time_ = base::Time();
   }
   if (thumbnail_db_) {
     thumbnail_db_->CommitTransaction();
     thumbnail_db_.reset();
   }
 }
 
+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,
     VisitID referring_visit,
     ui::PageTransition transition,
     VisitSource visit_source) {
   // Top-level frame navigations are visible, everything else is hidden
   bool new_hidden = !ui::PageTransitionIsMainFrame(transition);
 
   // NOTE: This code must stay in sync with
   // ExpireHistoryBackend::ExpireURLsForVisits().
   int typed_increment = 0;
   ui::PageTransition transition_type =
       ui::PageTransitionStripQualifier(transition);
   if (ui::PageTransitionIsNewNavigation(transition) &&
       ((transition_type == ui::PAGE_TRANSITION_TYPED &&
         !ui::PageTransitionIsRedirect(transition)) ||
        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);
   if (url_id) {
     // Update of an existing row.
     if (ui::PageTransitionStripQualifier(transition) !=
         ui::PAGE_TRANSITION_RELOAD)
       url_info.set_visit_count(url_info.visit_count() + 1);
     if (typed_increment)
       url_info.set_typed_count(url_info.typed_count() + typed_increment);
@@ skipping 1892 matching lines @@
   return true;
 }
 
 HistoryClient* HistoryBackend::GetHistoryClient() {
   if (history_client_)
     history_client_->BlockUntilBookmarksLoaded();
   return history_client_;
 }
 
 }  // namespace history