Closed Bug 1536454 Opened 5 years ago Closed 5 years ago

Record additional event telemetry on user interaction with permission prompts

Categories

(Firefox :: Site Identity, enhancement, P1)

enhancement

Tracking

()

VERIFIED FIXED
Firefox 68
Tracking Status
firefox67 --- verified
firefox68 --- verified

People

(Reporter: johannh, Assigned: johannh)

References

Details

Attachments

(6 files)

For limited field (shield) studies we would like to have precise event telemetry about different metric when a user interacts with permission prompts.

This could be used to build heuristics to automatically reject permission prompts in the future.

Almost none of the prompts that we are currently showing still get dismissed, which messes up our
measurements in this probe. Most of them are persistent now, which means that we should record when
they get removed instead of dismissed to receive meaningful data. This patch does that.

This is still lacking basic tests (we could test an endless combination of things but I'd like to have at least some basic assertions), and I will file the data review request after we're done with our conversation with legal on approving this study.

Thanks!

Attached file request.md
Attachment #9058548 - Flags: data-review?(chutten)
Attachment #9057302 - Attachment description: Bug 1536454 - Part 1 - Add documentHasUserInput and pageLoadTimeStamp attributes to nsIContentPermissionPrompt. r=Ehsan → Bug 1536454 - Part 1 - Add userHadInteractedWithDocument and documentDOMContentLoadedTimestamp attributes to nsIContentPermissionPrompt. r=Ehsan
Comment on attachment 9058548 [details]
request.md

DATA COLLECTION REVIEW RESPONSE:

    Is there or will there be documentation that describes the schema for the ultimate data set available publicly, complete and accurate?

Yes. This collection is Telemetry so is documented in its definitions file [Events.yaml](https://hg.mozilla.org/mozilla-central/file/tip/toolkit/components/telemetry/Events.yaml) and the [Probe Dictionary](https://telemetry.mozilla.org/probe-dictionary/).

    Is there a control mechanism that allows the user to turn the data collection on and off?

Yes. This collection is Telemetry so can be controlled through Firefox's Preferences.
Additionally, this collection is gated to a study, so users can disable studies to disable this collection.

    If the request is for permanent data collection, is there someone who will monitor the data over time?

No. This collection will expire in Firefox 72.
In addition, this collection is gated by a preference that will only be enabled as part of a study with limited duration.

    Using the category system of data types on the Mozilla wiki, what collection type of data do the requested measurements fall under?

Category 3, Web Activity.
The "Web Activity" being collected is a "Was the site that prompted this on a list of known sites?". This has been deemed to be an acceptable mitigation to permit its opt-out collection on release.

    Is the data collection request for default-on or default-off?

Default on for all channels.

    Does the instrumentation include the addition of any new identifiers?

No.

    Is the data collection covered by the existing Firefox privacy notice?

Yes.

    Does there need to be a check-in in the future to determine whether to renew the data?

Yes. :johannh is responsible for renewing or removing the collection before it expires in Firefox 72.

---
Result: datareview+
Attachment #9058548 - Flags: data-review?(chutten) → data-review+
Blocks: 1475404
Pushed by jhofmann@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/565279f28841
Part 1 - Add userHadInteractedWithDocument and documentDOMContentLoadedTimestamp attributes to nsIContentPermissionPrompt. r=Ehsan
https://hg.mozilla.org/integration/autoland/rev/c4beb7256d16
Part 2 - Add modificationTime attribute to nsIPermission.idl. r=Ehsan
https://hg.mozilla.org/integration/autoland/rev/8c6ec5c528ff
Part 3 - Make POPUP_NOTIFICATION_STATS probe collect data on notification removal instead of dismissal. r=MattN
https://hg.mozilla.org/integration/autoland/rev/19b40bd2e67f
Part 4 - Add event telemetry for permission prompt studies. r=Ehsan
https://hg.mozilla.org/integration/autoland/rev/4b317b418c14
Part 5 - Add tests for permission prompt event telemetry. r=Ehsan

Comment on attachment 9057302 [details]
Bug 1536454 - Part 1 - Add userHadInteractedWithDocument and documentDOMContentLoadedTimestamp attributes to nsIContentPermissionPrompt. r=Ehsan

Beta/Release Uplift Approval Request

  • User impact if declined: None, this is a preffed-off telemetry collection that will only be used for a study we are planning to run in Release 67 (we already announced this to rel-man), see https://blog.nightly.mozilla.org/2019/04/01/reducing-notification-permission-prompt-spam-in-firefox/
  • Is this code covered by automated tests?: Yes
  • Has the fix been verified in Nightly?: No
  • Needs manual test from QE?: Yes
  • If yes, steps to reproduce: We hope to get separate testing for the Telemetry itself (besides the automated tests), but it would be nice to get a sanity check that permission prompts are still working (see https://permission.site/) on Beta after uplift.
  • List of other uplifts needed: To avoid merge pains we would need bug 1508961 uplifted, too. :/
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): The main risk with this patch is it's size, and the fact that it depends on bug 1508961. Otherwise, this is entirely preffed-off, has extensive test coverage and should have no user facing consequences even if enabled.
  • String changes made/needed: None
Attachment #9057302 - Flags: approval-mozilla-beta?
Attachment #9057303 - Flags: approval-mozilla-beta?
Attachment #9057304 - Flags: approval-mozilla-beta?
Attachment #9057305 - Flags: approval-mozilla-beta?
Attachment #9058973 - Flags: approval-mozilla-beta?
Flags: qe-verify+

List of other uplifts needed: To avoid merge pains we would need bug 1508961 uplifted, too. :/

I just realized we need bug 1524619 as well.

Hi Johann, do we have new telemetry data coming in from Nightly based on commits in comment 15? I am always a bit wary of uplifting telemetry related changes in Beta until we know we are seeing some of the data from Nightly users.

Flags: needinfo?(jhofmann)
QA Whiteboard: [qa-triaged]

(In reply to Ritu Kothari (:ritu) from comment #18)

Hi Johann, do we have new telemetry data coming in from Nightly based on commits in comment 15? I am always a bit wary of uplifting telemetry related changes in Beta until we know we are seeing some of the data from Nightly users.

Hi Ritu, that's a very valid concern. We initially turned this off in Nightly by default, because this was only intended for the experiment. But seeing that we made a last minute decision to remove the C3 data collection after all, it should be fine to enable in Nightly. I'll file a bug for it.

However, that still means we won't have Telemetry data coming in for another couple of days. I would still like to get this onto Beta as early as possible. My main concern is about breaking things by merging all these patches into Beta, so I'd prefer to be optimistic about the telemetry here. It has good test coverage and I just tried it myself on Nightly and things show up correctly in about:telemetry.

Maybe instead of waiting on the data, the QA team can try to verify this on Nightly first?

STR are:

  • Enable permissions.eventTelemetry.enabled in about:config
  • Go to https://permission.site, click on "Notifications" or "Geo"
  • Visit about:telemetry -> Events
  • You should see an entry for security.ui.permissionprompt with the show method and the correct type (geo or notifications)
  • Respond to the prompt or leave the site
  • Refresh about:telemetry
  • You should see an entry for security.ui.permissionprompt with the method corresponding to what you did (allow, deny, leave)

Might also be nice to test this:

  • Enable permissions.eventTelemetry.enabled in about:config
  • Disable dom.webnotifications.requireuserinteraction
  • Go to https://www.cnet.com/, notice a prompt is shown
  • You should see an entry for security.ui.permissionprompt with the show method and the correct type (geo or notifications)
  • Respond to the prompt or leave the site
  • Refresh about:telemetry
  • You should see an entry for security.ui.permissionprompt with the method corresponding to what you did (allow, deny, leave)

Brindusa, are you the right person to tackle this? :)

Thank you!

Flags: needinfo?(jhofmann) → needinfo?(brindusa.tot)

Hi, I have retested this issue on our latest Version of Nightly and I couldn't find any issues, everything works according to plan.

Event>Telemetry shows the Correct Leave, Allow, Deny or Never messages according to my selection from the https://www.cnet.com/ website prompt.

Status: RESOLVED → VERIFIED
Flags: qe-verify+
Flags: needinfo?(brindusa.tot)

Forgot to mention that this issue was tested on all Operating Systems using Johann's steps from Comment 19. Thanks.

Awesome, thanks for the quick confirmation ⚡️

Flags: qe-verify+

Comment on attachment 9057302 [details]
Bug 1536454 - Part 1 - Add userHadInteractedWithDocument and documentDOMContentLoadedTimestamp attributes to nsIContentPermissionPrompt. r=Ehsan

P1, preffed-off by default and QA verified on nightly. uplift approved for 67 beta14. I reset the qe-verify+ keyword as I would like this uplift to be tested manually again by QA once it is on beta. Thanks.

Attachment #9057302 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #9057303 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #9057304 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #9057305 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #9058973 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Hi, I have retested this issue on all Operating systems with our latest Beta 67.0b14 and I can confirm this issue Verified as Fixed according to the steps from Comment 19.

I will mark this issue accordingly.

QA Whiteboard: [qa-triaged]
Flags: qe-verify+
Blocks: 1546988
Regressions: 1578266
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: