Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2795453003:
Add additional metrics for subresource filtering. (Closed)
|
Created:
3 years, 8 months ago by Bryan McQuade Modified:
3 years, 8 months ago CC:
chromium-reviews, csharrison+watch_chromium.org, loading-reviews+metrics_chromium.org, speed-metrics-reviews_chromium.org, subresource-filter-reviews_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd additional metrics for subresource filtering.
This change adds the following:
* parse blocking script breakouts, so we can better understand differences in
improvements to FCP/FMP due to doc.write blocking vs general subresource
filtering
* foreground duration, so we can understand time on page when subresource
filtering is active
* byte and resource counts for pages where media was played
BUG=708162
Review-Url: https://codereview.chromium.org/2795453003
Cr-Commit-Position: refs/heads/master@{#462278}
Committed: https://chromium.googlesource.com/chromium/src/+/4244d4e4d0a4dc0e6fd0eea57801dfbf41401600
Patch Set 1 #Patch Set 2 : Add additional subresource filter metrics. #Patch Set 3 : add histograms.xml entries #
Total comments: 6
Patch Set 4 : address comments #Patch Set 5 : fix tests #
Messages
Total messages: 32 (24 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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
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 parse blocking script breakouts for subresource filter metrics. BUG= ========== to ========== Add additional metrics for subresource filtering. This change adds the following: * parse blocking script breakouts, so we can better understand differences in improvements to FCP/FMP due to doc.write blocking vs general subresource filtering * foreground duration, so we can understand time on page when subresource filtering is active * byte and resource counts for pages where media was played BUG= ==========
Description was changed from ========== Add additional metrics for subresource filtering. This change adds the following: * parse blocking script breakouts, so we can better understand differences in improvements to FCP/FMP due to doc.write blocking vs general subresource filtering * foreground duration, so we can understand time on page when subresource filtering is active * byte and resource counts for pages where media was played BUG= ========== to ========== Add additional metrics for subresource filtering. This change adds the following: * parse blocking script breakouts, so we can better understand differences in improvements to FCP/FMP due to doc.write blocking vs general subresource filtering * foreground duration, so we can understand time on page when subresource filtering is active * byte and resource counts for pages where media was played BUG=708162 ==========
bmcquade@chromium.org changed reviewers: + csharrison@chromium.org
The CQ bit was checked by bmcquade@chromium.org to run a CQ dry run
bmcquade@chromium.org changed reviewers: + isherman@chromium.org
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
csharrison, PTAL for page_load_metrics isherman, PTAL for histograms.xml Thanks!
https://codereview.chromium.org/2795453003/diff/40001/chrome/browser/page_loa... File chrome/browser/page_load_metrics/observers/subresource_filter_metrics_observer.cc (right): https://codereview.chromium.org/2795453003/diff/40001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/subresource_filter_metrics_observer.cc:284: PAGE_RESOURCE_COUNT_HISTOGRAM( re: not renaming these, i intend to try to merge this to 58, and our field trials for this code are very limited in scope, so i'm not worried about old data and new data colliding here.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM https://codereview.chromium.org/2795453003/diff/40001/chrome/browser/page_loa... File chrome/browser/page_load_metrics/observers/subresource_filter_metrics_observer.cc (right): https://codereview.chromium.org/2795453003/diff/40001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/subresource_filter_metrics_observer.cc:284: PAGE_RESOURCE_COUNT_HISTOGRAM( On 2017/04/04 15:00:13, Bryan McQuade wrote: > re: not renaming these, i intend to try to merge this to 58, and our field > trials for this code are very limited in scope, so i'm not worried about old > data and new data colliding here. Acknowledged.
https://codereview.chromium.org/2795453003/diff/40001/chrome/browser/page_loa... File chrome/browser/page_load_metrics/observers/subresource_filter_metrics_observer.cc (right): https://codereview.chromium.org/2795453003/diff/40001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/subresource_filter_metrics_observer.cc:284: PAGE_RESOURCE_COUNT_HISTOGRAM( On 2017/04/04 15:00:13, Bryan McQuade wrote: > re: not renaming these, i intend to try to merge this to 58, and our field > trials for this code are very limited in scope, so i'm not worried about old > data and new data colliding here. If I'm understanding correctly, you've changed the semantics of these histograms? Could you please update their descriptions in histograms.xml accordingly? https://codereview.chromium.org/2795453003/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2795453003/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:121693: + <suffix name="AfterPaint" label="Limited to pages where media was played."/> Is this the correct suffix name, or did you mean to use "MediaPlayed" here?
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 unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks! Addressed comments. https://codereview.chromium.org/2795453003/diff/40001/chrome/browser/page_loa... File chrome/browser/page_load_metrics/observers/subresource_filter_metrics_observer.cc (right): https://codereview.chromium.org/2795453003/diff/40001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/subresource_filter_metrics_observer.cc:284: PAGE_RESOURCE_COUNT_HISTOGRAM( On 2017/04/04 at 21:01:51, Ilya Sherman wrote: > On 2017/04/04 15:00:13, Bryan McQuade wrote: > > re: not renaming these, i intend to try to merge this to 58, and our field > > trials for this code are very limited in scope, so i'm not worried about old > > data and new data colliding here. > > If I'm understanding correctly, you've changed the semantics of these histograms? Could you please update their descriptions in histograms.xml accordingly? Good point. After thinking about this a bit more, I decided that it's beneficial to have an 'overall' category for all pages, and 2 breakout sets of metrics, one for pages where media was played, and another where no media was played. I've updated the code to reflect this, thanks! https://codereview.chromium.org/2795453003/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2795453003/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:121693: + <suffix name="AfterPaint" label="Limited to pages where media was played."/> On 2017/04/04 at 21:01:51, Ilya Sherman wrote: > Is this the correct suffix name, or did you mean to use "MediaPlayed" here? Thanks for catching this! Foxed, thanks!
LGTM, thanks.
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 Link to the patchset: https://codereview.chromium.org/2795453003/#ps80001 (title: "fix tests")
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": 80001, "attempt_start_ts": 1491435480582610,
"parent_rev": "455e153fb90088ff8e0492c01f5c5f6ee427d1b7", "commit_rev":
"4244d4e4d0a4dc0e6fd0eea57801dfbf41401600"}
Message was sent while issue was closed.
Description was changed from ========== Add additional metrics for subresource filtering. This change adds the following: * parse blocking script breakouts, so we can better understand differences in improvements to FCP/FMP due to doc.write blocking vs general subresource filtering * foreground duration, so we can understand time on page when subresource filtering is active * byte and resource counts for pages where media was played BUG=708162 ========== to ========== Add additional metrics for subresource filtering. This change adds the following: * parse blocking script breakouts, so we can better understand differences in improvements to FCP/FMP due to doc.write blocking vs general subresource filtering * foreground duration, so we can understand time on page when subresource filtering is active * byte and resource counts for pages where media was played BUG=708162 Review-Url: https://codereview.chromium.org/2795453003 Cr-Commit-Position: refs/heads/master@{#462278} Committed: https://chromium.googlesource.com/chromium/src/+/4244d4e4d0a4dc0e6fd0eea57801... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/4244d4e4d0a4dc0e6fd0eea57801... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
