Add page load abort metrics to PageLoadMetrics system

Add page load abort metrics to PageLoadMetrics system

BUG=

Committed: https://crrev.com/ebf0bc25277c6a656337e442060fc51dfc201048
Cr-Commit-Position: refs/heads/master@{#352053}

[Charlie Harrison, 2015-09-30 22:22:51]

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

[Charlie Harrison, 2015-09-30 22:22:52]

Few questions: which net::Errors should we filter out? Should we just whitelist
ERR_ABORTED? Also, take a look at the enum names; they feel a little awkward to
me.

[Charlie Harrison, 2015-10-01 14:25:35]

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

[Charlie Harrison, 2015-10-01 14:25:36]

Adding Randy also cause I've been giving lots to Tuttle.

[Bryan McQuade, 2015-10-01 14:57:38]

basically looks good to me, thanks! just a few questions.

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

https://codereview.chromium.org/1377223002/diff/20001/components/page_load_me...
components/page_load_metrics/browser/metrics_web_contents_observer.cc:146:
UMA_HISTOGRAM_ENUMERATION("PageLoad.EventCounts", event, EVENT_COUNT);
is EVENT_COUNT supposed to be the value of the last enum entry plus one, or the
value of the last valid entry?

also is it valid to add additional entries to such an enum used with
UMA_HISTOGRAM_ENUMERATION (where EVENT_COUNT will increase)?

I've also seen people use UMA_HISTOGRAM_SPARSE_SLOWLY for enums - I'd be
interested to understand the differences just so we know which to choose in the
future. Can you investigate?

https://codereview.chromium.org/1377223002/diff/20001/components/page_load_me...
components/page_load_metrics/browser/metrics_web_contents_observer.cc:194:
finished_nav->RecordEvent(ABORTED_PROVISIONAL_LOAD);
could we call this PROVISIONAL_LOAD_FAILED, which captures all types of errors,
even if not user-initiated abort? we can add a separate ABORTED_PROVISIONAL_LOAD
later which tracks just user-initiated aborts.

[Charlie Harrison, 2015-10-01 15:26:20]

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

https://codereview.chromium.org/1377223002/diff/20001/components/page_load_me...
components/page_load_metrics/browser/metrics_web_contents_observer.cc:146:
UMA_HISTOGRAM_ENUMERATION("PageLoad.EventCounts", event, EVENT_COUNT);
On 2015/10/01 14:57:38, Bryan McQuade wrote:
> is EVENT_COUNT supposed to be the value of the last enum entry plus one, or
the
> value of the last valid entry?
> 
> also is it valid to add additional entries to such an enum used with
> UMA_HISTOGRAM_ENUMERATION (where EVENT_COUNT will increase)?
> 
> I've also seen people use UMA_HISTOGRAM_SPARSE_SLOWLY for enums - I'd be
> interested to understand the differences just so we know which to choose in
the
> future. Can you investigate?

The comments on LOCAL_HISTOGRAM_ENUMERATION clarify this:
// Support histograming of an enumerated value.  The samples should always be
// strictly less than |boundary_value| -- this prevents you from running into
// problems down the line if you add additional buckets to the histogram.

So EVENT_COUNT bounds the enum values, and allows us to add elements later.
HISTOGRAM_SPARSE_SLOWLY I gathered is for sparse histograms, with tons of
buckets. This doesn't really fit our use case and I think this enum histograms
are a better fit.

[Bryan McQuade, 2015-10-01 15:48:00]

LGTM pending addition of adding a case for net::ERR_ABORTED that you proposed.

[Randy Smith (Not in Mondays), 2015-10-01 16:07:39]

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

https://codereview.chromium.org/1377223002/diff/20001/components/page_load_me...
components/page_load_metrics/browser/metrics_web_contents_observer.h:29:
STARTED_PROVISIONAL_LOAD,
I'd like to have comments on these values that explain to someone who isn't
knowledgeable in the renderer space (like, say, me :-}) what the enums mean. 
What is a provisional load and how does it compare with a non-provisional load? 


