1344295 - Consolidate Telemetry prefs in head.js

Status: RESOLVED FIXED

(Toolkit :: Telemetry, enhancement, P1)

Description

[Alessio Placitelli [:Dexter]]

Some of the Telemetry tests are changing preferences and re-defining them in each test file [1].

We should consolidate all the used preferences in the head.js file [2] and make other test files use these ones.

[1] - https://dxr.mozilla.org/mozilla-central/source/toolkit/components/telemetry/tests/unit/head.js
[2] - https://dxr.mozilla.org/mozilla-central/source/toolkit/components/telemetry/tests/unit/head.js

Comment 1

[Gabriele Svelto [:gsvelto]]

BTW there's a fair bit of duplication in the telemetry sources proper. It might be nice to lump them all in a single place.

Comment 2

[Deepjyoti Mondal]

@Alessio 
I would like to work on this. How should I proceed?

Comment 3

[abhi.taker20]

I am interested in solving this bug.how should i go about it?

Comment 4

[Alessio Placitelli [:Dexter]]

The link [1] from comment 0 is wrong, it should have been: https://dxr.mozilla.org/mozilla-central/search?q=-path%3Aobj+path%3Atoolkit%2Fcomponents%2Ftelemetry%2Ftest+%22const+PREF_%22&redirect=false

Comment 5

[Deepjyoti Mondal]

@Alessio Okay, so all the pref settings needs to be moved from the other test files into the head.js file and remove duplicate values. How do I make the other test files use head.js pref values. Should I export the prefs and import in other files?

Comment 6

[Alessio Placitelli [:Dexter]]

(In reply to Deepjyoti Mondal from comment #5)
> @Alessio Okay, so all the pref settings needs to be moved from the other
> test files into the head.js file and remove duplicate values. How do I make
> the other test files use head.js pref values. Should I export the prefs and
> import in other files?

Since there are about 200 preference usages between Telemetry code and test, let's restrict this bug to only dealing with the tests. I'll file a follow-up bug to refactor the non-test code.

We could:

1) Add a new object in TelemetryUtils.jsm [1] that will hold the preferences. Something like

> Preferences: Object.freeze({
>   TelemetryEnabled: "toolkit.telemetry.enabled",
> }),

2) For any pref used in the test code (see comment 4) add an entry to the Preferences object.
3) Import and use TelemetryUtils.Preferences.<pref-name> in the tests.

[1] - https://dxr.mozilla.org/mozilla-central/rev/8d026c60151005ad942e3d4389318fe28a0c8c54/toolkit/components/telemetry/TelemetryUtils.jsm#27

Comment 7

[Deepjyoti Mondal]

Thanks a lot for the pointers, I will start working ASAP

Comment 8

[Deepjyoti Mondal]

Attachment: bug1344295.patch

Moved all the pereferences to its Preferences object in TelemetryUtils.jsm

Comment 9

[Alessio Placitelli [:Dexter]]

Comment on attachment 8845223 [details] [diff] [review]
bug1344295.patch

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

Thank you for the patch, that's indeed a good start!

A few observations:

- Let's not put test only preferences in TelemetryUtils.
- Don't put the "Branch" entries in TelemetryUtils, as they were mainly used to save some typing.
- Change the commit message to something like "Bug 1344295: Consolidate Telemetry preferences in TelemetryUtils.Preferences. r?dexter"

