Aggregate DocumentSubresourceFilter counters on page level.

Aggregate DocumentSubresourceFilter counters on page level.

Factor out statistics about the DocumentSubresourceFilter's work into a
DocumentLoadStatistics structure. Send it to the browser in a
SubresourceFilterHostMsg_DocumentLoadStatistics message, and aggregate the
counters and time measurements for each page that a
ContentSubresourceFilterDriverFactory represents. Report the aggregated counters
and time measurements via newly introduced histograms.

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

[pkalinnikov, 2016-12-27 20:05:44]

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

[pkalinnikov, 2016-12-27 20:05:44]

Balazs, please take an initial look.

I will add the new histograms to the browsertest shortly. Do you see any better
way to test it? I think it's already tested enough in the agent's unittests.

[engedy, 2016-12-28 09:36:02]

LGTM % nits, and missing `document_load_statistics.h`.

Agreed that unit testing coverage is already as much as possible, the rest can
only be verified in the browsertest.

https://codereview.chromium.org/2600253002/diff/1/components/subresource_filt...
File
components/subresource_filter/content/browser/content_subresource_filter_driver_factory.h
(right):

https://codereview.chromium.org/2600253002/diff/1/components/subresource_filt...
components/subresource_filter/content/browser/content_subresource_filter_driver_factory.h:146:
DocumentLoadStatistics page_statistics_;
nit: How about calling this |aggregated_document_statistics_|?

It's a bit confusing that the time says `Document` but the instance says `page`.

https://codereview.chromium.org/2600253002/diff/1/components/subresource_filt...
File components/subresource_filter/content/common/subresource_filter_messages.h
(right):

https://codereview.chromium.org/2600253002/diff/1/components/subresource_filt...
components/subresource_filter/content/common/subresource_filter_messages.h:59:
// finished, just before the DidFinishLoad message. Contains statistics about
Phrasing suggestion: ..., and contains statistics collected by the
DocumentSubresourceFilter up until that point: the number of subresources
evaluated/disallowed/etc, and ...

https://codereview.chromium.org/2600253002/diff/1/components/subresource_filt...
components/subresource_filter/content/common/subresource_filter_messages.h:62:
// time equals to zero if performance measurements were disabled for the load.
nit: The time metrics are equal to...

https://codereview.chromium.org/2600253002/diff/1/components/subresource_filt...
File components/subresource_filter/content/renderer/subresource_filter_agent.h
(right):

https://codereview.chromium.org/2600253002/diff/1/components/subresource_filt...
components/subresource_filter/content/renderer/subresource_filter_agent.h:60: //
Sends statistics about DocumentSubresourceFilter's work to the browser.
nit: about the

https://codereview.chromium.org/2600253002/diff/1/tools/metrics/histograms/hi...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2600253002/diff/1/tools/metrics/histograms/hi...
tools/metrics/histograms/histograms.xml:64380: +    Whenever a document load is
finished in a main frame or subframe with
nit: Update this line.

[pkalinnikov, 2016-12-28 13:43:23]

Addressed all nits, thanks. While I am editing browsertests, you can review the
forgotten "document_load_statistics.h" file.

https://codereview.chromium.org/2600253002/diff/1/components/subresource_filt...
File
components/subresource_filter/content/browser/content_subresource_filter_driver_factory.h
(right):

https://codereview.chromium.org/2600253002/diff/1/components/subresource_filt...
components/subresource_filter/content/browser/content_subresource_filter_driver_factory.h:146:
DocumentLoadStatistics page_statistics_;
On 2016/12/28 09:36:02, engedy wrote:
> nit: How about calling this |aggregated_document_statistics_|?
> 
> It's a bit confusing that the time says `Document` but the instance says
`page`.

Done.

https://codereview.chromium.org/2600253002/diff/1/components/subresource_filt...
File components/subresource_filter/content/common/subresource_filter_messages.h
(right):

https://codereview.chromium.org/2600253002/diff/1/components/subresource_filt...
components/subresource_filter/content/common/subresource_filter_messages.h:59:
// finished, just before the DidFinishLoad message. Contains statistics about
On 2016/12/28 09:36:02, engedy wrote:
> Phrasing suggestion: ..., and contains statistics collected by the
> DocumentSubresourceFilter up until that point: the number of subresources
> evaluated/disallowed/etc, and ...

Done.

https://codereview.chromium.org/2600253002/diff/1/components/subresource_filt...
components/subresource_filter/content/common/subresource_filter_messages.h:62:
// time equals to zero if performance measurements were disabled for the load.
On 2016/12/28 09:36:02, engedy wrote:
> nit: The time metrics are equal to...

Done.

https://codereview.chromium.org/2600253002/diff/1/components/subresource_filt...
File components/subresource_filter/content/renderer/subresource_filter_agent.h
(right):

