Closed Bug 1543499 Opened 8 months ago Closed 7 months ago

Keep track of which entry point is used to open Saved Logins

Categories

(Toolkit :: Password Manager, enhancement, P1)

enhancement

Tracking

()

VERIFIED FIXED
mozilla68
Tracking Status
firefox-esr60 --- unaffected
firefox66 --- unaffected
firefox67 --- unaffected
firefox68 --- verified

People

(Reporter: MattN, Assigned: sfoster)

References

Details

User Story

Keys:
* Preferences
* Autocomplete
* MainMenu
* PageInfo
* ContextMenu

We should be recording these in a way so that they are recorded when Lockbox is installed (and `signon.management.overrideURI` is set) so I think that means that all of them but `Preferences` can be recorded inside `LoginHelper.openPasswordManager`

Attachments

(2 files)

We currently have the PWMGR_MANAGE_OPENED probe which keeps track of contextual vs. non-contextual entrypoints. We want to replace this probe with one which is more fine-grained to track the specific UI used.

Flags: qe-verify+

Were you thinking of a scalar for each entry point, or some event telemetry?
Event telemetry seems designed for this use case; if the events have [timestamp, category, method, object, value, extra], that could be something like this strawman:

[timestamp, "ui", "entrypoint", "about.logins", "autocomplete"]

Is there any vision for how instrumenting the ui and user journeys through it should be structured? The best practices doc suggest a dot-path for the category rather than something very broad (like "ui") so I'm not sure where this stands.

Flags: needinfo?(MattN+bmo)

