Closed Bug 1548878 Opened 9 months ago Closed 7 months ago

Add telemetry for when a generated password is first filled

Categories

(Toolkit :: Password Manager, task, P2)

task

Tracking

()

VERIFIED FIXED
mozilla69
Tracking Status
firefox69 --- fixed
firefox70 --- verified

People

(Reporter: MattN, Assigned: sfoster)

References

(Blocks 1 open bug)

Details

(Whiteboard: [passwords:generation] [skyline])

Attachments

(3 files)

We want to know if people are using our generated passwords.

Flags: qe-verify+

Tentative plan is to register a new UI event and record it (from the parent process)

category: pwmgr
methods: autocomplete_field
object: generatedpassword

.. then in bug 1548880 we can use something like method: edit_field, object: generatedpassword

Assignee: nobody → sfoster
Status: NEW → ASSIGNED

This is just for the event telemetry for filling a field with a generated password. We have a different bug for user edits of that value (bug 1548880.) It goes without saying (but I'll say anyway) that the event recorded only records the fact that a generated password was filled, not what that password was.

Attachment #9069149 - Flags: data-review?(chutten)
Attachment #9069109 - Attachment description: Bug 1548878 - (WIP) Add telemetry for when a generated password is first filled → Bug 1548878 - Add telemetry for when a generated password is first filled. r?MattN
Comment on attachment 9069149 [details]
bug-1548878-data-review-form.md

Load-balancing redirect to :tdsmith
Attachment #9069149 - Flags: data-review?(chutten) → data-review?(tdsmith)
Comment on attachment 9069149 [details]
bug-1548878-data-review-form.md

* Is there or will there be documentation that describes the schema for the ultimate data set in a public, complete, and accurate way?

The documentation in Events.yaml implies that an event is sent any time a login field is autocompleted with any content, which I don't think is the intent of this patch. Can you limit the description of the event to the data collection you're implementing in this bug?

Can you also clarify whether you intend to send an event the first time a generated password is filled (like the title of the bug suggests) or any time a generated password is filled (like the event description would suggest)?
Attachment #9069149 - Flags: data-review?(tdsmith) → data-review-

(In reply to Tim Smith 👨‍🔬 [:tdsmith] from comment #6)

Comment on attachment 9069149 [details]
bug-1548878-data-review-form.md

Thanks, I'm working on an updated patch which moves the event recording to the content process and I can clarify both these questions in the new patch.

  • Is there or will there be documentation that describes the schema for the
    ultimate data set in a public, complete, and accurate way?

I'm happy to put this together. I'm not sure where it would live if not in the Events.yaml itself - do we have something similar for the in-product telemetry for other features I can look at?

Flags: needinfo?(tdsmith)

Great, thanks!

I think I created some confusion—the bulleted question is the stock question from the data collection review form that I was responding to; sorry! A sentence or two in the description string in Events.yaml is the right place and right amount of documentation; I just want to be sure that it matches the collection.

Flags: needinfo?(tdsmith)
Attachment #9069109 - Attachment description: Bug 1548878 - Add telemetry for when a generated password is first filled. r?MattN → Bug 1548878 - (WIP) Add telemetry for when a generated password is first filled. r?MattN
Attachment #9069109 - Attachment description: Bug 1548878 - (WIP) Add telemetry for when a generated password is first filled. r?MattN → Bug 1548878 - Add telemetry for when a generated password is first filled. r?MattN
Comment on attachment 9069149 [details]
bug-1548878-data-review-form.md

The data review form hasn't changed, but the patch has. I've clarified in the description field in which circumstances we'll record this event. The significance of the "first filled" distinction is that we don't want to record 2 events when the result of selecting the generated password from the autocomplete dropdown is to fill 2 fields e.g a "new password" and "confirm password" inputs. 

Also changed: this event is now recorded from the main/parent process.
Attachment #9069149 - Flags: data-review- → data-review?(tdsmith)
Comment on attachment 9069149 [details]
bug-1548878-data-review-form.md

Thanks!

1) Is there or will there be **documentation** that describes the schema for the ultimate data set in a public, complete, and accurate way?

Yes, in Events.yaml.

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

Yes, the [Firefox telemetry opt-out](https://support.mozilla.org/en-US/kb/share-data-mozilla-help-improve-firefox).

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

Yes, sfoster.

4) Using the **[category system of data types](https://wiki.mozilla.org/Firefox/Data_Collection)** on the Mozilla wiki, what collection type of data do the requested measurements fall under?

Category 2, interaction data.

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

Default-on.

6) Does the instrumentation include the addition of **any *new* identifiers**?

No.

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

Yes.


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

No, permanent collection.

9) Does the data collection use a third-party collection tool?

