Closed Bug 1343943 Opened 6 years ago Closed 6 years ago

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


(Toolkit :: Telemetry, enhancement, P3)




Tracking Status
firefox54 --- wontfix
firefox55 --- fixed


(Reporter: mkaply, Assigned: mkaply)



(Whiteboard: [measurement:client])


(1 file, 2 obsolete files)

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.
Attached patch First pass (obsolete) — Splinter Review
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 :)
Attachment #8843047 - Flags: feedback?(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>.
Attachment #8843047 - Flags: feedback?(chutten) → feedback?(gfritzsche)
Attached patch More sane patch (obsolete) — Splinter Review
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 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:

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.
Attachment #8843047 - Attachment is obsolete: true
Attachment #8843047 - Flags: feedback?(gfritzsche)
Priority: -- → P3
Whiteboard: [measurement:client]
(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?:

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

And we don't set prefValue in that case (because we're not putting anything in prefData)
Attached patch Patch with testsSplinter Review
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.
Attachment #8843122 - Attachment is obsolete: true
Attachment #8844113 - Flags: review?(chutten)
Blocks: 1336560
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.

Attachment #8844113 - Flags: review?(chutten) → review+
Bug 1343943 - Add support for recording default prefs in telemetry. r=chutten
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Duplicate of this bug: 1404238
Blocks: 1404343

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?

Flags: needinfo?(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.

Flags: needinfo?(chutten) → needinfo?(mozilla)

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

Flags: needinfo?(mozilla)
You need to log in before you can comment on or make changes to this bug.