Closed Bug 1462725 Opened 6 years ago Closed 6 years ago

Add SavantShieldStudy.jsm and "savant" event telemetry category

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

(9 files, 9 obsolete files)

Bug 1462725 - Init Savant module at Firefox startup with study pref;
59 bytes, text/x-review-board-request
rhelmer
: review+
RyanVM
: approval-mozilla-beta+
Details
Bug 1462725 - Add pref-controlled logging to Savant module;
59 bytes, text/x-review-board-request
rhelmer
: review+
Details
Bug 1462725 - Add TelemetryEvents class to Savant JSM;
59 bytes, text/x-review-board-request
rhelmer
: review+
Details
Bug 1462725 - Dupe 'search' event probe under 'savant' category;
59 bytes, text/x-review-board-request
rhelmer
: review+
Details
Bug 1462725 - Rename study module;
59 bytes, text/x-review-board-request
rhelmer
: review+
Details
Bug 1462725 - Uninit study on Firefox shutdown;
59 bytes, text/x-review-board-request
rhelmer
: review+
Details
Bug 1462725 - Add eligibility check;
59 bytes, text/x-review-board-request
rhelmer
: review+
Details
Bug 1462725 - Add study expiration and testing override pref;
59 bytes, text/x-review-board-request
rhelmer
: review+
Details
Bug 1462725 - Add 'end_study' event;
59 bytes, text/x-review-board-request
rhelmer
: review+
Details 
This is the first of a series of bugs that together comprise the Savant Shield study (tracking bug 1457226).

In order to add subsequent probes, we need to lay the groundwork. This includes:
* adding the SavantShieldStudy.jsm module
* adding a "savant" event telemetry category
  - All probes for this study will be registered under this category.
* adding a duplicate of the "navigation" category's search probe
  - This isolates the probe, so that this study’s use of the probe does not affect and is not affected by other uses of the probe.
Blocks: 1457226
Summary: Add SavantShieldSavant.jsm and "savant" event telemetry category → Add SavantShieldStudy.jsm and "savant" event telemetry category
Comment on attachment 8979948 [details]
Bug 1462725 - Init Savant module at Firefox startup with study pref;

https://reviewboard.mozilla.org/r/246140/#review252624

::: browser/app/profile/firefox.js:1743
(Diff revision 1)
>  pref("app.shield.optoutstudies.enabled", false);
>  #endif
> +
> +// Savant Shield study preferences
> +pref("shield.savant.enabled", false);
> +pref("shield.savant.loglevel", "debug");

Should this be `"warn"` by default?
Attachment #8979948 - Flags: review?(rhelmer)
Comment on attachment 8979949 [details]
Bug 1462725 - Add pref-controlled logging to Savant module;

https://reviewboard.mozilla.org/r/246142/#review252628
Attachment #8979949 - Flags: review?(rhelmer) → review+
Comment on attachment 8979950 [details]
Bug 1462725 - Add TelemetryEvents class to Savant JSM;

https://reviewboard.mozilla.org/r/246144/#review252630
Attachment #8979950 - Flags: review?(rhelmer) → review+
Comment on attachment 8979951 [details]
Bug 1462725 - Dupe 'search' event probe under 'savant' category;

https://reviewboard.mozilla.org/r/246146/#review252634
Attachment #8979951 - Flags: review?(rhelmer) → review+
Comment on attachment 8979952 [details]
Bug 1462725 - Rename study module;

https://reviewboard.mozilla.org/r/246148/#review252636
Attachment #8979952 - Flags: review?(rhelmer) → review+
Comment on attachment 8979948 [details]
Bug 1462725 - Init Savant module at Firefox startup with study pref;

https://reviewboard.mozilla.org/r/246140/#review252638

Oh, I see this was done in a later commit. I wouldn't worry next time too much about showing your work in each patch; the important part is that any patch in isolation not break anything, but you can edit down the set of patches locally into some logical progression if you want (there's no rule that you *have* to split into multiple patches, just if it aids clarity)
Attachment #8979948 - Flags: review+
Comment on attachment 8979953 [details]
Bug 1462725 - Uninit study on Firefox shutdown;

https://reviewboard.mozilla.org/r/246150/#review252642
Attachment #8979953 - Flags: review?(rhelmer) → review+
Comment on attachment 8979954 [details]
Bug 1462725 - Add eligibility check;

https://reviewboard.mozilla.org/r/246152/#review252644
Attachment #8979954 - Flags: review?(rhelmer) → review+
Comment on attachment 8979955 [details]
Bug 1462725 - Add study expiration and testing override pref;

