Add metrics for the Clear-Site-Data header.

Add metrics for the Clear-Site-Data header.

We add two histograms:
1. Navigation.ClearSiteData.Parameters - the mask of parameters
2. Navigation.ClearSiteData.Duration - the duration of the clearing task

BUG=607897
Committed: https://crrev.com/ccde81aef18c81da91165e0b31c7b286e584af9a
Cr-Commit-Position: refs/heads/master@{#414597}

[msramek, 2016-08-24 13:09:48]

The CQ bit was checked by msramek@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-24 13:09:59]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[msramek, 2016-08-24 13:14:57]

msramek@chromium.org changed reviewers:
+ clamy@chromium.org, isherman@chromium.org

[msramek, 2016-08-24 13:14:58]

Hi folks,

Please have a look!

Thanks,
Martin

[commit-bot: I haz the power, 2016-08-24 14:04:12]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-24 14:04:13]

Dry run: This issue passed the CQ dry run.

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

LGTM % comments:

https://codereview.chromium.org/2274853002/diff/1/content/browser/browsing_da...
File content/browser/browsing_data/clear_site_data_throttle.cc (right):

https://codereview.chromium.org/2274853002/diff/1/content/browser/browsing_da...
content/browser/browsing_data/clear_site_data_throttle.cc:11: #include
"base/metrics/histogram.h"
nit: Please #include histogram_macros instead.

https://codereview.chromium.org/2274853002/diff/1/content/browser/browsing_da...
content/browser/browsing_data/clear_site_data_throttle.cc:276:
base::TimeTicks::Now() - clearing_started_);
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]

https://codereview.chromium.org/2274853002/diff/1/content/browser/browsing_da...
File content/browser/browsing_data/clear_site_data_throttle.cc (right):

https://codereview.chromium.org/2274853002/diff/1/content/browser/browsing_da...
content/browser/browsing_data/clear_site_data_throttle.cc:276:
base::TimeTicks::Now() - clearing_started_);
On 2016/08/24 22:27:56, Ilya Sherman wrote:
> 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.

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:52:24]

The CQ bit was checked by msramek@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-25 08:52:42]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

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

https://codereview.chromium.org/2274853002/diff/1/content/browser/browsing_da...
File content/browser/browsing_data/clear_site_data_throttle.cc (right):

https://codereview.chromium.org/2274853002/diff/1/content/browser/browsing_da...
content/browser/browsing_data/clear_site_data_throttle.cc:11: #include
"base/metrics/histogram.h"
On 2016/08/24 22:27:56, Ilya Sherman wrote:
> nit: Please #include histogram_macros instead.

Done.

https://codereview.chromium.org/2274853002/diff/1/content/browser/browsing_da...
content/browser/browsing_data/clear_site_data_throttle.cc:276:
base::TimeTicks::Now() - clearing_started_);
On 2016/08/24 23:15:01, clamy wrote:
> On 2016/08/24 22:27:56, Ilya Sherman wrote:
> > 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.
> 
> 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).

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 :)

[commit-bot: I haz the power, 2016-08-25 10:09:27]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-25 10:09:28]

Dry run: This issue passed the CQ dry run.

[clamy, 2016-08-25 17:53:48]

https://codereview.chromium.org/2274853002/diff/1/content/browser/browsing_da...
File content/browser/browsing_data/clear_site_data_throttle.cc (right):

https://codereview.chromium.org/2274853002/diff/1/content/browser/browsing_da...
content/browser/browsing_data/clear_site_data_throttle.cc:276:
base::TimeTicks::Now() - clearing_started_);
On 2016/08/25 08:56:09, msramek wrote:
> On 2016/08/24 23:15:01, clamy wrote:
> > On 2016/08/24 22:27:56, Ilya Sherman wrote:
> > > 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.
> > 
> > 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).
> 
> 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 :)

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:45]

https://codereview.chromium.org/2274853002/diff/1/content/browser/browsing_da...
File content/browser/browsing_data/clear_site_data_throttle.cc (right):

https://codereview.chromium.org/2274853002/diff/1/content/browser/browsing_da...
content/browser/browsing_data/clear_site_data_throttle.cc:276:
base::TimeTicks::Now() - clearing_started_);
On 2016/08/25 17:53:47, clamy wrote:
> On 2016/08/25 08:56:09, msramek wrote:
> > On 2016/08/24 23:15:01, clamy wrote:
> > > On 2016/08/24 22:27:56, Ilya Sherman wrote:
> > > > 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.
> > > 
> > > 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).
> > 
> > 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 :)
> 
> 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.

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.

[clamy, 2016-08-25 22:11:23]

Thanks! Lgtm.

[msramek, 2016-08-25 23:24:57]

Thanks! Landing this now.

[msramek, 2016-08-25 23:25:04]

The CQ bit was checked by msramek@chromium.org

[msramek, 2016-08-25 23:25:04]

The patchset sent to the CQ was uploaded after l-g-t-m from
isherman@chromium.org
Link to the patchset: https://codereview.chromium.org/2274853002/#ps60001
(title: "git cl format")

[commit-bot: I haz the power, 2016-08-25 23:25:41]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-26 00:31:30]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-08-26 00:33:18]

Description was changed from

==========
Add metrics for the Clear-Site-Data header.

We add two histograms:
1. Navigation.ClearSiteData.Parameters - the mask of parameters
2. Navigation.ClearSiteData.Duration - the duration of the clearing task

BUG=607897
==========

to

==========
Add metrics for the Clear-Site-Data header.

We add two histograms:
1. Navigation.ClearSiteData.Parameters - the mask of parameters
2. Navigation.ClearSiteData.Duration - the duration of the clearing task

BUG=607897

Committed: https://crrev.com/ccde81aef18c81da91165e0b31c7b286e584af9a
Cr-Commit-Position: refs/heads/master@{#414597}
==========

[commit-bot: I haz the power, 2016-08-26 00:33:19]

Patchset 4 (id:??) landed as
https://crrev.com/ccde81aef18c81da91165e0b31c7b286e584af9a
Cr-Commit-Position: refs/heads/master@{#414597}