Closed Bug 1286118 Opened 5 years ago Closed 5 years ago

Add telemetry probe to measure changes to permissions after initial prompt

Categories

(Firefox :: Site Permissions, defect, P2)

defect

Tracking

()

RESOLVED FIXED
Firefox 51
Iteration:
51.3 - Sep 19
Tracking Status
firefox51 --- fixed

People

(Reporter: pdol, Assigned: johannh)

References

Details

(Whiteboard: [fxprivacy])

User Story

As a product owner, I want to be able to determine how often users clear permissions after the initial prompt so that I can determine how frequent this use case is and if there is an issue with certain permission prompts (eg. if users constantly change one permission prompt, we know there may be need to make modifications).

Acceptance Criteria:
- Aggregate numbers of permission clears should be provided for each permission prompt (eg. geolocation, push, etc.)
- Relative numbers of permission clears should be provided for each permission prompt (eg. how many times was geolocation cleared relative to how many times geolocation prompt was shown in the same time period)

Attachments

(1 file)

No description provided.
Whiteboard: [fxprivacy][triage]
User Story: (updated)
Priority: -- → P2
We'll need to wait for Bug 1203292 to finish the buttons that we'd like to track.
Depends on: 1203292
Whiteboard: [fxprivacy][triage] → [fxprivacy]
Ash, in the new UI, is there only one spot to "clear" permission prompts?
User Story: (updated)
Flags: needinfo?(agrigas)
(In reply to Peter Dolanjski [:pdol] from comment #2)
> Ash, in the new UI, is there only one spot to "clear" permission prompts?

yes in the control panel for all the newly migrated permissions. For the legacy ones like plug-ins it will be its own doorhanger...
Flags: needinfo?(agrigas)
Assignee: nobody → jhofmann
Status: NEW → ASSIGNED
Flags: qe-verify?
Iteration: --- → 50.4 - Aug 1
Flags: qe-verify? → qe-verify-
This adds a telemetry probe for measuring how many times a user clears a permission in the control center.

We have a Histogram which is keyed by permission id (geo, camera, microphone, etc.) and has the following values:
0: total number of all clears
1: total number of clearing permanently allowed permissions
2: total number of clearing permanently blocked permissions
3: total number of clearing temporarily allowed permissions
4: total number of clearing temporarily blocked permissions

Since we don't have the concept of temporarily allowed/blocked permissions yet, we can either choose to leave this patch open until then or merge it now and add them later.

Peter: does the described histogram look useful enough for you? If I recall correctly, we said we wanted to go only for aggregate numbers for now, since we also have aggregate numbers for doorhanger interaction. Let me know if you imagined something else. :)
Flags: needinfo?(pdolanjski)
Attachment #8776559 - Flags: review?(florian) → review-
Comment on attachment 8776559 [details]
Bug 1286118 - Add telemetry probe to measure changes to permissions after initial prompt.

https://reviewboard.mozilla.org/r/68296/#review65648

::: browser/base/content/browser.js:7323
(Diff revision 1)
> +
> +      // Set telemetry values for clearing a permission
> +      let histogram = Services.telemetry.getKeyedHistogramById("WEB_PERMISSION_CLEARED");
> +
> +      // 0 : clear any permission (always set)
> +      histogram.add(aPermission.id, 0);

I don't see the value of recording this, isn't this something we get by just adding up the other values recorded in that histogram?

I think you wanted something like:
 histogram.add("(all)", permissionType);
This would match what was done at: http://searchfox.org/mozilla-central/rev/3df383b8552c1f8059f5c21258388ddb5a2f33d0/toolkit/modules/PopupNotifications.jsm#152