https://codereview.chromium.org/2600253002/diff/1/components/subresource_filt...
components/subresource_filter/content/renderer/subresource_filter_agent.h:60: //
Sends statistics about DocumentSubresourceFilter's work to the browser.
On 2016/12/28 09:36:02, engedy wrote:
> nit: about the

Done.

https://codereview.chromium.org/2600253002/diff/1/tools/metrics/histograms/hi...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2600253002/diff/1/tools/metrics/histograms/hi...
tools/metrics/histograms/histograms.xml:64380: +    Whenever a document load is
finished in a main frame or subframe with
On 2016/12/28 09:36:02, engedy wrote:
> nit: Update this line.

Done.

[engedy, 2016-12-28 13:56:06]

Still LGTM % 2 nits in document_load_statistics.h. Thanks!

https://codereview.chromium.org/2600253002/diff/20001/components/subresource_...
File components/subresource_filter/content/common/document_load_statistics.h
(right):

https://codereview.chromium.org/2600253002/diff/20001/components/subresource_...
components/subresource_filter/content/common/document_load_statistics.h:8:
#include <cstdint>
nit: By convention we usually include <stdint.h> flavor.

https://codereview.chromium.org/2600253002/diff/20001/components/subresource_...
components/subresource_filter/content/common/document_load_statistics.h:14: //
Contains statistics about different phases of DocumentsSubresourceFilter's
I'm not sure I understand what `phases` means in this context, how about: 

Contains statistics and performance metrics collected by the
DocumentSubresourceFilter.

[pkalinnikov, 2016-12-28 14:04:50]

Added checks to browsertests. PTAL again.

[pkalinnikov, 2016-12-28 14:08:38]

Description was changed from

==========
Aggregate DocumentSubresourceFilter counters on page level.

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

to

==========
Aggregate DocumentSubresourceFilter counters on page level.

Factor out statistics about the DocumentSubresourceFilter's work into a
DocumentLoadStatistics structure. Send it to the browser in a
SubresourceFilterHostMsg_DocumentLoadStatistics message, and aggregate the
counters and time measurements for each page that a
ContentSubresourceFilterDriverFactory represents.

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

[pkalinnikov, 2016-12-28 14:10:10]

Description was changed from

==========
Aggregate DocumentSubresourceFilter counters on page level.

Factor out statistics about the DocumentSubresourceFilter's work into a
DocumentLoadStatistics structure. Send it to the browser in a
SubresourceFilterHostMsg_DocumentLoadStatistics message, and aggregate the
counters and time measurements for each page that a
ContentSubresourceFilterDriverFactory represents.

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

to

==========
Aggregate DocumentSubresourceFilter counters on page level.

Factor out statistics about the DocumentSubresourceFilter's work into a
DocumentLoadStatistics structure. Send it to the browser in a
SubresourceFilterHostMsg_DocumentLoadStatistics message, and aggregate the
counters and time measurements for each page that a
ContentSubresourceFilterDriverFactory represents. Report the aggregated counters
and time measurements via newly introduced histograms.

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

[pkalinnikov, 2016-12-28 14:28:52]

Thanks!

https://codereview.chromium.org/2600253002/diff/20001/components/subresource_...
File components/subresource_filter/content/common/document_load_statistics.h
(right):

https://codereview.chromium.org/2600253002/diff/20001/components/subresource_...
components/subresource_filter/content/common/document_load_statistics.h:8:
#include <cstdint>
On 2016/12/28 13:56:06, engedy wrote:
> nit: By convention we usually include <stdint.h> flavor.

Done.

https://codereview.chromium.org/2600253002/diff/20001/components/subresource_...
components/subresource_filter/content/common/document_load_statistics.h:14: //
Contains statistics about different phases of DocumentsSubresourceFilter's
On 2016/12/28 13:56:06, engedy wrote:
> I'm not sure I understand what `phases` means in this context, how about: 
> 
> Contains statistics and performance metrics collected by the
> DocumentSubresourceFilter.

Done.

[pkalinnikov, 2016-12-28 14:34:45]

pkalinnikov@chromium.org changed reviewers:
+ isherman@chromium.org, palmer@chromium.org

[pkalinnikov, 2016-12-28 14:34:46]

Hi Chris and Ilya,

palmer@: Please review the IPC change in "subresource_filter_messages.h" and
"document_load_statistics.h".

isherman@: Please review the changes in "histograms.xml".

Thank you!

[engedy, 2016-12-28 14:49:42]

Still LGTM.

[Ilya Sherman, 2016-12-28 20:54:57]

metrics lgtm

[palmer, 2016-12-28 23:05:47]

lgtm

https://codereview.chromium.org/2600253002/diff/60001/components/subresource_...
File
components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc
(right):

https://codereview.chromium.org/2600253002/diff/60001/components/subresource_...
components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc:97:
aggregated_document_statistics_.num_loads_total += statistics.num_loads_total;
Do you care about potential integer overflow here? (Not from a security
perspective, which is how I usually bring it up, but from a correctness
perspective.) Maybe it's super rare, or the noise it would create in your
measurements would be easily filterable, in which case maybe you don't care.
Just wondering.