::: toolkit/components/telemetry/TelemetryUtils.jsm
@@ +24,4 @@
>  })();
>  
>  this.TelemetryUtils = {
> +  Preferences: Object.freeze({

Please group the preferences as follows and sort alphabetically within each group:

// General preferences
ArchiveEnabled: "telemetry...",
...
TelemetryEnabled: "telemetry...",
...

// Data reporting preferences
FhrUploadEnabled: "datareporting....",
...

@@ +25,5 @@
>  
>  this.TelemetryUtils = {
> +  Preferences: Object.freeze({
> +    TelemetryEnabled: "toolkit.telemetry.enabled",
> +    Branch: "toolkit.telemetry.",

Please remove "Branch" as it was just used to build the preference string. We don't need it here.

@@ +27,5 @@
> +  Preferences: Object.freeze({
> +    TelemetryEnabled: "toolkit.telemetry.enabled",
> +    Branch: "toolkit.telemetry.",
> +    ArchiveEnabled: "toolkit.telemetry.archive.enabled",
> +    Test: "toolkit.telemetry.test.pref1",

Please remove all the test preferences from here, we don't need to share them across the different modules.

@@ +28,5 @@
> +    TelemetryEnabled: "toolkit.telemetry.enabled",
> +    Branch: "toolkit.telemetry.",
> +    ArchiveEnabled: "toolkit.telemetry.archive.enabled",
> +    Test: "toolkit.telemetry.test.pref1",
> +    Enabled: "toolkit.telemetry.enabled",

Please check for duplicates, as this is the same of TelemetryEnabled. Let's drop this one as "Enabled" is too vague.

@@ +32,5 @@
> +    Enabled: "toolkit.telemetry.enabled",
> +    FhrUploadEnabled: "datareporting.healthreport.uploadEnabled",
> +    Unified: "toolkit.telemetry.unified",
> +    CachedClientId: "toolkit.telemetry.cachedClientID",
> +    Test1: "toolkit.telemetry.test.pref_new",

Remove these tests preferences: Test1 to Test5.

@@ +37,5 @@
> +    Test2: "toolkit.telemetry.test.pref1",
> +    Test3: "toolkit.telemetry.test.pref2",
> +    Test4: "toolkit.telemetry.test.pref_old",
> +    Test5: "toolkit.telemetry.test.requiresRestart",
> +    PolicyBranch: "datareporting.policy.",

Remove this policy branch for the same reason we're removing the "Branch" entry.

@@ +42,5 @@
> +    BypassNotification: "datareporting.policy.dataSubmissionPolicyBypassNotification",
> +    DataSubmissionEnabled: "datareporting.policy.dataSubmissionEnabled",
> +    CurrentPolicyVersion: "datareporting.policy.currentPolicyVersion",
> +    MinimumPolicyVersion: "datareporting.policy.minimumPolicyVersion",
> +    MinimumChannelPolicyVersion: "datareporting.policy.minimumPolicyVersion.channel-TestChannelABC",

This is a test preference, we don't need to share it here.

::: toolkit/components/telemetry/tests/unit/head.js
@@ +4,4 @@
>  var { classes: Cc, utils: Cu, interfaces: Ci, results: Cr } = Components;
>  
>  Cu.import("resource://gre/modules/TelemetryController.jsm", this);
> +Cu.import("resource://gre/modules/TelemetryUtils.jsm", this);

Let's not include this here. We won't need it if we remove PREF_TELEMETRY_ENABLED. See below.

@@ +32,4 @@
>  const MILLISECONDS_PER_HOUR = 60 * MILLISECONDS_PER_MINUTE;
>  const MILLISECONDS_PER_DAY = 24 * MILLISECONDS_PER_HOUR;
>  
> +const PREF_TELEMETRY_ENABLED = TelemetryUtils.Preferences.TelemetryEnabled;

Let's just remove PREF_TELEMETRY_ENABLED and simply use TelemetryUtils.Preferences.TelemetryEnabled at the call sites.

::: toolkit/components/telemetry/tests/unit/test_SubsessionChaining.js
@@ +15,5 @@
>  const MS_IN_ONE_HOUR  = 60 * 60 * 1000;
>  const MS_IN_ONE_DAY   = 24 * MS_IN_ONE_HOUR;
>  
> +const PREF_BRANCH = TelemetryUtils.Preferences.Branch;
> +const PREF_ARCHIVE_ENABLED = TelemetryUtils.Preferences.ArchiveEnabled;

Let's remove the PREF_* consts and use the one you introduced in TelemetryUtils at the call sites.

@@ +103,4 @@
>      return;
>    }
>  
> +  const PREF_TEST = TelemetryUtils.Preferences.Test;

You don't need to share test preferences in TelemetryUtils. Let's just change this to be const PREF_TEST = "toolkit.telemetry.test.pref1";

::: toolkit/components/telemetry/tests/unit/test_TelemetryController.js
@@ +28,4 @@
>  const APP_VERSION = "1";
>  const APP_NAME = "XPCShell";
>  
> +const PREF_BRANCH = TelemetryUtils.Preferences.Branch;

Let's drop this PREF_* const and the others below. Just use TelemetryUtils... at the call sites.

::: toolkit/components/telemetry/tests/unit/test_TelemetryControllerShutdown.js
@@ +15,4 @@
>  Cu.import("resource://gre/modules/AsyncShutdown.jsm", this);
>  Cu.import("resource://testing-common/httpd.js", this);
>  
> +const PREF_BRANCH = TelemetryUtils.Preferences.Branch;

Same as the other file.

::: toolkit/components/telemetry/tests/unit/test_TelemetryController_idle.js
@@ +12,5 @@
>  Cu.import("resource://gre/modules/TelemetryController.jsm", this);
>  Cu.import("resource://gre/modules/TelemetrySession.jsm", this);
>  Cu.import("resource://gre/modules/TelemetrySend.jsm", this);
>  
> +const PREF_FHR_UPLOAD_ENABLED = TelemetryUtils.Preferences.FhrUploadEnabled;

Same as the other file.

::: toolkit/components/telemetry/tests/unit/test_TelemetryEnvironment.js
@@ +857,4 @@
>  });
>  
>  add_task(function* test_prefWatchPolicies() {
> +  const PREF_TEST_1 = TelemetryUtils.Preferences.Test1;

Don't change this prefs, as they are test only. You won't be exporting them in TelemetryUtils.

@@ +914,4 @@
>  });
>  
>  add_task(function* test_prefWatch_prefReset() {
> +  const PREF_TEST = TelemetryUtils.Preferences.Test2;

Same here.

@@ +1434,4 @@
>    Services.search.removeEngine(engine);
>  
>    // Define and reset the test preference.
> +  const PREF_TEST = TelemetryUtils.Preferences.Test2;

Same here.

@@ +1507,4 @@
>  
>  add_task(function* test_environmentShutdown() {
>    // Define and reset the test preference.
> +  const PREF_TEST = TelemetryUtils.Preferences.Test2;

Same here.

::: toolkit/components/telemetry/tests/unit/test_TelemetryReportingPolicy.js
@@ +16,4 @@
>  Cu.import("resource://gre/modules/XPCOMUtils.jsm", this);
>  Cu.import("resource://gre/modules/UpdateUtils.jsm", this);
>  
> +const PREF_BRANCH = TelemetryUtils.Preferences.Branch;

As for the other files, just directly use TelemetryUtils.. at the call sites instead of changing the constants here.

::: toolkit/components/telemetry/tests/unit/test_TelemetrySend.js
@@ +15,4 @@
>  Cu.import("resource://gre/modules/Preferences.jsm", this);
>  Cu.import("resource://gre/modules/osfile.jsm", this);
>  
> +const PREF_TELEMETRY_SERVER = TelemetryUtils.Preferences.Server;

Same here.

::: toolkit/components/telemetry/tests/unit/test_TelemetrySendOldPings.js
@@ +34,4 @@
>  
>  const TOTAL_EXPECTED_PINGS = OVERDUE_PINGS + RECENT_PINGS + OLD_FORMAT_PINGS;
>  
> +const PREF_FHR_UPLOAD = TelemetryUtils.Preferences.FhrUploadEnabled;

Same here.

::: toolkit/components/telemetry/tests/unit/test_TelemetrySession.js
@@ +60,5 @@
>  const MS_IN_ONE_HOUR  = 60 * 60 * 1000;
>  const MS_IN_ONE_DAY   = 24 * MS_IN_ONE_HOUR;
>  
> +const PREF_BRANCH = TelemetryUtils.Preferences.Branch;
> +const PREF_SERVER = TelemetryUtils.Preferences.Server;

Same here.

@@ +1208,4 @@
>    let now = fakeNow(2040, 1, 1, 12, 0, 0);
>    gMonotonicNow = fakeMonotonicNow(gMonotonicNow + 10 * MILLISECONDS_PER_MINUTE);
>  
> +  const PREF_TEST = TelemetryUtils.Preferences.Test2;

Don't change this as it's a test only pref.

@@ +1314,4 @@
>    };
>    yield CommonUtils.writeJSON(sessionState, dataFilePath);
>  
> +  const PREF_TEST = TelemetryUtils.Preferences.Test2;

Don't change this as it's a test only pref.

@@ +1695,4 @@
>    }
>  
>    // Reset the test preference.
> +  const PREF_TEST = TelemetryUtils.Preferences.Test2;

Don't change this as it's a test only pref.

@@ +1982,4 @@
>      return TelemetrySession.getPayload().info.subsessionCounter;
>    };
>  
> +  const PREF_TEST = TelemetryUtils.Preferences.Test2;

Don't change this as it's a test only pref.

Comment 10

[Alessio Placitelli [:Dexter]]

Also, it would be great if you could also update the in-tree docs to mention that all the preferences are exposed from TelemetryUtils. The documentation lives here: http://searchfox.org/mozilla-central/source/toolkit/components/telemetry/docs/internals/preferences.rst

Comment 11

[Deepjyoti Mondal]

I will make the required changes and upload a PR soon.

Comment 12

[Deepjyoti Mondal]

Attachment: bug1344295.patch

Made the changes you asked for.

Comment 13

[Deepjyoti Mondal]

@Alessio I would love to work on refactoring the non-test code.

Comment 14

[Deepjyoti Mondal]

@Alesso could you also suggest me some other telemetry issues related to JS

Comment 15

[Alessio Placitelli [:Dexter]]

Comment on attachment 8848683 [details] [diff] [review]
bug1344295.patch

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

Please make sure to replace all the uses of the non test only PREF_* constants, as you're missing a few of them (see this search for PREF_TELEMETRY_ENABLED, but there's more of them - http://searchfox.org/mozilla-central/search?q=PREF_TELEMETRY_ENABLED&case=false&regexp=false&path=telemetry%2Ftest).

Some tests are also using |TelemetryController.Constants.PREF_SERVER|. Make them use the one in TelemetryUtils instead for the tests (and remove it from TelemetryController if it's not used anymore outside of tests).

::: toolkit/components/telemetry/TelemetryUtils.jsm
@@ +24,5 @@
>  })();
>  
>  this.TelemetryUtils = {
> +  Preferences: Object.freeze({
> +

Please remove this empty line.

@@ +42,5 @@
> +    DataSubmissionEnabled: "datareporting.policy.dataSubmissionEnabled",
> +    FhrUploadEnabled: "datareporting.healthreport.uploadEnabled",
> +    MinimumPolicyVersion: "datareporting.policy.minimumPolicyVersion",
> +    OverrideOfficialCheck: "toolkit.telemetry.send.overrideOfficialCheck"
> +  }),

Please leave a whitespace after this line.

::: toolkit/components/telemetry/tests/unit/test_TelemetryControllerShutdown.js
@@ +31,3 @@
>    yield setEmptyPrefWatchlist();
>  
>    Services.prefs.setBoolPref(PREF_TELEMETRY_ENABLED, true);

This is probably TelemetryUtils.Preferences.TelemetryEnabled

::: toolkit/components/telemetry/tests/unit/test_TelemetryController_idle.js
@@ +22,3 @@
>    yield setEmptyPrefWatchlist();
>  
>    Services.prefs.setBoolPref(PREF_TELEMETRY_ENABLED, true);

This is probably TelemetryUtils.Preferences.TelemetryEnabled

::: toolkit/components/telemetry/tests/unit/test_TelemetryReportingPolicy.js
@@ +19,3 @@
>  const TEST_CHANNEL = "TestChannelABC";
>  
> +const PREF_MINIMUM_CHANNEL_POLICY_VERSION = TelemetryUtils.Preferences.MinimumPolicyVersion + 

nit: remove the trailing whitespace

@@ +57,2 @@
>  
>    Services.prefs.setBoolPref(PREF_TELEMETRY_ENABLED, true);

This is probably TelemetryUtils.Preferences.TelemetryEnabled

::: toolkit/components/telemetry/tests/unit/test_TelemetrySession.js
@@ +497,3 @@
>    yield setEmptyPrefWatchlist();
>  
>    Services.prefs.setBoolPref(PREF_TELEMETRY_ENABLED, true);

This is probably TelemetryUtils.Preferences.TelemetryEnabled

Comment 16

[Deepjyoti Mondal]

Attachment: bug1344295.patch

I have removed all the not test PREF_* with the ones I have declared in TelemetryUtils.js

Comment 17

[Alessio Placitelli [:Dexter]]

Comment on attachment 8850514 [details] [diff] [review]
bug1344295.patch

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

This looks good now, thanks!

Comment 18

[Alessio Placitelli [:Dexter]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c35053ac2526

Comment 19

[Alessio Placitelli [:Dexter]]

Deepjyoti, it looks like the patch makes some xpcshell tests fail (see comment 18). Would you mind checking that?

Comment 20

[Deepjyoti Mondal]

@Alessio yeah sure, I am on it

Comment 21

[Deepjyoti Mondal]

@Alessio the tests are failing due to 3 files, test_PingAPI.js, test_TelemetryController.js and test_TelemetryReportingPolicy.js I checked test_PingAPI.js and found a typo, I have corrected it, now it passes all tests

However test_TelemetryReportingPolicy.js is producing this error:  ERROR Unexpected exception NS_ERROR_ILLEGAL_VALUE: Component returned failure code: 0x80070057 (NS_ERROR_ILLEGAL_VALUE) [nsIPrefBranch.setStringPref] and
test_TelemetryController.js is producing the following error : TEST_STATUS: Thread-1 test_archivePings FAIL [test_archivePings : 315] Telemetry must not send pings if not allowed to. - false == true
/home_extension/firefox3/firefox/mozilla-central/obj-x86_64-pc-linux-gnu/_tests/xpcshell/toolkit/components/telemetry/tests/unit/test_TelemetryController.js:test_archivePings/<:315

Comment 22

[Alessio Placitelli [:Dexter]]

That's probably another typo. Please attach the updated patch here so that I can have a look.

Comment 23

[Deepjyoti Mondal]

Attachment: bug1344295.patch

This is my updated patch

Comment 24

[Alessio Placitelli [:Dexter]]

(In reply to Deepjyoti Mondal from comment #21)
> @Alessio the tests are failing due to 3 files, test_PingAPI.js,
> test_TelemetryController.js and test_TelemetryReportingPolicy.js I checked
> test_PingAPI.js and found a typo, I have corrected it, now it passes all
> tests
> 
> However test_TelemetryReportingPolicy.js is producing this error:  ERROR
> Unexpected exception NS_ERROR_ILLEGAL_VALUE: Component returned failure
> code: 0x80070057 (NS_ERROR_ILLEGAL_VALUE) [nsIPrefBranch.setStringPref] and
> test_TelemetryController.js is producing the following error : TEST_STATUS:
> Thread-1 test_archivePings FAIL [test_archivePings : 315] Telemetry must not
> send pings if not allowed to. - false == true
> /home_extension/firefox3/firefox/mozilla-central/obj-x86_64-pc-linux-gnu/
> _tests/xpcshell/toolkit/components/telemetry/tests/unit/
> test_TelemetryController.js:test_archivePings/<:315

I've applied your patch and, as you reported, it is failing. It's because of some typos :-)
Please follow the trails left by the logs to figure out where the problems are. For example, in test_TelemetryReportingPolicy.js, you're calling:

> Preferences.set(TelemetryUtils.Preferences.server, ...

But the property you defined in TelemetryUtils.Preferences starts with a capital S.

Please make sure all your tests run fine locally before uploading the next patch ;-)

Comment 25

[Deepjyoti Mondal]

Attachment: bug1344295.patch

@Alessio Thanks a lot for pointing it out :)

I checked test_TelemetryController.js howerver it is till showing : 

Thread-1 test_archivePings FAIL [test_archivePings : 315] Telemetry must not send pings if not allowed to. - false == true
/home_extension/firefox3/firefox/mozilla-central/obj-x86_64-pc-linux-gnu/_tests/xpcshell/toolkit/components/telemetry/tests/unit/test_TelemetryController.js:test_archivePings/<:315


I have checked test_archiveings method for any possible typos. But I have have not found any.I think I am missing something. Would you please have look ?

Comment 26

[Alessio Placitelli [:Dexter]]

Comment on attachment 8852754 [details] [diff] [review]
bug1344295.patch

What you could do to figure out the problem is the following:

- Restore this file to its previous state.
- Try to change the prefs one function at a time.
- Run the test.
- See when it starts to fail.

This would allow you to pinpoint the issue.

Comment 27

[Deepjyoti Mondal]

@Alessio Okay I will follow the steps you mentioned :)

There might be some delay in uploading the next patch as I will not be available for some time (travelling).

Comment 28

[Alessio Placitelli [:Dexter]]

(In reply to Deepjyoti Mondal from comment #27)
> @Alessio Okay I will follow the steps you mentioned :)
> 
> There might be some delay in uploading the next patch as I will not be
> available for some time (travelling).

No problem!

Comment 29

[Deepjyoti Mondal]

Attachment: bug1344295.patch

@Alessio I have updated my patch, all the tests are now passing.

Comment 30

[Alessio Placitelli [:Dexter]]

Comment on attachment 8855055 [details] [diff] [review]
bug1344295.patch

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

This looks good now, thanks. However, a new test file (or a couple) were added meanwhile. Would you mind rebasing this patch so that I can review the new additions too?

Comment 31

[Deepjyoti Mondal]

Attachment: bug1344295_v2.patch

Updated patch.

Comment 32

[Alessio Placitelli [:Dexter]]

Comment on attachment 8856080 [details] [diff] [review]
bug1344295_v2.patch

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

This looks good now, thanks!

Comment 33

[Alessio Placitelli [:Dexter]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=22d1e66fcc6edc573b0bd79a597d2037e30d87ad

Comment 34

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/16e85d7e7400
Consolidate Telemetry preferences in TelemetryUtils.Preferences. r=dexter

Comment 35

[Alessio Placitelli [:Dexter]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/16e85d7e7400febe85909d6f1baa5b0ff03fb2ab
Bug 1344295 - Consolidate Telemetry preferences in TelemetryUtils.Preferences. r=dexter

Comment 36

[Carsten Book [:Tomcat]]

backed out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=90102719&repo=mozilla-inbound

Comment 37

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d7af59656111
Backed out changeset 16e85d7e7400 for test failures in test_TelemetrySendOldPings.js

Comment 38

[Deepjyoti Mondal]

@Alessio I ran all the tests locally, all the tests are passing.test_TelemetrySendOldPings.js is not failing any [e-mail]

Comment 40

[Alessio Placitelli [:Dexter]]

(In reply to Deepjyoti Mondal from comment #38)
> @Alessio I ran all the tests locally, all the tests are
> passing.test_TelemetrySendOldPings.js is not failing any
> [e-mail]

It was a problem with rebasing the patch on mozilla-inbound. I'll rebase it again and attempt a new landing today.

Comment 41

[Alessio Placitelli [:Dexter]]

Attachment: bug1344295_v2.patch

Just rebased the patch, it's working locally.

Comment 42

[Alessio Placitelli [:Dexter]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=187f4d0d36064eeadf00695f37677e8f2b8312b2

Comment 43

[Alessio Placitelli [:Dexter]]

Attachment: bug1344295_v2.patch

This should hopefully fix the test fallout.

Comment 44

[Alessio Placitelli [:Dexter]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=df059a4140daaa511645b6047dff41c951b71fae

Comment 45

[Alessio Placitelli [:Dexter]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1d9dbeee6130f6acbcd83a10beff2b7cf58d1aa5
Bug 1344295 - Consolidate Telemetry preferences in TelemetryUtils.Preferences. r=dexter

Comment 46

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/1d9dbeee6130