[subresource_filter] Add metrics for all NavigationThrottle delays

[subresource_filter] Add metrics for all NavigationThrottle delays

These are more high level than the standard DocumentSubresourceFilter
ones, and account for thread hops, task starvation, etc.

BUG=609747
Review-Url: https://codereview.chromium.org/2861053004
Cr-Commit-Position: refs/heads/master@{#470927}
Committed: https://chromium.googlesource.com/chromium/src/+/adbcf1feb08731127872db3b215e9c7742335647

[Charlie Harrison, 2017-05-05 15:54:21]

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

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

Dry run: CQ is trying da patch.

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

[Charlie Harrison, 2017-05-05 15:55:57]

csharrison@chromium.org changed reviewers:
+ rkaplow@chromium.org, shivanisha@chromium.org

[Charlie Harrison, 2017-05-05 15:55:58]

shivanisha, rkaplow: PTAL?

[Charlie Harrison, 2017-05-05 15:58:29]

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

[commit-bot: I haz the power, 2017-05-05 15:58:43]

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-05 17:14:33]

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

[commit-bot: I haz the power, 2017-05-05 17:14:34]

Dry run: This issue passed the CQ dry run.

[shivanisha, 2017-05-05 18:43:24]

On 2017/05/05 at 15:55:58, csharrison wrote:
> shivanisha, rkaplow: PTAL?

lgtm % nits

[shivanisha, 2017-05-05 18:44:51]

https://codereview.chromium.org/2861053004/diff/20001/components/subresource_...
File
components/subresource_filter/content/browser/subframe_navigation_filtering_throttle.h
(right):

https://codereview.chromium.org/2861053004/diff/20001/components/subresource_...
components/subresource_filter/content/browser/subframe_navigation_filtering_throttle.h:50:
base::TimeTicks last_defer_time_;
nit: Rename to defer_start_timestamp_ to emphasize it is actually a timestamp?

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

https://codereview.chromium.org/2861053004/diff/20001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:71420: +    Records the total time a the
activation state navigation throttle within a
Remove 'a '

https://codereview.chromium.org/2861053004/diff/20001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:71430: +    Records the total time a the
activation state navigation throttle within a
Remove 'a '

[Charlie Harrison, 2017-05-05 18:55:57]

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

[Charlie Harrison, 2017-05-05 18:56:07]

https://codereview.chromium.org/2861053004/diff/20001/components/subresource_...
File
components/subresource_filter/content/browser/subframe_navigation_filtering_throttle.h
(right):

https://codereview.chromium.org/2861053004/diff/20001/components/subresource_...
components/subresource_filter/content/browser/subframe_navigation_filtering_throttle.h:50:
base::TimeTicks last_defer_time_;
On 2017/05/05 18:44:50, shivanisha wrote:
> nit: Rename to defer_start_timestamp_ to emphasize it is actually a timestamp?

What about last_defer_timestamp_? Since we defer not only at the start but also
during redirects.

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

https://codereview.chromium.org/2861053004/diff/20001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:71420: +    Records the total time a the
activation state navigation throttle within a
On 2017/05/05 18:44:51, shivanisha wrote:
> Remove 'a '

Done.

https://codereview.chromium.org/2861053004/diff/20001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:71430: +    Records the total time a the
activation state navigation throttle within a
On 2017/05/05 18:44:50, shivanisha wrote:
> Remove 'a '

Done.

[commit-bot: I haz the power, 2017-05-05 18:56:52]

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-05 22:29:24]

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

[commit-bot: I haz the power, 2017-05-05 22:29:25]

Dry run: This issue passed the CQ dry run.

[pkalinnikov, 2017-05-08 12:00:04]

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

[pkalinnikov, 2017-05-08 12:00:05]

A drive-by comment.

https://codereview.chromium.org/2861053004/diff/40001/components/subresource_...
File
components/subresource_filter/content/browser/activation_state_computing_navigation_throttle.cc
(right):

https://codereview.chromium.org/2861053004/diff/40001/components/subresource_...
components/subresource_filter/content/browser/activation_state_computing_navigation_throttle.cc:109:
UMA_HISTOGRAM_TIMES("SubresourceFilter.DocumentLoad.ActivationComputingDelay",
Do you expect it to be mostly >= 1 ms?

We also have microseconds-precision macros in
c/s_f/core/common/time_measurements.h (see UMA_HISTOGRAM_MICRO_TIMES).

[Charlie Harrison, 2017-05-08 12:53:33]

https://codereview.chromium.org/2861053004/diff/40001/components/subresource_...
File
components/subresource_filter/content/browser/activation_state_computing_navigation_throttle.cc
(right):

https://codereview.chromium.org/2861053004/diff/40001/components/subresource_...
components/subresource_filter/content/browser/activation_state_computing_navigation_throttle.cc:109:
UMA_HISTOGRAM_TIMES("SubresourceFilter.DocumentLoad.ActivationComputingDelay",
On 2017/05/08 12:00:05, pkalinnikov wrote:
> Do you expect it to be mostly >= 1 ms?
> 
> We also have microseconds-precision macros in
> c/s_f/core/common/time_measurements.h (see UMA_HISTOGRAM_MICRO_TIMES).

Yeah we can us microseconds for these metrics (maybe for the filtering ones as
well). I think I would feel more comfortable in the latter case using a
histogram with a ceiling > 1 second though using custom times.

[rkaplow, 2017-05-10 14:43:53]

lgtm

histograms lg

[Charlie Harrison, 2017-05-10 15:02:11]

Thanks! I've updated the histograms to be in microseconds in the 1us - 10s range
with 50 buckets. I think this should be fine for all of them.

Pavel: WDYT?

[Charlie Harrison, 2017-05-10 15:02:14]

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

[commit-bot: I haz the power, 2017-05-10 15:03:02]

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-10 16:28:12]

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

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

Dry run: This issue passed the CQ dry run.

[pkalinnikov, 2017-05-11 08:55:26]

LGTM.

[Charlie Harrison, 2017-05-11 11:55:57]

The CQ bit was checked by csharrison@chromium.org

[Charlie Harrison, 2017-05-11 11:55:58]

The patchset sent to the CQ was uploaded after l-g-t-m from
shivanisha@chromium.org, rkaplow@chromium.org
Link to the patchset: https://codereview.chromium.org/2861053004/#ps60001
(title: "microseconds")

[commit-bot: I haz the power, 2017-05-11 11:56:21]

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-11 12:00:33]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1494503757765140,
"parent_rev": "cecb6d2ec467966fcd43f0664f9820f526039765", "commit_rev":
"adbcf1feb08731127872db3b215e9c7742335647"}

[commit-bot: I haz the power, 2017-05-11 12:00:44]

Description was changed from

==========
[subresource_filter] Add metrics for all NavigationThrottle delays

These are more high level than the standard DocumentSubresourceFilter
ones, and account for thread hops, task starvation, etc.

BUG=609747
==========

to

==========
[subresource_filter] Add metrics for all NavigationThrottle delays

These are more high level than the standard DocumentSubresourceFilter
ones, and account for thread hops, task starvation, etc.

BUG=609747

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

[commit-bot: I haz the power, 2017-05-11 12:00:45]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/adbcf1feb08731127872db3b215e...