(In reply to Sam Foster [:sfoster] (he/him) from comment #1)

Were you thinking of a scalar for each entry point, or some event telemetry?

The simplest would be to take the histogram that we have with 2 buckets and convert it to have a bucket with each entrypoint and change the name though I agree that a histogram isn't the right tool at this time.

Event telemetry seems designed for this use case; if the events have [timestamp, category, method, object, value, extra], that could be something like this strawman:

[timestamp, "ui", "entrypoint", "about.logins", "autocomplete"]

Is there any vision for how instrumenting the ui and user journeys through it should be structured? The best practices doc suggest a dot-path for the category rather than something very broad (like "ui") so I'm not sure where this stands.

I think it would be nice to keep the "pwmgr" prefix like we have for our other instrumentation: https://telemetry.mozilla.org/probe-dictionary/?search=pwmgr (that URL is linked from the public wiki).

My understanding is that method should ideally be an action/event and that value is optional so I would suggest:

Services.telemetry.recordEvent("pwmgr", "open_management", "autocomplete")
Flags: needinfo?(MattN+bmo)
User Story: (updated)

:chutten, I'm not seeing about:telemetry#events-tab with this patch - to verify my work - and I assume that's because I did something wrong. But I don't see any warnings or other logs and AFAICT the new entry in Events.yaml is valid and complete. Any suggestions?

The goal here is to replace a histogram we record when the password management UI is opened with a telemetry event with named objects for each entry point (main menu, preferences, autocomplete etc)

Assignee: nobody → sfoster
Status: NEW → ASSIGNED
Flags: needinfo?(chutten)

I found https://firefox-source-docs.mozilla.org/toolkit/components/telemetry/telemetry/collection/events.html#testing which seems to have got me unblocked, by calling Services.telemetry.setEventRecordingEnabled("pwmgr", true); I now have the Events tab and can see the new entries being created as I hit each of those end points.

Sorry for the bugmail noise.

Flags: needinfo?(chutten)
User Story: (updated)
Attachment #9062505 - Attachment description: Bug 1543499 - (WIP) record telemetry event when opening the password management ui → Bug 1543499 - Record telemetry event when opening the password management UI. r?MattN
Blocks: 1550631

Do we need a new data review for this patch? We are replacing the PWMGR_MANAGE_OPENED histogram which previously recorded just 1 and 0 values (opening the password management UI with/without domain filter) with a pwmgr open_management telemetry event where the entry point (object) is one of a set of possible values: "autocomplete", "capturedoorhanger", "contextmenu", "pageinfo", "preferences", "mainmenu"

Also, is there an equivalent for "record_into_store": ["main", "pre-account"] for events?

Flags: needinfo?(chutten)
  1. Yes, you need a new data review. The data collection has changed as now we know when and more about what happened.
  2. No, there is no multi-store for Event Telemetry. (it wasn't in scope of the project)
Flags: needinfo?(chutten)

Here's the data collection review form filled out.
I'll also add you as a reviewer to the patch to verify that the Events.yaml and other changes agree with what I'm requesting here, and in case you spot other telemetry-related issues.

Flags: needinfo?(chutten)
Comment on attachment 9064121 [details]
data-collection-review.md

Data review for expanded telemetry for the password management UI
Flags: needinfo?(chutten)
Attachment #9064121 - Flags: data-review?(chutten)

It looks like the test failures I was getting from earlier try pushes were mostly artifact-build related. I tried the binary and I see a warning if I call Services.telemetry.setEventRecordingEnabled("pwmgr", true);

Unknown category for SetEventRecordingEnabled: pwmgr

..which sounds to me like the Events.yaml changes are not picked up. Pushing to try with --no-artifact [1] looks pretty clean.
I have one test-verify error. I see some skip-if = verify in the manifest file, I guess I need that for this test too?

TEST-UNEXPECTED-FAIL | toolkit/components/passwordmgr/test/browser/browser_entry_point_telemetry.js | Uncaught exception received from previously timed out test - at chrome://mochitests/content/browser/toolkit/components/passwordmgr/test/browser/browser_entry_point_telemetry.js:20 - ReferenceError: is is not defined

My only other open question is if I should handle a (hypothetical) call to LoginHelper.openPasswordManager with no entryPoint property. As the patch stands, that will throw and we'll get no password management UI. That might be exactly what we want. Alternately, I could catch and log the exception, leaving everything functional but with no symptoms that there was any problem. Or, I could provide a default value - which would probably be worse as we'd silently recover and have no way to know how we arrived in that state.

  1. https://treeherder.mozilla.org/#/jobs?repo=try&revision=01b97f39f47d356922ff7e677fa6eaade4abc89e&selectedJob=246200368

oops, need-info for comment 10.

Flags: needinfo?(MattN+bmo)

(In reply to Sam Foster [:sfoster] (he/him) from comment #10)

It looks like the test failures I was getting from earlier try pushes were mostly artifact-build related. I tried the binary and I see a warning if I call Services.telemetry.setEventRecordingEnabled("pwmgr", true);

Unknown category for SetEventRecordingEnabled: pwmgr

..which sounds to me like the Events.yaml changes are not picked up.

Did you look in the build logs to see if it was using an artifact from your revision?

You should see something like:

Installing from remote pushhead …

Pushing to try with --no-artifact [1] looks pretty clean.

I'm not sure if there is a difference in behaviour for artifact builds on try vs. integration to know if that means you're good to land.

I have one test-verify error. I see some skip-if = verify in the manifest file, I guess I need that for this test too?

TEST-UNEXPECTED-FAIL | toolkit/components/passwordmgr/test/browser/browser_entry_point_telemetry.js | Uncaught exception received from previously timed out test - at chrome://mochitests/content/browser/toolkit/components/passwordmgr/test/browser/browser_entry_point_telemetry.js:20 - ReferenceError: is is not defined

The failure is something that should be investigated. Normally that means that is is running after the test ended. Maybe you forgot an await?

My only other open question is if I should handle a (hypothetical) call to LoginHelper.openPasswordManager with no entryPoint property. As the patch stands, that will throw and we'll get no password management UI. That might be exactly what we want. Alternately, I could catch and log the exception, leaving everything functional but with no symptoms that there was any problem. Or, I could provide a default value - which would probably be worse as we'd silently recover and have no way to know how we arrived in that state.

I think throwing is what we want.

  1. https://treeherder.mozilla.org/#/jobs?repo=try&revision=01b97f39f47d356922ff7e677fa6eaade4abc89e&selectedJob=246200368
Flags: needinfo?(MattN+bmo)

(In reply to Matthew N. [:MattN] (PM me if requests are blocking you) from comment #12)

(In reply to Sam Foster [:sfoster] (he/him) from comment #10)

I have one test-verify error. I see some skip-if = verify in the manifest file, I guess I need that for this test too?

TEST-UNEXPECTED-FAIL | toolkit/components/passwordmgr/test/browser/browser_entry_point_telemetry.js | Uncaught exception received from previously timed out test - at chrome://mochitests/content/browser/toolkit/components/passwordmgr/test/browser/browser_entry_point_telemetry.js:20 - ReferenceError: is is not defined

The failure is something that should be investigated. Normally that means that is is running after the test ended. Maybe you forgot an await?

After reading this closer, the real issue is the timeout, not the "is is not defined".

Comment on attachment 9064121 [details]
data-collection-review.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.

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

Yes, :sfoster is responsible.

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

Category 2, Interaction.

    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?

No. This collection is permanent.

---
Result: datareview+
Attachment #9064121 - Flags: data-review?(chutten) → data-review+
Pushed by sfoster@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/263b8a0e6c34
Record telemetry event when opening the password management UI. r=MattN

Verification of each of the password management UI ("Saved Logins") entry points for this patch can be done using about:telemetry#events-tab, where using each entry point should generate a corresponding new entry under the "pwmgr" category:

  • Preferences:
    • Load about:preferences#privacy and under the "Logins and Passwords" sub-heading, click the Saved Logins... button
  • Autocomplete:
    • Place focus in the username or password field for a site where you have previously saved a login.
    • Click the "View Saved Logins" footer on the autocomplete menu that pops up.
  • MainMenu:
    • Use the "Logins and Passwords" menu item in the main ("hamburger") menu
  • PageInfo:
    • There's a couple of ways to get to the page info dialog:
      1. Load any page such as bugzilla.mozilla.org, click the "i" site information icon in the addressbar, click the "Connection" menu item to open the Site Security sub-panel, and click the "More Information" footer.
      2. Under the "Tools" application menubar, select Page Info
      3. Use keyboard shortcut ctrl + i
    • Under the Security panel of the page info dialog, click the "View Saved Passwords" button
  • ContextMenu:
    • Use any login form such as bugzilla.mozilla.org, and right-click on either the username or password field
    • Open the submenu under the "fill login" or "fill password" entry and click the "View Saved Logins" item
Status: ASSIGNED → RESOLVED
Closed: 7 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla68

Lief, I just wanted to draw this to your attention: this patch removes the PWMGR_MANAGE_OPENED histogram and replaces it with event telemetry under a new "pwmgr" category and "open_management" method.

Flags: needinfo?(loines)

thanks for letting me know. this will break a couple of queries but I think I can adapt them to start counting this in addition (for historical comparisons).

Flags: needinfo?(loines)

Got the following events registered by checking each entry point:

pwmgr open_management preferences
pwmgr open_management autocomplete
pwmgr open_management mainmenu
pwmgr open_management pageinfo
pwmgr open_management contextmenu

Verified - fixed on latest Nightly 68.0a1 (2019-05-17) (64-bit) on Windows 10 x64, Mac OS 10.14 and Ubuntu 18.04.

As a side note, the "ctr + i" shortcut for the Page info window works only on MacOS and Ubuntu.

Status: RESOLVED → VERIFIED
Flags: qe-verify+

(In reply to Leif Oines [:loines] from comment #20)

thanks for letting me know. this will break a couple of queries but I think I can adapt them to start counting this in addition (for historical comparisons).

I made a query that takes the union of the events and histograms and buckets them the same way: https://sql.telemetry.mozilla.org/queries/64793#165192

You need to log in before you can comment on or make changes to this bug.