Closed Bug 1338713 Opened 7 years ago Closed 7 years ago

Basic telemetry for permissions notifications

Categories

(Toolkit :: Add-ons Manager, defect, P1)

Product:
Toolkit
Component:
Add-ons Manager
Version:
51 Branch
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: aswan, Assigned: aswan)

References

Details

(Whiteboard: metrics, triaged)

Attachments

(1 file)

Bug 1338713 Extension install telemetry
59 bytes, text/x-review-board-request
benjamin
: review+
rhelmer
: review+
Details 
Bug 1316074 outlines an extensive set of telemetry on add-on installations.  This bug is for a stripped-down set of measurements that we can use to gather data about the new permission prompts:
- Number of times permission prompts for installs from AMO / disco pane were displayed/accepted/rejected
- Number of times the third-party install warning (not the permission prompt) was displayed/accepted/rejected
- Number of times permission prompts for installs from a third-party site were displayed/accecpted/rejected
- Number of times prompts for sideloaded extensions were displayed/accepted/rejected
- Number of times prompts for extension permission updates were displayed/accepted/rejected
Priority: -- → P1
Whiteboard: metrics, triaged
Blocks: 1342142
It looks like we already have telemetry for the third-party install warning (in the SECURITY_UI histogram).
Which leaves us with permission prompts for various scenarios but we didn't cover all the corner cases that have emerged for permissions in the past weeks, including local file installation (both "Install from file..." and drag&drop) and installs from the search page.  I think the latter makes sense to throw together with "from AMO / disco pane".  Do we want a separate category for local files?  Frankly the simplest thing to do would be to also throw them in with "from AMO"...
Flags: needinfo?(amckay)
A seperate category for local installs would be great, although they have to be signed by AMO it would be interesting to know how many people use that facility.
Flags: needinfo?(amckay)
I will add tests and get a review from somebody familiar with the addon install flows before landing this but I don't expect the histogram definition to change, so requesting review for that now...
Attachment #8842601 - Flags: review?(benjamin)
Attachment #8842601 - Flags: review?(rhelmer)
Comment on attachment 8842601 [details]
Bug 1338713 Extension install telemetry

https://reviewboard.mozilla.org/r/116380/#review118060

