Closed Bug 1619498 Opened 8 months ago Closed 7 months ago

Record in telemetry how long it takes to display an autocomplete popup

Categories

(Toolkit :: Password Manager, task, P1)

task

Tracking

()

VERIFIED FIXED
mozilla76
Tracking Status
firefox74 --- wontfix
firefox75 + verified
firefox76 --- verified

People

(Reporter: MattN, Assigned: MattN)

References

(Blocks 1 open bug)

Details

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

User Story

category: form_autocomplete
method: show
object: logins
value: (time in milliseconds to show the autocomplete popup)
extra: (See https://searchfox.org/mozilla-central/rev/6cd54550a27e2f6ca0755a25328f769e41e524f4/toolkit/components/telemetry/Events.yaml#301-323 )

Caveats:
* The event should only be recorded if the length of the text in the field is 0 or 1 (this is simply to reduce volume of telemetry) as 0 and 1 are the most common and expensive cases.
* The event won't be recorded if the user disables the login autocomplete footer (but that's not common or really supported… we should remove the pref)
* The event doesn't record if there are no autocomplete results shown since the point of the probe is to measure the time to show the popup.

Attachments

(13 files)

47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
3.26 KB, text/plain
tdsmith
: data-review+
Details

We have had reports of autocomplete being slow for dozens of logins (probably due to decryption time e.g. bug 1564074) and we would like to try using Fathom for determining whether to offer password generation (bug 1595244) so it's useful to understand how long it's taking to display autocomplete results in the wild.

Variables we may want to account for:

  • password field or text field (username)
  • generation enabled and offered
  • empty searchString
  • has a previousResult
  • number of saved logins returned
  • resultStyles returned
  • autocompleteInfo.fieldName (to compare to previous "new-password" behaviour)

Given that there are so many variables that could affect the timing, I think we should use an event instead of a histogram so we can record these variables more easily in extra_keys rather than hacking together some composite key e.g. password_1_1_0_1 which will be fragile if we add a variable. Using events also means not being able to use TelemetryStopwatch, though I think we probably can't use it here anyways due to wanting to record times between different processes (bug 1354167), instead we'll need to use Services.telemetry.msSystemNow().

Other things to note:

  • We should be careful not to record too many events in one session… maybe only handle searchString of 0 and 1?
  • Since we know there are ideas about using Fathom for credit card autofill heuristics too, it would be nice to not limit this to login autocomplete results.
Flags: qe-verify+
Assignee: nobody → MattN+bmo
Status: NEW → ASSIGNED

The only difference was the conversion to vanilla objects.

Depends on D66725

The file is more than just the LoginAutoCompleteResult class, it handles all login autocompletion.

Depends on D66726

Consolidate login autocomplete code in one file.

Depends on D66727

Attachment #9133108 - Attachment description: Bug 1619498 - Record in telemetry how long it takes to display an autocomplete popup → Bug 1619498 - Record in telemetry how long it takes to display an autocomplete popup. r=sfoster

Also simplify checkAutoCompleteResults.

Depends on D66887

Also add some logging that seems generally useful.

Depends on D67087

https://hg.mozilla.org/mozilla-central/rev/aa78dbe7934f7b7c5ec3de348cb70109fa0e35aa#l8.32 deleted this member but didn't update this reference. This caused <!-- formless password field selector recipe test --> in test_formless_autofill.html to fail test verification.

Depends on D67088

Attachment #9133764 - Flags: data-review?(tdsmith)

I'm landing all but D66730 (since data-review is pending) to unblock other work.

Keywords: leave-open
Pushed by mozilla@noorenberghe.ca:
https://hg.mozilla.org/integration/autoland/rev/f89d7b0e4a76
Remove function.bind from LoginAutoCompleteResult.startSearch. r=sfoster
https://hg.mozilla.org/integration/autoland/rev/4ba98b434622
Assign to this._autoCompleteLookupPromise inside completeSearch. r=sfoster
https://hg.mozilla.org/integration/autoland/rev/02dca53e816a
Switch _autoCompleteSearchAsync to an async method. r=sfoster
https://hg.mozilla.org/integration/autoland/rev/07a87d64d4d8
Create previousResult once in LoginAutoCompleteResult. r=sfoster
https://hg.mozilla.org/integration/autoland/rev/6e5ce5c2ad45
Rename LoginAutoCompleteResult.jsm to LoginAutoComplete.jsm. r=sfoster
https://hg.mozilla.org/integration/autoland/rev/b6536f155576
Move _autoCompleteSearchAsync and gAutoCompleteListener to LoginAutoComplete.jsm. r=sfoster
https://hg.mozilla.org/integration/autoland/rev/de9538b840e9
Reduce duplication between _requestAutoCompleteResultsFromParent and startSearch. r=sfoster
https://hg.mozilla.org/integration/autoland/rev/1c8f108909cb
Check for a 'generatedPassword' result before accessing gGeneratedPasswordsByPrincipalOrigin. r=bdanforth
https://hg.mozilla.org/integration/autoland/rev/b3503cdeb434
Add missing await in passwordmgr mochitests. r=sfoster
https://hg.mozilla.org/integration/autoland/rev/c47897bb4792
Fix passwordmgr mochitest-plain test-verify failures. r=sfoster
https://hg.mozilla.org/integration/autoland/rev/18edd0bc728a
Restore access to LoginManagerParent._recipeManager to fix resetting recipes in mochitest. r=sfoster
Attachment #9133764 - Flags: data-review?(teon)

Sorry for the delay; where does this value come from? Can it contain arbitrary values from the page?

acFieldName: The "field name" token (last one) of the field's autocomplete attribute.