No.

—

result: data-review+
Attachment #9069149 - Flags: data-review?(tdsmith) → data-review+
Pushed by sfoster@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/323de6d7d422
Add telemetry for when a generated password is first filled. r=MattN

Backed out changeset 323de6d7d422 for causing xpcshell failures.

Backout link: https://hg.mozilla.org/integration/autoland/rev/df727b0ee1680c73e31113da1206c8713a3f5f28

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception&revision=323de6d7d42282f98ca8ab1d5be9788d83fa7581&selectedJob=251968997

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=251968997&repo=autoland&lineNumber=2773

[task 2019-06-14T23:29:24.831Z] 23:29:24 INFO - TEST-PASS | toolkit/components/passwordmgr/test/unit/test_LoginManagerParent_getGeneratedPassword.js | test_getGeneratedPassword - [test_getGeneratedPassword : 35] 1 added to cache - 1 == 1
[task 2019-06-14T23:29:24.832Z] 23:29:24 WARNING - TEST-UNEXPECTED-FAIL | toolkit/components/passwordmgr/test/unit/test_LoginManagerParent_getGeneratedPassword.js | test_getGeneratedPassword - [test_getGeneratedPassword : 36] Cache key and value - {"value":"7Hf8PNvaearVtmg","filled":false} == "7Hf8PNvaearVtmg"
[task 2019-06-14T23:29:24.832Z] 23:29:24 INFO - /builds/worker/workspace/build/tests/xpcshell/tests/toolkit/components/passwordmgr/test/unit/test_LoginManagerParent_getGeneratedPassword.js:test_getGeneratedPassword:36
[task 2019-06-14T23:29:24.833Z] 23:29:24 INFO - /builds/worker/workspace/build/tests/xpcshell/head.js:run_next_test/_run_next_test/<:1437
[task 2019-06-14T23:29:24.833Z] 23:29:24 INFO - /builds/worker/workspace/build/tests/xpcshell/head.js:_run_next_test:1437
[task 2019-06-14T23:29:24.834Z] 23:29:24 INFO - /builds/worker/workspace/build/tests/xpcshell/head.js:run:688
[task 2019-06-14T23:29:24.834Z] 23:29:24 INFO - /builds/worker/workspace/build/tests/xpcshell/head.js:_do_main:227
[task 2019-06-14T23:29:24.834Z] 23:29:24 INFO - /builds/worker/workspace/build/tests/xpcshell/head.js:_execute_test:529
[task 2019-06-14T23:29:24.834Z] 23:29:24 INFO - -e:null:1
[task 2019-06-14T23:29:24.835Z] 23:29:24 INFO - exiting test
[task 2019-06-14T23:29:24.836Z] 23:29:24 INFO - (xpcshell/head.js) | test run_next_test 1 finished (2)

Flags: needinfo?(sfoster)

