1343943 - Add the ability to record default pref values in TelemetryEnvironment.jsm

Status: RESOLVED FIXED

(Toolkit :: Telemetry, enhancement, P3)

Description

[Mike Kaply [:mkaply]]

Currently the telemetry code only records the value of user set prefs.

We should add the ability to record the value of some default prefs in cases where those default prefs are typically set for other purposes (autoconfig, distribution.ini, etc.).

This involves creating a new class for telemetry that uses .has instead of .set to check for the presence of the pref.

Comment 1

[Mike Kaply [:mkaply]]

Attachment: First pass

So I originally made it a part of userPrefs, but that just seemed wrong since these aren't userPrefs.

So I created a separate category called "defaultPrefs" and put them in there.

Let me know if you think I'm trying too hard :)

Comment 2

[Chris H-C :chutten]

Comment on attachment 8843047 [details] [diff] [review]
First pass

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

As much as I applaud the enthusiasm, I don't think I like this too much. Maybe I'm just thinking of all the documentation and other stuff that would need to be updated (oh, and the schema too) :)

Passing buck to :gfritzsche as the ultimate arbiter of this, but my vote is to stick it in userPrefs because userPrefs has never been _just_ user-set preferences. It shows any of those prefs that have been changed from their defaults by any means.

Past bad behaviour is generally no excuse for future bad behaviour, but labels are meaningless so <insert philosophy here>.

Comment 3

[Mike Kaply [:mkaply]]

Attachment: More sane patch

Here's a simpler version of the patch for comparison.

Just adds DEFAULT as an option. I looked into adding a test but honestly test_TelemetryEnvironment.js is really overwhelming. And advice on that would be appreciated.

Comment 4

[Chris H-C :chutten]

Comment on attachment 8843122 [details] [diff] [review]
More sane patch

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

As for tests, test_prefWatchPolicies is probably the one most relevant to your interests: https://dxr.mozilla.org/mozilla-central/source/toolkit/components/telemetry/tests/unit/test_TelemetryEnvironment.js#858

Adding couple of new prefs to watch, giving both the new policy, setting one's default (somehow?) and then checking that it is present and the other is absent ought to do it.

::: toolkit/components/telemetry/TelemetryEnvironment.jsm
@@ +876,5 @@
>    _getPrefData() {
>      let prefData = {};
>      for (let [pref, policy] of this._watchedPrefs.entries()) {
> +      if (policy.what == TelemetryEnvironment.RECORD_DEFAULTPREF_VALUE) {
> +        // For default prefs,  make sure they exist

double-space on this line

@@ +879,5 @@
> +      if (policy.what == TelemetryEnvironment.RECORD_DEFAULTPREF_VALUE) {
> +        // For default prefs,  make sure they exist
> +        if (!Preferences.has(pref)) {
> +          continue;
> +        }

You'll need to set prefValue to something.

Comment 5

[Mike Kaply [:mkaply]]

(In reply to Chris H-C :chutten from comment #4)

> double-space on this line
> 
> @@ +879,5 @@
> > +      if (policy.what == TelemetryEnvironment.RECORD_DEFAULTPREF_VALUE) {
> > +        // For default prefs,  make sure they exist
> > +        if (!Preferences.has(pref)) {
> > +          continue;
> > +        }
> 
> You'll need to set prefValue to something.

This is the same as this case, isn't it?:

https://dxr.mozilla.org/mozilla-central/source/toolkit/components/telemetry/TelemetryEnvironment.jsm#878

      // Only record preferences if they are non-default
      if (!Preferences.isSet(pref)) {
        continue;
      }

And we don't set prefValue in that case (because we're not putting anything in prefData)

Comment 6

[Mike Kaply [:mkaply]]

Attachment: Patch with tests

Patch with working test.

With regards to your prefValue comment, if the pref doesn't exist, we'll simply continue the same as the non default case.

If it does exist, we'll set its value the same way we set a non default value.

Comment 7

[Chris H-C :chutten]

Comment on attachment 8844113 [details] [diff] [review]
Patch with tests

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

Ah, good. I mentally had the "else" that set prefValue as a "else if (policy.what == TelemetryEnvironment.RECORD_PREF_VALUE)" which was my mistake.

LGTM

Comment 8

[Mike Kaply [:mkaply]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4031cd5354cfac3db99b6e7c440639243c9f86cd
Bug 1343943 - Add support for recording default prefs in telemetry. r=chutten

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4031cd5354cf

Comment 11

[Tim Smith (inactive) 👨‍🔬 [:tdsmith]]

The language in this patch keeps tripping me up and I want to make sure I understand it.

The comment in TelemetryEnvironment.jsm for RECORD_DEFAULTPREF_VALUE says:

We only record default pref if set

I believe what RECORD_DEFAULTPREF_VALUE actually does is to record any active value for the pref, irrespective of branch -- if a value is set on the user branch, the user branch value will be reported.

If that's right, I want to land a followup to clarify. :chutten, am I reading this right? https://searchfox.org/mozilla-central/rev/652014ca1183c56bc5f04daf01af180d4e50a91c/toolkit/components/telemetry/app/TelemetryEnvironment.jsm#1291-1325

Comment 12

[Chris H-C :chutten]

From my reading of the code it will record a value from any branch so long as its value isn't invalid. If it's set on any branch, it'll report either the value or the state (respectively).

I'm... not sure what mkaply meant by the comment. ni?mkaply for comment.

Comment 13

[Mike Kaply [:mkaply]]

Yeah, I'm not sure exactly what I meant there either.

But you are correct looking at the code.

We record the value of the preference whether it be a default or user preference.

Originally the code explicitly checked for prefhasuservalue and bailed.

So the commend should say

// Record the value even if it is default only

or something like that