https://reviewboard.mozilla.org/r/246154/#review252646

::: browser/modules/SavantShieldStudy.jsm:96
(Diff revision 1)
> +    // * 60 seconds/minute * 1000 milliseconds/second
> +    const studyDurationInMs =
> +      this.getDurationFromPref()
> +      || (this.STUDY_DURATION_WEEKS * 7 * 24 * 60 * 60 * 1000);
> +    const expirationDateInt = now + studyDurationInMs;
> +    Services.prefs.setStringPref(

Why a string pref and not int?

::: browser/modules/SavantShieldStudy.jsm:108
(Diff revision 1)
> +    return Services.prefs.getIntPref(this.STUDY_DURATION_OVERRIDE_PREF, 0);
> +  }
> +
> +  isStudyExpired() {
> +    const expirationDateInt =
> +      Date.parse(Services.prefs.getStringPref(this.STUDY_EXPIRATION_DATE_PREF));

What happens if this pref is not set (not present) or is set incorrectly?

I think as-is `Services.prefs.getStringPref` will throw... you could pass a second arg that is the default, or just wrap a `try`/`catch` so this returns `false` correctly

Oh also same question as above re: string vs int pref
Attachment #8979955 - Flags: review?(rhelmer)
Comment on attachment 8979956 [details]
Bug 1462725 - Add 'end_study' event;

https://reviewboard.mozilla.org/r/246156/#review252654

::: browser/modules/SavantShieldStudy.jsm:54
(Diff revision 1)
>        this.shouldCollect = !this.shouldCollect;
>        if (this.shouldCollect) {
>          this.startupStudy();
>        } else {
>          // The pref has been turned off
> -        this.endStudy("study-disabled");
> +        this.endStudy("study_disable");

Hm too bad these strings need to be duplicated, would be nice if there was a way to get these values from the `Events.yaml` via `nsITelemetry` or something, I don't see it from a quick look.
Attachment #8979956 - Flags: review?(rhelmer) → review+
Comment on attachment 8979950 [details]
Bug 1462725 - Add TelemetryEvents class to Savant JSM;

chutten: Is there a way to get event values from Events.yaml, to prevent the need for string duplication?
Flags: needinfo?(chutten)
rhelmer: Here is the try run for this patch with the study pref turned ON.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8e22090f04d4b7a32d82087885865e8051573e75&selectedJob=180107265

The same tests were run with the study pref turned OFF in Review Board (https://treeherder.mozilla.org/#/jobs?repo=try&revision=96a5245e9a5cf67fff6b433e20d2086210635825).
Flags: needinfo?(rhelmer)
Comment on attachment 8979956 [details]
Bug 1462725 - Add 'end_study' event;

https://reviewboard.mozilla.org/r/246156/#review252654

> Hm too bad these strings need to be duplicated, would be nice if there was a way to get these values from the `Events.yaml` via `nsITelemetry` or something, I don't see it from a quick look.

Yeah I don't see any examples of pulling events from Events.yaml; the "navigation" and "devtools.main" also hard code their event strings when recording events.
Comment on attachment 8979955 [details]
Bug 1462725 - Add study expiration and testing override pref;

https://reviewboard.mozilla.org/r/246154/#review252646

> Why a string pref and not int?

This is mostly from a QA/development perspective. It's easy for me to tell from a date string that the study expiration is set correctly than to read the number of milliseconds elapsed since 1 January 1970 00:00:00 UTC and convert it into something human readable. I am unsure of a way to do this with an int pref.
Comment on attachment 8979955 [details]
Bug 1462725 - Add study expiration and testing override pref;

https://reviewboard.mozilla.org/r/246154/#review252646

> What happens if this pref is not set (not present) or is set incorrectly?
> 
> I think as-is `Services.prefs.getStringPref` will throw... you could pass a second arg that is the default, or just wrap a `try`/`catch` so this returns `false` correctly
> 
> Oh also same question as above re: string vs int pref

Good point! I added a default value as a second argument in the third revision of this commit.
(In reply to Bianca Danforth [:bdanforth] from comment #29)
> Comment on attachment 8979950 [details]
> Bug 1462725 - Add TelemetryEvents class to Savant JSM;
> 
> chutten: Is there a way to get event values from Events.yaml, to prevent the
> need for string duplication?

I don't understand, what strings are being duplicated and in what way would event values help?
Flags: needinfo?(chutten)
(In reply to Chris H-C :chutten from comment #36)
> (In reply to Bianca Danforth [:bdanforth] from comment #29)
> > Comment on attachment 8979950 [details]
> > Bug 1462725 - Add TelemetryEvents class to Savant JSM;
> > 
> > chutten: Is there a way to get event values from Events.yaml, to prevent the
> > need for string duplication?
> 
> I don't understand, what strings are being duplicated and in what way would
> event values help?

Sorry that was not clear. What I am referring to are the event categories, methods, objects, etc. that are registered in Events.yaml. Is there a way for me to pull those string values from Events.yaml when I call `recordEvent` in a JSM, instead of having to duplicate those string literals in the JSM (e.g. `Services.telemetry.recordEvent("savant", "end_study", "study_disable");`?

This is not a blocker, I am just wondering if that's an option and if not, if it's worth adding in the future.
Flags: needinfo?(chutten)
It is not currently an option, no. 

I wonder what an API based on that would look like... Mapping strings to constants would look too wordy I think:

 Services.telemetry.recordEvent(Services.telemetry.Events.savant, Services.telemetry.Events.savant.end_study, Services.telemetry.Events.savant.end_study.study_disable, <the extras>);

Maybe if we put calls to record on namespace-y kinds of things:

 Services.telemetry.Events.savant.end_study.study_disable.record(<the extras>);

Ultimately it's all strings in the backend, but maybe there are efficiencies to be had if we didn't have to translate the strings from JS.

I'm not sure how to register that number of JS APIs efficiently, though. I'm not sure we're set up to handle such a thing... and it would put the burden of AOU setup firmly at the beginning of the application startup which is an expensive place to be.

I think we're likely to leave it as-is. String literals in JS are cheap enough in 2018 that we shouldn't have to be clever.
Flags: needinfo?(chutten)
(In reply to Chris H-C :chutten from comment #38)
> It is not currently an option, no. 
> 
> I wonder what an API based on that would look like... Mapping strings to
> constants would look too wordy I think:
> 
>  Services.telemetry.recordEvent(Services.telemetry.Events.savant,
> Services.telemetry.Events.savant.end_study,
> Services.telemetry.Events.savant.end_study.study_disable, <the extras>);
> 
> Maybe if we put calls to record on namespace-y kinds of things:
> 
>  Services.telemetry.Events.savant.end_study.study_disable.record(<the
> extras>);
> 
> Ultimately it's all strings in the backend, but maybe there are efficiencies
> to be had if we didn't have to translate the strings from JS.
> 
> I'm not sure how to register that number of JS APIs efficiently, though. I'm
> not sure we're set up to handle such a thing... and it would put the burden
> of AOU setup firmly at the beginning of the application startup which is an
> expensive place to be.
> 
> I think we're likely to leave it as-is. String literals in JS are cheap
> enough in 2018 that we shouldn't have to be clever.

I agree it's probably not worth changing, the reason I brought it up in review is less about the efficiency angle and more that having multiple copies of the same string tends to lead to silent failures when there's a typo.

For instance in a lot of Firefox code we prefer setting a string (like the name of a pref) in a constant once, then if the constant is typo'd somewhere a `ReferenceError` is thrown... otherwise you get bugs like silently reading and writing from two different prefs in different parts of the code etc.
Flags: needinfo?(rhelmer)
Comment on attachment 8979955 [details]
Bug 1462725 - Add study expiration and testing override pref;

https://reviewboard.mozilla.org/r/246154/#review252774

::: browser/modules/SavantShieldStudy.jsm:112
(Diff revision 3)
> +    return new Date(expirationDateInt).toISOString();
> +  }
> +
> +  isStudyExpired() {
> +    const expirationDateInt =
> +      Date.parse(Services.prefs.getStringPref(

Keeping your comment in mind about wanting this to be a string in the prefs so it's human readable and be used by QA (which makes sense to me):

What do you want to happen here if the string can't be parsed by `Date.parse()`? Currently this function will return `Date.now() > NaN` which is false, so there won't be any user-visible failure.

Probably worth catching this and reporting to the console that there is something wrong with the date, since it's likely to be a human fiddling with it that's causing the problem.

Seems important enough to be logged as an `error` and maybe re-throw too, since otherwise the study will never expire right?
Attachment #8979955 - Flags: review?(rhelmer)
Comment on attachment 8979955 [details]
Bug 1462725 - Add study expiration and testing override pref;

https://reviewboard.mozilla.org/r/246154/#review252774

> Keeping your comment in mind about wanting this to be a string in the prefs so it's human readable and be used by QA (which makes sense to me):
> 
> What do you want to happen here if the string can't be parsed by `Date.parse()`? Currently this function will return `Date.now() > NaN` which is false, so there won't be any user-visible failure.
> 
> Probably worth catching this and reporting to the console that there is something wrong with the date, since it's likely to be a human fiddling with it that's causing the problem.
> 
> Seems important enough to be logged as an `error` and maybe re-throw too, since otherwise the study will never expire right?

Thanks rhelmer for pointing this out. I have updated the code to log a descriptive error if the result from `Date.parse()` is `NaN`.
I have resolved all known issues from my Firefox Peer reviewer, rhelmer.

pdehaan: Can you QA review this patch for landing in the tree? I have updated the TESTPLAN (https://github.com/biancadanforth/gecko-dev/issues/5) to include instructions and some functional testing tasks.
Flags: needinfo?(pdehaan)
chutten: Can you data review this patch for landing in the tree? I added two new probes to Events.yaml: `search` and `end_study`.
Flags: needinfo?(chutten)
Comment on attachment 8979955 [details]
Bug 1462725 - Add study expiration and testing override pref;

https://reviewboard.mozilla.org/r/246154/#review253162

You can take-or-leave my review comment, no need to r? again in either case. Good work!

::: browser/modules/SavantShieldStudy.jsm:120
(Diff revision 4)
> +      ));
> +
> +    if (isNaN(expirationDateInt)) {
> +      log.error(
> +        `The value for the preference ${this.STUDY_EXPIRATION_DATE_PREF} is invalid.`
> +      );

I'd probably just do an early `return false` here as well so you're not depending on the logic outside of this scope (which could change), up to you though.
Attachment #8979955 - Flags: review?(rhelmer) → review+
(In reply to Bianca Danforth [:bdanforth] from comment #52)
> chutten: Can you data review this patch for landing in the tree? I added two
> new probes to Events.yaml: `search` and `end_study`.

Sure. Just follow the instructions over here: https://wiki.mozilla.org/Firefox/Data_Collection

When you fill out the request form (you can just copy-paste it into a bug comment if you'd like), ni? me again and I'll take a look.
Flags: needinfo?(chutten)
(In reply to Chris H-C :chutten from comment #54)
> (In reply to Bianca Danforth [:bdanforth] from comment #52)
> > chutten: Can you data review this patch for landing in the tree? I added two
> > new probes to Events.yaml: `search` and `end_study`.
> 
> Sure. Just follow the instructions over here:
> https://wiki.mozilla.org/Firefox/Data_Collection
> 
> When you fill out the request form (you can just copy-paste it into a bug
> comment if you'd like), ni? me again and I'll take a look.

Since this is one of many bugs in a series for a single Shield study (called "Savant", tracking bug 1457226; see https://bugzilla.mozilla.org/show_bug.cgi?id=1457226#c9), does it make sense to complete this form once for all proposed probes there? Otherwise we would submit about 8 very similar forms for each component's bug.
Flags: needinfo?(chutten)
(In reply to Bianca Danforth [:bdanforth] from comment #64)
> (In reply to Chris H-C :chutten from comment #54)
> > (In reply to Bianca Danforth [:bdanforth] from comment #52)
> > > chutten: Can you data review this patch for landing in the tree? I added two
> > > new probes to Events.yaml: `search` and `end_study`.
> > 
> > Sure. Just follow the instructions over here:
> > https://wiki.mozilla.org/Firefox/Data_Collection
> > 
> > When you fill out the request form (you can just copy-paste it into a bug
> > comment if you'd like), ni? me again and I'll take a look.
> 
> Since this is one of many bugs in a series for a single Shield study (called
> "Savant", tracking bug 1457226; see
> https://bugzilla.mozilla.org/show_bug.cgi?id=1457226#c9), does it make sense
> to complete this form once for all proposed probes there? Otherwise we would
> submit about 8 very similar forms for each component's bug.

Up to you how many you'd like in each review, but Data Review must come before you land any probes that collect data from users.
Flags: needinfo?(chutten)
Blocks: 1465685
Blocks: 1465694
Blocks: 1465697
Blocks: 1465698
Blocks: 1465703
Blocks: 1465704
Blocks: 1465707
I've downloaded and installed the custom build, run through the functional tests, and reviewed the diff via https://github.com/mozilla/gecko-dev/compare/master...biancadanforth:savant.
So far everything looks good from QA standpoint, so R+ from me, and I'll continue testing on Monday.

Great job, :bdanforth!
Flags: needinfo?(pdehaan)
Assignee: nobody → bdanforth
Status: NEW → ASSIGNED
Attachment #8979948 - Attachment is obsolete: true
Attachment #8979949 - Attachment is obsolete: true
Attachment #8979950 - Attachment is obsolete: true
Attachment #8979951 - Attachment is obsolete: true
Attachment #8979952 - Attachment is obsolete: true
Attachment #8979953 - Attachment is obsolete: true
Attachment #8979954 - Attachment is obsolete: true
Attachment #8979955 - Attachment is obsolete: true
Attachment #8979956 - Attachment is obsolete: true
Comment on attachment 8983243 [details]
Bug 1462725 - Init Savant module at Firefox startup with study pref;

https://reviewboard.mozilla.org/r/249122/#review255328
Attachment #8983243 - Flags: review?(rhelmer) → review+
Comment on attachment 8983244 [details]
Bug 1462725 - Add pref-controlled logging to Savant module;

https://reviewboard.mozilla.org/r/249124/#review255330
Attachment #8983244 - Flags: review?(rhelmer) → review+
Comment on attachment 8983245 [details]
Bug 1462725 - Add TelemetryEvents class to Savant JSM;

https://reviewboard.mozilla.org/r/249126/#review255332
Attachment #8983245 - Flags: review?(rhelmer) → review+
Comment on attachment 8983246 [details]
Bug 1462725 - Dupe 'search' event probe under 'savant' category;

https://reviewboard.mozilla.org/r/249128/#review255334
Attachment #8983246 - Flags: review?(rhelmer) → review+
Comment on attachment 8983247 [details]
Bug 1462725 - Rename study module;

https://reviewboard.mozilla.org/r/249130/#review255336
Attachment #8983247 - Flags: review?(rhelmer) → review+
Comment on attachment 8983248 [details]
Bug 1462725 - Uninit study on Firefox shutdown;

https://reviewboard.mozilla.org/r/249132/#review255338
Attachment #8983248 - Flags: review?(rhelmer) → review+
Comment on attachment 8983249 [details]
Bug 1462725 - Add eligibility check;

https://reviewboard.mozilla.org/r/249134/#review255340
Attachment #8983249 - Flags: review?(rhelmer) → review+
Comment on attachment 8983250 [details]
Bug 1462725 - Add study expiration and testing override pref;

https://reviewboard.mozilla.org/r/249136/#review255342
Attachment #8983250 - Flags: review?(rhelmer) → review+
Comment on attachment 8983251 [details]
Bug 1462725 - Add 'end_study' event;

https://reviewboard.mozilla.org/r/249138/#review255344
Attachment #8983251 - Flags: review?(rhelmer) → review+
Pushed by rhelmer@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ac7e4d209fb4
Init Savant module at Firefox startup with study pref; r=rhelmer
https://hg.mozilla.org/integration/autoland/rev/227bfae843a7
Add pref-controlled logging to Savant module; r=rhelmer
https://hg.mozilla.org/integration/autoland/rev/cd6cbad8bafd
Add TelemetryEvents class to Savant JSM; r=rhelmer
https://hg.mozilla.org/integration/autoland/rev/5cb316ef4aab
Dupe 'search' event probe under 'savant' category; r=rhelmer
https://hg.mozilla.org/integration/autoland/rev/59a1d61466cc
Rename study module; r=rhelmer
https://hg.mozilla.org/integration/autoland/rev/4760d154118b
Uninit study on Firefox shutdown; r=rhelmer
https://hg.mozilla.org/integration/autoland/rev/109ec8a11ab8
Add eligibility check; r=rhelmer
https://hg.mozilla.org/integration/autoland/rev/21631d8f5133
Add study expiration and testing override pref; r=rhelmer
https://hg.mozilla.org/integration/autoland/rev/04b38008d657
Add 'end_study' event; r=rhelmer
Priority: -- → P1
Comment on attachment 8983243 [details]
Bug 1462725 - Init Savant module at Firefox startup with study pref;

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 #8983243 - Flags: approval-mozilla-beta?
Comment on attachment 8983243 [details]
Bug 1462725 - Init Savant module at Firefox startup with study pref;

Telemetry code needed to support the Fx61 Savant work. Approved for 61.0b13.
Attachment #8983243 - 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: