[NoStatePrefetch] Track redirects in UMA

chrome/browser/prerender/prerender_histograms.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 "chrome/browser/prerender/prerender_histograms.h"
 
 #include <string>
 
 #include "base/format_macros.h"
 #include "base/logging.h"
 #include "base/metrics/histogram.h"
 #include "base/strings/stringprintf.h"
 #include "chrome/browser/prerender/prerender_manager.h"
 #include "chrome/browser/prerender/prerender_util.h"
 
 namespace prerender {
 
 namespace {
 
 // This enum is used to define the buckets for the
 // "Prerender.NoStatePrefetchResourceCount" histogram family.
 // Hence, existing enumerated constants should never be deleted or reordered,
 // and new constants should only be appended at the end of the enumeration.
 enum NoStatePrefetchResourceType {
-  MAIN_RESOURCE_CACHEABLE = 0,
-  MAIN_RESOURCE_NO_STORE = 1,
-  SUB_RESOURCE_CACHEABLE = 2,
-  SUB_RESOURCE_NO_STORE = 3,
+  SUB_RESOURCE_CACHEABLE = 0,
+  SUB_RESOURCE_NO_STORE = 1,
+  SUB_RESOURCE_REDIRECT_CACHEABLE = 2,
+  SUB_RESOURCE_REDIRECT_NO_STORE = 3,
+  MAIN_RESOURCE_CACHEABLE = 4,
+  MAIN_RESOURCE_NO_STORE = 5,
+  MAIN_RESOURCE_REDIRECT_CACHEABLE = 6,
+  MAIN_RESOURCE_REDIRECT_NO_STORE = 7,
 
   NO_STATE_PREFETCH_RESOURCE_TYPE_COUNT  // Must be the last.
 };
 
+NoStatePrefetchResourceType GetResourceType(bool is_main_resource,
+                                            bool is_redirect,
+                                            bool is_no_store) {
+  return static_cast<NoStatePrefetchResourceType>(

[pasko, 2016/09/02 16:34:48]

This was confusing to me:
* casting explicitly to the enum that gets casted back when FactoryGet is called
* not using the values declared in the enum except one

I think the code will be more straightforward if we return int32_t (or perhaps
base::HistogramBase::Sample?)

This would be equivalent, but would better suggest that we are using bit fields:

sample = is_main_resource << 2 | is_redirect << 1 | is_no_store;
kPossibleSampleValues = 1 << 3;

[droger, 2016/09/05 12:38:47]

Done using a bitfield.

+      4 * is_main_resource + 2 * is_redirect + is_no_store);
+}
+
 // Time window for which we will record windowed PLTs from the last observed
 // link rel=prefetch tag. This is not intended to be the same as the prerender
 // ttl, it's just intended to be a window during which a prerender has likely
 // affected performance.
 const int kWindowDurationSeconds = 30;
 
 std::string ComposeHistogramName(const std::string& prefix_type,
                                  const std::string& name) {
   if (prefix_type.empty())
     return std::string("Prerender.") + name;
@@ skipping 346 matching lines @@
     PREFIXED_HISTOGRAM(
         "NetworkBytesWasted",
         origin,
         UMA_HISTOGRAM_CUSTOM_COUNTS(
             name, prerender_bytes, kHistogramMin, kHistogramMax, kBucketCount));
   }
 }
 
 void PrerenderHistograms::RecordResourcePrefetch(Origin origin,
                                                  bool is_main_resource,
+                                                 bool is_redirect,
                                                  bool is_no_store) const {
   DCHECK(thread_checker_.CalledOnValidThread());
   NoStatePrefetchResourceType type =
-      is_main_resource
-          ? (is_no_store ? MAIN_RESOURCE_NO_STORE : MAIN_RESOURCE_CACHEABLE)
-          : (is_no_store ? SUB_RESOURCE_NO_STORE : SUB_RESOURCE_CACHEABLE);
+      GetResourceType(is_main_resource, is_redirect, is_no_store);
   DCHECK_LT(type, NO_STATE_PREFETCH_RESOURCE_TYPE_COUNT);
 
   std::string histogram_name =
       GetHistogramName(origin, IsOriginWash(), "NoStatePrefetchResourceTypes");
 
   // Unrolls UMA_HISTOGRAM_ENUMERATION, required to support dynamic histogram
   // name.
   base::HistogramBase* histogram_pointer = base::LinearHistogram::FactoryGet(
       histogram_name, 1, NO_STATE_PREFETCH_RESOURCE_TYPE_COUNT,
       NO_STATE_PREFETCH_RESOURCE_TYPE_COUNT + 1,
       base::HistogramBase::kUmaTargetedHistogramFlag);
   histogram_pointer->Add(type);
 }
 
+void PrerenderHistograms::RecordPrefetchRedirectCount(
+    Origin origin,
+    bool is_main_resource,
+    size_t redirect_count) const {
+  DCHECK(thread_checker_.CalledOnValidThread());
+
+  std::string histogram_base_name = base::StringPrintf(
+      "NoStatePrefetch%sResourceRedirects", is_main_resource ? "Main" : "Sub");
+  std::string histogram_name =
+      GetHistogramName(origin, IsOriginWash(), histogram_base_name);
+
+  // Unrolls UMA_HISTOGRAM_CUSTOM_COUNTS, required to support dynamic histogram

[pasko, 2016/09/02 16:34:48]

can we avoid repeating ourselves twice here?

void PrerenderHistograms::RecordEnumHistogram(std::sting name,
base::HistogramBase::Sample sample, base::HistogramBase::Sample
possible_value_count) {
  DCHECK(thread_checker_.CalledOnValidThread());
  base::LinearHistogram::FactoryGet(name, 1, possible_value_count,
possible_value_count + 1,
base::HistogramBase::kUmaTargetedHistogramFlag)->Add(sample);
}

[droger, 2016/09/05 12:38:47]

Done.

+  // name.
+  const int kMaxRedirectCount = 10;
+  base::HistogramBase* histogram_pointer = base::LinearHistogram::FactoryGet(
+      histogram_name, 1, kMaxRedirectCount, kMaxRedirectCount + 1,
+      base::HistogramBase::kUmaTargetedHistogramFlag);
+  histogram_pointer->Add(redirect_count);
+}
+
 bool PrerenderHistograms::IsOriginWash() const {
   if (!WithinWindow())
     return false;
   return origin_wash_;
 }
 
 }  // namespace prerender