Add metrics for the Clear-Site-Data header.

content/browser/browsing_data/clear_site_data_throttle.cc

Index: content/browser/browsing_data/clear_site_data_throttle.cc
diff --git a/content/browser/browsing_data/clear_site_data_throttle.cc b/content/browser/browsing_data/clear_site_data_throttle.cc
index ca040a49e8e5ba8e435da0a734fd7b5f7e7994ce..788be8bf27e29fb9fb7e3a31de878cb155024551 100644
--- a/content/browser/browsing_data/clear_site_data_throttle.cc
+++ b/content/browser/browsing_data/clear_site_data_throttle.cc
@@ -8,6 +8,7 @@
 #include "base/json/json_reader.h"
 #include "base/json/json_string_value_serializer.h"
 #include "base/memory/ptr_util.h"
+#include "base/metrics/histogram.h"

[Ilya Sherman, 2016/08/24 22:27:56]

nit: Please #include histogram_macros instead.

[msramek, 2016/08/25 08:56:09]

Done.

 #include "base/strings/string_util.h"
 #include "base/strings/stringprintf.h"
 #include "base/values.h"
@@ -50,6 +51,14 @@ bool AreExperimentalFeaturesEnabled() {
       switches::kEnableExperimentalWebPlatformFeatures);
 }
 
+// Represents the parameters as a single number to be recorded in a histogram.
+int ParametersMask(
+    bool clear_cookies, bool clear_storage, bool clear_cache) {
+  return static_cast<int>(clear_cookies) * (1 << 0) +
+      static_cast<int>(clear_storage) * (1 << 1) +
+      static_cast<int>(clear_cache) * (1 << 2);
+}
+
 }  // namespace
 
 // static
@@ -138,6 +147,11 @@ void ClearSiteDataThrottle::HandleHeader() {
     return;
   }
 
+  // Record the call parameters.
+  UMA_HISTOGRAM_ENUMERATION(
+      "Navigation.ClearSiteData.Parameters",
+      ParametersMask(clear_cookies, clear_storage, clear_cache), (1 << 3));
+
   // If the header is valid, clear the data for this browser context and origin.
   BrowserContext* browser_context =
       navigation_handle()->GetWebContents()->GetBrowserContext();
@@ -150,6 +164,7 @@ void ClearSiteDataThrottle::HandleHeader() {
   }
 
   clearing_in_progress_ = true;
+  clearing_started_ = base::TimeTicks::Now();
   GetContentClient()->browser()->ClearSiteData(
       browser_context, origin, clear_cookies, clear_storage, clear_cache,
       base::Bind(&ClearSiteDataThrottle::TaskFinished,
@@ -256,6 +271,10 @@ bool ClearSiteDataThrottle::ParseHeader(const std::string& header,
 void ClearSiteDataThrottle::TaskFinished() {
   DCHECK(clearing_in_progress_);
   clearing_in_progress_ = false;
+
+  UMA_HISTOGRAM_TIMES("Navigation.ClearSiteData.Duration",
+                      base::TimeTicks::Now() - clearing_started_);

[Ilya Sherman, 2016/08/24 22:27:56]

Do you have a guess for roughly how long a "typical" execution of this task
takes?  I somewhat suspect that 10 seconds -- the upper bound for
UMA_HISTOGRAM_TIMES -- is too small for this histogram.  Probably
UMA_HISTOGRAM_MEDIUM_TIMES would be more appropriate... though you understand
the expected time range better than I do.

[clamy, 2016/08/24 23:15:01]

IIUC, you're blocking the navigation during the time the data gets cleared, so I
imagined it's not taking 10s (or at least I hope).

[msramek, 2016/08/25 08:56:09]

Clearing browsing data *can* potentially take a lot of time, but we hope that it
usually won't, since we're only targeting a small set of entries in each data
storage backend (i.e. those for one site).

In any case, we are indeed blocking the navigation and we don't want to do that
for more than 10 seconds. The current plan is that if clearing often takes more
than a few seconds, we will implement a hybrid solution where the navigation
will be allowed to continue and we will finish the deletion asynchronously. If
it regularly takes more than a few seconds, I guess we'll have to return to the
asynchronous approach.

So in general, if we don't fit into the histogram, we see it as a problem of the
implementation, not of the histogram :)

[clamy, 2016/08/25 17:53:47]

The median Page Load Time on Android is 2.5s right now. Blocking the navigation
for more than 100ms would mean a 4% decrease in PLT. When we consider how hard
it is to get us an improvement of 5% PLT, I would argue that anything even
around that figure is an issue, which the loading team will point out. Which is
why I would like you to follow Ilya's advice and move to a type of histogram
that has a much smaller highest bucket.

[msramek, 2016/08/25 18:39:44]

I lowered it to 50 buckets per 1 second. That's 20ms resolution. Would that be
OK?

It's still more than 100ms of course, but I don't need a higher resolution
either. I'm happy to discuss with you what delays are acceptable for the loading
team once we have the data, but for now, this is an experimental feature behind
a flag, so it doesn't affect PLTs in the real world. And I don't want to
underestimate it and get unusable data.

+
   navigation_handle()->Resume();
 }