Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2651163002:
Add UMA for autoblocking and embargoing. (Closed)
|
Created:
3 years, 11 months ago by meredithl Modified:
3 years, 10 months ago CC:
chromium-reviews, mlamouri+watch-permissions_chromium.org, asvitkine+watch_chromium.org, raymes+watch_chromium.org, chrome-apps-syd-reviews_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd UMA for autoblocking and embargoing.
Metrics are recorded in PermissionUmaUtil. Currently, the client records
what the response time was from Safe Browsing, as well as what the
response was. The autoblocker records embargo status for both
Permissions Blacklisting and repeated dismissals. Recording that a permission
was not embargoed happens in either PermissionsContextBase once the permission
has been responded to, or in PermissionRequestImpl when it is destroyed after
not being responded to. These are mutually exclusive events so they should not
interfere with one another.
BUG=679877
Patch Set 1 #
Total comments: 26
Patch Set 2 : Nits, review #Patch Set 3 : Rebase #Patch Set 4 : Sync branch, add test #
Total comments: 4
Patch Set 5 : Review comments, add histogram checks #
Total comments: 16
Patch Set 6 : Review #
Total comments: 2
Patch Set 7 : Review #
Total comments: 4
Dependent Patchsets: Messages
Total messages: 43 (26 generated)
The CQ bit was checked by meredithl@google.com 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...
meredithl@google.com changed reviewers: + dominickn@chromium.org, kcarattini@chromium.org, raymes@chromium.org
Hey guys. PTAL!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2651163002/diff/1/chrome/browser/permissions/... File chrome/browser/permissions/permission_blacklist_client.cc (right): https://codereview.chromium.org/2651163002/diff/1/chrome/browser/permissions/... chrome/browser/permissions/permission_blacklist_client.cc:92: // record elapsed time in PermissionUmaUtil This comment is probably not needed. https://codereview.chromium.org/2651163002/diff/1/chrome/browser/permissions/... File chrome/browser/permissions/permission_decision_auto_blocker.cc (right): https://codereview.chromium.org/2651163002/diff/1/chrome/browser/permissions/... chrome/browser/permissions/permission_decision_auto_blocker.cc:116: bool PreviouslyEmbargoed(Profile* profile, Call this WasPreviouslyEmbargoed. Add a comment saying, "Returns true if |url| has been embargoed from requesting |permission| for the reason corresponding to |key|." https://codereview.chromium.org/2651163002/diff/1/chrome/browser/permissions/... chrome/browser/permissions/permission_decision_auto_blocker.cc:253: Nit: probably don't need this extra newline https://codereview.chromium.org/2651163002/diff/1/chrome/browser/permissions/... chrome/browser/permissions/permission_decision_auto_blocker.cc:376: PermissionUmaUtil::RecordPermissionEmbargoReason( Will this double count since you're calling RecordPermissionEmbargoReason in PermissionDecided()? https://codereview.chromium.org/2651163002/diff/1/chrome/browser/permissions/... File chrome/browser/permissions/permission_decision_auto_blocker.h (right): https://codereview.chromium.org/2651163002/diff/1/chrome/browser/permissions/... chrome/browser/permissions/permission_decision_auto_blocker.h:13: #include "chrome/browser/permissions/permission_uma_util.h" Remove this include (move it to the cc file). https://codereview.chromium.org/2651163002/diff/1/chrome/browser/permissions/... File chrome/browser/permissions/permission_decision_auto_blocker_unittest.cc (right): https://codereview.chromium.org/2651163002/diff/1/chrome/browser/permissions/... chrome/browser/permissions/permission_decision_auto_blocker_unittest.cc:413: histograms.ExpectBucketCount("Permissions.AutoBlocker.SafeBrowsingResponse", I'm not sure if this bucket check will be racy or not. Might be safest just to check the total count for the response time. https://codereview.chromium.org/2651163002/diff/1/chrome/browser/permissions/... chrome/browser/permissions/permission_decision_auto_blocker_unittest.cc:424: histograms.ExpectBucketCount("Permissions.AutoBlocker.SafeBrowsingResponse", Ditto. https://codereview.chromium.org/2651163002/diff/1/chrome/browser/permissions/... File chrome/browser/permissions/permission_uma_util.cc (right): https://codereview.chromium.org/2651163002/diff/1/chrome/browser/permissions/... chrome/browser/permissions/permission_uma_util.cc:62: #define PERMISSION_UMA Remove this :) https://codereview.chromium.org/2651163002/diff/1/chrome/browser/permissions/... File chrome/browser/permissions/permission_uma_util.h (right): https://codereview.chromium.org/2651163002/diff/1/chrome/browser/permissions/... chrome/browser/permissions/permission_uma_util.h:52: PERMISSIONS_BLACKLISTING = 1, This can probably just be called BLACKLISTED https://codereview.chromium.org/2651163002/diff/1/chrome/browser/permissions/... chrome/browser/permissions/permission_uma_util.h:56: EMBARGO_NUM, REASON_NUM
The CQ bit was checked by meredithl@google.com 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! https://codereview.chromium.org/2651163002/diff/1/chrome/browser/permissions/... File chrome/browser/permissions/permission_blacklist_client.cc (right): https://codereview.chromium.org/2651163002/diff/1/chrome/browser/permissions/... chrome/browser/permissions/permission_blacklist_client.cc:92: // record elapsed time in PermissionUmaUtil On 2017/01/27 06:05:46, dominickn wrote: > This comment is probably not needed. Done. https://codereview.chromium.org/2651163002/diff/1/chrome/browser/permissions/... File chrome/browser/permissions/permission_decision_auto_blocker.cc (right): https://codereview.chromium.org/2651163002/diff/1/chrome/browser/permissions/... chrome/browser/permissions/permission_decision_auto_blocker.cc:116: bool PreviouslyEmbargoed(Profile* profile, On 2017/01/27 06:05:47, dominickn wrote: > Call this WasPreviouslyEmbargoed. Add a comment saying, > > "Returns true if |url| has been embargoed from requesting |permission| for the > reason corresponding to |key|." Done. https://codereview.chromium.org/2651163002/diff/1/chrome/browser/permissions/... chrome/browser/permissions/permission_decision_auto_blocker.cc:253: On 2017/01/27 06:05:47, dominickn wrote: > Nit: probably don't need this extra newline Done. https://codereview.chromium.org/2651163002/diff/1/chrome/browser/permissions/... chrome/browser/permissions/permission_decision_auto_blocker.cc:376: PermissionUmaUtil::RecordPermissionEmbargoReason( On 2017/01/27 06:05:47, dominickn wrote: > Will this double count since you're calling RecordPermissionEmbargoReason in > PermissionDecided()? Hmm, I believe so. Imo that should have been picked up in the tests, is there a way I can be sure that this doesn't happen? It appears to behave as expected. https://codereview.chromium.org/2651163002/diff/1/chrome/browser/permissions/... File chrome/browser/permissions/permission_decision_auto_blocker.h (right): https://codereview.chromium.org/2651163002/diff/1/chrome/browser/permissions/... chrome/browser/permissions/permission_decision_auto_blocker.h:13: #include "chrome/browser/permissions/permission_uma_util.h" On 2017/01/27 06:05:47, dominickn wrote: > Remove this include (move it to the cc file). Done. https://codereview.chromium.org/2651163002/diff/1/chrome/browser/permissions/... File chrome/browser/permissions/permission_decision_auto_blocker_unittest.cc (right): https://codereview.chromium.org/2651163002/diff/1/chrome/browser/permissions/... chrome/browser/permissions/permission_decision_auto_blocker_unittest.cc:413: histograms.ExpectBucketCount("Permissions.AutoBlocker.SafeBrowsingResponse", On 2017/01/27 06:05:47, dominickn wrote: > I'm not sure if this bucket check will be racy or not. Might be safest just to > check the total count for the response time. Done. https://codereview.chromium.org/2651163002/diff/1/chrome/browser/permissions/... chrome/browser/permissions/permission_decision_auto_blocker_unittest.cc:424: histograms.ExpectBucketCount("Permissions.AutoBlocker.SafeBrowsingResponse", On 2017/01/27 06:05:47, dominickn wrote: > Ditto. Done. https://codereview.chromium.org/2651163002/diff/1/chrome/browser/permissions/... File chrome/browser/permissions/permission_uma_util.cc (right): https://codereview.chromium.org/2651163002/diff/1/chrome/browser/permissions/... chrome/browser/permissions/permission_uma_util.cc:62: #define PERMISSION_UMA On 2017/01/27 06:05:47, dominickn wrote: > Remove this :) Done. https://codereview.chromium.org/2651163002/diff/1/chrome/browser/permissions/... File chrome/browser/permissions/permission_uma_util.h (right): https://codereview.chromium.org/2651163002/diff/1/chrome/browser/permissions/... chrome/browser/permissions/permission_uma_util.h:52: PERMISSIONS_BLACKLISTING = 1, On 2017/01/27 06:05:47, dominickn wrote: > This can probably just be called BLACKLISTED I get a name collision with SafeBrowsingResponse, so it seemed easiest to just change the name of the embargo reason. Is there a better solution? I know I can use an enum class but then how do I record that in the metric? https://codereview.chromium.org/2651163002/diff/1/chrome/browser/permissions/... chrome/browser/permissions/permission_uma_util.h:56: EMBARGO_NUM, On 2017/01/27 06:05:47, dominickn wrote: > REASON_NUM Done.
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_...)
https://codereview.chromium.org/2651163002/diff/1/chrome/browser/permissions/... File chrome/browser/permissions/permission_decision_auto_blocker.cc (right): https://codereview.chromium.org/2651163002/diff/1/chrome/browser/permissions/... chrome/browser/permissions/permission_decision_auto_blocker.cc:376: PermissionUmaUtil::RecordPermissionEmbargoReason( On 2017/01/29 23:48:29, meredithl wrote: > On 2017/01/27 06:05:47, dominickn wrote: > > Will this double count since you're calling RecordPermissionEmbargoReason in > > PermissionDecided()? > > Hmm, I believe so. Imo that should have been picked up in the tests, is there a > way I can be sure that this doesn't happen? It appears to behave as expected. You don't actually have any test which checks the NOT_EMBARGOED enum. You need a test where you check the histograms for a permission that's not blacklisted, and verify that only one NOT_EMBARGOED is recorded. https://codereview.chromium.org/2651163002/diff/1/chrome/browser/permissions/... File chrome/browser/permissions/permission_uma_util.h (right): https://codereview.chromium.org/2651163002/diff/1/chrome/browser/permissions/... chrome/browser/permissions/permission_uma_util.h:52: PERMISSIONS_BLACKLISTING = 1, On 2017/01/29 23:48:30, meredithl wrote: > On 2017/01/27 06:05:47, dominickn wrote: > > This can probably just be called BLACKLISTED > > I get a name collision with SafeBrowsingResponse, so it seemed easiest to just > change the name of the embargo reason. Is there a better solution? I know I can > use an enum class but then how do I record that in the metric? Ah, I see. Leave it as is then. :)
The CQ bit was checked by meredithl@google.com 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/2651163002/diff/1/chrome/browser/permissions/... File chrome/browser/permissions/permission_decision_auto_blocker.cc (right): https://codereview.chromium.org/2651163002/diff/1/chrome/browser/permissions/... chrome/browser/permissions/permission_decision_auto_blocker.cc:376: PermissionUmaUtil::RecordPermissionEmbargoReason( On 2017/01/30 00:06:20, dominickn wrote: > On 2017/01/29 23:48:29, meredithl wrote: > > On 2017/01/27 06:05:47, dominickn wrote: > > > Will this double count since you're calling RecordPermissionEmbargoReason in > > > PermissionDecided()? > > > > Hmm, I believe so. Imo that should have been picked up in the tests, is there > a > > way I can be sure that this doesn't happen? It appears to behave as expected. > > You don't actually have any test which checks the NOT_EMBARGOED enum. > > You need a test where you check the histograms for a permission that's not > blacklisted, and verify that only one NOT_EMBARGOED is recorded. I do a check of the histograms in Permission Context Base unit tests for the NOT_EMBARGOED enum. I can't test it in the autoblocker, all I can do is make sure that it hasn't recorded PERMISSIONS_BLACKLISTING or REPEATED_DISMISSAL. https://codereview.chromium.org/2651163002/diff/1/chrome/browser/permissions/... File chrome/browser/permissions/permission_uma_util.h (right): https://codereview.chromium.org/2651163002/diff/1/chrome/browser/permissions/... chrome/browser/permissions/permission_uma_util.h:52: PERMISSIONS_BLACKLISTING = 1, On 2017/01/30 00:06:20, dominickn wrote: > On 2017/01/29 23:48:30, meredithl wrote: > > On 2017/01/27 06:05:47, dominickn wrote: > > > This can probably just be called BLACKLISTED > > > > I get a name collision with SafeBrowsingResponse, so it seemed easiest to just > > change the name of the embargo reason. Is there a better solution? I know I > can > > use an enum class but then how do I record that in the metric? > > Ah, I see. Leave it as is then. :) Acknowledged.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2651163002/diff/1/chrome/browser/permissions/... File chrome/browser/permissions/permission_decision_auto_blocker.cc (right): https://codereview.chromium.org/2651163002/diff/1/chrome/browser/permissions/... chrome/browser/permissions/permission_decision_auto_blocker.cc:376: PermissionUmaUtil::RecordPermissionEmbargoReason( On 2017/01/30 00:34:33, meredithl wrote: > On 2017/01/30 00:06:20, dominickn wrote: > > On 2017/01/29 23:48:29, meredithl wrote: > > > On 2017/01/27 06:05:47, dominickn wrote: > > > > Will this double count since you're calling RecordPermissionEmbargoReason > in > > > > PermissionDecided()? > > > > > > Hmm, I believe so. Imo that should have been picked up in the tests, is > there > > a > > > way I can be sure that this doesn't happen? It appears to behave as > expected. > > > > You don't actually have any test which checks the NOT_EMBARGOED enum. > > > > You need a test where you check the histograms for a permission that's not > > blacklisted, and verify that only one NOT_EMBARGOED is recorded. > > I do a check of the histograms in Permission Context Base unit tests for the > NOT_EMBARGOED enum. I can't test it in the autoblocker, all I can do is make > sure that it hasn't recorded PERMISSIONS_BLACKLISTING or REPEATED_DISMISSAL. Right, but those tests don't have a safe browsing database manager and the blacklisting feature enabled. This codepath doesn't get exercised unless those two are present - you've only added the embargo histogram testing to the non-blacklisting tests. :)
The CQ bit was checked by meredithl@google.com 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/2651163002/diff/1/chrome/browser/permissions/... File chrome/browser/permissions/permission_decision_auto_blocker.cc (right): https://codereview.chromium.org/2651163002/diff/1/chrome/browser/permissions/... chrome/browser/permissions/permission_decision_auto_blocker.cc:376: PermissionUmaUtil::RecordPermissionEmbargoReason( On 2017/01/30 00:42:36, dominickn wrote: > On 2017/01/30 00:34:33, meredithl wrote: > > On 2017/01/30 00:06:20, dominickn wrote: > > > On 2017/01/29 23:48:29, meredithl wrote: > > > > On 2017/01/27 06:05:47, dominickn wrote: > > > > > Will this double count since you're calling > RecordPermissionEmbargoReason > > in > > > > > PermissionDecided()? > > > > > > > > Hmm, I believe so. Imo that should have been picked up in the tests, is > > there > > > a > > > > way I can be sure that this doesn't happen? It appears to behave as > > expected. > > > > > > You don't actually have any test which checks the NOT_EMBARGOED enum. > > > > > > You need a test where you check the histograms for a permission that's not > > > blacklisted, and verify that only one NOT_EMBARGOED is recorded. > > > > I do a check of the histograms in Permission Context Base unit tests for the > > NOT_EMBARGOED enum. I can't test it in the autoblocker, all I can do is make > > sure that it hasn't recorded PERMISSIONS_BLACKLISTING or REPEATED_DISMISSAL. > > Right, but those tests don't have a safe browsing database manager and the > blacklisting feature enabled. This codepath doesn't get exercised unless those > two are present - you've only added the embargo histogram testing to the > non-blacklisting tests. :) Okay, I've added an extra test in PDAB to check the SafeBrowsingResponse enum, and one in PermissionContextBase to check the EmbargoReason enum. I used ExpectUniqueSample, not sure if that's best.
https://codereview.chromium.org/2651163002/diff/60001/chrome/browser/permissi... File chrome/browser/permissions/permission_context_base.cc (right): https://codereview.chromium.org/2651163002/diff/60001/chrome/browser/permissi... chrome/browser/permissions/permission_context_base.cc:292: PermissionUmaUtil::RecordPermissionEmbargoReason( Nit: add a comment saying: // If a decision was made, the request cannot have been embargoed. https://codereview.chromium.org/2651163002/diff/60001/chrome/browser/permissi... chrome/browser/permissions/permission_context_base.cc:293: PermissionEmbargoReason::NOT_EMBARGOED); You need to move this into an else case on line 321 (otherwise when a prompt dismissal is embargoed, we'll double count a NOT_EMBARGOED and a REPEATED_DISMISSAL). You can add a test for that too (check the embargo histogram after repeated dismissal). That should just involve adding a histogram check in the existing repeated dismissal tests.
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 meredithl@google.com 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...
Still not sure on the use of ExpectedBucketCount vs. ExpectedTotalCount. https://codereview.chromium.org/2651163002/diff/60001/chrome/browser/permissi... File chrome/browser/permissions/permission_context_base.cc (right): https://codereview.chromium.org/2651163002/diff/60001/chrome/browser/permissi... chrome/browser/permissions/permission_context_base.cc:292: PermissionUmaUtil::RecordPermissionEmbargoReason( On 2017/01/30 02:41:36, dominickn wrote: > Nit: add a comment saying: > > // If a decision was made, the request cannot have been embargoed. Done. https://codereview.chromium.org/2651163002/diff/60001/chrome/browser/permissi... chrome/browser/permissions/permission_context_base.cc:293: PermissionEmbargoReason::NOT_EMBARGOED); On 2017/01/30 02:41:36, dominickn wrote: > You need to move this into an else case on line 321 (otherwise when a prompt > dismissal is embargoed, we'll double count a NOT_EMBARGOED and a > REPEATED_DISMISSAL). > > You can add a test for that too (check the embargo histogram after repeated > dismissal). That should just involve adding a histogram check in the existing > repeated dismissal tests. Done.
Thanks Meredith, nearly there! https://codereview.chromium.org/2651163002/diff/80001/chrome/browser/permissi... File chrome/browser/permissions/permission_blacklist_client.h (right): https://codereview.chromium.org/2651163002/diff/80001/chrome/browser/permissi... chrome/browser/permissions/permission_blacklist_client.h:7: #include <memory> for std::unique_ptr https://codereview.chromium.org/2651163002/diff/80001/chrome/browser/permissi... chrome/browser/permissions/permission_blacklist_client.h:10: #include "chrome/browser/permissions/permission_uma_util.h" Move this include to the cc file. https://codereview.chromium.org/2651163002/diff/80001/chrome/browser/permissi... File chrome/browser/permissions/permission_context_base_unittest.cc (right): https://codereview.chromium.org/2651163002/diff/80001/chrome/browser/permissi... chrome/browser/permissions/permission_context_base_unittest.cc:357: histograms.ExpectTotalCount("Permissions.AutoBlocker.EmbargoReason", Can you use ExpectUniqueSample("Permissions.AutoBlocker.EmbargoReason", PermissionEmbargoReason::NOT_EMBARGOED, i + 1) here? - ExpectUniqueSample(name, value, number) means "I expect there to be <number> count of <value> in <name> histogram ONLY (no other values present). That is, <value> was recorded <number> times exclusively - ExpectBucketCount(name, value, number) is the same, except other values might be present - ExpectTotalCount(name, number) means "I expect there to be <number> total recorded across all values. The first two will crash if the histogram hasn't had anything recorded in it yet. TotalCount can be used always. https://codereview.chromium.org/2651163002/diff/80001/chrome/browser/permissi... chrome/browser/permissions/permission_context_base_unittest.cc:365: histograms.ExpectBucketCount("Permissions.AutoBlocker.EmbargoReason", Remove this one, it's covered by the check in the loop. https://codereview.chromium.org/2651163002/diff/80001/chrome/browser/permissi... chrome/browser/permissions/permission_context_base_unittest.cc:390: 1); And an ExpectBucketCount for NOT_EMBARGOED (iterations - 1)? https://codereview.chromium.org/2651163002/diff/80001/chrome/browser/permissi... File chrome/browser/permissions/permission_decision_auto_blocker_unittest.cc (right): https://codereview.chromium.org/2651163002/diff/80001/chrome/browser/permissi... chrome/browser/permissions/permission_decision_auto_blocker_unittest.cc:260: histograms.ExpectTotalCount("Permissions.AutoBlocker.EmbargoReason", 1); You can collapse the TotalCount and BucketCount into: histograms.ExpectUniqueSample("Permissions.AutoBlocker.EmbargoReason", PermissionEmbargoReason::PERMISSIONS_BLACKLISTING, 1); histograms.ExpectUniqueSample("Permissions.AutoBlocker.SafeBrowsingResponse", SafeBrowsingResponse::BLACKLISTED, 1); https://codereview.chromium.org/2651163002/diff/80001/chrome/browser/permissi... File chrome/browser/permissions/permission_uma_util.h (right): https://codereview.chromium.org/2651163002/diff/80001/chrome/browser/permissi... chrome/browser/permissions/permission_uma_util.h:25: class TimeDelta; This doesn't work - you're passing base::TimeDelta by value so you need its full definition (it must be transitively included from another header file). Remove this chunk and just #include "base/time/time.h".
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 meredithl@google.com 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/2651163002/diff/80001/chrome/browser/permissi... File chrome/browser/permissions/permission_blacklist_client.h (right): https://codereview.chromium.org/2651163002/diff/80001/chrome/browser/permissi... chrome/browser/permissions/permission_blacklist_client.h:7: On 2017/01/30 04:55:25, dominickn wrote: > #include <memory> for std::unique_ptr Done. https://codereview.chromium.org/2651163002/diff/80001/chrome/browser/permissi... chrome/browser/permissions/permission_blacklist_client.h:10: #include "chrome/browser/permissions/permission_uma_util.h" On 2017/01/30 04:55:25, dominickn wrote: > Move this include to the cc file. Done. https://codereview.chromium.org/2651163002/diff/80001/chrome/browser/permissi... File chrome/browser/permissions/permission_context_base_unittest.cc (right): https://codereview.chromium.org/2651163002/diff/80001/chrome/browser/permissi... chrome/browser/permissions/permission_context_base_unittest.cc:357: histograms.ExpectTotalCount("Permissions.AutoBlocker.EmbargoReason", On 2017/01/30 04:55:25, dominickn wrote: > Can you use ExpectUniqueSample("Permissions.AutoBlocker.EmbargoReason", > PermissionEmbargoReason::NOT_EMBARGOED, i + 1) here? > > - ExpectUniqueSample(name, value, number) means "I expect there to be <number> > count of <value> in <name> histogram ONLY (no other values present). That is, > <value> was recorded <number> times exclusively > - ExpectBucketCount(name, value, number) is the same, except other values might > be present > - ExpectTotalCount(name, number) means "I expect there to be <number> total > recorded across all values. > > The first two will crash if the histogram hasn't had anything recorded in it > yet. TotalCount can be used always. That will succeed for the first N-1 requests, but for the Nth request, it has been recorded as PermissionEmbargoReason::REPEATED_DISMISSALS by the autoblocker, which is why I just verify that N things have been recorded, and then verify after the loop that the right number of NOT_EMBARGO is recorded. It was easier than writing it into an if statement inside the loop for just the last request. https://codereview.chromium.org/2651163002/diff/80001/chrome/browser/permissi... chrome/browser/permissions/permission_context_base_unittest.cc:365: histograms.ExpectBucketCount("Permissions.AutoBlocker.EmbargoReason", On 2017/01/30 04:55:25, dominickn wrote: > Remove this one, it's covered by the check in the loop. See above, i either need to move the inner loop check to an if statement to check which request we are on, or just check it after the loop has exited. https://codereview.chromium.org/2651163002/diff/80001/chrome/browser/permissi... chrome/browser/permissions/permission_context_base_unittest.cc:390: 1); On 2017/01/30 04:55:25, dominickn wrote: > And an ExpectBucketCount for NOT_EMBARGOED (iterations - 1)? I've moved the one from above down to here. https://codereview.chromium.org/2651163002/diff/80001/chrome/browser/permissi... File chrome/browser/permissions/permission_decision_auto_blocker_unittest.cc (right): https://codereview.chromium.org/2651163002/diff/80001/chrome/browser/permissi... chrome/browser/permissions/permission_decision_auto_blocker_unittest.cc:260: histograms.ExpectTotalCount("Permissions.AutoBlocker.EmbargoReason", 1); On 2017/01/30 04:55:26, dominickn wrote: > You can collapse the TotalCount and BucketCount into: > > histograms.ExpectUniqueSample("Permissions.AutoBlocker.EmbargoReason", > PermissionEmbargoReason::PERMISSIONS_BLACKLISTING, 1); > histograms.ExpectUniqueSample("Permissions.AutoBlocker.SafeBrowsingResponse", > SafeBrowsingResponse::BLACKLISTED, 1); Done. https://codereview.chromium.org/2651163002/diff/80001/chrome/browser/permissi... File chrome/browser/permissions/permission_uma_util.h (right): https://codereview.chromium.org/2651163002/diff/80001/chrome/browser/permissi... chrome/browser/permissions/permission_uma_util.h:25: class TimeDelta; On 2017/01/30 04:55:26, dominickn wrote: > This doesn't work - you're passing base::TimeDelta by value so you need its full > definition (it must be transitively included from another header file). Remove > this chunk and just #include "base/time/time.h". Done.
lgtm. Thanks! https://codereview.chromium.org/2651163002/diff/80001/chrome/browser/permissi... File chrome/browser/permissions/permission_context_base_unittest.cc (right): https://codereview.chromium.org/2651163002/diff/80001/chrome/browser/permissi... chrome/browser/permissions/permission_context_base_unittest.cc:357: histograms.ExpectTotalCount("Permissions.AutoBlocker.EmbargoReason", On 2017/01/30 05:37:40, meredithl wrote: > On 2017/01/30 04:55:25, dominickn wrote: > > Can you use ExpectUniqueSample("Permissions.AutoBlocker.EmbargoReason", > > PermissionEmbargoReason::NOT_EMBARGOED, i + 1) here? > > > > - ExpectUniqueSample(name, value, number) means "I expect there to be > <number> > > count of <value> in <name> histogram ONLY (no other values present). That is, > > <value> was recorded <number> times exclusively > > - ExpectBucketCount(name, value, number) is the same, except other values > might > > be present > > - ExpectTotalCount(name, number) means "I expect there to be <number> total > > recorded across all values. > > > > The first two will crash if the histogram hasn't had anything recorded in it > > yet. TotalCount can be used always. > > That will succeed for the first N-1 requests, but for the Nth request, it has > been recorded as PermissionEmbargoReason::REPEATED_DISMISSALS by the > autoblocker, which is why I just verify that N things have been recorded, and > then verify after the loop that the right number of NOT_EMBARGO is recorded. It > was easier than writing it into an if statement inside the loop for just the > last request. Ah, acknowledged. I forgot that we changed this to a straight loop and then back to this change-on-the-last design again. https://codereview.chromium.org/2651163002/diff/80001/chrome/browser/permissi... chrome/browser/permissions/permission_context_base_unittest.cc:365: histograms.ExpectBucketCount("Permissions.AutoBlocker.EmbargoReason", On 2017/01/30 05:37:40, meredithl wrote: > On 2017/01/30 04:55:25, dominickn wrote: > > Remove this one, it's covered by the check in the loop. > > See above, i either need to move the inner loop check to an if statement to > check which request we are on, or just check it after the loop has exited. Acknowledged.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Sorry for the delay on this! Hmm, I'm not quite sure what we're trying to measure here. Are we trying to measure the number of permission requests that fail as a result of being embargoed vs the total number of requests? Or are we trying to measure the number of requests that result in an embargo happening vs the total number of requests? Or something else? I'm thinking that maybe rather than measure the "not embargoed" case, it might be simpler to measure the total number of requests and then we can compare the numbers. But I'm not sure. Also - with the permission_decision_auto_blocker_unittest.cc tests, it would be nice to break the new checks into 1 or more separate tests that are specifically for metrics. This avoids making the existing tests less clear (and increasingly so as we add new metrics). Ideally we would have the same for permission_context_base_unittests.cc but that's already got histogram checks scattered throughout. Thanks! https://codereview.chromium.org/2651163002/diff/100001/chrome/browser/permiss... File chrome/browser/permissions/permission_decision_auto_blocker.cc (right): https://codereview.chromium.org/2651163002/diff/100001/chrome/browser/permiss... chrome/browser/permissions/permission_decision_auto_blocker.cc:253: PermissionEmbargoReason::REPEATED_DISMISSALS); Can these added lines be moved into PlaceUnderEmbargo (same below)?
On 2017/02/01 19:15:44, raymes wrote: > Sorry for the delay on this! > > Hmm, I'm not quite sure what we're trying to measure here. Are we trying to > measure the number of permission requests that fail as a result of being > embargoed vs the total number of requests? > > Or are we trying to measure the number of requests that result in an embargo > happening vs the total number of requests? Or something else? I'm thinking that > maybe rather than measure the "not embargoed" case, it might be simpler to > measure the total number of requests and then we can compare the numbers. But > I'm not sure. > > Also - with the permission_decision_auto_blocker_unittest.cc tests, it would be > nice to break the new checks into 1 or more separate tests that are specifically > for metrics. This avoids making the existing tests less clear (and increasingly > so as we add new metrics). Ideally we would have the same for > permission_context_base_unittests.cc but that's already got histogram checks > scattered throughout. > > Thanks! > > https://codereview.chromium.org/2651163002/diff/100001/chrome/browser/permiss... > File chrome/browser/permissions/permission_decision_auto_blocker.cc (right): > > https://codereview.chromium.org/2651163002/diff/100001/chrome/browser/permiss... > chrome/browser/permissions/permission_decision_auto_blocker.cc:253: > PermissionEmbargoReason::REPEATED_DISMISSALS); > Can these added lines be moved into PlaceUnderEmbargo (same below)? Haha, no apology necessary! You are away after all. I think the idea was to have each permission request fall into exactly one bucket to make it easier/clearer to compare later? The way it currently is would allow us to compare embargoing vs. not embargoing and the reasons for each, although that could definitely be achieved by just recording the number of total requests vs the number of records for embargoing. I'm keen for discussion on this however, as I'm brand new to metrics and I'm not the one who has to look at the data later :) Yeah, I just kept the tests as was for permission_context_base_unittests.cc to keep everything in line, but I'll separate out the histogram tests in permission_decision_auto_blocker_unittests.cc if that's better. Cheers!
https://codereview.chromium.org/2651163002/diff/100001/chrome/browser/permiss... File chrome/browser/permissions/permission_decision_auto_blocker.cc (right): https://codereview.chromium.org/2651163002/diff/100001/chrome/browser/permiss... chrome/browser/permissions/permission_decision_auto_blocker.cc:253: PermissionEmbargoReason::REPEATED_DISMISSALS); On 2017/02/01 19:15:44, raymes wrote: > Can these added lines be moved into PlaceUnderEmbargo (same below)? Done. I used strcmp, is that allowable?
Hi Meredith, I talked to Dom and realized that we might actually want to record the number of requests that are blocked due to embargo vs the number let through (as opposed to just when an embargo occurs). I think that might change the implementation a bit and I've added some notes below, but it might be easier to chat about this in the office next week. You may want to pull out the changes in chrome/browser/permissions/permission_blacklist_client.h/cc into a separate CL since they seem ok to me and it would allow us to focus on the trickier metric here in this CL. Thanks! Raymes https://codereview.chromium.org/2651163002/diff/120001/chrome/browser/permiss... File chrome/browser/permissions/permission_context_base.cc (right): https://codereview.chromium.org/2651163002/diff/120001/chrome/browser/permiss... chrome/browser/permissions/permission_context_base.cc:115: GetPermissionStatus(requesting_origin, embedding_origin); I think we want to record that it was embargoed here, if the reason it was blocked is due to embargo. https://codereview.chromium.org/2651163002/diff/120001/chrome/browser/permiss... chrome/browser/permissions/permission_context_base.cc:146: bool permission_blocked) { If we pass in the reason for the permission being blocked here, we can record the metric at this layer. Basically we would just add the metric recording into the if (permission_blocked) case below https://codereview.chromium.org/2651163002/diff/120001/chrome/browser/permiss... chrome/browser/permissions/permission_context_base.cc:169: const GURL& embedding_origin) const { I think we might want to have a version of this function which returns a denial reason. https://codereview.chromium.org/2651163002/diff/120001/chrome/browser/permiss... chrome/browser/permissions/permission_context_base.cc:318: content_setting = CONTENT_SETTING_BLOCK; This is slightly orthogonal to this CL, but I think we can just remove this line and rework this into: if (content_setting == CONTENT_SETTING_DEFAULT) PermissionUmaUtil::RecordPermissionEmbargoReason
Description was changed from ========== Add UMA for autoblocking and embargoing. Metrics are recorded in PermissionUmaUtil. Currently, the client records what the response time was from Safe Browsing, as well as what the response was. The autoblocker records embargo status for both Permissions Blacklisting and repeated dismissals. Recording that a permission was not embargoed happens in either PermissionsContextBase once the permission has been responded to, or in PermissionRequestImpl when it is destroyed after not being responded to. These are mutually exclusive events so they should not interfere with one another. BUG=679877 ========== to ========== Add UMA for autoblocking and embargoing. Metrics are recorded in PermissionUmaUtil. Currently, the client records what the response time was from Safe Browsing, as well as what the response was. The autoblocker records embargo status for both Permissions Blacklisting and repeated dismissals. Recording that a permission was not embargoed happens in either PermissionsContextBase once the permission has been responded to, or in PermissionRequestImpl when it is destroyed after not being responded to. These are mutually exclusive events so they should not interfere with one another. BUG=679877 ==========
Message was sent while issue was closed.
On 2017/02/03 19:56:20, raymes wrote: > Hi Meredith, > > I talked to Dom and realized that we might actually want to record the number of > requests that are blocked due to embargo vs the number let through (as opposed > to just when an embargo occurs). > > I think that might change the implementation a bit and I've added some notes > below, but it might be easier to chat about this in the office next week. > > You may want to pull out the changes in > chrome/browser/permissions/permission_blacklist_client.h/cc into a separate CL > since they seem ok to me and it would allow us to focus on the trickier metric > here in this CL. > > Thanks! > Raymes > > https://codereview.chromium.org/2651163002/diff/120001/chrome/browser/permiss... > File chrome/browser/permissions/permission_context_base.cc (right): > > https://codereview.chromium.org/2651163002/diff/120001/chrome/browser/permiss... > chrome/browser/permissions/permission_context_base.cc:115: > GetPermissionStatus(requesting_origin, embedding_origin); > I think we want to record that it was embargoed here, if the reason it was > blocked is due to embargo. > > https://codereview.chromium.org/2651163002/diff/120001/chrome/browser/permiss... > chrome/browser/permissions/permission_context_base.cc:146: bool > permission_blocked) { > If we pass in the reason for the permission being blocked here, we can record > the metric at this layer. Basically we would just add the metric recording into > the if (permission_blocked) case below > > https://codereview.chromium.org/2651163002/diff/120001/chrome/browser/permiss... > chrome/browser/permissions/permission_context_base.cc:169: const GURL& > embedding_origin) const { > I think we might want to have a version of this function which returns a denial > reason. > > https://codereview.chromium.org/2651163002/diff/120001/chrome/browser/permiss... > chrome/browser/permissions/permission_context_base.cc:318: content_setting = > CONTENT_SETTING_BLOCK; > This is slightly orthogonal to this CL, but I think we can just remove this line > and rework this into: > if (content_setting == CONTENT_SETTING_DEFAULT) > PermissionUmaUtil::RecordPermissionEmbargoReason After the meeting with Dominick and Raymes, it has been decided that we were conflating two different metrics. I'm closing this issue for now, and will split out the two metrics we want into different CLs. The current plan is to have one metric that records the total number of requests that could trigger a prompt. This should be comprised of the buckets: prompts not shown due to blacklist embargo, prompts not shown due to dismissal embargo, prompts not shown due to a new blacklist embargo (placed under embargo for the current request) and everything that shows a prompt. The other metric records any embargo interactions through PermissionDecisionAutoBlocker. These include when a permission is placed under embargo for blacklisting, placed under embargo for dismissals (once the prompt has been dismissed), or if the permission is granted/denied/ignored it is recorded as not embargoed. These will be recorded from PermissionContextBase, as its the only place we can record both permissions blacklisting and dismissals. I'll be sending out those CLs (hopefully) very soon. Email me with questions, this will all be added to the design doc soon. |
