1122048 - Telemetry Environment: generic pref collection and change detection

Status: RESOLVED FIXED

(Firefox Health Report Graveyard :: Client: Desktop, defect)

Description

[Georg Fritzsche [:gfritzsche]]

We need a generic implementation for pref collection & change detection for the Telemetry environment implementation.

Comment 1

[Alessio Placitelli [:Dexter]]

Attachment: bug1122048.patch

This patch adds preferences whitelist watching and data collection to TelemetryEnvironment from bug 1122047.

Comment 2

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8554578 [details] [diff] [review]
bug1122048.patch

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

::: toolkit/components/telemetry/TelemetryEnvironment.jsm
@@ +24,5 @@
>    _changeListeners: new Map(),
>    _notifyTask: null,
>    _doNotify: false,
>  
> +  _prefWhitelist: null,

Add a comment explaining what this is for.
I wouldn't call this a whitelist here - its just a list of prefs we watch etc.
The actual whitelist comes later in another bug.

@@ +33,5 @@
>     */
>    init: Task.async(function* () {
>      this._log = Log.repository.getLoggerWithMessagePrefix(LOGGER_NAME, "TelemetryEnvironment::");
>      this._log.trace("init");
>      this._shutdown = false;

Shouldn't we set up the preference watching here?

@@ +41,5 @@
>     * Shutdown TelemetryEnvironment.
>     * @return Promise<> that is resolved when shutdown is finished.
>     */
>    shutdown: Task.async(function* () {
> +    this._removeWhitelist();

This should probably be after the trace("shutdown").

@@ +74,5 @@
>    /**
> +   * Get a map containing the values for the whitelisted preferences.
> +   * @return A Map containing the preferences values using names as keys.
> +   */
> +  getWhitelistData: function () {

I don't think we need to expose getters for indidual sections - we can just use getEnvironmentData() in tests and elsewhere.
I'd suggest naming mentioning preferences, something like _getPrefData() or similar?

@@ +76,5 @@
> +   * @return A Map containing the preferences values using names as keys.
> +   */
> +  getWhitelistData: function () {
> +    this._checkForShutdown();
> +    this._log.trace("removePreferencesWhitelist");

Wrong trace message.

@@ +78,5 @@
> +  getWhitelistData: function () {
> +    this._checkForShutdown();
> +    this._log.trace("removePreferencesWhitelist");
> +
> +    let prefDataMap = new Map();

Lets build an object, so we can assemble this in getEnvironmentData() instead.
E.g.:
  data.settings.userPrefs = _getPrefData();

@@ +80,5 @@
> +    this._log.trace("removePreferencesWhitelist");
> +
> +    let prefDataMap = new Map();
> +    this._prefWhitelist.map(
> +      pref => prefDataMap.set(pref, Preferences.get(pref, undefined)));

This is not actually mapping the array to anything useful.
Why not just |for (let pref of ...)|?

@@ +88,5 @@
> +  /**
> +   * Set the preferences whitelist, a list of preferences for changes.
> +   * @param aWhitelist An array of preferences to watch for changes.
> +   */
> +  setWhitelist: function (aWhitelist) {

Better naming please. _startWatchingPrefs() or something like that?
This shouldn't need to be set from the outside, except for testing.

@@ +90,5 @@
> +   * @param aWhitelist An array of preferences to watch for changes.
> +   */
> +  setWhitelist: function (aWhitelist) {
> +    this._checkForShutdown();
> +    this._log.trace("setPreferencesWhitelist - Whitelist Size " + aWhitelist.length);

You could just as well log it here: trace("foo", aWhitelist)

@@ +97,5 @@
> +      this._removeWhitelist();
> +    }
> +
> +    this._prefWhitelist = aWhitelist;
> +    this._prefWhitelist.map(pref => Preferences.observe(pref, this));

Why not set a separate observer function, say _onPrefChanged()?

@@ +103,5 @@
> +
> +  /**
> +   * Do not receive any more change notifications for the preferences.
> +   */
> +  _removeWhitelist: function () {

_stopWatchingPrefs()?

::: toolkit/components/telemetry/tests/unit/test_TelemetryEnvironment.js
@@ +51,5 @@
>    Assert.ok(results.every(val => val));
>  });
>  
> +add_task(function* test_prefWhitelist() {
> +  const prefWhitelist = [

Maybe don't mention whitelists?

@@ +56,5 @@
> +    "toolkit.telemetry.test.pref",
> +    "toolkit.telemetry.test.not_set",
> +  ];
> +
> +  let prefPromise = Promise.defer();

Lets move this definition down to where it actually gets used.

@@ +65,5 @@
> +  TelemetryEnvironment.setWhitelist(prefWhitelist);
> +
> +  // Register a change listener.
> +  TelemetryEnvironment.registerChangeListener("testWhitelist", () => {
> +    let values = TelemetryEnvironment.getWhitelistedValues();

We don't need to check the data in the listener - we can do that after the |yield prefPromise;|.

@@ +66,5 @@
> +
> +  // Register a change listener.
> +  TelemetryEnvironment.registerChangeListener("testWhitelist", () => {
> +    let values = TelemetryEnvironment.getWhitelistedValues();
> +    is(true, values.get(prefWhitelist[0]), "Set preferences are correctly handled.");

We should use the Assert.* functions where possible in tests.

@@ +77,5 @@
> +
> +  // Unregister listeners
> +  TelemetryEnvironment.unregisterChangeListener("testWhitelist");
> +
> +  yield prefPromise;

First wait for the promise, then unregisterChangeListener.
This test should just fail the way it is now?

Comment 3

[Alessio Placitelli [:Dexter]]

Attachment: bug1122048.patch - v2

Thanks for your time reviewing this, Georg. In this patch I've renamed the methods and got rid of the |.map|.

The test is no longer checking for data validity, as this should probably be done when testing the |getEnvironmentData| function.

Comment 4

[Georg Fritzsche [:gfritzsche]]

(In reply to Alessio Placitelli [:Dexter] from comment #3)
> The test is no longer checking for data validity, as this should probably be
> done when testing the |getEnvironmentData| function.

We should just start doing that here for that part of the data.

Slight hiccup: Please check the attachment in bug 1122046 for changes to this mechanism.
As we already have the pref list there, we could just as well add it to the implementation here in a second patch.

Comment 5

[Georg Fritzsche [:gfritzsche]]

(In reply to Georg Fritzsche [:gfritzsche] from comment #4)
> As we already have the pref list there, we could just as well add it to the
> implementation here in a second patch.

Actually, we shouldn't block this bug on it. We can easily add that later when that bug is closed.

Comment 6

[Alessio Placitelli [:Dexter]]

Attachment: part 1 - Add preferences watching to Environment (v3)

Thanks Georg! Changes:

- Add 2 default preferences to the test profile;
- Populate the environment data object;
- Implemented the pref value recording policy;
- Changed the test to validate the preferences from the Environemnt data.

As you suggested I'll be following up with another patch to add the preferences.

Comment 7

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8555171 [details] [diff] [review]
part 1 - Add preferences watching to Environment (v3)

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

We only want to record prefs if they are non-default.

::: toolkit/components/telemetry/TelemetryEnvironment.jsm
@@ +24,5 @@
> +};
> +
> +// Constants to show that a preference was not modified by user in the preference data object.
> +const PREF_USER_SET = "telemetry-pref-user-set";
> +const PREF_DEFAULT = "telemetry-pref-is-default";

We don't need these two as we don't want to record prefs that are not user-set anyway.

@@ +174,5 @@
>      this._log.trace("getEnvironmentData");
> +
> +    let data = {};
> +    data.settings = {};
> +    data.settings.userPrefs = this._getPrefData();

Thought:
I want us to recover from exceptions in collection functions.

What do you think about:
let sections = {
  "settings": () => this._getSettings(),
  ...
};
for (let s in sections) {
  try {
    sections[s]();
  } catch (e) {
    // report
  }
}

::: toolkit/components/telemetry/tests/unit/test_TelemetryEnvironment.js
@@ +54,5 @@
> +add_task(function* test_prefWatch() {
> +  const prefsToWatch = {
> +    "toolkit.telemetry.test.pref_new": 2, //PREF_RECORD_POLICY.Value
> +    "toolkit.telemetry.test.pref1": 1, //PREF_RECORD_POLICY.State
> +    "toolkit.telemetry.test.pref2": 1, //PREF_RECORD_POLICY.State

Lets just export those as constants on the module instead of using magic values. E.g.:
"toolkit.telemetry.test.pref_new": TelemetryEnvironment.PREF_RECORD_VALUE,

Can we also use consts for the pref names? They are repeated up to three times here.

@@ +65,5 @@
> +
> +  // Set the Environment preferences to watch.
> +  TelemetryEnvironment.watchPreferences(prefsToWatch);
> +
> +  let prefPromise = Promise.defer();

This was obsoleted. We should use PromiseUtils.defer().

Comment 8

[Alessio Placitelli [:Dexter]]

Attachment: part 1 - Add preferences watching to Environment (v4)

This patch changes the behaviour of getEnvironmentData() and updates the test with the suggestions from Georg.

Comment 9

[Alessio Placitelli [:Dexter]]

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

Comment 10

[Alessio Placitelli [:Dexter]]

Whoops, wrong link posted. Sorry.

Comment 11

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8555827 [details] [diff] [review]
part 1 - Add preferences watching to Environment (v4)

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

::: toolkit/components/telemetry/TelemetryEnvironment.jsm
@@ +25,5 @@
>    _notifyTask: null,
>    _doNotify: false,
>  
> +  // Policy to use when saving preferences. Exported for using them in tests.
> +  // Don't record the preference value. Only whether it was set by the user or not.

Some nitpicking, please bear with me:
* It took me a moment to realize that this was describing the value below and not part of the initial comment. 
* It's enough to mention once that we only collect user-set values.
* PREF_* is misleading as we use that for pref name constants elsewhere.

How about:
// [...] We only record user-set prefs.
RECORD_PREF_STATE: 1, // Don't record the preference value
...

@@ +82,5 @@
>  
>    /**
> +   * Only used in tests, set the preferences to watch.
> +   */
> +  watchPreferences: function (aPreferences) {

Lets prefix with _ to make it clear this is not used from the outside usually.

@@ +115,5 @@
> +
> +      // Check the policy for the preference and decide if we need to store its value
> +      // or whether it changed from the default value.
> +      let prefValue = undefined;
> +      if (this._watchedPrefs[pref] === this.PREF_RECORD_STATE) {

Nit: At least per the style guide, we prefer == over === (unless there is a specific reason to use ===).

@@ +120,5 @@
> +        prefValue = null;
> +      } else {
> +        prefValue = Preferences.get(pref, undefined);
> +      }
> +      prefData[pref] = prefValue;

Is prefValue is still |undefined| here, i don't think it will end up in the JSON dump.
Is that the intent here?

@@ +191,5 @@
> +    };
> +
> +    let data = {};
> +    // Recover from exceptions in the collection functions and populate the data
> +    // object.

I'd like to document the why & how for the exception handling more clearly.
I.e. we want to recover from exceptions in the collection functions so that worst-case we only lose some data sections instead of all.

@@ +200,5 @@
> +        this._log.error("getEnvironmentData - There was an exception collecting " + s);
> +      }
> +    }
> +
> +    return Promise.resolve(data);

Just |return data|.

::: toolkit/components/telemetry/tests/unit/test_TelemetryEnvironment.js
@@ +55,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 expectedValue = Math.random().toString();

Let's not use random test values unless we have a good reason to.
If you're worried about the prefs already being set to the known value you choose, just assert that they are not equal.

@@ +84,5 @@
> +
> +  // Check environment contains the correct data.
> +  let environmentData = yield TelemetryEnvironment.getEnvironmentData();
> +
> +  // Test that we are recording the value from the first preference.

Nit: This should be self-documenting through the assertion message below.

@@ +86,5 @@
> +  let environmentData = yield TelemetryEnvironment.getEnvironmentData();
> +
> +  // Test that we are recording the value from the first preference.
> +  let changedNewPref = environmentData.settings.userPrefs[PREF_TEST_1];
> +  Assert.equal(changedNewPref, expectedValue, "Environment contains the correct preference value.");

Nit: Just |Assert.equal(userPrefs[PREF_TEST_1], ...|?

@@ +88,5 @@
> +  // Test that we are recording the value from the first preference.
> +  let changedNewPref = environmentData.settings.userPrefs[PREF_TEST_1];
> +  Assert.equal(changedNewPref, expectedValue, "Environment contains the correct preference value.");
> +
> +  // Test that we get a changed state for the second preference.

Nit: This should be self-documenting through the assertion message below.

@@ +90,5 @@
> +  Assert.equal(changedNewPref, expectedValue, "Environment contains the correct preference value.");
> +
> +  // Test that we get a changed state for the second preference.
> +  let nonDefaultPref = environmentData.settings.userPrefs[PREF_TEST_2];
> +  Assert.equal(nonDefaultPref, null, "Report that the pref was user set.");

Nit: Just |Assert.equal(userPrefs[PREF_TEST_2], ...|?

@@ +94,5 @@
> +  Assert.equal(nonDefaultPref, null, "Report that the pref was user set.");
> +
> +  // The third preference should not be in the environment data, as it's not user set.
> +  let defaultPref = environmentData.settings.userPrefs[PREF_TEST_3];
> +  Assert.equal(defaultPref, undefined, "Do not report if preference not user set.");

Nit: Just |Assert.ok(!(PREF_TEST_3 in userPrefs), ...)|?

Comment 12

[Georg Fritzsche [:gfritzsche]]

Oh, can we please cover pref resetting in the tests as well?

Comment 13

[Alessio Placitelli [:Dexter]]

Attachment: part 1 - Add preferences watching to Environment (v5)

Thank you for your review! I've addressed all the nits and added a new test to cover preferences reset.

Comment 14

[Alessio Placitelli [:Dexter]]

Attachment: part 1 - Add preferences watching to Environment (v6)

Removed useless blank lines from the pref reset test.

Comment 15

[Alessio Placitelli [:Dexter]]

Attachment: part 1 - Add preferences watching to Environment (v7)

Fixed test not yielding on the correct promise.

Comment 16

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8557577 [details] [diff] [review]
part 1 - Add preferences watching to Environment (v7)

This was already reviewed.

Comment 17

[Alessio Placitelli [:Dexter]]

Attachment: part 1 - Add preferences watching to Environment (v8)

Rebased the patch. Also updated it to work with latest Environment sketch from bug 1122047 (removed |checkForShutdown| usages).

Comment 18

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8562639 [details] [diff] [review]
part 1 - Add preferences watching to Environment (v8)

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

::: toolkit/components/telemetry/TelemetryEnvironment.jsm
@@ +31,5 @@
> +  RECORD_PREF_VALUE: 2, // We only record user-set prefs.
> +
> +  // A dictionary of watched preferences which trigger an Environment change when modified.
> +  // Every entry contains a recording policy (RECORD_PREF_STATE or RECORD_PREF_VALUE).
> +  _watchedPrefs: null,

// A Map of ...

@@ +93,5 @@
>      this._changeListeners.delete(name);
>    },
>  
>    /**
> +   * Only used in tests, set the preferences to watch.

Add that we expect a Map.

@@ +111,5 @@
> +   *
> +   * @return An object containing the preferences values.
> +   */
> +  _getPrefData: function () {
> +    this._log.trace("_getPrefData");

I don't think we need to trace this.

@@ +142,5 @@
> +   * Start watching the preferences.
> +   */
> +  _startWatchingPrefs: function () {
> +    this._log.trace("_startWatchingPrefs - " + this._watchedPrefs);
> +    if (this._shutdown) {

This is not supposed to be called from external callers, so no need for a shutdown check.

@@ +161,5 @@
> +   * Do not receive any more change notifications for the preferences.
> +   */
> +  _stopWatchingPrefs: function () {
> +    this._log.trace("_stopWatchingPrefs");
> +    if (this._shutdown) {

Same here.

@@ +187,5 @@
> +   * Get the settings data in object form.
> +   * @return Object containing the settings.
> +   */
> +  _getSettings: function () {
> +    this._log.trace("_getSettings");

Less tracing please.

@@ +230,5 @@
> +    for (let s in sections) {
> +      try {
> +        data[s] = sections[s]();
> +      } catch (e) {
> +        this._log.error("getEnvironmentData - There was an exception collecting " + s);

log e too? _log.error("...", e)?

::: toolkit/components/telemetry/tests/unit/test_TelemetryEnvironment.js
@@ +100,5 @@
> +  let userPrefs = environmentData.settings.userPrefs;
> +
> +  Assert.equal(userPrefs[PREF_TEST_1], expectedValue, "Environment contains the correct preference value.");
> +  Assert.equal(userPrefs[PREF_TEST_2], null, "Report that the pref was user set and has no value.");
> +  Assert.ok(!(PREF_TEST_3 in userPrefs), "Do not report if preference not user set.");

Nit: Line-breaks as they start to line-wrap.

Comment 19

[Alessio Placitelli [:Dexter]]

Attachment: part 1 - Add preferences watching to Environment (v9)

This patch removes the redundant traces and fixes the reported Nits.

Comment 20

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8562827 [details] [diff] [review]
part 1 - Add preferences watching to Environment (v9)

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

::: toolkit/components/telemetry/TelemetryEnvironment.jsm
@@ +59,5 @@
>        throw new Error("Already shut down");
>      }
>  
>      this._log.trace("shutdown");
> +    this._stopWatchingPrefs();

Minor: lets move this between _shutdown=true and the yield

Comment 21

[Alessio Placitelli [:Dexter]]

Attachment: part 1 - Add preferences watching to Environment (v10)

Rebasing and fixing the reported minor issue.

Comment 22

[Georg Fritzsche [:gfritzsche]]

Pushed in: https://hg.mozilla.org/integration/fx-team/pushloghtml?changeset=b83c3fa8c80f

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/28110812d5bd