::: browser/base/content/browser.js:7326
(Diff revision 1)
> +
> +      // 0 : clear any permission (always set)
> +      histogram.add(aPermission.id, 0);
> +
> +      let permissionType;
> +      if (aPermission.state === SitePermissions.ALLOW) {

The JS code in browser/ uses == rather than ===, unless there's a specific reason to need ===.

::: toolkit/components/telemetry/Histograms.json:10027
(Diff revision 1)
>      "description": "When restoring tabs on startup, reading from sessionstore.js failed, even though the file exists and is not containing an explicitly empty window.",
>      "cpp_guard": "ANDROID"
> +  },
> +  "WEB_PERMISSION_CLEARED": {
> +    "alert_emails": ["firefox-dev@mozilla.org"],
> +    "bug_numbers": [1242013],

not the right bug number

::: toolkit/components/telemetry/Histograms.json:10032
(Diff revision 1)
> +    "bug_numbers": [1242013],
> +    "expires_in_version": "55",
> +    "kind": "enumerated",
> +    "keyed": true,
> +    "n_values": 6,
> +    "description": "Web permissions that were revoked in the site control center."

We likely want this description to give enough details to interpret the values when looking at the histogram. See the description of POPUP_NOTIFICATION_STATS for a good example.
Comment on attachment 8776559 [details]
Bug 1286118 - Add telemetry probe to measure changes to permissions after initial prompt.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68296/diff/1-2/
Attachment #8776559 - Flags: review- → review?(florian)
Comment on attachment 8776559 [details]
Bug 1286118 - Add telemetry probe to measure changes to permissions after initial prompt.

https://reviewboard.mozilla.org/r/68296/#review65658

The code looks good to me, but you need an additional data collection review (see https://wiki.mozilla.org/Firefox/Data_Collection).

::: browser/base/content/browser.js:7330
(Diff revision 2)
> +        permissionType = 1;
> +      } else if (aPermission.state == SitePermissions.BLOCK) {
> +        // 2 : clear permanently blocked permission
> +        permissionType = 2;
> +      }
> +      // 3 : TODO clear temporary allowed permission

I don't know if we want to land with this comment, or wait a little bit (bug 1206233 will add the temporarily allowed state soon).

::: toolkit/components/telemetry/Histograms.json:10032
(Diff revision 2)
> +    "bug_numbers": [1286118],
> +    "expires_in_version": "55",
> +    "kind": "enumerated",
> +    "keyed": true,
> +    "n_values": 6,
> +    "description": "Number of revoke actions on web permissions in the site control center, keyed by permission id. Values represent the permission type that was revoked. (1=permanently allowed, 2=permanently blocked, 3=temporarily allowed, 4=temporarily blocked)"

nit: we could make this a bit more concise without losing any meaning:
web permissions -> permissions
site control center -> control center
permanently allowed -> allowed
permanently blocked -> blocked
Attachment #8776559 - Flags: review?(florian) → review+
Comment on attachment 8776559 [details]
Bug 1286118 - Add telemetry probe to measure changes to permissions after initial prompt.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68296/diff/2-3/
(In reply to Florian Quèze [:florian] [:flo] from comment #8)
> Comment on attachment 8776559 [details]
> Bug 1286118 - Add telemetry probe to measure changes to permissions after
> initial prompt.
> 
> https://reviewboard.mozilla.org/r/68296/#review65658
> 
> The code looks good to me, but you need an additional data collection review
> (see https://wiki.mozilla.org/Firefox/Data_Collection).
> 
> ::: browser/base/content/browser.js:7330
> (Diff revision 2)
> > +        permissionType = 1;
> > +      } else if (aPermission.state == SitePermissions.BLOCK) {
> > +        // 2 : clear permanently blocked permission
> > +        permissionType = 2;
> > +      }
> > +      // 3 : TODO clear temporary allowed permission
> 
> I don't know if we want to land with this comment, or wait a little bit (bug
> 1206233 will add the temporarily allowed state soon).

Since we're at the start of the 51 release how about we wait a bit to see if the required bugs arrive in time and if not we merge this in time so that it rides the trains.

> permanently allowed -> allowed
> permanently blocked -> blocked

With these two I think it's better to be explicit.
Depends on: 1206233
(In reply to Johann Hofmann [:johannh] from comment #5)
> Peter: does the described histogram look useful enough for you? If I recall
> correctly, we said we wanted to go only for aggregate numbers for now, since
> we also have aggregate numbers for doorhanger interaction. Let me know if
> you imagined something else. :)

This looks sufficient to me.  Thanks!
Flags: needinfo?(pdolanjski)
Depends on: 1291642
Depends on: 1299420
Comment on attachment 8776559 [details]
Bug 1286118 - Add telemetry probe to measure changes to permissions after initial prompt.

bsmedberg, I'd like to ask for data collection approval for this probe. (Do you need anything else?)

Thanks!
Attachment #8776559 - Flags: feedback?(benjamin)
Attachment #8776559 - Flags: feedback?(benjamin) → feedback+
https://hg.mozilla.org/integration/fx-team/rev/27ffae997b83a5bfeaf0967e692074cb097d94e8
Bug 1286118 - Add telemetry probe to measure changes to permissions after initial prompt. r=florian data-review=bsmedberg
Backed out for failing browser_devices_get_user_media.js:

https://hg.mozilla.org/integration/fx-team/rev/aa1a7886af087c5c13cd087ac1c67dac7e553492

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=fx-team&revision=27ffae997b83a5bfeaf0967e692074cb097d94e8
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=11389004&repo=fx-team

07:19:57     INFO -  201 INFO TEST-PASS | browser/base/content/test/webrtc/browser_devices_get_user_media.js | popup WebRTC indicator visible -
07:19:57     INFO -  202 INFO TEST-PASS | browser/base/content/test/webrtc/browser_devices_get_user_media.js | video global indicator attribute as expected -
07:19:57     INFO -  203 INFO TEST-PASS | browser/base/content/test/webrtc/browser_devices_get_user_media.js | audio global indicator attribute as expected -
07:19:57     INFO -  204 INFO TEST-PASS | browser/base/content/test/webrtc/browser_devices_get_user_media.js | screen global indicator attribute as expected -
07:19:57     INFO -  205 INFO TEST-PASS | browser/base/content/test/webrtc/browser_devices_get_user_media.js | only one global indicator window -
07:19:57     INFO -  206 INFO TEST-UNEXPECTED-FAIL | browser/base/content/test/webrtc/browser_devices_get_user_media.js | uncaught exception - Error: Not a number at gIdentityHandler._createPermissionItem/<@chrome://browser/content/browser.js:7437:7
07:19:57     INFO -  stopSharing@chrome://mochitests/content/browser/browser/base/content/test/webrtc/head.js:341:3
Flags: needinfo?(jhofmann)
Thanks for the backout, this was a small oversight that should be easy to fix!
Flags: needinfo?(jhofmann)
Peter, I think this is mostly a decision for you:

The tests were failing because we didn't correctly handle cases where the permission type that was cleared is not categorized by us as permanent (or temporary in the future). Once we add support for temporary states this should not happen too much, but depending on future implementations there could always be some freak state that doesn't fit our categorization of perm/temp.

What are we supposed to do with these? We can either

a) ignore and not report them at all
b) set them to the value of 0, meaning "others" and include them in the telemetry report

Which do you prefer?

Thanks!
Flags: needinfo?(pdolanjski)
(In reply to Johann Hofmann [:johannh] from comment #16)
> What are we supposed to do with these? We can either
> 
> a) ignore and not report them at all
> b) set them to the value of 0, meaning "others" and include them in the
> telemetry report
> 
> Which do you prefer?

I'd suggest b, so that we know they are occurring and can take action if the volume is high.
Flags: needinfo?(pdolanjski)
Sounds like a good choice, let's do it!
https://hg.mozilla.org/mozilla-central/rev/9431390a4d87
https://hg.mozilla.org/mozilla-central/rev/7a5d8ff573bc
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 51
Iteration: 50.4 - Aug 1 → 51.3 - Sep 12
Blocks: 1439721
You need to log in before you can comment on or make changes to this bug.