Also, arguably even more important: It should be clear to the reader (either
here or in histograms.xml) which of these events are exclusive, which are
subsets of each other, and which are independent.  I'm assuming that
STARTED_PROVISIONAL_LOAD will be recorded if any of the other three are
recorded, and that the other three are disjoint and (at least in theory?) at
least one of them will be recorded for any page.  That right?

https://codereview.chromium.org/1377223002/diff/20001/components/page_load_me...
components/page_load_metrics/browser/metrics_web_contents_observer.h:35:
EVENT_COUNT
I *think* that the custom for these is the enum name (in cap/underscore format)
with _LAST_ENTRY appended to it.  That's what appears to be used in cookies and
downloads (which may mean I've created the custom, which is why I'm being
hesitant).  Also, usually all entries in an enum have a prefix associated with
the enum type.  Both of these are for avoiding namespace collisions.  See, e.g.,
https://chromium.googlesource.com/chromium/src/+blame/master/content/browser/....

[Charlie Harrison, 2015-10-01 16:59:55]

Changed some names around, let me know what you think.

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

https://codereview.chromium.org/1377223002/diff/20001/components/page_load_me...
components/page_load_metrics/browser/metrics_web_contents_observer.h:29:
STARTED_PROVISIONAL_LOAD,
On 2015/10/01 16:07:39, rdsmith wrote:
> I'd like to have comments on these values that explain to someone who isn't
> knowledgeable in the renderer space (like, say, me :-}) what the enums mean. 
> What is a provisional load and how does it compare with a non-provisional
load? 
> 
> 
> Also, arguably even more important: It should be clear to the reader (either
> here or in histograms.xml) which of these events are exclusive, which are
> subsets of each other, and which are independent.  I'm assuming that
> STARTED_PROVISIONAL_LOAD will be recorded if any of the other three are
> recorded, and that the other three are disjoint and (at least in theory?) at
> least one of them will be recorded for any page.  That right?

Will do, good suggestions.

[Randy Smith (Not in Mondays), 2015-10-01 17:18:09]

LGTM with below comments (except for "suggestion", which you can do what you
want with, and "confirming", which is just FMI).

https://codereview.chromium.org/1377223002/diff/60001/components/page_load_me...
File components/page_load_metrics/browser/metrics_web_contents_observer.h
(right):

https://codereview.chromium.org/1377223002/diff/60001/components/page_load_me...
components/page_load_metrics/browser/metrics_web_contents_observer.h:31: // A
provisional load is a load before it commits.
Define commits?  I.e. (assuming I have the right definition) " ... before it
commits (i.e. before the first byte of data is received)."

https://codereview.chromium.org/1377223002/diff/60001/components/page_load_me...
components/page_load_metrics/browser/metrics_web_contents_observer.h:34:
PAGE_LOAD_FAILED_PROVISIONAL,
suggestion: I think "FAILED_BEFORE_COMMIT" would be clearer than
"FAILED_PROVISIONAL"?

https://codereview.chromium.org/1377223002/diff/60001/components/page_load_me...
components/page_load_metrics/browser/metrics_web_contents_observer.h:44: // We
increase this count if a page load successfully has a layout.
nit: "these counts"

https://codereview.chromium.org/1377223002/diff/60001/components/page_load_me...
components/page_load_metrics/browser/metrics_web_contents_observer.h:45: //
Differentiate between loads that were backgrounded before first layout.
Just confirming: If a page is backgrounded and then foregrounded before first
layout, it'll still go in the BG bucket?

https://codereview.chromium.org/1377223002/diff/60001/components/page_load_me...
components/page_load_metrics/browser/metrics_web_contents_observer.h:47:
PAGE_LOAD_SUCCESSFUL_FIRST_LAYOUT_BG,
We historically discourage abbreviations in chromium, so I'd recommend
FG->FOREGROUND, BG->BACKGROUND

https://codereview.chromium.org/1377223002/diff/60001/components/page_load_me...
components/page_load_metrics/browser/metrics_web_contents_observer.h:50: //
histograms.xml.
Actually, the note about the link to histograms.xml should be before the enum,
and it should also warn people not to *remove* values without revving the
histogram name.

