Closed Bug 1465707 Opened 6 years ago Closed 6 years ago

Add addon event telemetry for Savant Shield study

Categories

(Firefox :: General, enhancement, P1)

Product:
Firefox
Component:
General
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 62
Tracking Flags:
Tracking Status
firefox61 --- fixed
firefox62 --- fixed

People

(Reporter: bdanforth, Assigned: bdanforth)

References

Details

Attachments

(1 file)

Bug 1465707 - Add addon probes for Savant Shield study;
59 bytes, text/x-review-board-request
rhelmer
: review+
RyanVM
: approval-mozilla-beta+
Details 
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
Blocks: 1457226
Depends on: 1462725
Assignee: nobody → bdanforth
Status: NEW → ASSIGNED
Priority: -- → P1
: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
Flags: needinfo?(pdehaan)
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 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.
Attachment #8983714 - Flags: review?(rhelmer) → review+
Attachment #8983714 - Flags: review?(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.
Attachment #8983714 - Flags: review?(aswan) → review?(lgreco)
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.
Attachment #8983714 - Flags: review?(lgreco)
Attachment #8983714 - Flags: review?(aswan)
Attachment #8983714 - Flags: review?(aswan)
I think everything looks good. I randomly installed the "treestyletab@piro.sakura.ne.jp" 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.
Flags: needinfo?(pdehaan)
(In reply to Peter deHaan [:pdehaan] from comment #7)
> I think everything looks good. I randomly installed the
> "treestyletab@piro.sakura.ne.jp" 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.
(In reply to Peter deHaan [:pdehaan] from comment #7)
> I think everything looks good. I randomly installed the
> "treestyletab@piro.sakura.ne.jp" 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+?
Flags: needinfo?(pdehaan)
You can have all my QA r+'s.
Flags: needinfo?(pdehaan)
also, bugzilla would be much funner if I could embed whimsical animated GIFs.
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)
Pushed by rhelmer@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/267912246b07
Add addon probes for Savant Shield study; r=rhelmer
https://hg.mozilla.org/mozilla-central/rev/267912246b07
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox62: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 62
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.
Attachment #8983714 - Flags: approval-mozilla-beta?
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.
Attachment #8983714 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: