Plumb experiment flags through page_load_metrics and add a new observer

Plumb experiment flags through page_load_metrics and add a new observer

This patch is the second of two patches to plumb experimental flags into
the page_load_metrics system to separate histograms. This change adds
an int flag to the IPC message, which can then be used by various metrics
observers so they can log histograms only when the feature is triggered.

BUG=594159
Committed: https://crrev.com/b9a657db596e85b738c56f0ef896428ba5997227
Cr-Commit-Position: refs/heads/master@{#387175}

[Charlie Harrison, 2016-04-01 21:18:28]

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

[Charlie Harrison, 2016-04-01 21:18:28]

Bryan, PTAL at this draft. I'm only logging one metric right now cause I'm
waiting for some of your changes. It'll be trivial to add it as another
dependency soon though.

[Bryan McQuade, 2016-04-07 14:25:14]

This looks really good. Really glad to see this! Just a couple small comments.
Structurally this seems really good.

https://codereview.chromium.org/1857443002/diff/20001/chrome/test/data/page_l...
File chrome/test/data/page_load_metrics/dummy.js (right):

https://codereview.chromium.org/1857443002/diff/20001/chrome/test/data/page_l...
chrome/test/data/page_load_metrics/dummy.js:4: var dummy = true;
since this variable doesn't appear to be used, how do you feel about removing it
and just loading an empty file?

https://codereview.chromium.org/1857443002/diff/20001/components/page_load_me...
File components/page_load_metrics/common/page_load_timing.h (right):

https://codereview.chromium.org/1857443002/diff/20001/components/page_load_me...
components/page_load_metrics/common/page_load_timing.h:63: int experiment_flags;
nit: for structs with no constructor logic, i prefer setting defaults directly
here, rather than in the constructor. easier to reason about when everything is
in one place.

[Charlie Harrison, 2016-04-07 16:38:56]

csharrison@chromium.org changed reviewers:
+ brettw@chromium.org, isherman@chromium.org

[Charlie Harrison, 2016-04-07 16:38:57]

PTAL. Thanks!

https://codereview.chromium.org/1857443002/diff/20001/chrome/test/data/page_l...
File chrome/test/data/page_load_metrics/dummy.js (right):

https://codereview.chromium.org/1857443002/diff/20001/chrome/test/data/page_l...
chrome/test/data/page_load_metrics/dummy.js:4: var dummy = true;
On 2016/04/07 at 14:25:14, Bryan McQuade wrote:
> since this variable doesn't appear to be used, how do you feel about removing
it and just loading an empty file?

Done.

https://codereview.chromium.org/1857443002/diff/20001/components/page_load_me...
File components/page_load_metrics/common/page_load_timing.h (right):

https://codereview.chromium.org/1857443002/diff/20001/components/page_load_me...
components/page_load_metrics/common/page_load_timing.h:63: int experiment_flags;
On 2016/04/07 at 14:25:14, Bryan McQuade wrote:
> nit: for structs with no constructor logic, i prefer setting defaults directly
here, rather than in the constructor. easier to reason about when everything is
in one place.

Done.

[Charlie Harrison, 2016-04-07 16:39:50]

Sorry my message got cut off.

isherman: histograms/variation changes.
brettw: chrome/browser/DEPS for WebKit enum import.

[Ilya Sherman, 2016-04-07 18:46:04]

https://codereview.chromium.org/1857443002/diff/50001/testing/variations/fiel...
File testing/variations/fieldtrial_testing_config_linux.json (right):

https://codereview.chromium.org/1857443002/diff/50001/testing/variations/fiel...
testing/variations/fieldtrial_testing_config_linux.json:54: "group_name":
"Default"
It's generally not recommended to include the default group in the test config,
as that doubles the test load, which doesn't scale.  If there are some specific
tests that you want to run in both configurations, then please update just those
test files.

[Charlie Harrison, 2016-04-07 19:12:52]

https://codereview.chromium.org/1857443002/diff/50001/testing/variations/fiel...
File testing/variations/fieldtrial_testing_config_linux.json (right):

https://codereview.chromium.org/1857443002/diff/50001/testing/variations/fiel...
testing/variations/fieldtrial_testing_config_linux.json:54: "group_name":
"Default"
On 2016/04/07 at 18:46:04, Ilya Sherman wrote:
> It's generally not recommended to include the default group in the test
config, as that doubles the test load, which doesn't scale.  If there are some
specific tests that you want to run in both configurations, then please update
just those test files.

What about disabling the experiment completely for a particular platform? I'd
prefer that over cherry-picking certain tests to run in both configurations.

[Ilya Sherman, 2016-04-07 19:31:32]

https://codereview.chromium.org/1857443002/diff/50001/testing/variations/fiel...
File testing/variations/fieldtrial_testing_config_linux.json (right):

https://codereview.chromium.org/1857443002/diff/50001/testing/variations/fiel...
testing/variations/fieldtrial_testing_config_linux.json:54: "group_name":
"Default"
On 2016/04/07 19:12:52, csharrison wrote:
> On 2016/04/07 at 18:46:04, Ilya Sherman wrote:
> > It's generally not recommended to include the default group in the test
> config, as that doubles the test load, which doesn't scale.  If there are some
> specific tests that you want to run in both configurations, then please update
> just those test files.
> 
> What about disabling the experiment completely for a particular platform? I'd
> prefer that over cherry-picking certain tests to run in both configurations.

Okay, I was apparently mistaken.  The bots will actually ignore any groups other
than the first one; so even if you specify the Default group here, it will be
ignored.  (Maybe we should actually make it a hard error to specify multiple
groups, since it would basically never do what the author expects, apparently.)

However, there is FYI bot coverage that will run tests with the Default group
for all trials.

So, if you'd like to have multiple groups tested on non-FYI bots, I don't think
there's any option available other than cherry-picking specific tests of
interest.

[Charlie Harrison, 2016-04-07 19:46:32]

https://codereview.chromium.org/1857443002/diff/50001/testing/variations/fiel...
File testing/variations/fieldtrial_testing_config_linux.json (right):

https://codereview.chromium.org/1857443002/diff/50001/testing/variations/fiel...
testing/variations/fieldtrial_testing_config_linux.json:54: "group_name":
"Default"
On 2016/04/07 at 19:31:32, Ilya Sherman wrote:
> On 2016/04/07 19:12:52, csharrison wrote:
> > On 2016/04/07 at 18:46:04, Ilya Sherman wrote:
> > > It's generally not recommended to include the default group in the test
> > config, as that doubles the test load, which doesn't scale.  If there are
some
> > specific tests that you want to run in both configurations, then please
update
> > just those test files.
> > 
> > What about disabling the experiment completely for a particular platform?
I'd
> > prefer that over cherry-picking certain tests to run in both configurations.
> 
> Okay, I was apparently mistaken.  The bots will actually ignore any groups
other than the first one; so even if you specify the Default group here, it will
be ignored.  (Maybe we should actually make it a hard error to specify multiple
groups, since it would basically never do what the author expects, apparently.)
> 
> However, there is FYI bot coverage that will run tests with the Default group
for all trials.
> 
> So, if you'd like to have multiple groups tested on non-FYI bots, I don't
think there's any option available other than cherry-picking specific tests of
interest.

The FYI coverage is okay with me! Thanks for the clarification.

[Ilya Sherman, 2016-04-07 19:51:50]

histograms.xml lgtm, thanks

[Bryan McQuade, 2016-04-11 20:18:47]

Nice change! Overall LGTM - just a few small things.

https://codereview.chromium.org/1857443002/diff/70001/chrome/browser/page_loa...
File
chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer.cc
(right):

https://codereview.chromium.org/1857443002/diff/70001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer.cc:285:
if (WasStartedInForegroundEventInForeground(timing.first_contentful_paint,
thanks!

https://codereview.chromium.org/1857443002/diff/70001/chrome/browser/page_loa...
File
chrome/browser/page_load_metrics/observers/document_write_page_load_metrics_observer.cc
(right):

https://codereview.chromium.org/1857443002/diff/70001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/observers/document_write_page_load_metrics_observer.cc:10:
const char kHistogramDocWriteFirstContentfulPaint[] =
it may end up being overkill, but i could imagine that this work could end up
shifting user behavior such that some pages that would've been backgrounded
before first paint will end up staying in foreground until first paint. to track
that, can we add background variants for the metrics you think it'd be useful
for?

https://codereview.chromium.org/1857443002/diff/70001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/observers/document_write_page_load_metrics_observer.cc:18:
const char kHistogramDocWriteParseBlockedOnScriptParseComplete[] =
let's also add:
ParseBlockedOnScriptLoadFromDocumentWrite
ParseBlockedOnScriptLoadFromDocumentWrite.ParseComplete

(the data for these was added after you put this CL together)

https://codereview.chromium.org/1857443002/diff/70001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/observers/document_write_page_load_metrics_observer.cc:37:
// TODO(csharrison,bmcquade): Log parse blocked on document write when that
yep :) should be available now

https://codereview.chromium.org/1857443002/diff/70001/chrome/browser/page_loa...
File
chrome/browser/page_load_metrics/observers/page_load_metrics_observer_test_harness.cc
(right):

https://codereview.chromium.org/1857443002/diff/70001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/observers/page_load_metrics_observer_test_harness.cc:102:
observer_->OnMessageReceived(
can this method call the new SimulateTimingAndMetadataUpdate with an empty
PageLoadMetadata?

https://codereview.chromium.org/1857443002/diff/70001/components/page_load_me...
File components/page_load_metrics/browser/metrics_web_contents_observer.cc
(right):

https://codereview.chromium.org/1857443002/diff/70001/components/page_load_me...
components/page_load_metrics/browser/metrics_web_contents_observer.cc:332: bool
valid_timing_descendent =
just to be extra safe, could we do a similar test for valid_metadata_descendent
that makes sure that all flags in metadata_ are also present in new_metadata?

https://codereview.chromium.org/1857443002/diff/70001/components/page_load_me...
File components/page_load_metrics/renderer/page_timing_metrics_sender.cc
(right):

https://codereview.chromium.org/1857443002/diff/70001/components/page_load_me...
components/page_load_metrics/renderer/page_timing_metrics_sender.cc:40:
metadata_.behavior_flags |= behavior;
should we also start the send timer here if it's not already started? maybe we
can factor the logic for starting the timer into a helper and call it from this
method and from the end of Send(const PageLoadTiming&); otherwise, if a behavior
is observed shortly after the last timing update gets sent, we'll never get the
behavior update on the browser side.

if we do this, we might also add a check to see if the existing flag is already
set in the metadata, and return early if so, to avoid sending updates when no
new state has been observed.

https://codereview.chromium.org/1857443002/diff/70001/components/page_load_me...
components/page_load_metrics/renderer/page_timing_metrics_sender.cc:52: return;
perhaps a separate change, but i'm wondering if this early return is needed
anymore. IIRC we only dispatch timing updates for committed docs, so it seems
like it'd be an error now to ever get a mismatched nav start. perhaps in a
separate CL we could change this to assert as well?

[Charlie Harrison, 2016-04-12 15:18:51]

Thanks, Bryan.

brettw@, ptal at chrome DEPS

https://codereview.chromium.org/1857443002/diff/70001/chrome/browser/page_loa...
File
chrome/browser/page_load_metrics/observers/document_write_page_load_metrics_observer.cc
(right):

https://codereview.chromium.org/1857443002/diff/70001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/observers/document_write_page_load_metrics_observer.cc:10:
const char kHistogramDocWriteFirstContentfulPaint[] =
On 2016/04/11 at 20:18:47, Bryan McQuade wrote:
> it may end up being overkill, but i could imagine that this work could end up
shifting user behavior such that some pages that would've been backgrounded
before first paint will end up staying in foreground until first paint. to track
that, can we add background variants for the metrics you think it'd be useful
for?

Done. I left out the parse complete metrics because I thought it would be
overkill.

https://codereview.chromium.org/1857443002/diff/70001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/observers/document_write_page_load_metrics_observer.cc:18:
const char kHistogramDocWriteParseBlockedOnScriptParseComplete[] =
On 2016/04/11 at 20:18:47, Bryan McQuade wrote:
> let's also add:
> ParseBlockedOnScriptLoadFromDocumentWrite
> ParseBlockedOnScriptLoadFromDocumentWrite.ParseComplete
> 
> (the data for these was added after you put this CL together)

Done.

https://codereview.chromium.org/1857443002/diff/70001/chrome/browser/page_loa...
File
chrome/browser/page_load_metrics/observers/page_load_metrics_observer_test_harness.cc
(right):

https://codereview.chromium.org/1857443002/diff/70001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/observers/page_load_metrics_observer_test_harness.cc:102:
observer_->OnMessageReceived(
On 2016/04/11 at 20:18:47, Bryan McQuade wrote:
> can this method call the new SimulateTimingAndMetadataUpdate with an empty
PageLoadMetadata?

Done.

https://codereview.chromium.org/1857443002/diff/70001/components/page_load_me...
File components/page_load_metrics/browser/metrics_web_contents_observer.cc
(right):

https://codereview.chromium.org/1857443002/diff/70001/components/page_load_me...
components/page_load_metrics/browser/metrics_web_contents_observer.cc:332: bool
valid_timing_descendent =
On 2016/04/11 at 20:18:47, Bryan McQuade wrote:
> just to be extra safe, could we do a similar test for
valid_metadata_descendent that makes sure that all flags in metadata_ are also
present in new_metadata?

Done.

https://codereview.chromium.org/1857443002/diff/70001/components/page_load_me...
File components/page_load_metrics/renderer/page_timing_metrics_sender.cc
(right):

https://codereview.chromium.org/1857443002/diff/70001/components/page_load_me...
components/page_load_metrics/renderer/page_timing_metrics_sender.cc:40:
metadata_.behavior_flags |= behavior;
On 2016/04/11 at 20:18:47, Bryan McQuade wrote:
> should we also start the send timer here if it's not already started? maybe we
can factor the logic for starting the timer into a helper and call it from this
method and from the end of Send(const PageLoadTiming&); otherwise, if a behavior
is observed shortly after the last timing update gets sent, we'll never get the
behavior update on the browser side.
> 
> if we do this, we might also add a check to see if the existing flag is
already set in the metadata, and return early if so, to avoid sending updates
when no new state has been observed.

Done.

https://codereview.chromium.org/1857443002/diff/70001/components/page_load_me...
components/page_load_metrics/renderer/page_timing_metrics_sender.cc:52: return;
On 2016/04/11 at 20:18:47, Bryan McQuade wrote:
> perhaps a separate change, but i'm wondering if this early return is needed
anymore. IIRC we only dispatch timing updates for committed docs, so it seems
like it'd be an error now to ever get a mismatched nav start. perhaps in a
separate CL we could change this to assert as well?

Can investigate this in a separate change. I think you're right though.

[Bryan McQuade, 2016-04-12 15:25:32]

Thanks! LGTM.

https://codereview.chromium.org/1857443002/diff/90001/chrome/browser/page_loa...
File
chrome/browser/page_load_metrics/observers/document_write_page_load_metrics_observer.cc
(right):

https://codereview.chromium.org/1857443002/diff/90001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/observers/document_write_page_load_metrics_observer.cc:55:
// TODO(csharrison,bmcquade): Log parse blocked on document write when that
can remove this comment

[Charlie Harrison, 2016-04-12 15:38:06]

https://codereview.chromium.org/1857443002/diff/90001/chrome/browser/page_loa...
File
chrome/browser/page_load_metrics/observers/document_write_page_load_metrics_observer.cc
(right):

https://codereview.chromium.org/1857443002/diff/90001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/observers/document_write_page_load_metrics_observer.cc:55:
// TODO(csharrison,bmcquade): Log parse blocked on document write when that
On 2016/04/12 at 15:25:32, Bryan McQuade wrote:
> can remove this comment

Right. Done.

[Charlie Harrison, 2016-04-13 18:52:50]

brettw@ friendly ping for DEPS :)

[brettw, 2016-04-13 23:24:51]

DEPS files LGTM

[Charlie Harrison, 2016-04-13 23:28:17]

The CQ bit was checked by csharrison@chromium.org

[Charlie Harrison, 2016-04-13 23:28:18]

Thanks, everyone!! :)

[Charlie Harrison, 2016-04-13 23:28:18]

The patchset sent to the CQ was uploaded after l-g-t-m from
isherman@chromium.org, bmcquade@chromium.org
Link to the patchset: https://codereview.chromium.org/1857443002/#ps110001
(title: "remove todo and fix up a test script")

[commit-bot: I haz the power, 2016-04-13 23:28:35]

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

[commit-bot: I haz the power, 2016-04-14 00:41:52]

Committed patchset #7 (id:110001)

[commit-bot: I haz the power, 2016-04-14 00:43:17]

Description was changed from

==========
Plumb experiment flags through page_load_metrics and add a new observer

This patch is the second of two patches to plumb experimental flags into
the page_load_metrics system to separate histograms. This change adds
an int flag to the IPC message, which can then be used by various metrics
observers so they can log histograms only when the feature is triggered.

BUG=594159
==========

to

==========
Plumb experiment flags through page_load_metrics and add a new observer

This patch is the second of two patches to plumb experimental flags into
the page_load_metrics system to separate histograms. This change adds
an int flag to the IPC message, which can then be used by various metrics
observers so they can log histograms only when the feature is triggered.

BUG=594159

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

[commit-bot: I haz the power, 2016-04-14 00:43:19]

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