(In reply to Alexandru Michis [:malexandru] from comment #12)

Backed out changeset 323de6d7d422 for causing xpcshell failures.

Doh thanks for the backout, I had forgotten about that test.

Flags: needinfo?(sfoster)
Pushed by sfoster@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/218e6f863de3
Add telemetry for when a generated password is first filled. r=MattN

Backed out changeset 218e6f863de3 (Bug 1548878) for test_autocomplete_new_password.html failures

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&fromchange=218e6f863de3c5bd8a435b5a3da0d340f31eac59&tochange=39798e18e83ed891cf760e6d1e3269dc6edb83e7&selectedJob=252418253

Backout link: https://hg.mozilla.org/integration/autoland/rev/39798e18e83ed891cf760e6d1e3269dc6edb83e7

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=252418253&repo=autoland&lineNumber=1462

06:44:42 INFO - TEST-START | toolkit/components/passwordmgr/test/mochitest/test_autocomplete_new_password.html
06:44:42 INFO - GECKO(2212) | ++DOMWINDOW == 8 (07D30C00) [pid = 1648] [serial = 10] [outer = 07F33B80]
06:44:42 INFO - GECKO(2212) | [Parent 3996, Main Thread] WARNING: 'aRv.Failed()', file z:/build/build/src/dom/ipc/StructuredCloneData.cpp, line 120
06:44:42 INFO - GECKO(2212) | TEST-PASS | https://example.com/tests/toolkit/components/passwordmgr/test/mochitest/parent_utils.js | Got autocomplete popup - [object XULPopupElement] == true
06:44:42 INFO - GECKO(2212) | console.warn: nsLoginManager: "searchLogins: formActionOrigin or httpRealm is recommended"
06:44:42 INFO - GECKO(2212) | console.warn: nsLoginManager: "searchLogins: formActionOrigin or httpRealm is recommended"
06:44:42 INFO - GECKO(2212) | console.warn: nsLoginManager: "searchLogins: formActionOrigin or httpRealm is recommended"
06:44:42 INFO - GECKO(2212) | --DOMWINDOW == 7 (00862400) [pid = 1648] [serial = 3] [outer = 00000000] [url = about:blank]
06:44:42 INFO - GECKO(2212) | --DOMWINDOW == 6 (0A861400) [pid = 1648] [serial = 7] [outer = 00000000] [url = about:blank]
06:44:42 INFO - GECKO(2212) | --DOMWINDOW == 16 (138D6C00) [pid = 3996] [serial = 8] [outer = 00000000] [url = about:blank]
06:44:42 INFO - GECKO(2212) | --DOMWINDOW == 15 (1593DC00) [pid = 3996] [serial = 16] [outer = 00000000] [url = about:blank]
06:44:45 INFO - GECKO(2212) | --DOMWINDOW == 2 (01269400) [pid = 3524] [serial = 2] [outer = 00000000] [url = about:blank]
06:44:45 INFO - GECKO(2212) | --DOMWINDOW == 1 (08B46160) [pid = 3524] [serial = 1] [outer = 00000000] [url = chrome://gfxsanity/content/sanitytest.html]
06:44:45 INFO - TEST-INFO | started process screenshot
06:44:45 INFO - TEST-INFO | screenshot: exit 0
06:44:45 INFO - <snipped 3 output lines - if you need more context, please use SimpleTest.requestCompleteLog() in your test>
06:44:45 INFO - Buffered messages logged at 06:44:42
06:44:45 INFO - CONTENT: getTelemetryEvents gotResult: [[11916,"pwmgr","autocomplete_field","generatedpassword"]]
06:44:45 INFO - waitForTelemetryEventsCondition, got events: [[11916,"pwmgr","autocomplete_field","generatedpassword"]]
06:44:45 INFO - CONTENT: getTelemetryEvents gotResult: [[11916,"pwmgr","autocomplete_field","generatedpassword"]]
06:44:45 INFO - waitForTelemetryEventsCondition, got events: [[11916,"pwmgr","autocomplete_field","generatedpassword"]]
06:44:45 INFO - CONTENT: getTelemetryEvents gotResult: [[11916,"pwmgr","autocomplete_field","generatedpassword"]]
06:44:45 INFO - waitForTelemetryEventsCondition, got events: [[11916,"pwmgr","autocomplete_field","generatedpassword"]]
06:44:45 INFO - CONTENT: getTelemetryEvents gotResult: [[11916,"pwmgr","autocomplete_field","generatedpassword"]]
...
06:44:45 INFO - waitForTelemetryEventsCondition, got events: [[11916,"pwmgr","autocomplete_field","generatedpassword"]]
06:44:45 INFO - CONTENT: getTelemetryEvents gotResult: [[11916,"pwmgr","autocomplete_field","generatedpassword"]]
06:44:45 INFO - waitForTelemetryEventsCondition, got events: [[11916,"pwmgr","autocomplete_field","generatedpassword"]]
06:44:45 INFO - CONTENT: getTelemetryEvents gotResult: [[11916,"pwmgr","autocomplete_field","generatedpassword"]]
06:44:45 INFO - waitForTelemetryEventsCondition, got events: [[11916,"pwmgr","autocomplete_field","generatedpassword"]]
06:44:45 INFO - Buffered messages logged at 06:44:45
06:44:45 INFO - CONTENT: getTelemetryEvents gotResult: [[11916,"pwmgr","autocomplete_field","generatedpassword"]]
06:44:45 INFO - waitForTelemetryEventsCondition, got events: [[11916,"pwmgr","autocomplete_field","generatedpassword"]]
06:44:45 INFO - CONTENT: getTelemetryEvents gotResult: [[11916,"pwmgr","autocomplete_field","generatedpassword"]]
06:44:45 INFO - waitForTelemetryEventsCondition, got events: [[11916,"pwmgr","autocomplete_field","generatedpassword"]]
06:44:45 INFO - CONTENT: getTelemetryEvents gotResult: [[11916,"pwmgr","autocomplete_field","generatedpassword"]]
06:44:45 INFO - waitForTelemetryEventsCondition, got events: [[11916,"pwmgr","autocomplete_field","generatedpassword"]]
06:44:45 INFO - Buffered messages finished
06:44:45 INFO - TEST-UNEXPECTED-FAIL | toolkit/components/passwordmgr/test/mochitest/test_autocomplete_new_password.html | Wait for 0 telemetry events - timed out after 50 tries. - Should not throw any errors
06:44:45 INFO - nextTick/<@https://example.com/tests/SimpleTest/SimpleTest.js:1801:26
06:44:45 INFO - asyncnextTick@https://example.com/tests/SimpleTest/SimpleTest.js:1809:11
06:44:45 INFO - setTimeout handler
SimpleTest_setTimeoutShim@https://example.com/tests/SimpleTest/SimpleTest.js:684:43
06:44:45 INFO - add_task@https://example.com/tests/SimpleTest/SimpleTest.js:1753:7
06:44:45 INFO - @https://example.com/tests/toolkit/components/passwordmgr/test/mochitest/test_autocomplete_new_password.html:80:1
06:44:45 INFO - GECKO(2212) | Removing 1 popup notifications.
06:44:45 INFO - GECKO(2212) | MEMORY STAT | vsize 503MB | vsizeMaxContiguous 794MB | residentFast 87MB | heapAllocated 10MB
06:44:45 INFO - TEST-OK | toolkit/components/passwordmgr/test/mochitest/test_autocomplete_new_password.html | took 3155ms

Flags: needinfo?(sfoster)

Thanks for the backout (again). The TV failures look persistent and indicative of at least a problem with that test, if not a problem with the implementation itself. I'm looking into it.

Flags: needinfo?(sfoster)

Ok I have that test fixed. As test-verify re-runs the test, it was getting tripped up as the implementation here carefully avoids recording new telemetry events for re-use of a generated password. I now clear that map and also clear telemetry events at the start of the test.

try looks good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=937680030f87c9816a62388d590b6e47d9f3a942

Pushed by sfoster@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/292c5fa99131
Add telemetry for when a generated password is first filled. r=MattN
Status: ASSIGNED → RESOLVED
Closed: 7 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla69
Whiteboard: [passwords:generation] [skyline]

I have verified this implementation on Windows 10, Mac OS 10.14.5 and Ubuntu 18.04 on Nightly v70.0a1 from 2019-08-06.
It appears that password generations and generated password edits are being tracked correctly with instant telemetry events, but only ONCE for ANY SITE in EVERY ONE SESSION. Is this intended? If yes, then this implementation bug can be marked as verified. Thanks!

Flags: needinfo?(MattN+bmo)

Are the events correctly displayed? Is there no "Value" or "Extra" information needed?

(In reply to Bodea Daniel [:danibodea] from comment #20)

I have verified this implementation on Windows 10, Mac OS 10.14.5 and Ubuntu 18.04 on Nightly v70.0a1 from 2019-08-06.
It appears that password generations and generated password edits are being tracked correctly with instant telemetry events, but only ONCE for ANY SITE in EVERY ONE SESSION. Is this intended? If yes, then this implementation bug can be marked as verified. Thanks!

Yes, that is correct, we only record the first fill or edit for a site within a session.

(In reply to Bodea Daniel [:danibodea] from comment #21)

Are the events correctly displayed? Is there no "Value" or "Extra" information needed?

Yes, we don't have any other data to record.

Flags: needinfo?(MattN+bmo)
Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.