[pkalinnikov, 2016-12-29 11:22:03]

Thanks!

https://codereview.chromium.org/2600253002/diff/60001/components/subresource_...
File
components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc
(right):

https://codereview.chromium.org/2600253002/diff/60001/components/subresource_...
components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc:97:
aggregated_document_statistics_.num_loads_total += statistics.num_loads_total;
On 2016/12/28 23:05:47, palmer wrote:
> Do you care about potential integer overflow here? (Not from a security
> perspective, which is how I usually bring it up, but from a correctness
> perspective.) Maybe it's super rare, or the noise it would create in your
> measurements would be easily filterable, in which case maybe you don't care.
> Just wondering.

An overflow can occur if a page has > 2 bln. subresources. We think that the
chances to encounter such a page are negligible, and we can tolerate that. Added
a note to be explicit about that.

[pkalinnikov, 2016-12-29 11:22:21]

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

[commit-bot: I haz the power, 2016-12-29 11:22: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, 2016-12-29 12:07:10]

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

[commit-bot: I haz the power, 2016-12-29 12:07:10]

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

[pkalinnikov, 2016-12-29 23:54:30]

The CQ bit was checked by pkalinnikov@chromium.org

[pkalinnikov, 2016-12-29 23:54:31]

The patchset sent to the CQ was uploaded after l-g-t-m from palmer@chromium.org,
engedy@chromium.org, isherman@chromium.org
Link to the patchset: https://codereview.chromium.org/2600253002/#ps80001
(title: "Rebase, add note.")

[commit-bot: I haz the power, 2016-12-29 23:54:45]

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-12-30 00:51:35]

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

[commit-bot: I haz the power, 2016-12-30 00:51:36]

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

[pkalinnikov, 2017-01-02 17:06:52]

The CQ bit was checked by pkalinnikov@chromium.org

[commit-bot: I haz the power, 2017-01-02 17:06:59]

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-01-02 18:01:49]

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

[commit-bot: I haz the power, 2017-01-02 18:01:50]

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

[pkalinnikov, 2017-01-09 08:54:01]

The CQ bit was checked by pkalinnikov@chromium.org

[pkalinnikov, 2017-01-09 08:54:02]

The patchset sent to the CQ was uploaded after l-g-t-m from palmer@chromium.org,
engedy@chromium.org, isherman@chromium.org
Link to the patchset: https://codereview.chromium.org/2600253002/#ps100001
(title: "rebase")

[commit-bot: I haz the power, 2017-01-09 08:54:17]

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-01-09 09:43:41]

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

[commit-bot: I haz the power, 2017-01-09 09:43:42]

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

[pkalinnikov, 2017-01-13 20:07:22]

The CQ bit was checked by pkalinnikov@chromium.org

[commit-bot: I haz the power, 2017-01-13 20:08:09]

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-01-13 21:53:10]

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

[commit-bot: I haz the power, 2017-01-13 21:53:12]

Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[pkalinnikov, 2017-01-13 23:07:11]

The CQ bit was checked by pkalinnikov@chromium.org

[commit-bot: I haz the power, 2017-01-13 23:07:45]

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-01-13 23:59:43]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1484348831830320,
"parent_rev": "790e4f0610545c7e41ab4642e7856b058687e1e6", "commit_rev":
"828b1fae0b84215fdc1793a1a25ea3dd09d7bd2d"}

[commit-bot: I haz the power, 2017-01-14 00:00:13]

Description was changed from

==========
Aggregate DocumentSubresourceFilter counters on page level.

Factor out statistics about the DocumentSubresourceFilter's work into a
DocumentLoadStatistics structure. Send it to the browser in a
SubresourceFilterHostMsg_DocumentLoadStatistics message, and aggregate the
counters and time measurements for each page that a
ContentSubresourceFilterDriverFactory represents. Report the aggregated counters
and time measurements via newly introduced histograms.

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

to

==========
Aggregate DocumentSubresourceFilter counters on page level.

Factor out statistics about the DocumentSubresourceFilter's work into a
DocumentLoadStatistics structure. Send it to the browser in a
SubresourceFilterHostMsg_DocumentLoadStatistics message, and aggregate the
counters and time measurements for each page that a
ContentSubresourceFilterDriverFactory represents. Report the aggregated counters
and time measurements via newly introduced histograms.

BUG=609747

Review-Url: https://codereview.chromium.org/2600253002
Cr-Commit-Position: refs/heads/master@{#443731}
Committed:
https://chromium.googlesource.com/chromium/src/+/828b1fae0b84215fdc1793a1a25e...
==========

[commit-bot: I haz the power, 2017-01-14 00:00:15]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/828b1fae0b84215fdc1793a1a25e...