Various cleaups for AMP page load metrics.

Various cleaups for AMP page load metrics.

* add support for detecting URLs on the amp cache domain using the new
  host prefixing scheme

* add support for amp documents loaded in the google news viewer

* factor existing google url detection logic into a common helper, and use
  it in both amp observer and from search observer

* add metrics broken out by type of amp load (amp cache, google search viewer, etc)
  since performance may vary depending on view type (especially for bare amp cache
  urls)

BUG=722690
Review-Url: https://codereview.chromium.org/2880323003
Cr-Commit-Position: refs/heads/master@{#472226}
Committed: https://chromium.googlesource.com/chromium/src/+/d28685525ed6cda7ee3cd9a84bca764d70512132

[Bryan McQuade, 2017-05-15 16:49:35]

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

[commit-bot: I haz the power, 2017-05-15 16:50:05]

Dry run: CQ is trying da patch.

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

[Bryan McQuade, 2017-05-15 16:53:24]

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

[commit-bot: I haz the power, 2017-05-15 16:54:06]

Dry run: 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, 2017-05-15 17:07:56]

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

[commit-bot: I haz the power, 2017-05-15 17:07:56]

Dry run: Try jobs failed on following builders:
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Bryan McQuade, 2017-05-15 17:24:47]

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

[commit-bot: I haz the power, 2017-05-15 17:25:24]

Dry run: CQ is trying da patch.

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

[Bryan McQuade, 2017-05-15 17:28:36]

Description was changed from

==========
Various cleaups for AMP page load metrics.

BUG=
==========

to

==========
Various cleaups for AMP page load metrics.

* add support for detecting URLs on the amp cache domain using the new
  host prefixing scheme

* add support for amp documents loaded in the google news viewer

* factor existing google url detection logic into a common helper, and use
  it in both amp observer and from search observer

* add metrics broken out by type of amp load (amp cache, google search viewer,
etc)
  since performance may vary depending on view type (especially for bare amp
cache
  urls)

BUG=
==========

[Bryan McQuade, 2017-05-15 17:28:59]

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

[commit-bot: I haz the power, 2017-05-15 17:29:07]

Dry run: CQ is trying da patch.

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

[Bryan McQuade, 2017-05-15 18:38:03]

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

[commit-bot: I haz the power, 2017-05-15 18:39:34]

Dry run: 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, 2017-05-15 20:29:59]

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

[commit-bot: I haz the power, 2017-05-15 20:30:00]

Dry run: Try jobs failed on following builders:
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[RyanSturm, 2017-05-15 20:42:05]

ryansturm@chromium.org changed reviewers:
+ ryansturm@chromium.org

[RyanSturm, 2017-05-15 20:42:07]

https://codereview.chromium.org/2880323003/diff/80001/chrome/browser/page_loa...
File
chrome/browser/page_load_metrics/observers/amp_page_load_metrics_observer.cc
(right):

https://codereview.chromium.org/2880323003/diff/80001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/observers/amp_page_load_metrics_observer.cc:37:
PAGE_LOAD_HISTOGRAM(std::string(name).append(".None"), value);         \
Remove the PAGE_LOAD_HISTOGRAM from this case.

https://codereview.chromium.org/2880323003/diff/80001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/observers/amp_page_load_metrics_observer.cc:148:
if (google_hostname_prefix == "www" &&
As a note: it's a little strange that "www" is inline and
kGoogleSearchAmpViewerPathPrefix is defined above. Let's make them both inline
constants? Same with "news" & kGoogleNewsAmpViewerPath below

https://codereview.chromium.org/2880323003/diff/80001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/observers/amp_page_load_metrics_observer.cc:150:
base::CompareCase::SENSITIVE)) {
Not sure whether this should be case sensitive or not.

https://codereview.chromium.org/2880323003/diff/80001/chrome/browser/page_loa...
File chrome/browser/page_load_metrics/page_load_metrics_util.cc (right):

https://codereview.chromium.org/2880323003/diff/80001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/page_load_metrics_util.cc:48: bool
IsGoogleHostnameAndGetPrefix(const GURL& url,
nit: Thoughts on getting rid of the out param and returning a
base::Optional<base::StringPiece>? Maybe rename it GetPrefixForGoogleHostName if
that makes sense.

https://codereview.chromium.org/2880323003/diff/80001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2880323003/diff/80001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:89471: +<histogram_suffixes
name="PageLoadMetricsClientsAmpType" separator=".">
It might be cleaner to combine the two histogram_suffixes added in this CL,
since I expect they will always have the same list of affected-histograms.

[Bryan McQuade, 2017-05-16 03:18:44]

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

[commit-bot: I haz the power, 2017-05-16 03:19:08]

Dry run: CQ is trying da patch.

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

[Bryan McQuade, 2017-05-16 03:30:21]

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

[commit-bot: I haz the power, 2017-05-16 03:30:46]

Dry run: CQ is trying da patch.

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

[Bryan McQuade, 2017-05-16 03:57:45]

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

[commit-bot: I haz the power, 2017-05-16 03:58:10]

Dry run: CQ is trying da patch.

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

[Bryan McQuade, 2017-05-16 03:59:15]

Thanks! Addressed comments. PTAL.

https://codereview.chromium.org/2880323003/diff/80001/chrome/browser/page_loa...
File
chrome/browser/page_load_metrics/observers/amp_page_load_metrics_observer.cc
(right):

https://codereview.chromium.org/2880323003/diff/80001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/observers/amp_page_load_metrics_observer.cc:37:
PAGE_LOAD_HISTOGRAM(std::string(name).append(".None"), value);         \
On 2017/05/15 at 20:42:06, RyanSturm wrote:
> Remove the PAGE_LOAD_HISTOGRAM from this case.

Done

https://codereview.chromium.org/2880323003/diff/80001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/observers/amp_page_load_metrics_observer.cc:148:
if (google_hostname_prefix == "www" &&
On 2017/05/15 at 20:42:06, RyanSturm wrote:
> As a note: it's a little strange that "www" is inline and
kGoogleSearchAmpViewerPathPrefix is defined above. Let's make them both inline
constants? Same with "news" & kGoogleNewsAmpViewerPath below

Done

https://codereview.chromium.org/2880323003/diff/80001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/observers/amp_page_load_metrics_observer.cc:150:
base::CompareCase::SENSITIVE)) {
On 2017/05/15 at 20:42:06, RyanSturm wrote:
> Not sure whether this should be case sensitive or not.

My understanding is that hostnames are not case sensitive while paths are by
default case sensitive. I tested a capitalized version of this string and got a
404.

https://codereview.chromium.org/2880323003/diff/80001/chrome/browser/page_loa...
File chrome/browser/page_load_metrics/page_load_metrics_util.cc (right):

https://codereview.chromium.org/2880323003/diff/80001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/page_load_metrics_util.cc:48: bool
IsGoogleHostnameAndGetPrefix(const GURL& url,
On 2017/05/15 at 20:42:06, RyanSturm wrote:
> nit: Thoughts on getting rid of the out param and returning a
base::Optional<base::StringPiece>? Maybe rename it GetPrefixForGoogleHostName if
that makes sense.

This is a great idea. I needed to change to optional<std::string> since we
sometimes return string offsets into a stack-allocated string piece, which blows
up. using std::string guarantees consistent storage that can live beyond
returning from a function.

https://codereview.chromium.org/2880323003/diff/80001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2880323003/diff/80001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:89471: +<histogram_suffixes
name="PageLoadMetricsClientsAmpType" separator=".">
On 2017/05/15 at 20:42:06, RyanSturm wrote:
> It might be cleaner to combine the two histogram_suffixes added in this CL,
since I expect they will always have the same list of affected-histograms.

Done

[Bryan McQuade, 2017-05-16 04:05:20]

Description was changed from

==========
Various cleaups for AMP page load metrics.

* add support for detecting URLs on the amp cache domain using the new
  host prefixing scheme

* add support for amp documents loaded in the google news viewer

* factor existing google url detection logic into a common helper, and use
  it in both amp observer and from search observer

* add metrics broken out by type of amp load (amp cache, google search viewer,
etc)
  since performance may vary depending on view type (especially for bare amp
cache
  urls)

BUG=
==========

to

==========
Various cleaups for AMP page load metrics.

* add support for detecting URLs on the amp cache domain using the new
  host prefixing scheme

* add support for amp documents loaded in the google news viewer

* factor existing google url detection logic into a common helper, and use
  it in both amp observer and from search observer

* add metrics broken out by type of amp load (amp cache, google search viewer,
etc)
  since performance may vary depending on view type (especially for bare amp
cache
  urls)

BUG=722690
==========

[commit-bot: I haz the power, 2017-05-16 05:50:28]

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

[commit-bot: I haz the power, 2017-05-16 05:50:29]

Dry run: This issue passed the CQ dry run.

[RyanSturm, 2017-05-16 16:12:22]

lgtm % an optional nit

https://codereview.chromium.org/2880323003/diff/140001/chrome/browser/page_lo...
File
chrome/browser/page_load_metrics/observers/amp_page_load_metrics_observer.cc
(right):

https://codereview.chromium.org/2880323003/diff/140001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/amp_page_load_metrics_observer.cc:22:
#define RECORD_HISTOGRAM_FOR_TYPE(name, amp_view_type, value)                 \
Slight nit:

This macro is defined before kHistogramPrefix is declared/defined, but it
references kHistogramPrefix in it. That works because it's a macro and it is
only used below the definition of kHistogramPrefix, but it feels slightly
confusing from a readability stand point.

[Bryan McQuade, 2017-05-16 16:15:37]

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

[commit-bot: I haz the power, 2017-05-16 16:16:37]

Dry run: CQ is trying da patch.

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

[Bryan McQuade, 2017-05-16 17:29:20]

https://codereview.chromium.org/2880323003/diff/140001/chrome/browser/page_lo...
File
chrome/browser/page_load_metrics/observers/amp_page_load_metrics_observer.cc
(right):

https://codereview.chromium.org/2880323003/diff/140001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/amp_page_load_metrics_observer.cc:22:
#define RECORD_HISTOGRAM_FOR_TYPE(name, amp_view_type, value)                 \
On 2017/05/16 at 16:12:22, RyanSturm wrote:
> Slight nit:
> 
> This macro is defined before kHistogramPrefix is declared/defined, but it
references kHistogramPrefix in it. That works because it's a macro and it is
only used below the definition of kHistogramPrefix, but it feels slightly
confusing from a readability stand point.

Done

[Bryan McQuade, 2017-05-16 17:29:41]

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

[Bryan McQuade, 2017-05-16 17:29:54]

isherman, PTAL for histograms.xml, thanks!

[commit-bot: I haz the power, 2017-05-16 19:02:08]

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

[commit-bot: I haz the power, 2017-05-16 19:02:10]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Ilya Sherman, 2017-05-16 19:51:17]

Metrics LGTM, thanks.

[Bryan McQuade, 2017-05-16 19:54:02]

The CQ bit was checked by bmcquade@chromium.org

[Bryan McQuade, 2017-05-16 19:54:02]

The patchset sent to the CQ was uploaded after l-g-t-m from
ryansturm@chromium.org
Link to the patchset: https://codereview.chromium.org/2880323003/#ps160001
(title: "address comment")

[commit-bot: I haz the power, 2017-05-16 19:55:40]

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, 2017-05-16 22:11:56]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1494964442418340,
"parent_rev": "6e0f47b9cc33450f93d02d238cd4e60dffa89421", "commit_rev":
"d28685525ed6cda7ee3cd9a84bca764d70512132"}

