Don't log background navigations in StaleWhileRevalidateExperiment histograms

Don't log background navigations in StaleWhileRevalidateExperiment histograms

Early results from the
PageLoad.Clients.StaleWhileRevalidateExperiment.Timing2.* histograms
show that there is a fat tail of noise from background navigations.

Remove navigations that started in the background and those that were
backgrounded before the timing event took place. The method is identical
to the PageLoad.Clients.FromGWS.Timing2.* histograms.

BUG=565458
TEST=manually tested with about:histograms

Committed: https://crrev.com/096b91575a69187a3f9e7b6484db5ac8ee9ed452
Cr-Commit-Position: refs/heads/master@{#364366}

[Adam Rice, 2015-12-07 21:31:29]

ricea@chromium.org changed reviewers:
+ csharrison@chromium.org

[Charlie Harrison, 2015-12-07 22:15:25]

csharrison@chromium.org changed reviewers:
+ rdsmith@chromium.org

[Charlie Harrison, 2015-12-07 22:15:26]

lgtm, adding rdsmith for OWNERS.

[Charlie Harrison, 2015-12-08 18:44:49]

On 2015/12/07 22:15:26, csharrison wrote:
> lgtm, adding rdsmith for OWNERS.

Actually, would you mind renaming ShouldLogEvent and putting it in
page_load_metrics_util (and updating the FromGWS observer)? Maybe something like
EventOccurredInForeground.

[Adam Rice, 2015-12-08 20:00:17]

On 2015/12/08 18:44:49, csharrison wrote:
> Actually, would you mind renaming ShouldLogEvent and putting it in
> page_load_metrics_util (and updating the FromGWS observer)? Maybe something
like
> EventOccurredInForeground.

Done. I tried to rewrite the comment in a more general way. Let me know
what you think.

[Charlie Harrison, 2015-12-08 20:26:21]

Thanks!

https://codereview.chromium.org/1507813002/diff/20001/components/page_load_me...
File components/page_load_metrics/browser/page_load_metrics_util.h (right):

https://codereview.chromium.org/1507813002/diff/20001/components/page_load_me...
components/page_load_metrics/browser/page_load_metrics_util.h:27: // the
priority is lowered, and the load timing events no longer reflect the
I don't know if the priority is always lowered. rdsmith@ might know more. I
would also add that some events (e.g. paint) are delayed until foreground, which
is the root cause of many long tailed histograms.

[Adam Rice, 2015-12-09 06:29:00]

https://codereview.chromium.org/1507813002/diff/20001/components/page_load_me...
File components/page_load_metrics/browser/page_load_metrics_util.h (right):

https://codereview.chromium.org/1507813002/diff/20001/components/page_load_me...
components/page_load_metrics/browser/page_load_metrics_util.h:27: // the
priority is lowered, and the load timing events no longer reflect the
On 2015/12/08 20:26:21, csharrison wrote:
> I don't know if the priority is always lowered. rdsmith@ might know more. I
> would also add that some events (e.g. paint) are delayed until foreground,
which
> is the root cause of many long tailed histograms.

I reworded the comment. PTAL.

[Charlie Harrison, 2015-12-09 13:52:26]

On 2015/12/09 06:29:00, Adam Rice wrote:
>
https://codereview.chromium.org/1507813002/diff/20001/components/page_load_me...
> File components/page_load_metrics/browser/page_load_metrics_util.h (right):
> 
>
https://codereview.chromium.org/1507813002/diff/20001/components/page_load_me...
> components/page_load_metrics/browser/page_load_metrics_util.h:27: // the
> priority is lowered, and the load timing events no longer reflect the
> On 2015/12/08 20:26:21, csharrison wrote:
> > I don't know if the priority is always lowered. rdsmith@ might know more. I
> > would also add that some events (e.g. paint) are delayed until foreground,
> which
> > is the root cause of many long tailed histograms.
> 
> I reworded the comment. PTAL.

still lgtm

[Charlie Harrison, 2015-12-09 15:13:17]

On 2015/12/09 13:52:26, csharrison wrote:
> On 2015/12/09 06:29:00, Adam Rice wrote:
> >
>
https://codereview.chromium.org/1507813002/diff/20001/components/page_load_me...
> > File components/page_load_metrics/browser/page_load_metrics_util.h (right):
> > 
> >
>
https://codereview.chromium.org/1507813002/diff/20001/components/page_load_me...
> > components/page_load_metrics/browser/page_load_metrics_util.h:27: // the
> > priority is lowered, and the load timing events no longer reflect the
> > On 2015/12/08 20:26:21, csharrison wrote:
> > > I don't know if the priority is always lowered. rdsmith@ might know more.
I
> > > would also add that some events (e.g. paint) are delayed until foreground,
> > which
> > > is the root cause of many long tailed histograms.
> > 
> > I reworded the comment. PTAL.
> 
> still lgtm

Gah sorry for the back and forth. I applied your patch locally to use the helper
function in my next CL. Maybe we should change this to
bool EventOccurredInForeground(const base::TimeDelta& event, const
PageLoadExtraInfo& info) {
  return info.started_in_foreground && !event.is_zero() &&
(info.first_background.is_zero() || event < info.first_background);
}

WDYT? This way the function is precise (it doesn't allow events that occur in
any sort of background).

[Adam Rice, 2015-12-09 18:20:49]

On 2015/12/09 15:13:17, csharrison wrote:
> On 2015/12/09 13:52:26, csharrison wrote:
> > On 2015/12/09 06:29:00, Adam Rice wrote:
> > >
> >
>
https://codereview.chromium.org/1507813002/diff/20001/components/page_load_me...
> > > File components/page_load_metrics/browser/page_load_metrics_util.h
(right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1507813002/diff/20001/components/page_load_me...
> > > components/page_load_metrics/browser/page_load_metrics_util.h:27: // the
> > > priority is lowered, and the load timing events no longer reflect the
> > > On 2015/12/08 20:26:21, csharrison wrote:
> > > > I don't know if the priority is always lowered. rdsmith@ might know
more.
> I
> > > > would also add that some events (e.g. paint) are delayed until
foreground,
> > > which
> > > > is the root cause of many long tailed histograms.
> > > 
> > > I reworded the comment. PTAL.
> > 
> > still lgtm
> 
> Gah sorry for the back and forth. I applied your patch locally to use the
helper
> function in my next CL. Maybe we should change this to
> bool EventOccurredInForeground(const base::TimeDelta& event, const
> PageLoadExtraInfo& info) {
>   return info.started_in_foreground && !event.is_zero() &&
> (info.first_background.is_zero() || event < info.first_background);
> }
> 
> WDYT? This way the function is precise (it doesn't allow events that occur in
> any sort of background).

Yes, I think it's better. It means that the caller needs less code and has less
room for mistakes.

[Randy Smith (Not in Mondays), 2015-12-09 20:29:37]

LGTM owner stamp.

[kinuko, 2015-12-10 08:39:46]

Adam, if you're ready could we land this? (We want to use
EventOccurredInForeground in the other patch)

[Adam Rice, 2015-12-10 14:28:14]

The CQ bit was checked by ricea@chromium.org

[Adam Rice, 2015-12-10 14:28:14]

The patchset sent to the CQ was uploaded after l-g-t-m from
csharrison@chromium.org
Link to the patchset: https://codereview.chromium.org/1507813002/#ps60001
(title: "Make EventOccurredInForeground take a PageLoadExtraInfo object.")

[commit-bot: I haz the power, 2015-12-10 14:29:15]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1507813002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1507813002/60001

[commit-bot: I haz the power, 2015-12-10 15:37:31]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-12-10 15:38:25]

Description was changed from

==========
Don't log background navigations in StaleWhileRevalidateExperiment histograms

Early results from the
PageLoad.Clients.StaleWhileRevalidateExperiment.Timing2.* histograms
show that there is a fat tail of noise from background navigations.

Remove navigations that started in the background and those that were
backgrounded before the timing event took place. The method is identical
to the PageLoad.Clients.FromGWS.Timing2.* histograms.

BUG=565458
TEST=manually tested with about:histograms
==========

to

==========
Don't log background navigations in StaleWhileRevalidateExperiment histograms

Early results from the
PageLoad.Clients.StaleWhileRevalidateExperiment.Timing2.* histograms
show that there is a fat tail of noise from background navigations.

Remove navigations that started in the background and those that were
backgrounded before the timing event took place. The method is identical
to the PageLoad.Clients.FromGWS.Timing2.* histograms.

BUG=565458
TEST=manually tested with about:histograms

Committed: https://crrev.com/096b91575a69187a3f9e7b6484db5ac8ee9ed452
Cr-Commit-Position: refs/heads/master@{#364366}
==========

[commit-bot: I haz the power, 2015-12-10 15:38:26]

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