Add metrics for the Clear-Site-Data header.

content/browser/browsing_data/clear_site_data_throttle.cc

 // Copyright 2016 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 "content/browser/browsing_data/clear_site_data_throttle.h"
 
 #include "base/command_line.h"
 #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"
 #include "content/browser/frame_host/navigation_handle_impl.h"
 #include "content/public/browser/browser_context.h"
 #include "content/public/browser/content_browser_client.h"
 #include "content/public/browser/navigation_handle.h"
 #include "content/public/browser/web_contents.h"
 #include "content/public/common/content_client.h"
 #include "content/public/common/content_switches.h"
@@ skipping 22 matching lines @@
                 const std::string& text,
                 ConsoleMessageLevel level) {
   messages->push_back({url, text, level});
 }
 
 bool AreExperimentalFeaturesEnabled() {
   return base::CommandLine::ForCurrentProcess()->HasSwitch(
       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
 std::unique_ptr<NavigationThrottle>
 ClearSiteDataThrottle::CreateThrottleForNavigation(NavigationHandle* handle) {
   if (AreExperimentalFeaturesEnabled())
     return base::WrapUnique(new ClearSiteDataThrottle(handle));
 
   return std::unique_ptr<NavigationThrottle>();
 }
@@ skipping 68 matching lines @@
 
   bool clear_cookies;
   bool clear_storage;
   bool clear_cache;
 
   if (!ParseHeader(header_value, &clear_cookies, &clear_storage, &clear_cache,
                    &messages_)) {
     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();
   url::Origin origin(current_url_);
 
   if (origin.unique()) {
     ConsoleLog(&messages_, current_url_, "Not supported for unique origins.",
                CONSOLE_MESSAGE_LEVEL_ERROR);
     return;
   }
 
   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,
                  weak_ptr_factory_.GetWeakPtr()));
 }
 
 bool ClearSiteDataThrottle::ParseHeader(const std::string& header,
                                         bool* clear_cookies,
                                         bool* clear_storage,
                                         bool* clear_cache,
@@ skipping 86 matching lines @@
       NOTREACHED();
   }
   ConsoleLog(messages, current_url_, output, CONSOLE_MESSAGE_LEVEL_LOG);
 
   return true;
 }
 
 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();
 }
 
 }  // namespace content