Implement telemetry

Fixes #29.

* Opted to keep the telemetry calls in the redux middleware, and added
some actions that don't affect the redux store, but are only used by
the telemetry middleware. Seemed cleaner than giving various components
and containers knowledge of the telemetry library.

* Refactored the telemetry methods to use an options object instead of
positional args. In our new metrics, value is optional and sometimes
null, and extra is optional and sometimes null; and it seemed that
getting the positional order right could be bug-prone.

* Added embedded telemetry API experiment to work around recordEvent
bug in Firefox.

* Added an onReveal method to trigger an action, so we can count the
password reveal clicks via telemetry.

* Removed the `reconnecting_sync` event, because we don't yet show
any UI in the FxA menu in the error case.

* Set `record_on_release` to `true` for everything, because our users
are opting in by installing the addon (or clicking a button to enable
it).

* Added telemetry event info to pull request template.

docs/metrics.md

@@ -1,6 +1,6 @@
 # Lockbox Desktop Addon Metrics Plan
 
-_Last Updated: Feb 19, 2019_
+_Last Updated: Mar 20, 2019_
 
 <!-- TOC depthFrom:2 depthTo:6 withLinks:1 updateOnSave:1 orderedList:0 -->
 
@@ -68,34 +68,37 @@ Here's a breakdown of how a to register a typical event:
 
 
 ```javascript
-browser.telemetry.registerEvents("event_category", {
-    "event_name": {
+browser.telemetry.registerEvents("eventcategory", {
+    "eventName": {
         methods: ["click", ... ], // types of events that can occur
-        objects: ["a_button", ... ], // objects event can occur on
+        objects: ["aButton", ... ], // objects event can occur on
         extra: {"key": "value", ... } // key-value pairs (strings)
     }
 ```
 
 For our purposes, we will use the `extra` field for a few purposes:
 
-- To log the UUID of the item that has been added or changed (e.g. `"item_id": UUID`)
-- To log the fields that are modified when an item is updated in the datastore (e.g. `"fields": "username,passsword,hostname"` (because the value has to be a string we will have to concat the fields that were updated somehow)
+- To log the UUID of the item that has been added or changed (e.g. `"itemid": UUID`)
+  * TODO: Do we still want to do this? I removed the old datastore events, since they weren't in the list of planned events below.
+- To log the fields that are modified when an item is updated in the datastore (e.g. `"fields": "username,password,hostname"` (because the value has to be a string we will have to concat the fields that were updated somehow)
 
-Once an event is registered, we can record it with:
+Once an event is registered, we can record it with: (\*)
 
 `browser.telemetry.recordEvent(category, method, object, value, extra)`
 
 Note: The semantics of `value` is contingent on the event being recorded, see list below.
 
 See the Events section for specific examples of event registration and recording.
 
+\* Due to a bug in Firefox 67-, we will need to use a fallback embedded API experiment to correctly record telemetry events. See bug 1536877 for details.)
+
 #### Scalar recording
 
 We use the js api for scalar recording as well. Here registration happens with the following syntax:
 
 ```javascript
 browser.telemetry.registerScalars(category, {
-	"scalar_name": {
+	"scalarName": {
 		kind: browser.Telemetry.SCALAR_TYPE_COUNT, // SCALAR_TYPE_COUNT, SCALAR_TYPE_BOOLEAN. or SCALAR_TYPE_STRING
 		keyed: false,
 		record_on_release: false, // NEEDS TO BE SET TO RECORD ON RELEASE CHANNEL
@@ -124,23 +127,25 @@ These are the metrics we currently collect regarding the state of the user's log
 
 ## List of Planned Metrics Events
 
-All events are currently implemented under the **category: lockbox-addon**. The `extra` field contains `itemid` for events pertaining to a particular Lockbox item. They are listed and grouped together below based on the contents of the event's `method` field.
+All events are currently implemented under the **category: lockboxaddon**. The `extra` field contains `itemid` for events pertaining to a particular Lockbox item. They are listed and grouped together below based on the contents of the event's `method` field.
+
+1. `startup` fires when the webextension is loaded. **objects**: `webextension`. Note that this event fires whenever the browser is started, so is not indicative of direct user interaction with Lockbox. **value** is null.
 
-1. `startup` fires when the webextension is loaded. **objects**: webextension. Note that this event fires whenever the browser is started, so is not indicative of direct user interaction with Lockbox. **value** is null.
+2. `click` fires when someone clicks on one of the UI elements listed **objects**: `toolbar`, `getMobile`, `faq`, `accountSettings`, `giveFeedback`, `settingsMenu`, `signinSync`.  **value** is null. `sortMenu` has **value** of `lastUsed`, `lastChanged`, or `name`.
 
-2. `click` fires when someone clicks on one of the UI elements listed **objects**: `toolbar`, `get_mobile`, `faq`, `account_settings`, `give_feedback`, `settings_menu`, `reconnect_sync`, `signin_sync` (or whatever the menu is that contains links to account settings, faq, etc). **value** is null.
+3. `show` events fire when various UI elements are rendered shown to the user. **objects**: `itemListManager`, `itemListDoorhanger`, `itemDetailDoorhanger` `itemDetailManager`, `newItem`, `itemEdit`, `deleteConfirm`, `connectAnotherDevice` (referring to the dialog displayed after a user clicks on the button to learn about the mobile apps) **value** should be a boolean for `itemList*` events indicating whether any logins are in the login list (e.g. `False` if the list is empty).
 
-3. `show` events fire when various UI elements are rendered shown to the user. **objects**: `item_list_manager`, `item_list_doorhanger`, `item_detail_doorhanger` `item_detail_manager`, `new_item`, `item_edit`, `delete_confirm`, `connect_another_device` (referring to the dialog displayed after a user clicks on the button to learn about the mobile apps) **value** should be a boolean for `item_list_*` events indicating whether any logins are in the login list (e.g. `False` if the list is empty).
+4. `itemAdd`, `itemUpdate`, `itemDelete` fire after a successful adding, editing or deleting of a credential. **objects**: manager, doorhanger (contingent on where the user initiated the action). item GUID should be in the extra field. **value** is null
 
-4. `item_add`, `item_update`, `item_delete` fire after a successful adding, editing or deleting of a credential. **objects**: manager, doorhanger (contingent on where the user initiated the action). item GUID should be in the extra field. **value** is null
+5. `itemSelected` fires when a user clicks an item in the itemlist. **objects** manager, doorhanger  **value** is null
 
-5. `item_selected` fires when a user clicks an item in the itemlist. **objects** manager, doorhanger  **value** is null
+6. `copyPassword` and `copyUsername` fire when a user copies their username or password from an item. **objects**: `itemDetailManager`, `itemDetailDoorhanger` **value** is null. item GUID should be in the extra field.
 
-6. `copy_password` and `copy_username` fire when a user copies their username or password from an item. **objects**: `item_detail_manager`, `item_detail_doorhanger` **value** is null. item GUID should be in the extra field.
+7. `revealPassword` fires when a user reveals a password from within the item detail view. **objects**: `itemDetailManager`, `itemDetailDoorhanger` **value** is null item GUID should be in the extra field (or null, if the item is being created, and doesn't have an ID yet)
 
-7. `reveal_password` fires when a user reveals a password from within the item detail view. **objects**: `item_detail_manager`, `item_detail_doorhanger` **value** is null item GUID should be in the extra field.
+8. `concealPassword` fires when a user conceals a previously revealed password from within the item detail view. **objects**: `itemDetailManager`, `itemDetailDoorhanger` **value** is null item GUID should be in the extra field (or null, if the item is being created, and doesn't have an ID yet)
 
-8. `open_website` fires when a user clicks to open a credential's associated web address. **objects**: `item_detail_manager`, `item_detail_doorhanger` **value** is null item GUID should be in the extra field.
+8. `openWebsite` fires when a user clicks to open a credential's associated web address. **objects**: `itemDetailManager`, `itemDetailDoorhanger` **value** is null item GUID should be in the extra field.
 
 
 

docs/pull_request_template.md

@@ -26,3 +26,4 @@ _(Optional: to clearly demonstrate the feature or fix to help with testing and r
 - [ ] request the "UX" team perform a design review (if/when applicable)
 - [ ] make sure CI builds are passing (e.g.: fix lint and other errors)
 - [ ] check on the [accessibility](https://mozilla-lockbox.github.io/lockbox-addon/developer/test-plan-accessibility/) of any added UI
+- [ ] make sure any new UI has telemetry events wired up and documented in docs/metrics.md, and verify that the events are recording properly (visit `about:telemetry#events-tab`, then choose 'dynamic' from the dropdown menu at top right, to view events fired by the addon

package.json

@@ -101,7 +101,9 @@
     "run": {
       "pref": [
         "toolkit.telemetry.enabled=true",
-        "toolkit.telemetry.server=https://127.0.0.1/telemetry-dummy/"
+        "toolkit.telemetry.server=https://127.0.0.1/telemetry-dummy/",
+        "xpinstall.signatures.required=false",
+        "extensions.legacy.enabled=true"
       ]
     }
   },

src/background/README.md

@@ -37,3 +37,7 @@ Fetches the item with ID `id` from the datastore. Returns the item object in the
 ## `telemetry_event`
 
 Record a telemetry event with method `method`, object `object`, and extra `extra` (value is `null`).
+
+## `telemetry_add"
+
+Increment a telemetry counter with `name` by some `value`.

src/background/datastore.js

@@ -5,7 +5,6 @@
 import UUID from "uuid";
 
 import { broadcast } from "./message-ports";
-import telemetry from "./telemetry";
 
 export function convertInfo2Item(info) {
   let title;
@@ -92,19 +91,6 @@ export function convertItem2Info(item) {
   return info;
 }
 
-async function recordMetric(method, itemid, fields) {
-  let extra = {
-    itemid,
-  };
-  if (fields) {
-    extra = {
-      ...extra,
-      fields,
-    };
-  }
-  telemetry.recordEvent(method, "datastore", extra);
-}
-
 class DataStore {
   constructor() {
     this._all = {};
@@ -170,7 +156,6 @@ class DataStore {
     await browser.experiments.logins.add(added);
 
     added = convertInfo2Item(added);
-    recordMetric("added", added.id);
 
     return added;
   }
@@ -193,15 +178,13 @@ class DataStore {
     await browser.experiments.logins.update(updated);
 
     updated = convertInfo2Item(updated);
-    recordMetric("updated", item.id);
 
     return updated;
   }
   async remove(id) {
     const item = await this.get(id);
     if (item) {
       await browser.experiments.logins.remove(item.id);
-      recordMetric("deleted", item.id);
     }
     return item || null;
   }

src/background/message-ports.js

@@ -36,7 +36,7 @@ export default function initializeMessagePorts() {
     case "list_items":
       return openDataStore().then(async (ds) => {
         const entries = (await ds.list()).map(makeItemSummary);
-        telemetry.scalarSet("datastoreCount", entries.length);
+        telemetry.scalarSet({ name: "loginCount", value: entries.length });
         return { items: entries };
       });
     case "get_item":
@@ -58,18 +58,20 @@ export default function initializeMessagePorts() {
         await ds.remove(message.id);
         return {};
       });
-
     case "telemetry_event":
-        telemetry.recordEvent(message.method, message.object, message.extra);
+      telemetry.recordEvent(message.data);
       return {};
     case "telemetry_scalar":
-        telemetry.scalarSet(message.name, message.value);
-        return {};
+      telemetry.scalarSet({ name: message.name, value: message.value });
+      return {};
+    case "telemetry_add":
+      telemetry.scalarAdd({ name: message.name, value: message.value });
+      return {};
     case "copied_field":
-        await clipboard.copyToClipboard(message.field, message.toCopy);
-        return {};
+      await clipboard.copyToClipboard(message.field, message.toCopy);
+      return {};
     case "get_profile":
-        return getProfileInfo();
+      return getProfileInfo();
     default:
       return null;
     }