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

Status: VERIFIED FIXED

(Toolkit :: Password Manager, enhancement, P1)

Description

[Matthew N. [:MattN]]

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.

Comment 1

[Sam Foster [:sfoster] (he/him)]

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.

Comment 2

[Matthew N. [:MattN]]

(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")

Comment 3

[Sam Foster [:sfoster] (he/him)]

Attachment: Bug 1543499 - Record telemetry event when opening the password management UI. r?MattN

Comment 4

[Sam Foster [:sfoster] (he/him)]

: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)

Comment 5

[Sam Foster [:sfoster] (he/him)]

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.

Comment 6

[Sam Foster [:sfoster] (he/him)]

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?

Comment 7

[Chris H-C :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)

Comment 8

[Sam Foster [:sfoster] (he/him)]

Attachment: data-collection-review.md

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.

Comment 9

[Sam Foster [:sfoster] (he/him)]

Comment on attachment 9064121 [details]
data-collection-review.md

Data review for expanded telemetry for the password management UI

Comment 10

[Sam Foster [:sfoster] (he/him)]

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

Comment 11

[Sam Foster [:sfoster] (he/him)]

oops, need-info for comment 10.

Comment 12

[Matthew N. [:MattN]]

(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

Comment 13

[Matthew N. [:MattN]]

(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 14

[Chris H-C :chutten]

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+

Comment 15

[Sam Foster [:sfoster] (he/him)]

try results (finally) look good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ade7b051dcdf206abc1aaca5978bdee44f5026ed

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/263b8a0e6c34
Record telemetry event when opening the password management UI. r=MattN

Comment 17

[Sam Foster [:sfoster] (he/him)]

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

Comment 18

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/263b8a0e6c34

Comment 19

[Sam Foster [:sfoster] (he/him)]

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.

Comment 20

[Leif Oines [: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).

Comment 21

[Timea Cernea [:tbabos][inactive]]

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.

Comment 22

[Matthew N. [:MattN]]

(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