1688653 - Add telemetry in LoginCSVImport.jsm for results of imported logins

Status: VERIFIED FIXED

(Firefox :: about:logins, task, P2)

Description

[Tim Giles [:tgiles]]

When importing logins via CSV, these logins can be assigned a success result ("no_change", "modified", "added") or an error result ("error"). To help determine the common cases users are encountering, it would be beneficial to keep track of the number of these successes and failures.

Comment 1

[Andrei Petcu]

How should I implement this?

      Services.telemetry
        .getKeyedHistogramById("FX_MIGRATION_LOGINS_QUANTITY")
        .add(LoginCSVImport.MIGRATION_HISTOGRAM_KEY, summary.length);
      Services.telemetry
        .getKeyedHistogramById("FX_MIGRATION_LOGINS_SUCCESS")
        .add(LoginCSVImport.MIGRATION_HISTOGRAM_KEY, successCount);
      Services.telemetry
        .getKeyedHistogramById("FX_MIGRATION_LOGINS_ERROR")
        .add(LoginCSVImport.MIGRATION_HISTOGRAM_KEY, errorCount);

or...

      Services.telemetry
        .getKeyedHistogramById("FX_MIGRATION_LOGINS_QUANTITY")
        .add(LoginCSVImport.MIGRATION_HISTOGRAM_KEY, {total: summary.length, successCount, errorCount});

Comment 2

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

I owe you an answer to comment 1, : petcuandrei, need-info myself to get this to you next week.

Comment 3

[Andrei Petcu]

Note to self: In LoginCSVImport.jsm we also use the wrong telemetry key.
Here is Sam's comment that I need to take into consideration int this bug also https://phabricator.services.mozilla.com/D100639?id=403480#inline-593524

The description for this histogram is ""How many logins (passwords) we imported from another browser, 
keyed by the name of the browser." but we are recording the length of the report. So even if we imported 
0 logins, our telemetry would show the the total number of rows in the CSV. Can you file a bug to follow
up - we'll need to decide if this should be just the "added" logins, or if the count we record should include
 "updated" logins. I think the raw number is useful, but we should also record the successful logins imported.

Comment 4

[Andrei Petcu]

Other relevant comments
chutten:

Me, I'd have a "categorical" histogram with a few labels "success" "error" "other", then
accumulate to those labels. You can find the Guide for adding a new probe to Firefox
Telemetry here: https://firefox-source-docs.mozilla.org/toolkit/components/telemetry/start/adding-a-new-probe.html

https://matrix.to/#/!RaZlNLTJhRVCaDrapT:mozilla.org/$rvmJjj2Dbh8rzYkfdjKigdM9kpUf5JdZn_EnW0pYe7E?via=mozilla.org

I need to add Travis or chutten as revieers https://matrix.to/#/!RaZlNLTJhRVCaDrapT:mozilla.org/$UuNoMWMCy4Az-iWSC6IiE5VOmmTxBodWqtzIGHPXJpQ?via=mozilla.org

Comment 5

[Andrei Petcu]

Am I the best person to request data review? I don't really know why we need this data and for how long do we want to collect it.

https://wiki.mozilla.org/Data_Collection#Requesting_Data_Collection

Comment 6

[Andrei Petcu]

Attachment: Bug 1688653 Add telemetry in LoginCSVImport.jsm for results of imported logins

Comment 7

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

(In reply to petcuandrei from comment #5)

Am I the best person to request data review? I don't really know why we need this data and for how long do we want to collect it.

https://wiki.mozilla.org/Data_Collection#Requesting_Data_Collection

I can do the data review request, once we're clear on what we want to collect and how to collect it.
We just collided mid-air as I was submitting this comment. I think something like this is what :chutten is suggesting, and looks like you ended up in the same place:

diff --git a/toolkit/components/telemetry/Histograms.json b/toolkit/components/telemetry/Histograms.json
--- a/toolkit/components/telemetry/Histograms.json
+++ b/toolkit/components/telemetry/Histograms.json
@@ -7892,6 +7892,16 @@
     "keyed": true,
     "description": "How long it took to import logins (passwords) from another browser, keyed by the name of the browser. The time for users to unlock Keychain on macOS is included in this time."
   },
