Adding parse UMA and pingback metrics

Adding parse UMA and pingback metrics

This adds DataReductionProxyMetricsObserver UMA for ParseDuration and
and amount of time parse blocked on script load. This also adds
parse_stop and parse_blocked_on_script_load_duration to the
pageload_metrics proto message. This also deprecates last_request_time
in the proto message.

BUG=642113
Committed: https://crrev.com/a408ef7c009cb12d2f37af309a65d7ea8f24cc7c
Cr-Commit-Position: refs/heads/master@{#416726}

[RyanSturm, 2016-09-02 19:49:36]

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

[commit-bot: I haz the power, 2016-09-02 19:50:09]

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, 2016-09-02 20:37:48]

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

[commit-bot: I haz the power, 2016-09-02 20:37:49]

Dry run: This issue passed the CQ dry run.

[RyanSturm, 2016-09-02 20:46:33]

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

[RyanSturm, 2016-09-02 20:46:34]

tbansal: PTAL @ * when you are back in the office.

[tbansal1, 2016-09-06 17:28:17]

https://codereview.chromium.org/2305013002/diff/1/chrome/browser/page_load_me...
File
chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc
(right):

https://codereview.chromium.org/2305013002/diff/1/chrome/browser/page_load_me...
chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc:41:
#define RECORD_HISTOGRAMS_FOR_SUFFIX(data, event, value, info,              \
Why do you need to pass value?

https://codereview.chromium.org/2305013002/diff/1/components/data_reduction_p...
File
components/data_reduction_proxy/core/common/data_reduction_proxy_page_load_timing.h
(right):

https://codereview.chromium.org/2305013002/diff/1/components/data_reduction_p...
components/data_reduction_proxy/core/common/data_reduction_proxy_page_load_timing.h:31:
// All TimeDeltas are relative to navigation_start
Missing period at end.

[RyanSturm, 2016-09-06 17:47:15]

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

[RyanSturm, 2016-09-06 17:47:23]

tbansal: PTAL

https://codereview.chromium.org/2305013002/diff/1/chrome/browser/page_load_me...
File
chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc
(right):

https://codereview.chromium.org/2305013002/diff/1/chrome/browser/page_load_me...
chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc:41:
#define RECORD_HISTOGRAMS_FOR_SUFFIX(data, event, value, info,              \
On 2016/09/06 17:28:16, tbansal1 wrote:
> Why do you need to pass value?

parse_duration is the difference between two events, but uses the parse_stop
event to check if it is backgrounded.

https://codereview.chromium.org/2305013002/diff/1/components/data_reduction_p...
File
components/data_reduction_proxy/core/common/data_reduction_proxy_page_load_timing.h
(right):

https://codereview.chromium.org/2305013002/diff/1/components/data_reduction_p...
components/data_reduction_proxy/core/common/data_reduction_proxy_page_load_timing.h:31:
// All TimeDeltas are relative to navigation_start
On 2016/09/06 17:28:17, tbansal1 wrote:
> Missing period at end.

Done.

[commit-bot: I haz the power, 2016-09-06 17:47:55]

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, 2016-09-06 18:36:59]

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

[commit-bot: I haz the power, 2016-09-06 18:37:00]

Dry run: This issue passed the CQ dry run.

[RyanSturm, 2016-09-06 18:42:33]

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

[RyanSturm, 2016-09-06 18:42:34]

bmcquade: PTAL at data_reduction_proxy_metrics_observer*

[tbansal1, 2016-09-06 19:44:43]

https://codereview.chromium.org/2305013002/diff/1/chrome/browser/page_load_me...
File
chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc
(right):

https://codereview.chromium.org/2305013002/diff/1/chrome/browser/page_load_me...
chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc:41:
#define RECORD_HISTOGRAMS_FOR_SUFFIX(data, event, value, info,              \
On 2016/09/06 17:47:23, RyanSturm wrote:
> On 2016/09/06 17:28:16, tbansal1 wrote:
> > Why do you need to pass value?
> 
> parse_duration is the difference between two events, but uses the parse_stop
> event to check if it is backgrounded.

Acked. Can you add a comment that event is time delta from navigation start
until the time when the event happened.

https://codereview.chromium.org/2305013002/diff/20001/chrome/browser/page_loa...
File
chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc
(right):

https://codereview.chromium.org/2305013002/diff/20001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc:41:
#define RECORD_HISTOGRAMS_FOR_SUFFIX(data, event, value, info,              \
Can you add a comment explaining what event is (IIUC, it is time delta from
navigation start to when the event happened). Also, may be rename "event" to
something else?

https://codereview.chromium.org/2305013002/diff/20001/components/data_reducti...
File
components/data_reduction_proxy/core/common/data_reduction_proxy_page_load_timing.cc
(right):