::: browser/base/content/test/webextensions/head.js:310
(Diff revision 3)
>    //    accept the permissions to install the extension.  (Then uninstall
>    //    the extension to clean up.)
>    await runOnce(PERMS_XPI, false);
>  
> +  if (telemetryBase !== undefined) {
> +    // Should see 2 canceled installs followed by 1 successful install

Maybe this ship has sailed, but we should probably use `cancelled` or `canceled` - observer notifications use e.g. `addon-install-cancelled` so settling on that one would be good.

It makes search and search/replace sort of activities difficult, and we don't really have any alternative without type safety.

::: browser/base/content/test/webextensions/head.js:354
(Diff revision 3)
> +  let originalHistogram = ExtensionsUI.histogram;
> +  ExtensionsUI.histogram = {
> +    add(value) { collectedTelemetry.push(value); },
> +  };
> +  registerCleanupFunction(() => {
> +    is(collectedTelemetry.length, 0, "No unexamined telemetry after test is finished");

nice

::: browser/modules/ExtensionsUI.jsm:32
(Diff revision 3)
>  const BROWSER_PROPERTIES = "chrome://browser/locale/browser.properties";
>  const BRAND_PROPERTIES = "chrome://branding/locale/brand.properties";
>  
>  const HTML_NS = "http://www.w3.org/1999/xhtml";
>  
> +// Keys for the telemtry histogram are integers with 0 in the lowest bit

s/telemtry/telemetry/

::: browser/modules/ExtensionsUI.jsm:165
(Diff revision 3)
> +      if (info.type == "sideload") {
> +        histkey = HIST_KEY_SIDELOAD;
> +      } else if (info.type == "update") {
> +        histkey = HIST_KEY_UPDATE;
> +      } else {
> +        switch (info.source) {

I don't see the benefit to using `switch` here... why not just nested `if`:

```
if (info.type == "sideload") {
...
} else {
  if (info.source == "AMO") {...}
  ...
  } else {
    histkey = HIST_KEY_WEB_INSTALL_OTHER;
  }
}
```

It doesn't seem any less readable and avoids the "forgot to put in a `break`" hazard, especially for future modification.

Maybe more readable to have an object which maps info types to histogram keys and just use the `info.type` as the index.

::: toolkit/components/telemetry/Histograms.json:11060
(Diff revision 3)
>      "bug_numbers": [1307689],
>      "description": "Records a value each time a tab that is showing the loading throbber is interrupted. Desktop only.",
>      "labels": ["stop", "back", "forward", "historyNavigation", "reload", "tabClosed", "newURI"]
> +  },
> +  "EXTENSION_INSTALL_PROMPT_RESULT": {
> +    "alert_emails": ["aswan@mozilla.com", "andym@mozilla.com"],

would it be better to set up an alias for this, or do these usually go to individuals?
Attachment #8842601 - Flags: review?(rhelmer) → review+
Comment on attachment 8842601 [details]
Bug 1338713 Extension install telemetry

https://reviewboard.mozilla.org/r/116380/#review118060

> Maybe this ship has sailed, but we should probably use `cancelled` or `canceled` - observer notifications use e.g. `addon-install-cancelled` so settling on that one would be good.
> 
> It makes search and search/replace sort of activities difficult, and we don't really have any alternative without type safety.

Yeah all the actual symbols in code use cancelled but this is just a comment, I couldn't bring myself to misspell it there.

> I don't see the benefit to using `switch` here... why not just nested `if`:
> 
> ```
> if (info.type == "sideload") {
> ...
> } else {
>   if (info.source == "AMO") {...}
>   ...
>   } else {
>     histkey = HIST_KEY_WEB_INSTALL_OTHER;
>   }
> }
> ```
> 
> It doesn't seem any less readable and avoids the "forgot to put in a `break`" hazard, especially for future modification.
> 
> Maybe more readable to have an object which maps info types to histogram keys and just use the `info.type` as the index.

This is kind of clunky but depending on the case, `type` or `source` could be the key.  `type` is already used to control formatting of the messages in the notification so I don't want to change its behavior here, `source` was just added for the case of a regular interactive install to distinguish where the thing being installed is coming from.
But I did get rid of the switch and just extend the if chain.

> would it be better to set up an alias for this, or do these usually go to individuals?

I don't really know, I see a healthy mix of individuals and lists throughout that file.  I'm fine forwarding alerts as needed to whoever is appropriate.
Comment on attachment 8842601 [details]
Bug 1338713 Extension install telemetry

https://reviewboard.mozilla.org/r/116380/#review119288

I want to see the improved description. The rest of this is fine (and I'd suggest actually expiring this later... 60 perhaps, since we really want to see what happens to this over the 57 window where we're supporting webextensions only.

::: toolkit/components/telemetry/Histograms.json:11065
(Diff revision 4)
> +    "alert_emails": ["aswan@mozilla.com", "andym@mozilla.com"],
> +    "bug_numbers": [1338713],
> +    "expires_in_version": "58",
> +    "kind": "enumerated",
> +    "n_values": 20,
> +    "description": "Results of displaying add-on installation notifications.",

From this description, I can't tell what values are actually going to be recorded here. You either need to include the list of values here (and maybe used a categorical histogram instead of enumerated). Or you need to reference (link to?) an existing enumeration of values.
Attachment #8842601 - Flags: review?(benjamin) → review-
And you should be aware that alerts will not be helpful for this. Alerts really only work for scalar performance measures, *and* only process opt-in/prerelease users. To look at this measure you will need more custom queries, but I assume that Kev asked for this and is working with your data pod on the reporting.
Comment on attachment 8842601 [details]
Bug 1338713 Extension install telemetry

https://reviewboard.mozilla.org/r/116380/#review119288

> From this description, I can't tell what values are actually going to be recorded here. You either need to include the list of values here (and maybe used a categorical histogram instead of enumerated). Or you need to reference (link to?) an existing enumeration of values.

I switched the histogram to categorical, are the labels sufficiently descriptive or should I create separate documentation somewhere (I'm slightly wary about the latter due to the likelihood of it falling out of sync with the implementation)?
Comment on attachment 8842601 [details]
Bug 1338713 Extension install telemetry

https://reviewboard.mozilla.org/r/116380/#review119368

I think I know what all these values mean just by reading them, so that should be enough. data-r=me (I did not review the code)
Attachment #8842601 - Flags: review?(benjamin) → review+
Pushed by aswan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c1e2141e8efe
Extension install telemetry r=bsmedberg,rhelmer
https://hg.mozilla.org/mozilla-central/rev/c1e2141e8efe
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Verified in FF Nightly 55.0a1 (2017-04-07) (64-bit) in Win7 (64-bit)
The telemetry is correctly recorded in the EXTENSION_INSTALL_PROMPT_RESULT histogram.
The following scenarios were tested and are working properly:
Accept/reject permissions when installing from about:addons - ok
Accept/reject permissions when installing a local file - ok
Accept/reject permissions when installing the AMO website (I have used -dev environment addons-dev.allizom.org) - ok
Accept/reject permissions when installing a webextension when using sideloading method - ok
Accept/reject permissions when updating a webextensions.

I have a few questions:
1. There is a scenario when you update from v1 of a webextension to v2 and both have exactly the same permissions. In this scenario the permissions doorhanger is not displayed but the add-on is updated (this is the expected result) and the event is not recorded in the EXTENSION_INSTALL_PROMPT_RESULT histogram. Is this what we want? Should't we also record this scenario in the histogram?

2. The histogram has the following components that I could find
0 - installAmoAccepted	
1 - installAmoRejected 	
2 -  installLocalAccepted	
3 - installLocalRejected	
4 - installWebAccepted	
5 - installWebRejected	
6 - sideloadAccepted	
7 - sideloadRejected	
8 - updateAccepted	
9 - updateRejected	

but when testing I have observed that there is a 10th bar displayed in the histogram. Am I missing something? Is there a specific info that is recorded and displayed in the 10th bar? (https://www.screencast.com/t/oktDIAT180)

3. When installing a webextension from about:addons we record the data in the field called installAmoAccepted and when installing from the addons.mozilla.org website we record it in the installWebAccepted. Giving the fact that addons.mozilla.org has the AMO abbreviation I think that there might be confusion when we record data in the  installAmoAccepted when installing from about:addons.
Flags: needinfo?(aswan)
(In reply to Madalin Cotetiu from comment #17)
> 1. There is a scenario when you update from v1 of a webextension to v2 and
> both have exactly the same permissions. In this scenario the permissions
> doorhanger is not displayed but the add-on is updated (this is the expected
> result) and the event is not recorded in the EXTENSION_INSTALL_PROMPT_RESULT
> histogram. Is this what we want? Should't we also record this scenario in
> the histogram?

What would you do with that information?  The goal here was just to get information about how often users are seeing the prompts and how they are responding.  We have another bug (1316074) with much more extensive telemetry for various aspects of extension installation where that information could conceivably be gathered, but I think the big thing missing from that bug at this time is a clear vision for what we want to measure and what we'll do with the resulting data.

> 2. The histogram has the following components that I could find
> [...]	
> 
> but when testing I have observed that there is a 10th bar displayed in the
> histogram. Am I missing something? Is there a specific info that is recorded
> and displayed in the 10th bar? (https://www.screencast.com/t/oktDIAT180)

There's not actually a value associated with that bar, it looks to me like a quirk with how the charts are rendered.

> 3. When installing a webextension from about:addons we record the data in
> the field called installAmoAccepted and when installing from the
> addons.mozilla.org website we record it in the installWebAccepted. Giving
> the fact that addons.mozilla.org has the AMO abbreviation I think that there
> might be confusion when we record data in the  installAmoAccepted when
> installing from about:addons.

Sorry I don't understand the concern here.  When you say "installing from about:addons", do you mean from the discovery pane ("Get Add-ons" in the left menu)?
Flags: needinfo?(aswan)
(In reply to Andrew Swan [:aswan] from comment #18)
> (In reply to Madalin Cotetiu from comment #17)
> > 1. There is a scenario when you update from v1 of a webextension to v2 and
> > both have exactly the same permissions. In this scenario the permissions
> > doorhanger is not displayed but the add-on is updated (this is the expected
> > result) and the event is not recorded in the EXTENSION_INSTALL_PROMPT_RESULT
> > histogram. Is this what we want? Should't we also record this scenario in
> > the histogram?
> 
> What would you do with that information?  The goal here was just to get
> information about how often users are seeing the prompts and how they are
> responding.  We have another bug (1316074) with much more extensive
> telemetry for various aspects of extension installation where that
> information could conceivably be gathered, but I think the big thing missing
> from that bug at this time is a clear vision for what we want to measure and
> what we'll do with the resulting data.

If we record that information somewhere else I think we are ok. My feeling was that we don't record at all that scenario.


> > 3. When installing a webextension from about:addons we record the data in
> > the field called installAmoAccepted and when installing from the
> > addons.mozilla.org website we record it in the installWebAccepted. Giving
> > the fact that addons.mozilla.org has the AMO abbreviation I think that there
> > might be confusion when we record data in the  installAmoAccepted when
> > installing from about:addons.
> 
> Sorry I don't understand the concern here.  When you say "installing from
> about:addons", do you mean from the discovery pane ("Get Add-ons" in the
> left menu)?

Yes I was talking about discover pane. Sorry for the confusion.
My concern is that there might be confusion because when installing from disco pane we record the data in the installAmoAccepted and the AMO abbreviation comes from addons.mozilla.org. Some people might think that the data in that column comes from installing webextensions from the addons.mozilla.org website.
(In reply to Madalin Cotetiu from comment #19)
> (In reply to Andrew Swan [:aswan] from comment #18)
> > (In reply to Madalin Cotetiu from comment #17)
> > > 1. There is a scenario when you update from v1 of a webextension to v2 and
> > > both have exactly the same permissions. In this scenario the permissions
> > > doorhanger is not displayed but the add-on is updated (this is the expected
> > > result) and the event is not recorded in the EXTENSION_INSTALL_PROMPT_RESULT
> > > histogram. Is this what we want? Should't we also record this scenario in
> > > the histogram?
> > 
> > What would you do with that information?  The goal here was just to get
> > information about how often users are seeing the prompts and how they are
> > responding.  We have another bug (1316074) with much more extensive
> > telemetry for various aspects of extension installation where that
> > information could conceivably be gathered, but I think the big thing missing
> > from that bug at this time is a clear vision for what we want to measure and
> > what we'll do with the resulting data.
> 
> If we record that information somewhere else I think we are ok. My feeling
> was that we don't record at all that scenario.

Well again, we're not recording that information but as a general principle, we don't just gather data without some plan to do something with it and we don't yet have any plans to do anything with this sort of data.  All we have is a plan to make a plan :-)

> > > 3. When installing a webextension from about:addons we record the data in
> > > the field called installAmoAccepted and when installing from the
> > > addons.mozilla.org website we record it in the installWebAccepted. Giving
> > > the fact that addons.mozilla.org has the AMO abbreviation I think that there
> > > might be confusion when we record data in the  installAmoAccepted when
> > > installing from about:addons.
> > 
> > Sorry I don't understand the concern here.  When you say "installing from
> > about:addons", do you mean from the discovery pane ("Get Add-ons" in the
> > left menu)?
> 
> Yes I was talking about discover pane. Sorry for the confusion.
> My concern is that there might be confusion because when installing from
> disco pane we record the data in the installAmoAccepted and the AMO
> abbreviation comes from addons.mozilla.org. Some people might think that the
> data in that column comes from installing webextensions from the
> addons.mozilla.org website.

I still don't understand the problem, the discovery pane is just a different way to get at these add-ons, they still come from AMO.
(In reply to Madalin Cotetiu from comment #17)

> 1. There is a scenario when you update from v1 of a webextension to v2 and
> both have exactly the same permissions. In this scenario the permissions
> doorhanger is not displayed but the add-on is updated (this is the expected
> result) and the event is not recorded in the EXTENSION_INSTALL_PROMPT_RESULT
> histogram. Is this what we want? Should't we also record this scenario in
> the histogram?
If we decide that we need this kind of data I think the implementation can be tracked in bug 1316074.

> 3. When installing a webextension from about:addons we record the data in
> the field called installAmoAccepted and when installing from the
> addons.mozilla.org website we record it in the installWebAccepted. Giving
> the fact that addons.mozilla.org has the AMO abbreviation I think that there
> might be confusion when we record data in the  installAmoAccepted when
> installing from about:addons.
I'm still thinking that this naming conversion might be confusing for some but giving the fact that this information will be used only by specialized people we should be ok.

Marking bug as verified fixed.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: