Closed Bug 1192811 Opened 9 years ago Closed 9 years ago

Add TelemetryEnvironment pref collections that don't trigger a subsession split

Categories

(Toolkit :: Telemetry, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla43
Tracking Status
firefox42 --- wontfix
firefox43 --- fixed

People

(Reporter: gfritzsche, Assigned: robertthyberg, Mentored)

References

Details

(Whiteboard: [lang=js] [good next bug])

Attachments

(1 file, 5 obsolete files)

Bug 1185061 shows a use-case for collecting preferences that only affect behavior on startup and who require a restart after changes.

Those should be collected in the TelemetryEnvironment only on startup and we should not trigger subsession changes when they change.

We should just change the pref definitions here:
https://dxr.mozilla.org/mozilla-central/rev/d6ea652c579992daa9041cc9718bb7c6abefbc91/toolkit/components/telemetry/TelemetryEnvironment.jsm#82
... to be of the form:
> ...
> ["dom.ipc.processCount", {what: RECORD_PREF_VALUE, requiresRestart: true}],
> ...
Where |requiresRestart| is optional.

We can then skip adding & removing pref observers if that entry has |requiresRestart| set here:
https://dxr.mozilla.org/mozilla-central/rev/d6ea652c579992daa9041cc9718bb7c6abefbc91/toolkit/components/telemetry/TelemetryEnvironment.jsm#784
https://dxr.mozilla.org/mozilla-central/rev/d6ea652c579992daa9041cc9718bb7c6abefbc91/toolkit/components/telemetry/TelemetryEnvironment.jsm#802

To check the changes, run:
> mach xpcshell-test toolkit/components/telemetry/tests/unit
Some of the tests will need updating with this change; at the least test_TelemetryEnvironment.

We should extend test-coverage to cover |requiresRestart| around here:
https://dxr.mozilla.org/mozilla-central/rev/d6ea652c579992daa9041cc9718bb7c6abefbc91/toolkit/components/telemetry/tests/unit/test_TelemetryEnvironment.js#637
Georg: this could be a binary flag that we binary-OR with the RECORD_PREF value

Robert: are you interested in picking up this Telemetry bug next?
Flags: needinfo?(robertthyberg)
(In reply to Vladan Djeric (:vladan) -- please needinfo! from comment #1)
> Georg: this could be a binary flag that we binary-OR with the RECORD_PREF
> value

Do you think that's worth it? I was considering that, but i'd much prefer the readability of a separate property unless we have a pressing reason.
yeah I would like to work on this. Thanks
Flags: needinfo?(robertthyberg)
Assignee: nobody → robertthyberg
so I think I understand what needs to be done but please clarify. I need to add a what: and a requiresrestart: to each of the pref defintions. Then alter the code that starts and stops the pref observers to reflect the change we made to the definitions. Im not sure what would be needed in the test file.
(In reply to rthyberg from comment #4)
> so I think I understand what needs to be done but please clarify. I need to
> add a what: and a requiresrestart: to each of the pref defintions.

Yes - note that we don't need to add |requiresRestart| to each entry, i would only add it where we use |requiresRestart:true| (i.e. for now just for "dom.ipc.processCount").

The linked test already tests some pref variations; we can add a PREF_TEST_5 there that tests the requiresRestart behavior.

We would check that the pref is set correctly in the environment data initially here:
https://dxr.mozilla.org/mozilla-central/rev/d6ea652c579992daa9041cc9718bb7c6abefbc91/toolkit/components/telemetry/tests/unit/test_TelemetryEnvironment.js#661

Then trigger a change to it around here:
https://dxr.mozilla.org/mozilla-central/rev/d6ea652c579992daa9041cc9718bb7c6abefbc91/toolkit/components/telemetry/tests/unit/test_TelemetryEnvironment.js#669

Then check that it still shows up in the environment data with the old value around here:
https://dxr.mozilla.org/mozilla-central/rev/d6ea652c579992daa9041cc9718bb7c6abefbc91/toolkit/components/telemetry/tests/unit/test_TelemetryEnvironment.js#680
I wasnt able to test it as I started having trouble with my firefox not compling. Figure id at least upload the patch to see if I went the right direction on this.
Attachment #8647402 - Flags: review?(gfritzsche)
Comment on attachment 8647402 [details] [diff] [review]
Added what: and requiredRestart: to default pref map and updated unit tests

Review of attachment 8647402 [details] [diff] [review]:
-----------------------------------------------------------------

This looks pretty good already, some comments below on what should be changed.

What kind of issues are you running into with compiling?
Maybe you can ask in #introduction about the problems or contact me so we can figure that out.

::: toolkit/components/telemetry/TelemetryEnvironment.jsm
@@ +79,5 @@
>    },
>  };
>  
>  const DEFAULT_ENVIRONMENT_PREFS = new Map([
> +  ["app.feedback.baseURL", {what: TelemetryEnvironment.RECORD_PREF_VALUE}],

We should add short-hands before the DEFAULT_ENVIRONMENT_PREFS: 
> const RECORD_PREF_VALUE = TelemetryEnvironment.RECORD_PREF_VALUE;
> const RECORD_PREF_STATE = TelemetryEnvironment.RECORD_PREF_STATE;

Then the line lengths below become a bit saner.

@@ +786,5 @@
>      this._log.trace("_startWatchingPrefs - " + this._watchedPrefs);
>  
>      for (let pref of this._watchedPrefs.keys()) {
> +      if(!pref.hasOwnProperty('requiresRestart') | pref.requiresRestart == false)
> +         Preferences.observe(pref, this._onPrefChanged, this);

We could just do:
> if (!pref.requiresRestart) {
... or - if that triggers warnings - use:
> if (!("requiresRestart" in pref) || !pref.requiresRestart) {

Also, lets wrap the statements in braces, i.e.:
> if (...) {
>   ...
> }

@@ +805,5 @@
>      this._log.trace("_stopWatchingPrefs");
>  
>      for (let pref of this._watchedPrefs.keys()) {
> +      if(!pref.hasOwnProperty('requiresRestart') | pref.requiresRestart == false)
> +	 Preferences.ignore(pref, this._onPrefChanged, this);

Same here as below.

::: toolkit/components/telemetry/tests/unit/test_TelemetryEnvironment.js
@@ +638,5 @@
>    const PREF_TEST_1 = "toolkit.telemetry.test.pref_new";
>    const PREF_TEST_2 = "toolkit.telemetry.test.pref1";
>    const PREF_TEST_3 = "toolkit.telemetry.test.pref2";
>    const PREF_TEST_4 = "toolkit.telemetry.test.pref_old";
> +  const PREF_TEST_5 = "toolkit.telemtry.test.requiresRestart";

Nit: "toolkit.telemetry..." (missing an "e").

@@ +654,4 @@
>    ]);
>  
>    Preferences.set(PREF_TEST_4, expectedValue);
> +  

Nit: Unnecessary whitespace change.
We will need to set the value for the new test pref here already. _watchPreferences() below will update the data with the pref values, after that a |requiresRestart| pref wouldn't change anymore.

@@ +688,5 @@
>                 "Report that the pref was user set but the value is not shown.");
>    Assert.ok(!(PREF_TEST_3 in userPrefs),
>              "Do not report if preference not user set.");
> +  Assert.equal(userPrefs[PREF_TEST_5], undefined,
> +	      "Do not report pref until restart");

The pref value in the environment data should still be the same.
Attachment #8647402 - Flags: review?(gfritzsche) → feedback+
glandium ended up helping me resolve my compling issues. I ran the unit tests and I beleive it passed. I am not too good at understanding what is going on with the units test. But I ran the tests with the patch popped off and back on and I get the same 3 failed tests so im assuming thats from something else?
Attachment #8647402 - Attachment is obsolete: true
Attachment #8647854 - Flags: review?(gfritzsche)
Comment on attachment 8647854 [details] [diff] [review]
Added what: and requiredRestart: to default pref map  with const vars and updated unit tests

Review of attachment 8647854 [details] [diff] [review]:
-----------------------------------------------------------------

I definitely see test failures locally caused by this patch.
You may have seen similar failures if after popping the patch if you didn't build again?

You can e.g. start running just the environment test:
mach xpcshell-test toolkit/components/telemetry/tests/unit/test_TelemetryEnvironment.js

You will see some failures like:
 1:11.18 PROCESS_OUTPUT: Thread-39 (pid:72902) "*************************"
 1:11.18 PROCESS_OUTPUT: Thread-39 (pid:72902) "A coding exception was thrown and uncaught in a Task."
 1:11.18 PROCESS_OUTPUT: Thread-39 (pid:72902) "Full message: TypeError: invalid 'in' operand pref"
 1:11.18 PROCESS_OUTPUT: Thread-39 (pid:72902) "Full stack: EnvironmentCache.prototype._startWatchingPrefs@resource://gre/modules/TelemetryEnvironment.jsm:730:10"

... which points to the code changed here. I commented on that issue below.

There are also a few more places that have to be changed to the new preferences format (like you've already done to one test location in this patch):
https://dxr.mozilla.org/mozilla-central/search?q=path%3Atoolkit%2Fcomponents%2Ftelemetry%2Ftests%2Funit%2F+TelemetryEnvironment._watchPreferences

::: toolkit/components/telemetry/TelemetryEnvironment.jsm
@@ +80,5 @@
>  };
>  
> +const RECORD_PREF_STATE = TelemetryEnvironment.RECORD_PREF_STATE;
> +const RECORD_PREF_VALUE = TelemetryEnvironment.RECORD_PREF_VALUE;
> +const RECORD_PREF_NOTIFY_ONLY = TelemetryEnvironment.RECORD_PREF_NOTIFY_ONLY;

It looks like we're not using RECORD_PREF_NOTIFY_ONLY anymore.
Lets leave it out here and i'll file a bug on removing the other mentions.

@@ +788,5 @@
>    _startWatchingPrefs: function () {
>      this._log.trace("_startWatchingPrefs - " + this._watchedPrefs);
>  
>      for (let pref of this._watchedPrefs.keys()) {
> +      if(!("requiresRestart" in pref) || !pref.requiresRestart) {

So, i missed this before - this won't register any observers.
|pref.requiresRestart| will never be defined as |pref| is the key of each entry (i.e. the prefs name).

We could instead use:
for (let [pref, options] of this._watchedPrefs) {
  if (!("requiresRestart" in options) || ...

@@ +808,5 @@
>    _stopWatchingPrefs: function () {
>      this._log.trace("_stopWatchingPrefs");
>  
>      for (let pref of this._watchedPrefs.keys()) {
> +      if(!("requiresRestart" in pref) || !pref.requiresRestart) {

Same issue here as above:
for (let [pref, options] of this._watchedPrefs) {
  if (!("requiresRestart" in options) || ...

@@ +810,5 @@
>  
>      for (let pref of this._watchedPrefs.keys()) {
> +      if(!("requiresRestart" in pref) || !pref.requiresRestart) {
> +	 Preferences.ignore(pref, this._onPrefChanged, this);
> +      } 

Nit: Trailing whitespace.

::: toolkit/components/telemetry/tests/unit/test_TelemetryEnvironment.js
@@ +655,4 @@
>    ]);
>  
>    Preferences.set(PREF_TEST_4, expectedValue);
> +  Prefernces.set(PREF_TEST_5, expectedValue);

Typo: Preferences
Attachment #8647854 - Flags: review?(gfritzsche)
okay all tests passed this time. I didnt realise I had to build each time. Anyways I needed to change some functions to look for the .what like in the _getPrefData() function. 

   (!("requiresRestart" in options) || ... didnt work for me. It came up as an "invalid in operand", so I uses hasOwnProperty(). If those lines are too long, I could format it to be on two lines.
Attachment #8647854 - Attachment is obsolete: true
Attachment #8649043 - Flags: review?(gfritzsche)
(In reply to rthyberg from comment #10)
>    (!("requiresRestart" in options) || ... didnt work for me. It came up as
> an "invalid in operand", so I uses hasOwnProperty().

Those are real test failures:
"invalid in operand" says that |options| is not an object as we expect, which means that the pref definitions were not updated yet.
The error stacks point to the fake pref locations in tests that you haven't updated yet:
https://dxr.mozilla.org/mozilla-central/search?q=path%3Atoolkit%2Fcomponents%2Ftelemetry%2Ftests%2Funit%2F+PREFS_TO_WATCH
Attachment #8649043 - Flags: review?(gfritzsche)
(In reply to Georg Fritzsche [:gfritzsche] from comment #11)
> (In reply to rthyberg from comment #10)
> >    (!("requiresRestart" in options) || ... didnt work for me. It came up as
> > an "invalid in operand", so I uses hasOwnProperty().
> 
> Those are real test failures:
> "invalid in operand" says that |options| is not an object as we expect,
> which means that the pref definitions were not updated yet.
> The error stacks point to the fake pref locations in tests that you haven't
> updated yet:
> https://dxr.mozilla.org/mozilla-central/
> search?q=path%3Atoolkit%2Fcomponents%2Ftelemetry%2Ftests%2Funit%2F+PREFS_TO_W
> ATCH

E.g.:
> 0:08.08 LOG: Thread-1 INFO (xpcshell/head.js) | test test_changeThrottling pending (3)
> 0:08.10 PROCESS_OUTPUT: Thread-1 (pid:12255) "*************************"
> 0:08.10 PROCESS_OUTPUT: Thread-1 (pid:12255) "A coding exception was thrown and uncaught in a Task."
> 0:08.10 PROCESS_OUTPUT: Thread-1 (pid:12255) "Full message: TypeError: invalid 'in' operand options"
> 0:08.10 PROCESS_OUTPUT: Thread-1 (pid:12255) "Full stack: EnvironmentCache.prototype._startWatchingPrefs@resource://gre/modules/TelemetryEnvironment.jsm:729:10"
> 0:08.10 PROCESS_OUTPUT: Thread-1 (pid:12255) "EnvironmentCache.prototype._watchPreferences@resource://gre/modules/TelemetryEnvironment.jsm:691:5"
> 0:08.10 PROCESS_OUTPUT: Thread-1 (pid:12255) "this.TelemetryEnvironment._watchPreferences@resource://gre/modules/TelemetryEnvironment.jsm:79:12"
> 0:08.10 PROCESS_OUTPUT: Thread-1 (pid:12255) "test_changeThrottling@/Users/gfritzsche/moz/mc1/obj/_tests/xpcshell/toolkit/components/telemetry/tests/unit/test_TelemetryEnvironment.js:992:3"
...

This points you to line 992 in test_TelemetryEnvironment.js; from there you can see that the pref definitions used there were not updated yet.
That makes alot more sense and all tests passed again, thanks. 

I used "requiresRestart" in options and fixed the rest of the pref maps in the test files. I didnt realise there were so many I forgot to do.
Attachment #8649043 - Attachment is obsolete: true
Attachment #8649547 - Flags: review?(gfritzsche)
Blocks: 1196219
Comment on attachment 8649547 [details] [diff] [review]
bug 1192811: added what and required restart to all necessary pref_maps and updated functions to look for what and required restart

Review of attachment 8649547 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks, this looks good.

I only have minor nitpicks below - lets change that and then push the patch to try. [0]
Do you have access to try? If not you can just upload the patch with the below comments addressed and i'll push it for you.

0: https://wiki.mozilla.org/ReleaseEngineering/TryServer

::: toolkit/components/telemetry/TelemetryEnvironment.jsm
@@ +786,5 @@
>     */
>    _startWatchingPrefs: function () {
>      this._log.trace("_startWatchingPrefs - " + this._watchedPrefs);
>  
> +    for (let [pref, options] of this._watchedPrefs.entries()) {

Nit: No need for .entries(), you can just use:
for (let [pref, options] of this._watchedPrefs) {
  ...

Ditto below.

::: toolkit/components/telemetry/tests/unit/test_TelemetryEnvironment.js
@@ +1088,5 @@
>  
>    // Define and reset the test preference.
>    const PREF_TEST = "toolkit.telemetry.test.pref1";
>    const PREFS_TO_WATCH = new Map([
> +    [PREF_TEST,{what: TelemetryEnvironment.RECORD_PREF_STATE}],

Nit: Add a space before the "{".
Attachment #8649547 - Flags: review?(gfritzsche) → review+
(In reply to Georg Fritzsche [:gfritzsche] from comment #9)
> ::: toolkit/components/telemetry/TelemetryEnvironment.jsm
> @@ +80,5 @@
> >  };
> >  
> > +const RECORD_PREF_STATE = TelemetryEnvironment.RECORD_PREF_STATE;
> > +const RECORD_PREF_VALUE = TelemetryEnvironment.RECORD_PREF_VALUE;
> > +const RECORD_PREF_NOTIFY_ONLY = TelemetryEnvironment.RECORD_PREF_NOTIFY_ONLY;
> 
> It looks like we're not using RECORD_PREF_NOTIFY_ONLY anymore.
> Lets leave it out here and i'll file a bug on removing the other mentions.

I filed bug 1196219 on this if you are interested.
fixed nits. I dont have try server access. I am not entirely sure what it is. My understanding is it's  where the finaly tests are ran before it gets added to the main branch? What do you do after you run it on the try server?
Attachment #8649547 - Attachment is obsolete: true
Flags: needinfo?(gfritzsche)
Thanks! Pushed to try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3f073abd2da6

One last request:
Can you update the patch with a more succinct commit message and add "r=gfritzsche" to show that i reviewed it?
(e.g.: "Enable TelemetryEnvironment collecting preferences without triggering subsession splits. r=gfritzsche")

(In reply to rthyberg from comment #16)
> fixed nits. I dont have try server access. I am not entirely sure what it
> is. My understanding is it's  where the finaly tests are ran before it gets
> added to the main branch? What do you do after you run it on the try server?

Right, this is for checking that the changes in the patch don't break any automated tests before we land it (so we can avoid breaking the main repositories others are working from).

After the test runs there look good (all green runs or only "known" failures) we can land the patch, e.g. by setting checkin-needed here or checking it in ourselves.
I think this covers the patch/review/landing cycle ok:
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/How_to_Submit_a_Patch
Flags: needinfo?(gfritzsche)
That looks good, thanks!

The try run from comment 17 looks all green, so i'm setting checkin-needed here.
Keywords: checkin-needed
Second patch doesn't apply. Please rebase on top of fx-team tip.
Keywords: checkin-needed
Attachment #8650683 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/30d36e36bae8
Status: NEW → RESOLVED
Closed: 9 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
You need to log in before you can comment on or make changes to this bug.