https://codereview.chromium.org/2305013002/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/common/data_reduction_proxy_page_load_timing.cc:27:
DataReductionProxyPageLoadTiming::DataReductionProxyPageLoadTiming(
nit: May be add a default assignment operator.

https://codereview.chromium.org/2305013002/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/common/data_reduction_proxy_page_load_timing.cc:27:
DataReductionProxyPageLoadTiming::DataReductionProxyPageLoadTiming(
nit: Why not use default copy constructor.

https://codereview.chromium.org/2305013002/diff/20001/components/data_reducti...
File components/data_reduction_proxy/proto/pageload_metrics.proto (right):

https://codereview.chromium.org/2305013002/diff/20001/components/data_reducti...
components/data_reduction_proxy/proto/pageload_metrics.proto:23: reserved 3;
Why not change the field to deprecated=true
https://cs.chromium.org/search/?q=f:proto+deprecated%5C%3Dtrue&sq=package:chr...

[RyanSturm, 2016-09-06 20:02:05]

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

[RyanSturm, 2016-09-06 20:02:57]

tbansal: PTAL

https://codereview.chromium.org/2305013002/diff/20001/chrome/browser/page_loa...
File
chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc
(right):

https://codereview.chromium.org/2305013002/diff/20001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc:41:
#define RECORD_HISTOGRAMS_FOR_SUFFIX(data, event, value, info,              \
On 2016/09/06 19:44:43, tbansal1 wrote:
> Can you add a comment explaining what event is (IIUC, it is time delta from
> navigation start to when the event happened). Also, may be rename "event" to
> something else?

Added comment, no name change since I don't have a better name.

https://codereview.chromium.org/2305013002/diff/20001/components/data_reducti...
File
components/data_reduction_proxy/core/common/data_reduction_proxy_page_load_timing.cc
(right):

https://codereview.chromium.org/2305013002/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/common/data_reduction_proxy_page_load_timing.cc:27:
DataReductionProxyPageLoadTiming::DataReductionProxyPageLoadTiming(
On 2016/09/06 19:44:43, tbansal1 wrote:
> nit: Why not use default copy constructor.

Done.

https://codereview.chromium.org/2305013002/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/common/data_reduction_proxy_page_load_timing.cc:27:
DataReductionProxyPageLoadTiming::DataReductionProxyPageLoadTiming(
On 2016/09/06 19:44:43, tbansal1 wrote:
> nit: May be add a default assignment operator.

base::Time only doesn't have a a copy assignment operator, so deafult won't
work.

https://codereview.chromium.org/2305013002/diff/20001/components/data_reducti...
File components/data_reduction_proxy/proto/pageload_metrics.proto (right):

https://codereview.chromium.org/2305013002/diff/20001/components/data_reducti...
components/data_reduction_proxy/proto/pageload_metrics.proto:23: reserved 3;
On 2016/09/06 19:44:43, tbansal1 wrote:
> Why not change the field to deprecated=true
>
https://cs.chromium.org/search/?q=f:proto+deprecated%5C%3Dtrue&sq=package:chr...

It's unsupported, and there shouldn't be code generated.

[commit-bot: I haz the power, 2016-09-06 20:03:21]

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

[tbansal1, 2016-09-06 20:13:17]

lgtm

https://codereview.chromium.org/2305013002/diff/20001/components/data_reducti...
File components/data_reduction_proxy/proto/pageload_metrics.proto (right):

https://codereview.chromium.org/2305013002/diff/20001/components/data_reducti...
components/data_reduction_proxy/proto/pageload_metrics.proto:23: reserved 3;
On 2016/09/06 20:02:57, RyanSturm wrote:
> On 2016/09/06 19:44:43, tbansal1 wrote:
> > Why not change the field to deprecated=true
> >
>
https://cs.chromium.org/search/?q=f:proto+deprecated%5C%3Dtrue&sq=package:chr...
> 
> It's unsupported, and there shouldn't be code generated.

Acknowledged.

[RyanSturm, 2016-09-06 20:32:20]

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

[RyanSturm, 2016-09-06 20:32:21]

jpfeiff: PTAL @ .proto

[Bryan McQuade, 2016-09-06 20:38:57]

lgtm

[jpfeiff, 2016-09-06 20:43:36]

lgtm

[RyanSturm, 2016-09-06 20:44:08]

The CQ bit was unchecked by ryansturm@chromium.org

[RyanSturm, 2016-09-06 20:44:09]

The CQ bit was checked by ryansturm@chromium.org

[commit-bot: I haz the power, 2016-09-06 20:45:22]

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-09-06 20:54:45]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-09-06 20:57:49]

Description was changed from

==========
Adding parse UMA and pingback metrics

This adds DataReductionProxyMetricsObserver UMA for ParseDuration and
and amount of time parse blocked on script load. This also adds
parse_stop and parse_blocked_on_script_load_duration to the
pageload_metrics proto message. This also deprecates last_request_time
in the proto message.

BUG=642113
==========

to

==========
Adding parse UMA and pingback metrics

This adds DataReductionProxyMetricsObserver UMA for ParseDuration and
and amount of time parse blocked on script load. This also adds
parse_stop and parse_blocked_on_script_load_duration to the
pageload_metrics proto message. This also deprecates last_request_time
in the proto message.

BUG=642113

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

[commit-bot: I haz the power, 2016-09-06 20:57:50]

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