[commit-bot: I haz the power, 2017-05-16 22:12:10]

Description was changed from

==========
Various cleaups for AMP page load metrics.

* add support for detecting URLs on the amp cache domain using the new
  host prefixing scheme

* add support for amp documents loaded in the google news viewer

* factor existing google url detection logic into a common helper, and use
  it in both amp observer and from search observer

* add metrics broken out by type of amp load (amp cache, google search viewer,
etc)
  since performance may vary depending on view type (especially for bare amp
cache
  urls)

BUG=722690
==========

to

==========
Various cleaups for AMP page load metrics.

* add support for detecting URLs on the amp cache domain using the new
  host prefixing scheme

* add support for amp documents loaded in the google news viewer

* factor existing google url detection logic into a common helper, and use
  it in both amp observer and from search observer

* add metrics broken out by type of amp load (amp cache, google search viewer,
etc)
  since performance may vary depending on view type (especially for bare amp
cache
  urls)

BUG=722690

Review-Url: https://codereview.chromium.org/2880323003
Cr-Commit-Position: refs/heads/master@{#472226}
Committed:
https://chromium.googlesource.com/chromium/src/+/d28685525ed6cda7ee3cd9a84bca...
==========

[commit-bot: I haz the power, 2017-05-16 22:12:11]

Committed patchset #9 (id:160001) as
https://chromium.googlesource.com/chromium/src/+/d28685525ed6cda7ee3cd9a84bca...