1465707 - Add addon event telemetry for Savant Shield study

Status: RESOLVED FIXED

(Firefox :: General, enhancement, P1)

Description

[Bianca Danforth [:bdanforth]]

This is one of a series of bugs that together comprise the Savant Shield study (tracking bug 1457226).

This bug will register and record (for the duration of the study only):
* When an addon begins an install
* When an addon finishes an install
* When an addon is enabled
* When an addon is disabled
* When an addon begins an uninstall
* When an addon finishes an uninstall

Comment 1

[Bianca Danforth [:bdanforth]]

Attachment: Bug 1465707 - Add addon probes for Savant Shield study;

When the study preference (shield.savant.enabled) is set to true, this will record:
* When an addon begins an install
* When an addon finishes an install
* When an addon is enabled
* When an addon is disabled
* When an addon begins an uninstall
* When an addon finishes an uninstall

Review commit: https://reviewboard.mozilla.org/r/249540/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/249540/

Comment 2

[Bianca Danforth [:bdanforth]]

:pdehaan, could you provide QA review on this patch? Instructions for obtaining the test build are here: https://github.com/biancadanforth/gecko-dev/issues/5

Comment 3

[Bianca Danforth [:bdanforth]]

I neglected to mention that for the event telemetry to show up in `about:telemetry#events-tab`, the study pref `shield.savant.enabled` needs to be set to `true`. Normandy will take care of turning this pref ON and OFF when the study is officially launched.

Comment 4

[Robert Helmer [:rhelmer]]

Comment on attachment 8983714 [details]
Bug 1465707 - Add addon probes for Savant Shield study;

https://reviewboard.mozilla.org/r/249540/#review255960

This looks ok to me overall, I noticed a few small things to fix. I'd like aswan to take a look if he has time just to make sure I am not missing anything.

::: browser/modules/SavantShieldStudy.jsm:195
(Diff revision 1)
> +    this.METHOD = "addon";
> +    this.EXTRA_SUBCATEGORY = "customize";
> +  }
> +
> +  init() {
> +    // TODO: for each event, record addon_id in `extra` or `value` fiels IFF

s/fiels/fields/ ?

::: browser/modules/SavantShieldStudy.jsm:195
(Diff revision 1)
> +    this.METHOD = "addon";
> +    this.EXTRA_SUBCATEGORY = "customize";
> +  }
> +
> +  init() {
> +    // TODO: for each event, record addon_id in `extra` or `value` fiels IFF

Did you want to resolve this TODO before landing or is that for later?

::: browser/modules/SavantShieldStudy.jsm:201
(Diff revision 1)
> +    // it is one of stakeholder's top X from a list; can't record ALL addon_ids,
> +    // since that would be category 3 data.
> +    this.listener = {
> +      onInstalling: (addon, needsRestart) => {
> +        const addon_id = addon.id;
> +        log.debug(`Addon with ID ${addon_id} installing.`);

I'd just put a single `log.debug` in the `recordEvent` function if you want to keep it so it doesn't need to be duplicated here.

Comment 5

[Andrew Swan [:aswan]]

Comment on attachment 8983714 [details]
Bug 1465707 - Add addon probes for Savant Shield study;

If you want some eyes on this from the addons side, I think Luca would be great.

Comment 6

[Robert Helmer [:rhelmer]]

Comment on attachment 8983714 [details]
Bug 1465707 - Add addon probes for Savant Shield study;

I don't think we need to bother Luca with this, was just out for a bit and wasn't sure I'd get to it in time. lgtm with the suggested changes on my review.

Comment 7

[Peter deHaan [:pdehaan]]

I think everything looks good. I randomly installed the "[e-mail]" add-on since it was in about:addons.
I get an "install_start", "install_finish", "disable", "enable", "disable" (these last 3 were me just pushing buttons in about:addons), and then a "remove_start". I only got a "remove_finish" after I refreshed the about:addons page after disabling.

Oddly, I don't seem to have gotten an initial "enable" event after the "install_finish" event.
I'll try again with a different add-on.

Comment 8

[Bianca Danforth [:bdanforth]]

Comment on attachment 8983714 [details]
Bug 1465707 - Add addon probes for Savant Shield study;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/249540/diff/1-2/

Comment 9

[Robert Helmer [:rhelmer]]

(In reply to Peter deHaan [:pdehaan] from comment #7)
> I think everything looks good. I randomly installed the
> "[e-mail]" add-on since it was in about:addons.
> I get an "install_start", "install_finish", "disable", "enable", "disable"
> (these last 3 were me just pushing buttons in about:addons), and then a
> "remove_start". I only got a "remove_finish" after I refreshed the
> about:addons page after disabling.

This is expected it has to do with how the undo feature is implemented.
Closing the about:addons tab should do it as well.

> Oddly, I don't seem to have gotten an initial "enable" event after the
> "install_finish" event.
> I'll try again with a different add-on.

Comment 10

[Bianca Danforth [:bdanforth]]

(In reply to Peter deHaan [:pdehaan] from comment #7)
> I think everything looks good. I randomly installed the
> "[e-mail]" add-on since it was in about:addons.
> I get an "install_start", "install_finish", "disable", "enable", "disable"
> (these last 3 were me just pushing buttons in about:addons), and then a
> "remove_start". I only got a "remove_finish" after I refreshed the
> about:addons page after disabling.
> 
> Oddly, I don't seem to have gotten an initial "enable" event after the
> "install_finish" event.
> I'll try again with a different add-on.

Per kmaglione, one of the primary maintainers for the Add-ons Manager component, there should not be an "enable" event fired after add-on install.

Does this mean I have your QA r+?

Comment 11

[Peter deHaan [:pdehaan]]

You can have all my QA r+'s.

Comment 12

[Peter deHaan [:pdehaan]]

also, bugzilla would be much funner if I could embed whimsical animated GIFs.

Comment 13

[Mozilla Autoland]

We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 19639656754207c160fcb8b4c0b67d38a76e8267 -d f718887bdbc3: rebasing 467360:196396567542 "Bug 1465707 - Add addon probes for Savant Shield study; r=rhelmer" (tip)
merging toolkit/components/telemetry/Events.yaml
warning: conflicts while merging toolkit/components/telemetry/Events.yaml! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)

Comment 14

[Bianca Danforth [:bdanforth]]

Comment on attachment 8983714 [details]
Bug 1465707 - Add addon probes for Savant Shield study;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/249540/diff/2-3/

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/267912246b07
Add addon probes for Savant Shield study; r=rhelmer

Comment 16

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/267912246b07

Comment 17

[Bianca Danforth [:bdanforth]]

Comment on attachment 8983714 [details]
Bug 1465707 - Add addon probes for Savant Shield study;

Approval Request Comment
[Feature/Bug causing the regression]: There is no regression. The tracking bug for this study is 1457226.
[User impact if declined]: None. This patch is part of a priority Shield study.
[Is this code covered by automated tests?]: There are no study-specific tests, but all code has been run on the try server for regression testing and has been approved by both a Firefox peer and QA (pdehaan).
[Has the fix been verified in Nightly?]: The study’s functionality has been verified in Nightly.
[Needs manual test from QE? If yes, steps to reproduce]: No.
[List of other uplifts needed for the feature/fix]: This request applies to all patches in this bug. In general, here is a comprehensive list of bugs whose patches need to be uplifted (in this order) to complete the study: 1462725, 1465698, 1465707, 1465703, 1465704, 1465697, 1465685, 1465694
[Is the change risky?]: No.
[Why is the change risky/not risky?]: This is an observational pref-flip study with no branches and no UI. It adds some event telemetry probes in various places and a study module to enable/disable collection. Each patch has been QA tested and R+ by pdehaan.
[String changes made/needed]: None.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8983714 [details]
Bug 1465707 - Add addon probes for Savant Shield study;

Telemetry code needed to support the Fx61 Savant work. Approved for 61.0b13.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/f28277267cb2