Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

edit metrics doc #82

Merged
merged 3 commits into from Feb 21, 2019

Conversation

@irrationalagent
Copy link
Contributor

commented Feb 19, 2019

Connected to #29 and #5

Testing and Review Notes

These are the metrics I'm proposing for the first iteration of the addon.

I based the event schemas on the invision designs. If there's other nomenclature that is better compatible with what's in the code, I'm fine with that.

Also, note that a couple of scalars that I proposed might pose technical hurdles. Happy to talk about that.

I also left out the monitor bits for now.

@irrationalagent irrationalagent requested a review from mozilla-lockwise/desktop-engineering as a code owner Feb 19, 2019

@ghost ghost assigned irrationalagent Feb 19, 2019

@ghost ghost added the in progress label Feb 19, 2019

@linuxwolf
Copy link
Member

left a comment

Overall this looks great, and understandable.

There's a few questions inline, and one (reconnect_sync) that might want to be changed.

docs/metrics.md Show resolved Hide resolved
@@ -94,13 +79,13 @@ Services.telemetry.registerEvents("event_category", {
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": "password,notes"` (because the value has to be a string we will have to concat the fields that were updated somehow)
- To log the fields that are modified when an item is updated in the datastore (e.g. `"fields": "username,passsword,address"` (because the value has to be a string we will have to concat the fields that were updated somehow)

This comment has been minimized.

Copy link
@linuxwolf

linuxwolf Feb 19, 2019

Member

For clarification: this address refers to the "Website address" in the UI prototypes?

This comment has been minimized.

Copy link
@irrationalagent

irrationalagent Feb 19, 2019

Author Contributor

yes, I should change it to hostname

2. `iconClick` fires when someone clicks the toolbar icon. **objects**: toolbar
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`, `reconnect_sync` **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).

This comment has been minimized.

Copy link
@linuxwolf

linuxwolf Feb 19, 2019

Member

For clarification: connect_another_device is for when the in-content dialog is opened?

This comment has been minimized.

Copy link
@linuxwolf

linuxwolf Feb 19, 2019

Member

Note that "sign into Sync" and "reconnect Sync" take the user outside of Lockbox and into Firefox preference views.

I don't think we can technically record a show event for reconnect_sync, though we could record a click event.

docs/metrics.md Outdated Show resolved Hide resolved
- `loginCount` (SCALAR_TYPE_COUNT). Current count of the number of items in the user's login storage. If possible, this scalar should be set upon opening either the doorhanger or full extension interface.
- `loginsAccessedAllTime` (SCALAR_TYPE_COUNT). Running total of logins accessed (copied, revealed) through the extension interface or doorhanger. Should never be reset.

This comment has been minimized.

Copy link
@linuxwolf

linuxwolf Feb 19, 2019

Member

it's not clear to me, from the interface descriptions, how possible either this or loginsAccessed is from the Telemetry WebExtension API. One of them definitely is, but it's not clear which; not clear how much effort it will take to have both.

This comment has been minimized.

Copy link
@6a68

6a68 Feb 20, 2019

Member

I'm pretty sure that scalarAdd increments a per-browsing session counter, so loginsAccessedAllTime would require saving the all-time count in webextension browser storage (not that hard to do).

@linuxwolf

This comment has been minimized.

Copy link
Member

commented Feb 19, 2019

Would still very much want to get @6a68 thoughts.

@6a68
Copy link
Member

left a comment

Looks great! Added a few comments for you 👍


Instead we will define our events by **registering** them using a call to `Services.telemetry.registerEvents(category, eventData)`.
Events are defined by **registering** them using a call to `Services.telemetry.registerEvents(category, eventData)`.

This comment has been minimized.

Copy link
@6a68

6a68 Feb 20, 2019

Member

Not sure if it matters, but the registration happens via browser.telemetry, though that's just a passthru to Services.telemetry 🤷‍♂️

These are the metrics we currently collect regarding the state of user datastores.
These are the metrics we currently collect regarding the state of the user's login storage.
- `loginCount` (SCALAR_TYPE_COUNT). Current count of the number of items in the user's login storage. If possible, this scalar should be set upon opening either the doorhanger or full extension interface.

This comment has been minimized.

Copy link
@6a68

6a68 Feb 20, 2019

Member

We can definitely trigger this ping when the user opens the doorhanger or loads the management page.

Would it also be useful to check this value if the user doesn't interact with Lockbox? For instance, we could check this value at browser startup, or periodically (whenever Firefox is open).

This comment has been minimized.

Copy link
@linuxwolf

linuxwolf Feb 20, 2019

Member

Relaying from an earlier IRL conversation when I asked exactly this: these metrics are also measuring as user's engagement with Lockbox specifically, so it's preferable to limit to triggers specific to interacting with Lockbox.

This comment has been minimized.

Copy link
@irrationalagent

irrationalagent Feb 20, 2019

Author Contributor

Yes +1 to Matt, trying to limit pings with extension data to those who are actually using it (makes counting things like "active" users a bit easier).

For our internal alpha release, we will be making use of the public JavaScript API that allows recording and sending of event data and scalar data through an add-on. The API is documented here:

https://firefox-source-docs.mozilla.org/toolkit/components/telemetry/telemetry/collection/events.html#the-api
To record Telemetry we will be making use of the public JavaScript API that allows recording and sending of [events](https://firefox-source-docs.mozilla.org/toolkit/components/telemetry/telemetry/collection/events.html#public-js-api) and [scalar measurements](https://firefox-source-docs.mozilla.org/toolkit/components/telemetry/telemetry/collection/scalars.html#js-api) through a webextension.

This comment has been minimized.

Copy link
@6a68

6a68 Feb 20, 2019

Member

About the use of the term 'public' here: the telemetry API is only exposed to Mozilla addons; 'Mozilla privileged' might be more accurate.

- `loginCount` (SCALAR_TYPE_COUNT). Current count of the number of items in the user's login storage. If possible, this scalar should be set upon opening either the doorhanger or full extension interface.
- `loginsAccessedAllTime` (SCALAR_TYPE_COUNT). Running total of logins accessed (copied, revealed) through the extension interface or doorhanger. Should never be reset.

This comment has been minimized.

Copy link
@6a68

6a68 Feb 20, 2019

Member

I'm pretty sure that scalarAdd increments a per-browsing session counter, so loginsAccessedAllTime would require saving the all-time count in webextension browser storage (not that hard to do).

- `loginsAccessedAllTime` (SCALAR_TYPE_COUNT). Running total of logins accessed (copied, revealed) through the extension interface or doorhanger. Should never be reset.
- `loginsAccessed` (SCALAR_TYPE_COUNT). Total of logins accessed (copied, revealed) since last ping through the extension interface or doorhanger. TODO: in native code scalars are automatically reset when a new ping is cut. Will that happen for us?

This comment has been minimized.

Copy link
@6a68

6a68 Feb 20, 2019

Member

This should be easy on a per-session basis.

I believe the pings should be reset the same way as native code, since the browser.telemetry APIs are just a passthrough to the Services.telemetry APIs. @georgf might know for sure.

This comment has been minimized.

Copy link
@georgf

georgf Feb 20, 2019

That’s correct, they reset per ping/subsession.

The semantics for the web extension Telemetry API are the same, it’s just a different way to access them.

This comment has been minimized.

Copy link
@irrationalagent

irrationalagent Feb 20, 2019

Author Contributor

Thanks for the clarification Georg. I'm going to remove the All-time scalar for now, this is relatively straightforward to compute on a per-client basis. It also mitigates some of the anonymity issues discussed with Jared.

docs/metrics.md Show resolved Hide resolved
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.
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.

This comment has been minimized.

Copy link
@6a68

6a68 Feb 20, 2019

Member

Do we want a separate event if users do things with the keyboard instead of the mouse? (Accessibility and keyboard-centric power users)

This comment has been minimized.

Copy link
@irrationalagent

irrationalagent Feb 20, 2019

Author Contributor

Good question. I don't see myself utilizing that information, but if it would help engineering to have that data I'm happy to add it. And I should say more broadly, if there is any data that engineering would find useful to collect, we should add that.

This comment has been minimized.

Copy link
@6a68

6a68 Feb 21, 2019

Member

I guess it's really a question of whether product would find the key vs. click (vs. tap, since some desktops have touch screens...) distinction useful or not.

This comment has been minimized.

Copy link
@linuxwolf

linuxwolf Feb 21, 2019

Member

@mozilla-lockbox/product is the distinction (click vs tap vs keypress) useful to track?

This comment has been minimized.

Copy link
@sandysage

sandysage Feb 21, 2019

It'd be cool to track but I don't think it's going to give us relevant insights on their own. For now, we probably want to just track that someone did the thing. We can always layer in the distinction at a later time when we have a specific need to identify the methods by which people navigate.

@linuxwolf
Copy link
Member

left a comment

All of my concerns are addressed, and it appears all of @6a68's are also.

Approving, though we might want to give some time for @mozilla-lockbox/product to think about how granular we want to be in tracking user interaction (click vs tap vs keypress).

@6a68 6a68 force-pushed the metrics-md branch from 9eaea23 to 20c7535 Feb 21, 2019

@ghost ghost assigned 6a68 Feb 21, 2019

@6a68

This comment has been minimized.

Copy link
Member

commented Feb 21, 2019

Rebased and force-pushed.

@linuxwolf linuxwolf merged commit 4d15207 into master Feb 21, 2019

5 checks passed

WIP Ready for review
Details
codecov/patch Coverage not affected when comparing dacf0cf...20c7535
Details
codecov/project 95.9% remains the same compared to dacf0cf
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@ghost ghost removed the in progress label Feb 21, 2019

@linuxwolf linuxwolf deleted the metrics-md branch May 22, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.