Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2684153003:
Add subresource filter page load metrics observer (Closed)
|
Created:
3 years, 10 months ago by Bryan McQuade Modified:
3 years, 10 months ago CC:
chromium-reviews, csharrison+watch_chromium.org, loading-reviews+metrics_chromium.org, subresource-filter-reviews_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd subresource filter page load metrics observer
SubresourceFilterMetricsObserver tracks key page load
performance metrics, as well as bytes loaded per page,
for page loads where at least one subresource was filtered.
BUG=690161
Review-Url: https://codereview.chromium.org/2684153003
Cr-Commit-Position: refs/heads/master@{#450167}
Committed: https://chromium.googlesource.com/chromium/src/+/f9bf372ebfcea5043ba08a15320e8ddace508f19
Patch Set 1 #Patch Set 2 : more histograms #Patch Set 3 : renames #Patch Set 4 : histogram description fix #Patch Set 5 : Add subresource filter page load metrics observer #
Total comments: 10
Patch Set 6 : address comment #Patch Set 7 : add browsertest #Patch Set 8 : address comments #Patch Set 9 : address comments #
Total comments: 7
Patch Set 10 : address comments #
Total comments: 4
Patch Set 11 : address comments #
Total comments: 2
Patch Set 12 : address comment #Messages
Total messages: 58 (39 generated)
The CQ bit was checked by bmcquade@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Add subresource filter page load metrics observer BUG=690161 ========== to ========== Add subresource filter page load metrics observer SubresourceFilterMetricsObserver tracks key page load performance metrics, as well as bytes loaded per page, for page loads where at least one subresource was filtered. BUG=690161 ==========
The CQ bit was checked by bmcquade@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Add subresource filter page load metrics observer SubresourceFilterMetricsObserver tracks key page load performance metrics, as well as bytes loaded per page, for page loads where at least one subresource was filtered. BUG=690161 ========== to ========== Add subresource filter page load metrics observer SubresourceFilterMetricsObserver tracks key page load performance metrics, as well as bytes loaded per page, for page loads where at least one subresource was filtered. This change also promotes request count histograms to common histograms, reported by the core observer, DRP observer, and subresource filter observer. BUG=690161 ==========
bmcquade@chromium.org changed reviewers: + csharrison@chromium.org, engedy@chromium.org
PTAL, thanks!
The CQ bit was checked by bmcquade@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/02/09 at 15:16:10, Bryan McQuade wrote: > PTAL, thanks! BTW happy to break out the resource count histograms into a separate change if you prefer.
The CQ bit was checked by bmcquade@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by bmcquade@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Add subresource filter page load metrics observer SubresourceFilterMetricsObserver tracks key page load performance metrics, as well as bytes loaded per page, for page loads where at least one subresource was filtered. This change also promotes request count histograms to common histograms, reported by the core observer, DRP observer, and subresource filter observer. BUG=690161 ========== to ========== Add subresource filter page load metrics observer SubresourceFilterMetricsObserver tracks key page load performance metrics, as well as bytes loaded per page, for page loads where at least one subresource was filtered. BUG=690161 ==========
On 2017/02/09 at 15:57:13, Bryan McQuade wrote: > On 2017/02/09 at 15:16:10, Bryan McQuade wrote: > > PTAL, thanks! > > BTW happy to break out the resource count histograms into a separate change if you prefer. I moved resource count histogram changes into another change - this change focuses solely on subresource filter page load metrics observer now.
Generally looks good but I would love a browser test that ensures that the behavior flag is set correctly in Blink. https://codereview.chromium.org/2684153003/diff/80001/chrome/browser/page_loa... File chrome/browser/page_load_metrics/observers/subresource_filter_metrics_observer.cc (right): https://codereview.chromium.org/2684153003/diff/80001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/subresource_filter_metrics_observer.cc:12: bool IsSubresourceFilteredPageLoad( Why not just inline this when we observe the loading behavior change, and just query subresource_filter_observed_ everywhere? https://codereview.chromium.org/2684153003/diff/80001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/subresource_filter_metrics_observer.cc:156: UMA_HISTOGRAM_COUNTS(internal::kHistogramSubresourceFilterCount, 1); I think you need a more specialized histogram, as UMA_HISTOGRAM_COUNTS by default uses 50 buckets, but this is only a unary count. I think there's a bool one, or you can do a custom count. https://codereview.chromium.org/2684153003/diff/80001/chrome/browser/page_loa... File chrome/browser/page_load_metrics/observers/subresource_filter_metrics_observer_unittest.cc (right): https://codereview.chromium.org/2684153003/diff/80001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/subresource_filter_metrics_observer_unittest.cc:21: bool NoMetricsRecorded() { Can we make this a positive case, to avoid double negatives with EXPECT_FALSE(NoMetricsRecorded())? https://codereview.chromium.org/2684153003/diff/80001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/subresource_filter_metrics_observer_unittest.cc:74: (timing.first_contentful_paint.value() - timing.parse_start.value()) Should we initialize parse_start in InitializePageLoadTiming?
Thanks! One quick comment to get your feedback. Will address other comments shortly. https://codereview.chromium.org/2684153003/diff/80001/chrome/browser/page_loa... File chrome/browser/page_load_metrics/observers/subresource_filter_metrics_observer.cc (right): https://codereview.chromium.org/2684153003/diff/80001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/subresource_filter_metrics_observer.cc:12: bool IsSubresourceFilteredPageLoad( On 2017/02/09 at 16:51:14, Charlie Harrison wrote: > Why not just inline this when we observe the loading behavior change, and just query subresource_filter_observed_ everywhere? This is a really good idea. In the current implementation, we invoke OnLoadingBehaviorObserved after dispatching to any of the other OnFoo methods, so we'd not record metrics correctly for updates that come in the same IPC with the current impl. I'm inclined to change this to dispatch to OnLoadingBehaviorObserved before the other OnFoo handlers, to enable the pattern you describe. Can you see any issues with this? If not, I'll go ahead and make that change. I'll be sure to verify the current ordering isn't significant for any of the other observers as well.
https://codereview.chromium.org/2684153003/diff/80001/chrome/browser/page_loa... File chrome/browser/page_load_metrics/observers/subresource_filter_metrics_observer.cc (right): https://codereview.chromium.org/2684153003/diff/80001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/subresource_filter_metrics_observer.cc:12: bool IsSubresourceFilteredPageLoad( On 2017/02/09 17:07:56, Bryan McQuade wrote: > On 2017/02/09 at 16:51:14, Charlie Harrison wrote: > > Why not just inline this when we observe the loading behavior change, and just > query subresource_filter_observed_ everywhere? > > This is a really good idea. In the current implementation, we invoke > OnLoadingBehaviorObserved after dispatching to any of the other OnFoo methods, > so we'd not record metrics correctly for updates that come in the same IPC with > the current impl. > > I'm inclined to change this to dispatch to OnLoadingBehaviorObserved before the > other OnFoo handlers, to enable the pattern you describe. Can you see any issues > with this? If not, I'll go ahead and make that change. I'll be sure to verify > the current ordering isn't significant for any of the other observers as well. Yeah that solution sounds good to me. For some reason I thought that was the current behavior, but in fact the opposite is the case :P
The CQ bit was checked by bmcquade@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by bmcquade@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by bmcquade@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by bmcquade@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks! Addressed comments. https://codereview.chromium.org/2684153003/diff/80001/chrome/browser/page_loa... File chrome/browser/page_load_metrics/observers/subresource_filter_metrics_observer.cc (right): https://codereview.chromium.org/2684153003/diff/80001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/subresource_filter_metrics_observer.cc:12: bool IsSubresourceFilteredPageLoad( On 2017/02/09 at 17:10:09, Charlie Harrison wrote: > On 2017/02/09 17:07:56, Bryan McQuade wrote: > > On 2017/02/09 at 16:51:14, Charlie Harrison wrote: > > > Why not just inline this when we observe the loading behavior change, and just > > query subresource_filter_observed_ everywhere? > > > > This is a really good idea. In the current implementation, we invoke > > OnLoadingBehaviorObserved after dispatching to any of the other OnFoo methods, > > so we'd not record metrics correctly for updates that come in the same IPC with > > the current impl. > > > > I'm inclined to change this to dispatch to OnLoadingBehaviorObserved before the > > other OnFoo handlers, to enable the pattern you describe. Can you see any issues > > with this? If not, I'll go ahead and make that change. I'll be sure to verify > > the current ordering isn't significant for any of the other observers as well. > > Yeah that solution sounds good to me. For some reason I thought that was the current behavior, but in fact the opposite is the case :P Sounds good, I made this change, thanks! https://codereview.chromium.org/2684153003/diff/80001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/subresource_filter_metrics_observer.cc:156: UMA_HISTOGRAM_COUNTS(internal::kHistogramSubresourceFilterCount, 1); On 2017/02/09 at 16:51:14, Charlie Harrison wrote: > I think you need a more specialized histogram, as UMA_HISTOGRAM_COUNTS by default uses 50 buckets, but this is only a unary count. I think there's a bool one, or you can do a custom count. ah, my mistake. switched to UMA_HISTOGRAM_BOOLEAN. thanks! https://codereview.chromium.org/2684153003/diff/80001/chrome/browser/page_loa... File chrome/browser/page_load_metrics/observers/subresource_filter_metrics_observer_unittest.cc (right): https://codereview.chromium.org/2684153003/diff/80001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/subresource_filter_metrics_observer_unittest.cc:21: bool NoMetricsRecorded() { On 2017/02/09 at 16:51:14, Charlie Harrison wrote: > Can we make this a positive case, to avoid double negatives with EXPECT_FALSE(NoMetricsRecorded())? Done https://codereview.chromium.org/2684153003/diff/80001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/subresource_filter_metrics_observer_unittest.cc:74: (timing.first_contentful_paint.value() - timing.parse_start.value()) On 2017/02/09 at 16:51:14, Charlie Harrison wrote: > Should we initialize parse_start in InitializePageLoadTiming? Done.
LGTM https://codereview.chromium.org/2684153003/diff/160001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/subresource_filter_metrics_observer.cc (right): https://codereview.chromium.org/2684153003/diff/160001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/subresource_filter_metrics_observer.cc:153: UMA_HISTOGRAM_BOOLEAN(internal::kHistogramSubresourceFilterCount, true); I wonder if we should be logging an enum histogram with two values {All observed commits, used filtering}, so that we can easily analyze this number without pulling up another histogram. https://codereview.chromium.org/2684153003/diff/160001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/subresource_filter_metrics_observer.h (right): https://codereview.chromium.org/2684153003/diff/160001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/subresource_filter_metrics_observer.h:71: bool subresource_filter_observed_; nit: = false? https://codereview.chromium.org/2684153003/diff/160001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/page_load_tracker.cc (right): https://codereview.chromium.org/2684153003/diff/160001/chrome/browser/page_lo... chrome/browser/page_load_metrics/page_load_tracker.cc:259: if (extra_info.metadata.behavior_flags != last_metadata.behavior_flags) Why not pull this above OnTimingUpdate too?
The CQ bit was checked by bmcquade@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2684153003/diff/160001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/subresource_filter_metrics_observer.cc (right): https://codereview.chromium.org/2684153003/diff/160001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/subresource_filter_metrics_observer.cc:153: UMA_HISTOGRAM_BOOLEAN(internal::kHistogramSubresourceFilterCount, true); On 2017/02/09 at 19:19:13, Charlie Harrison wrote: > I wonder if we should be logging an enum histogram with two values {All observed commits, used filtering}, so that we can easily analyze this number without pulling up another histogram. we don't do this in other observers, so i'm disinclined to do this for the time being. there's support for computing ratios between 2 histogram counts in the uma dashboard, for the timeline view, so i'm inclined to try that and see if it's sufficient for us first. https://codereview.chromium.org/2684153003/diff/160001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/subresource_filter_metrics_observer.h (right): https://codereview.chromium.org/2684153003/diff/160001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/subresource_filter_metrics_observer.h:71: bool subresource_filter_observed_; On 2017/02/09 at 19:19:13, Charlie Harrison wrote: > nit: = false? good catch, thanks! https://codereview.chromium.org/2684153003/diff/160001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/page_load_tracker.cc (right): https://codereview.chromium.org/2684153003/diff/160001/chrome/browser/page_lo... chrome/browser/page_load_metrics/page_load_tracker.cc:259: if (extra_info.metadata.behavior_flags != last_metadata.behavior_flags) On 2017/02/09 at 19:19:13, Charlie Harrison wrote: > Why not pull this above OnTimingUpdate too? done
https://codereview.chromium.org/2684153003/diff/160001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/subresource_filter_metrics_observer.cc (right): https://codereview.chromium.org/2684153003/diff/160001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/subresource_filter_metrics_observer.cc:153: UMA_HISTOGRAM_BOOLEAN(internal::kHistogramSubresourceFilterCount, true); On 2017/02/09 19:30:49, Bryan McQuade wrote: > On 2017/02/09 at 19:19:13, Charlie Harrison wrote: > > I wonder if we should be logging an enum histogram with two values {All > observed commits, used filtering}, so that we can easily analyze this number > without pulling up another histogram. > > we don't do this in other observers, so i'm disinclined to do this for the time > being. there's support for computing ratios between 2 histogram counts in the > uma dashboard, for the timeline view, so i'm inclined to try that and see if > it's sufficient for us first. Do we log these single count histograms for other observers? In any case, comparing to another histogram is fine by me, you can consider this suggestion optional.
On 2017/02/09 at 19:33:36, csharrison wrote: > https://codereview.chromium.org/2684153003/diff/160001/chrome/browser/page_lo... > File chrome/browser/page_load_metrics/observers/subresource_filter_metrics_observer.cc (right): > > https://codereview.chromium.org/2684153003/diff/160001/chrome/browser/page_lo... > chrome/browser/page_load_metrics/observers/subresource_filter_metrics_observer.cc:153: UMA_HISTOGRAM_BOOLEAN(internal::kHistogramSubresourceFilterCount, true); > On 2017/02/09 19:30:49, Bryan McQuade wrote: > > On 2017/02/09 at 19:19:13, Charlie Harrison wrote: > > > I wonder if we should be logging an enum histogram with two values {All > > observed commits, used filtering}, so that we can easily analyze this number > > without pulling up another histogram. > > > > we don't do this in other observers, so i'm disinclined to do this for the time > > being. there's support for computing ratios between 2 histogram counts in the > > uma dashboard, for the timeline view, so i'm inclined to try that and see if > > it's sufficient for us first. > > Do we log these single count histograms for other observers? In any case, comparing to another histogram is fine by me, you can consider this suggestion optional. Yeah, we have this for docwrite blocking as well. Sounds good. Once data comes in I'll send out a link to compute the ratio and we can see if it works or if we should try something else. Thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
bmcquade@chromium.org changed reviewers: + rkaplow@chromium.org
rkaplow, PTAL for histograms.xml, thanks!
LGTM % one nit. https://codereview.chromium.org/2684153003/diff/180001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/subresource_filter_metrics_observer.cc (right): https://codereview.chromium.org/2684153003/diff/180001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/subresource_filter_metrics_observer.cc:153: UMA_HISTOGRAM_BOOLEAN(internal::kHistogramSubresourceFilterCount, true); We don't record `false` values here because I suppose we already have plenty of preexisting histograms we can use as a baseline to count the number of loads without a subresource being filtered, right? Can we name one either here or in `histograms.xml` so it is obvious to everyone which one is it?
https://codereview.chromium.org/2684153003/diff/180001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/subresource_filter_metrics_observer_unittest.cc (right): https://codereview.chromium.org/2684153003/diff/180001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/subresource_filter_metrics_observer_unittest.cc:45: SimulateTimingUpdate(timing); nit: Should we NavigateToUntrackedUrl() here to ensure no request/byte metrics are recorded?
The CQ bit was checked by bmcquade@chromium.org to run a CQ dry run
Thanks! Addressed comments. https://codereview.chromium.org/2684153003/diff/180001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/subresource_filter_metrics_observer.cc (right): https://codereview.chromium.org/2684153003/diff/180001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/subresource_filter_metrics_observer.cc:153: UMA_HISTOGRAM_BOOLEAN(internal::kHistogramSubresourceFilterCount, true); On 2017/02/10 at 12:35:44, engedy wrote: > We don't record `false` values here because I suppose we already have plenty of preexisting histograms we can use as a baseline to count the number of loads without a subresource being filtered, right? Can we name one either here or in `histograms.xml` so it is obvious to everyone which one is it? Good idea, done. I've updated histograms.xml. Once this goes live I'll send out a link to the timeline view that shows the ratio of affected page loads over time. https://codereview.chromium.org/2684153003/diff/180001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/subresource_filter_metrics_observer_unittest.cc (right): https://codereview.chromium.org/2684153003/diff/180001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/subresource_filter_metrics_observer_unittest.cc:45: SimulateTimingUpdate(timing); On 2017/02/10 at 12:41:18, engedy wrote: > nit: Should we NavigateToUntrackedUrl() here to ensure no request/byte metrics are recorded? Yes, good idea, done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/2684153003/diff/200001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2684153003/diff/200001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:43676: + Counts the number of pages where the subresource filter matched a I'd also be explicit about this only emitting trues (and that is the count)
The CQ bit was checked by bmcquade@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by bmcquade@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from csharrison@chromium.org, engedy@chromium.org, rkaplow@chromium.org Link to the patchset: https://codereview.chromium.org/2684153003/#ps220001 (title: "address comment")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 220001, "attempt_start_ts": 1487030847800130,
"parent_rev": "906ce688ee9742245387dd42581020c905ab0e86", "commit_rev":
"f9bf372ebfcea5043ba08a15320e8ddace508f19"}
Message was sent while issue was closed.
Description was changed from ========== Add subresource filter page load metrics observer SubresourceFilterMetricsObserver tracks key page load performance metrics, as well as bytes loaded per page, for page loads where at least one subresource was filtered. BUG=690161 ========== to ========== Add subresource filter page load metrics observer SubresourceFilterMetricsObserver tracks key page load performance metrics, as well as bytes loaded per page, for page loads where at least one subresource was filtered. BUG=690161 Review-Url: https://codereview.chromium.org/2684153003 Cr-Commit-Position: refs/heads/master@{#450167} Committed: https://chromium.googlesource.com/chromium/src/+/f9bf372ebfcea5043ba08a15320e... ==========
Message was sent while issue was closed.
Committed patchset #12 (id:220001) as https://chromium.googlesource.com/chromium/src/+/f9bf372ebfcea5043ba08a15320e...
Message was sent while issue was closed.
https://codereview.chromium.org/2684153003/diff/200001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2684153003/diff/200001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:43676: + Counts the number of pages where the subresource filter matched a On 2017/02/13 at 18:13:50, rkaplow wrote: > I'd also be explicit about this only emitting trues (and that is the count) Done |