+  "FX_MIGRATION_LOGINS_IMPORT_RESULTS": {
+    "record_in_processes": ["main"],
+    "products": ["firefox"],
+    "alert_emails": ["[e-mail]", "[e-mail]"],
+    "expires_in_version": "92",
+    "kind": "categorical",
+    "labels": ["error", "error_missing_field", "no_change", "added", "modified", ""],
+    "bug_numbers": [1688653],
+    "description": "Count results when attempting to import logins from CSV file"
+  },
   "FX_MIGRATION_BOOKMARKS_JANK_MS": {
     "record_in_processes": ["main"],
     "products": ["firefox"],

Though, the FX_MIGRATION namespace isn't quite right here. Maybe it should be PWMGR_CSV_IMPORT_RESULTS? Either way, lets leave FX_MIGRATION_LOGINS_QUANTITY alone so it is at least consistent.

Comment 8

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

Attachment: data-collection-review-bug-1688653.md

I amended the boilerplate answer for "7) How long will this data be collected?" as I'm not sure 6 months will give us enough data. The probes in Histograms.json as set to expire in version 99. But if that's a problem, I can amend.

Comment 9

[Chris H-C :chutten]

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

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?

No. This collection will expire in Firefox 99.

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

Category 1, Technical.

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. :sfoster is responsible for renewing or removing the collection before it expires in Firefox 99.

---

Result: datareview+

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/39ae40980dda
Add telemetry in LoginCSVImport.jsm for results of imported logins r=tgiles,sfoster

Comment 11

[Narcis Beleuzu [:NarcisB]]

Backed out for bustages on Histograms.json

Backout link: https://hg.mozilla.org/integration/autoland/rev/a48085099ca1c61fd0013455b98243e518006f2b
Log link: https://treeherder.mozilla.org/logviewer?job_id=333309500&repo=autoland&lineNumber=3056

Comment 12

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

Oops, that's my bad - trailing comma in the Histograms.json and I didn't realize we don't lint this as JSON. I'll get this :petcuandrei.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1e574d5e2fb2
Add telemetry in LoginCSVImport.jsm for results of imported logins r=tgiles,sfoster

Comment 14

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/1e574d5e2fb2

Comment 15

[Simona Rosu [:srosu], Ecosystem QA]

I have started verifying this issue using the latest Firefox Nightly 88.0a1 (Build ID: 20210318093109) on Windows 10 x64, Ubuntu 20.04 and macOS 11.1.

• In order to verify this bug, I have tested the following scenarios:

1. The “PWMGR_IMPORT_LOGINS_FROM_FILE_MS”, “PWMGR_IMPORT_LOGINS_FROM_FILE_JANK_MS” and “PWMGR_IMPORT_LOGINS_FROM_FILE_CATEGORICAL” histograms are successfully registered in the browser after importing a CSV file that contains multiple valid logins, existing logins, updated logins, invalid logins and duplicates. However, I have observed that the values for the “PWMGR_IMPORT_LOGINS_FROM_FILE_CATEGORICAL” histogram are incremented with the numbers of valid logins, existing logins updated, errors and duplicates which are displayed in the “Import Complete” modal.
2. The “pwmgr#mgmt_menu_item_used#import_csv_complete” keyed scalar is successfully registered after importing a valid CSV file.
3. The “pwmgr#mgmt_menu_item_used#import_from_csv” keyed scalar is successfully registered in the browser after clicking on the “Import from a File…” option from the Ellipsis menu.

@Tim, @Andrei could you please let us know if there are other scenarios or telemetry that we should verify for this patch?

Comment 16

[Tim Giles [:tgiles]]

:srosu, thanks for the quick update. Those scenarios are the only one I can think of that involve this telemetry. As for your point about the histogram being incremented with the number of valid logins, existing logins, etc., that's the expected behavior.

Comment 17

[Simona Rosu [:srosu], Ecosystem QA]

Thank you for the quick response. Based on comment 15 and comment 16 from above, I’m marking this issue as Verified Fixed.