Flags: needinfo?(MattN+bmo)

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

Sorry for the delay; where does this value come from? Can it contain arbitrary values from the page?

acFieldName: The "field name" token (last one) of the field's autocomplete attribute.

It comes from the HTML standard and we only return valid values so it can't contain arbitrary values. These are the valid values: https://searchfox.org/mozilla-central/rev/61f224ec08ddc6f9a93ac45c8c3c5f7159be7c2a/dom/base/AutocompleteFieldList.h#91-147 (we actually only support a subset of those)

HTML spec link

Flags: needinfo?(MattN+bmo)
Comment on attachment 9133764 [details]
Request for data collection review form

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, Events.yaml and the probe dictionary.

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

Yes, the Firefox telemetry opt-out.

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

Yes, MattN.

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** (whether anonymous or otherwise; e.g., username, random IDs, etc.  See the appendix for more details)?

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.
Attachment #9133764 - Flags: data-review?(teon)
Attachment #9133764 - Flags: data-review?(tdsmith)
Attachment #9133764 - Flags: data-review+
Pushed by mozilla@noorenberghe.ca:
https://hg.mozilla.org/integration/autoland/rev/ed44b5d1ca3f
Record in telemetry how long it takes to display an autocomplete popup. r=sfoster

[Tracking Requested - why for this release]: Need to establish a performance baseline for bug 1595244 to know whether we can ship it in Fx76

Timea, is there any chance this can get verified soon so we can try uplift it to beta so we have a baseline for bug 1595244?

Flags: needinfo?(tbabos)
Keywords: leave-open
Status: ASSIGNED → RESOLVED
Closed: 7 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla76
Regressions: 1623657

Verified-fixed on MacOS 10.13 and Ubuntu 16.04 on the latest Nightly 76.0a1 (2020-13-19).
The "form_autocomplete" "show" "logins" "time" telemetry is recorded.
The autocomplete dropdown is toggled for the Username/Mail or the Password field. Checked with 10, 20 and 1000 login entries.

Nightly updates on Windows are not working due to signing issues for now so can't verify on that. Will mark FX76 as verified after checking it on Windows too.
Submitted Bug 1623657 as a regression, more info there.

Flags: needinfo?(tbabos)
Depends on: 1623657
No longer regressions: 1623657

(In reply to Timea Cernea [:tbabos] from comment #26)

Submitted Bug 1623657 as a regression, more info there.

Btw. I believe this isn't a problem because we record stringLength in the event.

User Story: (updated)

Comment on attachment 9133108 [details]
Bug 1619498 - Record in telemetry how long it takes to display an autocomplete popup. r=sfoster

Beta/Release Uplift Approval Request

  • User impact if declined: Need to establish a performance baseline for bug 1595244 to know whether we can ship it in Fx76. Not having this data could mean that the major improvement to password generation will have to be delayed and we are trying to market 4 different password manager projects for Fx76.
    This will also allow us to potentially prioritize bug 1564074 sooner as we will know how well login autocomplete performs in aggregate in the wild.
  • Is this code covered by automated tests?: Yes
  • Has the fix been verified in Nightly?: Yes
  • Needs manual test from QE?: Yes
  • If yes, steps to reproduce: Already done.
  • List of other uplifts needed: None
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): The first 7 commits are smallish cleanups/refactorings to make the telemetry recording easier and more efficient. They are low risk given our test coverage.
    The 8th fixes a regression from bug 1616356 which is straightforward as it just inverts the order of two checks. This is trivial.
    The 9th and 10th are test-only fixes to avoid intermittent failures with this series. These have no product risk.
    The 11th is the core patch that address the telemetry event recording and includes automated tests. Initial data in STMO seems to be valid with no unusual records. e.g. all time measurements are >0. It's pretty straightforward code as it passes data along with the autocomplete results in a way we already use so that it can be recorded when the popup is shown. We have a PI request for this feature so we could ask QA to do additional autocomplete smoke testing if wanted.
  • String changes made/needed: None
Attachment #9133108 - Flags: approval-mozilla-beta?
Attachment #9133101 - Flags: approval-mozilla-beta?
Attachment #9133102 - Flags: approval-mozilla-beta?
Attachment #9133103 - Flags: approval-mozilla-beta?
Attachment #9133104 - Flags: approval-mozilla-beta?
Attachment #9133105 - Flags: approval-mozilla-beta?
Attachment #9133106 - Flags: approval-mozilla-beta?
Attachment #9133107 - Flags: approval-mozilla-beta?
Attachment #9133424 - Flags: approval-mozilla-beta?
Attachment #9133756 - Flags: approval-mozilla-beta?
Attachment #9133757 - Flags: approval-mozilla-beta?
Attachment #9133758 - Flags: approval-mozilla-beta?
QA Whiteboard: [qa-triaged]

Verified on Windows 10 as well, waiting for uplift to Beta. Aside from this verification, we will also test telemetry on top sites as part of the PI request for FX76.

Comment on attachment 9133108 [details]
Bug 1619498 - Record in telemetry how long it takes to display an autocomplete popup. r=sfoster

thanks for the details in the request, very helpful.

Approved for 75.0b7

Attachment #9133108 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #9133101 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #9133102 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #9133103 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #9133104 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #9133105 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #9133106 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #9133107 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #9133424 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #9133756 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #9133757 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #9133758 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Verified-fixed on latest Beta 75.0b7 on Windows 10 x64, Ubuntu 16.04 and MacOS 10.13.

Status: RESOLVED → VERIFIED
QA Whiteboard: [qa-triaged]
Flags: qe-verify+
Depends on: 1629848
You need to log in before you can comment on or make changes to this bug.