Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2703093002:
Add metrics for tracking subresource filter activation suppression. (Closed)
|
Created:
3 years, 10 months ago by Bryan McQuade Modified:
3 years, 10 months ago CC:
asvitkine+watch_chromium.org, chromium-reviews, csharrison+watch_chromium.org, darin-cc_chromium.org, jam, loading-reviews+metrics_chromium.org, subresource-filter-reviews_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd metrics for tracking subresource filter activation suppression.
This change adds an API on ContentSubresourceFilterDriverFactory that
the SubresourceFilterMetricsObserver queries to determine whether
and why activation was suppressed for the currently committed page load.
The SubresourceFilterMetricsObserver then logs histograms that allow us
to understand how often activation is suppressed.
BUG=694036
Review-Url: https://codereview.chromium.org/2703093002
Cr-Commit-Position: refs/heads/master@{#452027}
Committed: https://chromium.googlesource.com/chromium/src/+/ec69818af043485daccd4d4f43718390e9ddcb08
Patch Set 1 #Patch Set 2 : cleanup #Patch Set 3 : rebase #Patch Set 4 : update tests #Patch Set 5 : fixes #Patch Set 6 : cleanup #Patch Set 7 : cleanup #Patch Set 8 : fix naming #Patch Set 9 : fix browsertest #
Total comments: 9
Patch Set 10 : expand histograms.xml comment #
Total comments: 6
Patch Set 11 : Rename to ActivationDecision. #Patch Set 12 : address comments #
Total comments: 4
Patch Set 13 : address comments #
Total comments: 4
Patch Set 14 : address comments #Messages
Total messages: 72 (56 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: ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
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: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
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 metrics for tracking subresource filter activation suppression. BUG= ========== to ========== Add metrics for tracking subresource filter activation suppression. BUG=694036 ==========
Description was changed from ========== Add metrics for tracking subresource filter activation suppression. BUG=694036 ========== to ========== Add metrics for tracking subresource filter activation suppression. This change adds an API on ContentSubresourceFilterDriverFactory that the SubresourceFilterMetricsObserver queries to determine whether and why activation was suppressed for the currently committed page load. The SubresourceFilterMetricsObserver then logs histograms that allow us to understand how often activation is suppressed. BUG=694036 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_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...
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...
bmcquade@chromium.org changed reviewers: + engedy@chromium.org, melandory@chromium.org
PTAL, thanks! This change introduces an enum that explains why activation was suppressed. This enables us to log activation suppression reasons to understand how often each case is encountered in the wild. It also allows us to more directly test that a specific expected suppression reason was triggered in unit tests, which is a nice plus. The only real logic change I made here was to remove propagation of the 'bool failed_navigation' value, short-circuiting processing in ReadyToCommitNavigation. As best I could tell, this didn't change behavior at all (other than not logging redirect metrics for failed navigations - I assume that's ok) and simplified the code flow a bit. Let me know if I missed something & this part needs to be reverted.
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 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: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
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! Please see two questions below. https://codereview.chromium.org/2703093002/diff/160001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/subresource_filter_metrics_observer.cc (right): https://codereview.chromium.org/2703093002/diff/160001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/subresource_filter_metrics_observer.cc:65: UMA_HISTOGRAM_ENUMERATION( Unlike all other histograms in the PageLoad.Clients.SubresourceFilter.* family, recording a sample into these histograms is not contingent on observing at least one disallowed resource load during the page load. Therefore the page load counts in this histogram would not be comparable to those in the rest. Is this intended? What would you think about moving these histograms to the SubresourceFilter.PageLoad.* family, where one sample is recorded for each page load? The recording of samples could be done in the DriverFactory, which already does most of the heavy lifting. Another, more complicated, solution would be to not fully turn off filtering on whitelisting, but switch it to dry-run mode, so we would still have the loading behavior signal. https://codereview.chromium.org/2703093002/diff/160001/components/subresource... File components/subresource_filter/content/browser/content_subresource_filter_driver_factory.h (right): https://codereview.chromium.org/2703093002/diff/160001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory.h:49: enum ActivationSuppressionReason { I'm trying to think of a name here that better reflects that the default state of the component is no activation, and that activation is rare. In my mental model, the word "suppression" has the connotation of partially/wholly eliminating something which would otherwise exist.
Thanks! A couple high level responses to your earlier comments. Let me know what you think, thanks! https://codereview.chromium.org/2703093002/diff/160001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/subresource_filter_metrics_observer.cc (right): https://codereview.chromium.org/2703093002/diff/160001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/subresource_filter_metrics_observer.cc:65: UMA_HISTOGRAM_ENUMERATION( On 2017/02/20 at 16:19:13, engedy (slow) wrote: > Unlike all other histograms in the PageLoad.Clients.SubresourceFilter.* family, recording a sample into these histograms is not contingent on observing at least one disallowed resource load during the page load. > > Therefore the page load counts in this histogram would not be comparable to those in the rest. Is this intended? > > What would you think about moving these histograms to the SubresourceFilter.PageLoad.* family, where one sample is recorded for each page load? The recording of samples could be done in the DriverFactory, which already does most of the heavy lifting. > > Another, more complicated, solution would be to not fully turn off filtering on whitelisting, but switch it to dry-run mode, so we would still have the loading behavior signal. Thanks! Yes, good point. I see the SuppressionReason histogram as the histogram that tracks the % of page loads where we activate (NOT_SUPPRESSED) as well as all the cases where we didn't (URL_WHITELISTED, etc). In general, we do log different metrics at different times for a given set of PageLoad metrics. I'll make sure to update the histograms.xml entry to be very clear about the fact that filtering is only eligible for pages that report NOT_SUPPRESSED, and all other cases do not have subresource filtering applied. The main reason I want to keep these in PageLoad.* is that page load metrics has some logic for determining what we consider a 'page load', such as excluding non-HTML responses, non-200 responses, same-page navigations, the NTP, and others, and I want to understand how many of those page loads we didn't activate for. I don't think these histograms would be as useful if they didn't have this same filtering logic applied, as then they aren't comparable to anything else in PageLoad.* Given this, I think staying in PageLoad.* makes sense here since we want the same filtering of what is considered a valid 'page load' as all of the other PageLoad.* metrics. How does this sound? https://codereview.chromium.org/2703093002/diff/160001/components/subresource... File components/subresource_filter/content/browser/content_subresource_filter_driver_factory.h (right): https://codereview.chromium.org/2703093002/diff/160001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory.h:49: enum ActivationSuppressionReason { On 2017/02/20 at 16:19:13, engedy (slow) wrote: > I'm trying to think of a name here that better reflects that the default state of the component is no activation, and that activation is rare. In my mental model, the word "suppression" has the connotation of partially/wholly eliminating something which would otherwise exist. Yes, I'm entirely open to naming changes here. Let me know what you decide on. Thanks! FWIW I don't know that it will always be the case that activation is rare. For the lightspeed experiment, for example, activation happens on most page loads.
https://codereview.chromium.org/2703093002/diff/160001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/subresource_filter_metrics_observer.cc (right): https://codereview.chromium.org/2703093002/diff/160001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/subresource_filter_metrics_observer.cc:65: UMA_HISTOGRAM_ENUMERATION( On 2017/02/20 at 19:18:04, Bryan McQuade wrote: > On 2017/02/20 at 16:19:13, engedy (slow) wrote: > > Unlike all other histograms in the PageLoad.Clients.SubresourceFilter.* family, recording a sample into these histograms is not contingent on observing at least one disallowed resource load during the page load. > > > > Therefore the page load counts in this histogram would not be comparable to those in the rest. Is this intended? > > > > What would you think about moving these histograms to the SubresourceFilter.PageLoad.* family, where one sample is recorded for each page load? The recording of samples could be done in the DriverFactory, which already does most of the heavy lifting. > > > > Another, more complicated, solution would be to not fully turn off filtering on whitelisting, but switch it to dry-run mode, so we would still have the loading behavior signal. > > Thanks! Yes, good point. I see the SuppressionReason histogram as the histogram that tracks the % of page loads where we activate (NOT_SUPPRESSED) as well as all the cases where we didn't (URL_WHITELISTED, etc). > > In general, we do log different metrics at different times for a given set of PageLoad metrics. I'll make sure to update the histograms.xml entry to be very clear about the fact that filtering is only eligible for pages that report NOT_SUPPRESSED, and all other cases do not have subresource filtering applied. > > The main reason I want to keep these in PageLoad.* is that page load metrics has some logic for determining what we consider a 'page load', such as excluding non-HTML responses, non-200 responses, same-page navigations, the NTP, and others, and I want to understand how many of those page loads we didn't activate for. I don't think these histograms would be as useful if they didn't have this same filtering logic applied, as then they aren't comparable to anything else in PageLoad.* Given this, I think staying in PageLoad.* makes sense here since we want the same filtering of what is considered a valid 'page load' as all of the other PageLoad.* metrics. > > How does this sound? If you'd prefer, I am also open to logging these in both places, given the slight differences in behavior. But having a view of the data for loads considered valid page loads by PageLoad.* is necessary so I can present a consistent summary of the data in PageLoad.* terms.
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/2703093002/diff/160001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/subresource_filter_metrics_observer.cc (right): https://codereview.chromium.org/2703093002/diff/160001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/subresource_filter_metrics_observer.cc:65: UMA_HISTOGRAM_ENUMERATION( On 2017/02/20 at 21:37:58, Bryan McQuade wrote: > On 2017/02/20 at 19:18:04, Bryan McQuade wrote: > > On 2017/02/20 at 16:19:13, engedy (slow) wrote: > > > Unlike all other histograms in the PageLoad.Clients.SubresourceFilter.* family, recording a sample into these histograms is not contingent on observing at least one disallowed resource load during the page load. > > > > > > Therefore the page load counts in this histogram would not be comparable to those in the rest. Is this intended? > > > > > > What would you think about moving these histograms to the SubresourceFilter.PageLoad.* family, where one sample is recorded for each page load? The recording of samples could be done in the DriverFactory, which already does most of the heavy lifting. > > > > > > Another, more complicated, solution would be to not fully turn off filtering on whitelisting, but switch it to dry-run mode, so we would still have the loading behavior signal. > > > > Thanks! Yes, good point. I see the SuppressionReason histogram as the histogram that tracks the % of page loads where we activate (NOT_SUPPRESSED) as well as all the cases where we didn't (URL_WHITELISTED, etc). > > > > In general, we do log different metrics at different times for a given set of PageLoad metrics. I'll make sure to update the histograms.xml entry to be very clear about the fact that filtering is only eligible for pages that report NOT_SUPPRESSED, and all other cases do not have subresource filtering applied. > > > > The main reason I want to keep these in PageLoad.* is that page load metrics has some logic for determining what we consider a 'page load', such as excluding non-HTML responses, non-200 responses, same-page navigations, the NTP, and others, and I want to understand how many of those page loads we didn't activate for. I don't think these histograms would be as useful if they didn't have this same filtering logic applied, as then they aren't comparable to anything else in PageLoad.* Given this, I think staying in PageLoad.* makes sense here since we want the same filtering of what is considered a valid 'page load' as all of the other PageLoad.* metrics. > > > > How does this sound? > > If you'd prefer, I am also open to logging these in both places, given the slight differences in behavior. But having a view of the data for loads considered valid page loads by PageLoad.* is necessary so I can present a consistent summary of the data in PageLoad.* terms. I expanded the histograms.xml comment to better explain what the new histogram records, and how it relates to other histograms. Take a look and see what you think. Thanks!
Looks good % comments. I would like to take a look at the final enum names. https://codereview.chromium.org/2703093002/diff/160001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/subresource_filter_metrics_observer.cc (right): https://codereview.chromium.org/2703093002/diff/160001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/subresource_filter_metrics_observer.cc:65: UMA_HISTOGRAM_ENUMERATION( On 2017/02/21 13:38:22, Bryan McQuade wrote: > On 2017/02/20 at 21:37:58, Bryan McQuade wrote: > > On 2017/02/20 at 19:18:04, Bryan McQuade wrote: > > > On 2017/02/20 at 16:19:13, engedy (slow) wrote: > > > > Unlike all other histograms in the PageLoad.Clients.SubresourceFilter.* > family, recording a sample into these histograms is not contingent on observing > at least one disallowed resource load during the page load. > > > > > > > > Therefore the page load counts in this histogram would not be comparable > to those in the rest. Is this intended? > > > > > > > > What would you think about moving these histograms to the > SubresourceFilter.PageLoad.* family, where one sample is recorded for each page > load? The recording of samples could be done in the DriverFactory, which already > does most of the heavy lifting. > > > > > > > > Another, more complicated, solution would be to not fully turn off > filtering on whitelisting, but switch it to dry-run mode, so we would still have > the loading behavior signal. > > > > > > Thanks! Yes, good point. I see the SuppressionReason histogram as the > histogram that tracks the % of page loads where we activate (NOT_SUPPRESSED) as > well as all the cases where we didn't (URL_WHITELISTED, etc). > > > > > > In general, we do log different metrics at different times for a given set > of PageLoad metrics. I'll make sure to update the histograms.xml entry to be > very clear about the fact that filtering is only eligible for pages that report > NOT_SUPPRESSED, and all other cases do not have subresource filtering applied. > > > > > > The main reason I want to keep these in PageLoad.* is that page load metrics > has some logic for determining what we consider a 'page load', such as excluding > non-HTML responses, non-200 responses, same-page navigations, the NTP, and > others, and I want to understand how many of those page loads we didn't activate > for. I don't think these histograms would be as useful if they didn't have this > same filtering logic applied, as then they aren't comparable to anything else in > PageLoad.* Given this, I think staying in PageLoad.* makes sense here since we > want the same filtering of what is considered a valid 'page load' as all of the > other PageLoad.* metrics. > > > > > > How does this sound? > > > > If you'd prefer, I am also open to logging these in both places, given the > slight differences in behavior. But having a view of the data for loads > considered valid page loads by PageLoad.* is necessary so I can present a > consistent summary of the data in PageLoad.* terms. > > I expanded the histograms.xml comment to better explain what the new histogram > records, and how it relates to other histograms. Take a look and see what you > think. Thanks! Thank you for the detailed explanation, that sounds perfectly reasonable. Pointing this out in the histogram description make this very clear too. https://codereview.chromium.org/2703093002/diff/160001/components/subresource... File components/subresource_filter/content/browser/content_subresource_filter_driver_factory.h (right): https://codereview.chromium.org/2703093002/diff/160001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory.h:49: enum ActivationSuppressionReason { On 2017/02/20 19:18:04, Bryan McQuade wrote: > On 2017/02/20 at 16:19:13, engedy (slow) wrote: > > I'm trying to think of a name here that better reflects that the default state > of the component is no activation, and that activation is rare. In my mental > model, the word "suppression" has the connotation of partially/wholly > eliminating something which would otherwise exist. > > Yes, I'm entirely open to naming changes here. Let me know what you decide on. > Thanks! > > FWIW I don't know that it will always be the case that activation is rare. For > the lightspeed experiment, for example, activation happens on most page loads. Even so, the experiment affects very few users; so I'd propose inverting this enum. I am somewhat torn on the name, but maybe "ActivationDecision" or "ActivationDecisionReason"? If you have a name that better reflects that it also enumerates reasons for no activation, I'm open to suggestions. https://codereview.chromium.org/2703093002/diff/180001/components/subresource... File components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc (right): https://codereview.chromium.org/2703093002/diff/180001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc:47: bool ContentSubresourceFilterDriverFactory::NavigationIsPageReload( nit: Please move this two static functions further down to match the ordering in the header. https://codereview.chromium.org/2703093002/diff/180001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc:145: ComputeActivationSuppressionReasonForMainFrameURL(const GURL& url) const { nit: Maybe ComputeActivationDecision... or DecideActivation...? https://codereview.chromium.org/2703093002/diff/180001/components/subresource... File components/subresource_filter/content/browser/content_subresource_filter_driver_factory.h (right): https://codereview.chromium.org/2703093002/diff/180001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory.h:112: GetActivationSuppressionReasonForMainFrameDocument() const { nit: GetActivationDecisionForLastCommittedPageLoad?
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 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. PTAL. https://codereview.chromium.org/2703093002/diff/160001/components/subresource... File components/subresource_filter/content/browser/content_subresource_filter_driver_factory.h (right): https://codereview.chromium.org/2703093002/diff/160001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory.h:49: enum ActivationSuppressionReason { On 2017/02/21 at 14:13:13, engedy (slow) wrote: > On 2017/02/20 19:18:04, Bryan McQuade wrote: > > On 2017/02/20 at 16:19:13, engedy (slow) wrote: > > > I'm trying to think of a name here that better reflects that the default state > > of the component is no activation, and that activation is rare. In my mental > > model, the word "suppression" has the connotation of partially/wholly > > eliminating something which would otherwise exist. > > > > Yes, I'm entirely open to naming changes here. Let me know what you decide on. > > Thanks! > > > > FWIW I don't know that it will always be the case that activation is rare. For > > the lightspeed experiment, for example, activation happens on most page loads. > > Even so, the experiment affects very few users; so I'd propose inverting this enum. I am somewhat torn on the name, but maybe "ActivationDecision" or "ActivationDecisionReason"? If you have a name that better reflects that it also enumerates reasons for no activation, I'm open to suggestions. ActivationDecision sounds good to me, thanks! https://codereview.chromium.org/2703093002/diff/180001/components/subresource... File components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc (right): https://codereview.chromium.org/2703093002/diff/180001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc:47: bool ContentSubresourceFilterDriverFactory::NavigationIsPageReload( On 2017/02/21 at 14:13:13, engedy (slow) wrote: > nit: Please move this two static functions further down to match the ordering in the header. Done https://codereview.chromium.org/2703093002/diff/180001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc:145: ComputeActivationSuppressionReasonForMainFrameURL(const GURL& url) const { On 2017/02/21 at 14:13:13, engedy (slow) wrote: > nit: Maybe ComputeActivationDecision... or DecideActivation...? Done https://codereview.chromium.org/2703093002/diff/180001/components/subresource... File components/subresource_filter/content/browser/content_subresource_filter_driver_factory.h (right): https://codereview.chromium.org/2703093002/diff/180001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory.h:112: GetActivationSuppressionReasonForMainFrameDocument() const { On 2017/02/21 at 14:13:13, engedy (slow) wrote: > nit: GetActivationDecisionForLastCommittedPageLoad? Done
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
bmcquade@chromium.org changed reviewers: + isherman@chromium.org
isherman, PTAL for histograms.xml, thanks!
Metrics LGTM % comments: https://codereview.chromium.org/2703093002/diff/220001/components/subresource... File components/subresource_filter/content/browser/content_subresource_filter_driver_factory.h (right): https://codereview.chromium.org/2703093002/diff/220001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory.h:49: enum ActivationDecision { Please document that this enum is used to back an UMA histogram, and should therefore be treated as append-only. https://codereview.chromium.org/2703093002/diff/220001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2703093002/diff/220001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:108610: + <int value="5" label="Activated"/> Optional: You might want to arrange the enum so that this is item 1 (following "Unknown"). That way, if you ever add reasons for why activation failed, this success state doesn't end up buried in the middle of the list.
The CQ bit was checked by bmcquade@chromium.org to run a CQ dry run
Thanks! Addressed comments. PTAL. https://codereview.chromium.org/2703093002/diff/220001/components/subresource... File components/subresource_filter/content/browser/content_subresource_filter_driver_factory.h (right): https://codereview.chromium.org/2703093002/diff/220001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory.h:49: enum ActivationDecision { On 2017/02/21 at 22:58:24, Ilya Sherman wrote: > Please document that this enum is used to back an UMA histogram, and should therefore be treated as append-only. Ah, good idea, thanks! Done. https://codereview.chromium.org/2703093002/diff/220001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2703093002/diff/220001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:108610: + <int value="5" label="Activated"/> On 2017/02/21 at 22:58:24, Ilya Sherman wrote: > Optional: You might want to arrange the enum so that this is item 1 (following "Unknown"). That way, if you ever add reasons for why activation failed, this success state doesn't end up buried in the middle of the list. Yes, great idea. Done, thanks!
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
(Still LGTM)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
LGTM % comments below, thanks a lot! Sorry for being really nit-picky, it is just that histograms are hard to change, so I usually want to make sure we get them right the first time. https://codereview.chromium.org/2703093002/diff/240001/components/subresource... File components/subresource_filter/content/browser/content_subresource_filter_driver_factory.h (right): https://codereview.chromium.org/2703093002/diff/240001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory.h:50: enum ActivationDecision { nit: Please make this an enum class. You could also make the unit tests a bit less verbose, by declaring a type alias for ContentSubresourceFilterDriverFactory::ActivationDecision, so you can refer to ActivationDecision::FOO instead of ContentSubresourceFilterDriverFactory::FOO which is currently the case. https://codereview.chromium.org/2703093002/diff/240001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory.h:60: // Did not activate because the main resource URL had an unsupported scheme. nit: s/main resource/main frame document/g (The term `main resource` is ambiguous as it can refer to subframe documents 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...
Thanks! Addressed comments. https://codereview.chromium.org/2703093002/diff/240001/components/subresource... File components/subresource_filter/content/browser/content_subresource_filter_driver_factory.h (right): https://codereview.chromium.org/2703093002/diff/240001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory.h:50: enum ActivationDecision { On 2017/02/22 at 12:26:51, engedy (slow) wrote: > nit: Please make this an enum class. > > You could also make the unit tests a bit less verbose, by declaring a type alias for ContentSubresourceFilterDriverFactory::ActivationDecision, so you can refer to ActivationDecision::FOO instead of ContentSubresourceFilterDriverFactory::FOO which is currently the case. Done https://codereview.chromium.org/2703093002/diff/240001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory.h:60: // Did not activate because the main resource URL had an unsupported scheme. On 2017/02/22 at 12:26:51, engedy (slow) wrote: > nit: s/main resource/main frame document/g > > (The term `main resource` is ambiguous as it can refer to subframe documents too.) Done
Still LGTM, thanks!
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 isherman@chromium.org Link to the patchset: https://codereview.chromium.org/2703093002/#ps260001 (title: "address comments")
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": 260001, "attempt_start_ts": 1487771593264510,
"parent_rev": "d844eb57169233599e3908198e4784236d2bce03", "commit_rev":
"ec69818af043485daccd4d4f43718390e9ddcb08"}
Message was sent while issue was closed.
Description was changed from ========== Add metrics for tracking subresource filter activation suppression. This change adds an API on ContentSubresourceFilterDriverFactory that the SubresourceFilterMetricsObserver queries to determine whether and why activation was suppressed for the currently committed page load. The SubresourceFilterMetricsObserver then logs histograms that allow us to understand how often activation is suppressed. BUG=694036 ========== to ========== Add metrics for tracking subresource filter activation suppression. This change adds an API on ContentSubresourceFilterDriverFactory that the SubresourceFilterMetricsObserver queries to determine whether and why activation was suppressed for the currently committed page load. The SubresourceFilterMetricsObserver then logs histograms that allow us to understand how often activation is suppressed. BUG=694036 Review-Url: https://codereview.chromium.org/2703093002 Cr-Commit-Position: refs/heads/master@{#452027} Committed: https://chromium.googlesource.com/chromium/src/+/ec69818af043485daccd4d4f4371... ==========
Message was sent while issue was closed.
Committed patchset #14 (id:260001) as https://chromium.googlesource.com/chromium/src/+/ec69818af043485daccd4d4f4371... |