[Charlie Harrison, 2015-10-01 17:58:17]

Thanks for the feedback!

https://codereview.chromium.org/1377223002/diff/60001/components/page_load_me...
File components/page_load_metrics/browser/metrics_web_contents_observer.h
(right):

https://codereview.chromium.org/1377223002/diff/60001/components/page_load_me...
components/page_load_metrics/browser/metrics_web_contents_observer.h:31: // A
provisional load is a load before it commits.
On 2015/10/01 17:18:08, rdsmith wrote:
> Define commits?  I.e. (assuming I have the right definition) " ... before it
> commits (i.e. before the first byte of data is received)."

Done.

https://codereview.chromium.org/1377223002/diff/60001/components/page_load_me...
components/page_load_metrics/browser/metrics_web_contents_observer.h:34:
PAGE_LOAD_FAILED_PROVISIONAL,
On 2015/10/01 17:18:08, rdsmith wrote:
> suggestion: I think "FAILED_BEFORE_COMMIT" would be clearer than
> "FAILED_PROVISIONAL"?

Done.

https://codereview.chromium.org/1377223002/diff/60001/components/page_load_me...
components/page_load_metrics/browser/metrics_web_contents_observer.h:44: // We
increase this count if a page load successfully has a layout.
On 2015/10/01 17:18:09, rdsmith wrote:
> nit: "these counts"

Done.

https://codereview.chromium.org/1377223002/diff/60001/components/page_load_me...
components/page_load_metrics/browser/metrics_web_contents_observer.h:45: //
Differentiate between loads that were backgrounded before first layout.
On 2015/10/01 17:18:08, rdsmith wrote:
> Just confirming: If a page is backgrounded and then foregrounded before first
> layout, it'll still go in the BG bucket?

Yupp, I edited the comment.

https://codereview.chromium.org/1377223002/diff/60001/components/page_load_me...
components/page_load_metrics/browser/metrics_web_contents_observer.h:50: //
histograms.xml.
On 2015/10/01 17:18:09, rdsmith wrote:
> Actually, the note about the link to histograms.xml should be before the enum,
> and it should also warn people not to *remove* values without revving the
> histogram name.

Done.

[Charlie Harrison, 2015-10-01 22:05:52]

csharrison@chromium.org changed reviewers:
+ asvitkine@chromium.org
- ttuttle@chromium.org

[Charlie Harrison, 2015-10-01 22:05:53]

@asvitkine PTAL

[Charlie Harrison, 2015-10-01 22:07:57]

On 2015/10/01 22:05:53, csharrison wrote:
> @asvitkine PTAL

@asvitkine only need review for histograms.xml

[Alexei Svitkine (slow), 2015-10-02 14:57:15]

lgtm

https://codereview.chromium.org/1377223002/diff/80001/components/page_load_me...
File components/page_load_metrics/browser/metrics_web_contents_observer.h
(right):

https://codereview.chromium.org/1377223002/diff/80001/components/page_load_me...
components/page_load_metrics/browser/metrics_web_contents_observer.h:28: // If
you add or remove elements from this enum, make sure you update the enum
Nit: Please rephrase this, as current comment implies it's safe to remove
elements from the enum, whereas in reality this shouldn't be done - as it would
make the enums inconsistent between versions.

[Charlie Harrison, 2015-10-02 15:13:38]

The CQ bit was checked by csharrison@chromium.org

[Charlie Harrison, 2015-10-02 15:13:38]

The patchset sent to the CQ was uploaded after l-g-t-m from
bmcquade@chromium.org, rdsmith@chromium.org, asvitkine@chromium.org
Link to the patchset: https://codereview.chromium.org/1377223002/#ps120001
(title: "remove TODO")

[commit-bot: I haz the power, 2015-10-02 15:14:35]

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

[commit-bot: I haz the power, 2015-10-02 16:46:08]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-10-02 16:46:40]

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