Make toolkit.telemetry.enabled pref reflect release/pre-release channels

VERIFIED FIXED in Firefox 58

Status

()

enhancement
P1
normal
VERIFIED FIXED
2 years ago
2 years ago

People

(Reporter: gfritzsche, Assigned: chutten)

Tracking

(Depends on 1 bug, Blocks 1 bug)

Trunk
mozilla58
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox57 wontfix, firefox58 fixed)

Details

Attachments

(2 attachments, 1 obsolete attachment)

This is per the preferences changes plan in here:
https://docs.google.com/document/d/12U_s9zHvpt7iGEMF5DazSzzx3whGbCnVWEsZTyclta0/edit#

"toolkit.telemetry.enabled" should now be "true" on pre-release channels, "false" on release channels.

Fennec/Android may not be able to support this behavior yet.
Reporter

Updated

2 years ago
Blocks: 1406392
Reporter

Updated

2 years ago
No longer blocks: 1406392
Assignee

Updated

2 years ago
Assignee: nobody → chutten
Priority: P2 → P1
Assignee

Updated

2 years ago
Status: NEW → ASSIGNED
Assignee

Updated

2 years ago
Depends on: 1406425
Assignee

Comment 1

2 years ago
We have a bit of a problem with the small %ge of users on pre-release channels that have left base collection on, but have turned extended collection off.

IOW: they unchecked the second box in the old preferences.

How do we communicate to these users that their configuration is no longer supported? Or, since these are pre-release users do we assume they self-educate and/or if they had issues with the change they would have already brought them up with the UI redesign?
Reporter

Comment 2

2 years ago
I think the important and visible change here was the UI for the preferences changing.
This was accompanied by a set of related work, outlined here:
https://groups.google.com/d/msg/mozilla.dev.platform/Ed4pLE8kOYU/yju5FSW6AgAJ
https://blog.mozilla.org/futurereleases/2017/09/06/data-just-living/

This bug would be a follow-up to make the internal behavior match what already happened, so i think communication is not a concern here.
Assignee

Comment 3

2 years ago
So I'm running into a little problem with SpecialPowers and locking the pref. It turns out that content processes don't (can't?) know when preferences are locked. SpecialPowers is how mochitests set and check preferences. I did a little test where I asked to check whether a pref was locked before allowing it to be set and every e10s test failed. Which was fun.

The mechanism is this: setPrefEnv sets and then waits for the pref set to notify back, resolving a promise. If the pref set doesn't notify back it hangs, and the test times out. Locked prefs do not notify back.

I see a few possible ways through this:

1) "Blacklist" toolkit.telemetry.enabled in SpecialPowers so that it doesn't wait for the pref to be set and resolves the promise immediately. (This is what it would do if it could tell that t.t.e is locked)
2) Fix the tests using toolkit.telemetry.enabled to _not_ use it. All of the tests I've seen so far that use it also mock out canRecordExtended, so they don't even need to mess with the pref in the first place. This would be very satisfying, but would remove all those lovely good first bugs I filed recently.
3) Modify preferences to notify on the not-change of a locked pref. I'm not sure that this is a good idea.

ni?Alessio, what do you think? How does this jive with your canRecordExtended work? :gfritzsche says you're running into test fun as well.
Flags: needinfo?(alessio.placitelli)
(In reply to Chris H-C :chutten from comment #3)
> I see a few possible ways through this:
> 
> 1) "Blacklist" toolkit.telemetry.enabled in SpecialPowers so that it doesn't
> wait for the pref to be set and resolves the promise immediately. (This is
> what it would do if it could tell that t.t.e is locked)
> 2) Fix the tests using toolkit.telemetry.enabled to _not_ use it. All of the
> tests I've seen so far that use it also mock out canRecordExtended, so they
> don't even need to mess with the pref in the first place. This would be very
> satisfying, but would remove all those lovely good first bugs I filed
> recently.

I think this is the ideal solution: "toolkit.telemetry.enabled", in general, doesn't really do anything useful in *mochitests* because Firefox isn't really restarting. If a particular test setting that pref gets executed after the Telemetry init completes, then |canRecordExtended| won't pick up the new pref value. And that's the reason why tests are setting both things most of the times.

I'd remove the toolkit.telemetry.enabled. I'm unlinking the pref from canRecordExtended anyway.

We could still test pref locking in Telemetry's xpcshell tests. We should have better luck there.

> 3) Modify preferences to notify on the not-change of a locked pref. I'm not
> sure that this is a good idea.
> 
> ni?Alessio, what do you think? How does this jive with your
> canRecordExtended work? :gfritzsche says you're running into test fun as
> well.

Yes :-\ I'm having my share of fun :D Georg was of course right in suggesting to introduce an helper test preference: xpcshell tests are restarting telemetry so often that it's much simpler to set a pref compared to add a |canRecordExtended = true;| call after each |TelemetryController.testReset()|.
Flags: needinfo?(alessio.placitelli)
Assignee

Comment 5

2 years ago
An interesting case is reftests. layout/tools/reftest/reftest-preferences.js specifically sets both unified and enabled to false to ensure we don't try to hit the network.

I'm thinking this could be an actual proper use of the pref, as for Android it'll work as intended. I'm going to add datareporting.healthreport.uploadEnabled to the list to cover the bases.
(In reply to Chris H-C :chutten from comment #5)
> An interesting case is reftests. layout/tools/reftest/reftest-preferences.js
> specifically sets both unified and enabled to false to ensure we don't try
> to hit the network.
> 
> I'm thinking this could be an actual proper use of the pref, as for Android
> it'll work as intended. I'm going to add
> datareporting.healthreport.uploadEnabled to the list to cover the bases.

Yup, that case is legit and your intuition of using uploadEnabled sounds fine!
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Comment 9

2 years ago
trypush for an unsquashed version of all these patches can be found here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=968f5943294591fea4b58f3da673061f4f37e516

Near as I can tell the oranges are all unrelated intermittents, though there are still some retriggers outstanding.
(In reply to Chris H-C :chutten from comment #9)
> trypush for an unsquashed version of all these patches can be found here:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=968f5943294591fea4b58f3da673061f4f37e516
> 
> Near as I can tell the oranges are all unrelated intermittents, though there
> are still some retriggers outstanding.

There seems to be a potentially related failure: https://treeherder.mozilla.org/#/jobs?repo=try&revision=968f5943294591fea4b58f3da673061f4f37e516&selectedJob=139217865 (Linux x64 debug - TV suite)
Comment on attachment 8921563 [details]
bug 1406391 - Remove toolkit.telemetry.enabled manipulation from tests

https://reviewboard.mozilla.org/r/192576/#review197982

::: browser/experiments/test/xpcshell/head.js
(Diff revision 1)
>    });
>  }
> -
> -// Experiments require Telemetry to be enabled, and that's not true for debug
> -// builds. Let's just enable it here instead of going through each test.
> -Services.prefs.setBoolPref(PREF_TELEMETRY_ENABLED, true);

Experiments still need to check canRecordExtended, so let's set it here.

This will also make it possible for test_telemetry_disabled.js to be "saved".

::: browser/experiments/test/xpcshell/xpcshell.ini
(Diff revision 1)
>  [test_cacherace.js]
>  [test_conditions.js]
>  [test_disableExperiments.js]
>  [test_fetch.js]
>  [test_telemetry.js]
> -[test_telemetry_disabled.js]

Let's keep this test, since we want to retain this behaviour.

::: js/src/tests/user.js
(Diff revision 1)
>  user_pref("browser.EULA.override", true);
>  user_pref("javascript.options.strict", false);
>  user_pref("javascript.options.werror", false);
>  user_pref("toolkit.startup.max_resumed_crashes", -1);
>  user_pref("security.turn_off_all_security_so_that_viruses_can_take_over_this_computer", true);
> -user_pref("toolkit.telemetry.enabled", false);

Can you change this to datareporting.healthreport.uploadEnabled = false?

::: toolkit/components/telemetry/tests/unit/test_TelemetrySend.js
(Diff revision 1)
>      return;
>    }
>  
>    await TelemetrySend.setup(true);
>  
> -  let origTelemetryEnabled = Services.prefs.getBoolPref(TelemetryUtils.Preferences.TelemetryEnabled);

Do we need to retain this part of the test for Fennec/Android?

Comment 12

2 years ago
mozreview-review
Comment on attachment 8921562 [details]
bug 1406391 - Lock toolkit.telemetry.enabled based on channel

https://reviewboard.mozilla.org/r/192578/#review198112

I have some questions before I can OK this.

::: commit-message-967c9:1
(Diff revision 1)
> +bug 1406391 - Lock toolkit.telemetry.enabled based on channel r?froydnj
> +

This commit message should describe why we are making the change.  It shouldn't link the Google doc, but should condense the important information contained in the Google doc.  I read through the bug and the Google doc, and I'm still not clear what the desired behavior is here.

::: modules/libpref/Preferences.cpp:4858
(Diff revision 1)
>  
>    rv = pref_LoadPrefsInDirList(NS_APP_PREFS_DEFAULTS_DIR_LIST);
>    NS_ENSURE_SUCCESS(
>      rv, Err("pref_LoadPrefsInDirList(NS_APP_PREFS_DEFAULTS_DIR_LIST) failed"));
>  
> +#ifdef MOZ_WIDGET_ANDROID

Why are we not enabling this new behavior for Android?

::: modules/libpref/Preferences.cpp:4878
(Diff revision 1)
> +  if (!strcmp(NS_STRINGIFY(MOZ_UPDATE_CHANNEL), "nightly") ||
> +      !strcmp(NS_STRINGIFY(MOZ_UPDATE_CHANNEL), "aurora") ||
> +      !strcmp(NS_STRINGIFY(MOZ_UPDATE_CHANNEL), "beta")) {

Why is this not just `#ifdef MOZ_TELEMETRY_ON_BY_DEFAULT` and why are we changing the defaults for actual behavior from what `MOZ_TELEMETRY_ON_BY_DEFAULT` means?

::: modules/libpref/Preferences.cpp:4885
(Diff revision 1)
> +      !strcmp(NS_STRINGIFY(MOZ_UPDATE_CHANNEL), "beta")) {
> +    PREF_SetBoolPref(kTelemetryPref, true, true);
> +  } else {
> +    PREF_SetBoolPref(kTelemetryPref, false, true);
> +  }
> +  PREF_LockPref(kTelemetryPref, true);

Why are we not locking the pref for Android?

And we're locking unconditionally?  We're not permitting the user to change this value from `about:preferences`?
Attachment #8921562 - Flags: review?(nfroyd)
Assignee

Comment 13

2 years ago
mozreview-review-reply
Comment on attachment 8921562 [details]
bug 1406391 - Lock toolkit.telemetry.enabled based on channel

https://reviewboard.mozilla.org/r/192578/#review198112

> This commit message should describe why we are making the change.  It shouldn't link the Google doc, but should condense the important information contained in the Google doc.  I read through the bug and the Google doc, and I'm still not clear what the desired behavior is here.

Ack, sorry. I'm usually too-verbose with my commit messages. I'll get this fixed.

> Why are we not enabling this new behavior for Android?

Android doesn't use "Unified Telemetry". The behaviour of "toolkit.telemetry.enabled" is different if !UT... namely, it does what it says it does: it enables/disables Telemetry.

In UT Land (Firefox Desktop) t.t.e only controls "extended collection"... namely the extra 80-90% of histograms and miscellaneous data we collect only from the smaller prerelease populations because we don't want to be swamped (and because they often don't have tests to make sure they don't break on release).

> Why is this not just `#ifdef MOZ_TELEMETRY_ON_BY_DEFAULT` and why are we changing the defaults for actual behavior from what `MOZ_TELEMETRY_ON_BY_DEFAULT` means?

MOZ_TELEMETRY_ON_BY_DEFAULT is 1 when MOZ_TELEMETRY_REPORTING is 1 and we're not RELEASE OR BETA. We want to disable extended Telemetry on anything that isn't pre-release.

An example: On channel "gardiflax" we don't want or need extended data.

...or are you asking why don't I make the change to MOZ_TELEMETRY_ON_BY_DEFAULT? That's a little of "Don't mess with Android" mixed with a little "At the time I didn't understand MOZ_TELEMETRY_ON_BY_DEFAULT, so don't mess with it". If you'd prefer that we adjust MOZ_TELEMETRY_ON_BY_DEFAULT to be used for determining whether we want pre-release/extended collection or just basic collection... that'll require a little dancing around Android but it might be possible.

> Why are we not locking the pref for Android?
> 
> And we're locking unconditionally?  We're not permitting the user to change this value from `about:preferences`?

This value cannot be changed from about:preferences since Firefox... 55? 56?. The UI refactor removed the second checkbox from Firefox Desktop's UI, so now there's just the one "Allow Nightly to send technical and interaction data to Mozilla" left. That checkbox controls "datareporting.healthreport.uploadEnabled" which is Unified Telemetry's version of "toolkit.telemetry.enabled" (iow, the Off Switch)

I'm attempting to make a bit of a study of the current state of affairs for Telemetry build switches, constants, and prefs over in bug 1411266. Android still using old Telemetry makes me sad, and forces us to include rather a lot more code than I'd like. On the other hand, it _is_ a special flower and should be treated as such, what with its limited battery and data requirements.

We're locking unconditionally because we want to turn this into (for UT land) a pref that simply tells us whether we're on pre-release Firefox or not. This is Step 1 in deprecating and then removing toolkit.telemetry.enabled entirely (from Unified Telemetry builds). We can't get through all of the rejiggering in Q4 so we're just trying to align with the UI Preferences changes for now.
Assignee

Comment 14

2 years ago
(In reply to Alessio Placitelli [:Dexter] from comment #10)
> (In reply to Chris H-C :chutten from comment #9)
> > trypush for an unsquashed version of all these patches can be found here:
> > https://treeherder.mozilla.org/#/
> > jobs?repo=try&revision=968f5943294591fea4b58f3da673061f4f37e516
> > 
> > Near as I can tell the oranges are all unrelated intermittents, though there
> > are still some retriggers outstanding.
> 
> There seems to be a potentially related failure:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=968f5943294591fea4b58f3da673061f4f37e516&selectedJob=1
> 39217865 (Linux x64 debug - TV suite)

That seems to be bug 1411567 (which looks like it might be a ping-too-big error)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment on attachment 8921563 [details]
bug 1406391 - Remove toolkit.telemetry.enabled manipulation from tests

https://reviewboard.mozilla.org/r/192576/#review198464

This looks good, thank you Chris!
Attachment #8921563 - Flags: review?(alessio.placitelli) → review+

Comment 18

2 years ago
mozreview-review
Comment on attachment 8921562 [details]
bug 1406391 - Lock toolkit.telemetry.enabled based on channel

https://reviewboard.mozilla.org/r/192578/#review198606

Thank you for the explanation; I agree that this code conforms to your explanation. :)

Is this locking of the pref going to cause issues for tests (for instance, submitting pings from our test machines, which probably isn't going to work...), since tests run with telemetry disabled?  My understanding is that telemetry is disabled through prefs on those tests, and that's not going to work very well with locked prefs...

We should try to determine whether tests will blow up (perhaps by pushing tests to try running as though they were beta) before landing this.

::: modules/libpref/Preferences.cpp:4858
(Diff revision 2)
> +#ifdef MOZ_WIDGET_ANDROID
>    // Set up the correct default for toolkit.telemetry.enabled. If this build
>    // has MOZ_TELEMETRY_ON_BY_DEFAULT *or* we're on the beta channel, telemetry
>    // is on by default, otherwise not. This is necessary so that beta users who
>    // are testing final release builds don't flipflop defaults.
>    if (Preferences::GetDefaultType(kTelemetryPref) ==
>        nsIPrefBranch::PREF_INVALID) {
>      bool prerelease = false;
>  #ifdef MOZ_TELEMETRY_ON_BY_DEFAULT
>      prerelease = true;
>  #else
>      nsAutoCString prefValue;
>      Preferences::GetDefaultCString(kChannelPref, prefValue);
>      if (prefValue.EqualsLiteral("beta")) {
>        prerelease = true;
>      }
>  #endif
>      PREF_SetBoolPref(kTelemetryPref, prerelease, true);
>    }
> +#else
> +  if (!strcmp(NS_STRINGIFY(MOZ_UPDATE_CHANNEL), "nightly") ||
> +      !strcmp(NS_STRINGIFY(MOZ_UPDATE_CHANNEL), "aurora") ||
> +      !strcmp(NS_STRINGIFY(MOZ_UPDATE_CHANNEL), "beta")) {
> +    PREF_SetBoolPref(kTelemetryPref, true, true);
> +  } else {
> +    PREF_SetBoolPref(kTelemetryPref, false, true);
> +  }
> +  PREF_LockPref(kTelemetryPref, true);
> +#endif // MOZ_WIDGET_ANDROID

We should have some sort of comment here, something like:

```
For platforms with Unified Telemetry, whether telemetry is enabled by default depends on the update channel.  For platforms without Unified Telemetry (only Android, at this point), whether telemetry is enabled by default is more complex.
```
Attachment #8921562 - Flags: review?(nfroyd) → review+
Comment hidden (mozreview-request)
Assignee

Updated

2 years ago
Attachment #8921563 - Attachment is obsolete: true
Assignee

Comment 20

2 years ago
mozreview-review-reply
Comment on attachment 8921562 [details]
bug 1406391 - Lock toolkit.telemetry.enabled based on channel

https://reviewboard.mozilla.org/r/192578/#review198606

On not-Android t.t.e controls extended data collection, so the problem is more "Do tests work when extended collection is off".

Since development machines have channel=default, my previous try runs have been successfully testing the "t.t.e is locked to the 'off' position".

I've done some local testing with it forced to the 'on' position, and have pushed it to try for a broader look: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f36b8a27816a5516fecf534bbc488e46234fc0cf
Comment hidden (mozreview-request)
Hey Chris, I think something weird happened with the attached patches: the one I r+'d was obsoleted and I was added as a reviewer on the one from Nathan. The latter just have changes to a single file now. Is this intended?
Flags: needinfo?(chutten)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Comment 25

2 years ago
I tried to add a commit to the review, but it turns out it isn't as simple as just pushing the single, new commit. I've squashed it into the experiments change (it disables experiments directly in layout/tools/reftest/reftest-preferences.js) and repushed the two commits. 

It looks like it worked this time.
Flags: needinfo?(chutten)
Comment on attachment 8922784 [details]
bug 1406391 - Remove toolkit.telemetry.enabled manipulation from tests

https://reviewboard.mozilla.org/r/193932/#review199018

Looks good!
Attachment #8922784 - Flags: review?(alessio.placitelli) → review+

Comment 27

2 years ago
Pushed by chutten@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e1f34ba9cecc
Lock toolkit.telemetry.enabled based on channel r=froydnj
https://hg.mozilla.org/integration/mozilla-inbound/rev/15d959b9123e
Remove toolkit.telemetry.enabled manipulation from tests r=Dexter
https://hg.mozilla.org/mozilla-central/rev/e1f34ba9cecc
https://hg.mozilla.org/mozilla-central/rev/15d959b9123e
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
MOZ_UPDATE_CHANNEL, you say? That's "nightly" on nightly builds, and "default" on on-push builds on inbound/autoland/central, so if you go looking at MOZ_UPDATE_CHANNEL, you're likely to draw wrong conclusions, and land code which is untested on the branch where you land it, and not tested until a nightly build runs on some merge cset some time after your patch is merged to central, which is rather suboptimal.

Backed out in https://hg.mozilla.org/mozilla-central/rev/ad08d9064a7d for attempting to contact incoming.telemetry.mozilla.org during (and these are an odd pair of suites, no idea why it's just these) jsreftest-1 and reftest-4 on linux32 and linux64 nightlies, and jsreftest-1 on win32 and win64 nightlies, which is a fatal error, https://treeherder.mozilla.org/logviewer.html#?job_id=140346590&repo=mozilla-central
Status: RESOLVED → REOPENED
Flags: needinfo?(chutten)
Resolution: FIXED → ---
Target Milestone: mozilla58 → ---
Not sure the thing you (thought) you want actually exists in a safe way to use, because what you were trying to do, do things differently on mozilla-release than on beta, is generally a really really bad idea that flies in the face of the entire reason we have a beta tree. The two things that were added to be the safe alternative to looking at MOZ_UPDATE_CHANNEL are NIGHTLY_BUILD and RELEASE_OR_BETA, because for the most part (like, the 99.9999% part) beta should be release in all but having been released. There is the only slightly sketchy EARLY_BETA_OR_EARLIER, which is set for everything from trunk down until the point in the beta schedule when relman unsets it (dunno what that time actually is, but it's possible to find that out), which is probably the closest to what you wanted.
Assignee

Comment 31

2 years ago
Thank you for backing it out when these failures were noticed.

This is interesting in a couple of ways. The first is that the "do something different on beta vs release" is encoded in Preferences.cpp and has hung around there for some time. Whether that was a good idea or not is a question for history, I guess. 

The second is that toolkit.telemetry.enabled doesn't control contact to inbound.telemetry.mozilla.org at all (except on Android, which we specifically exempted from this at this time). t.t.e only affects the amount of data collected and sent, not whether it is collected or sent. I have no idea why certain reftests are trying to ping inbound all of a sudden. datareporting.healthreport.uploadEnabled _is_ disabled for those suites, right? Otherwise it's only been luck so far that it hasn't caught fire like this in the past.

ni?gfritzsche and Dexter for the "beta should essentially be release" comments. We were preserving existing behaviour and knowingly were aiming for a release/prerelease split. It seems as though there are good reasons to not do this, but I don't know how they balance the desire (need?) to have solid extended collection data from our broad beta population. This may require discussion.

:philor, for your part and point of view, how necessary is it that beta report the same Telemetry information as release does? Also, I would appreciate any intuition you might have on the topic of how those reftests would suddenly misbehave. (and whether it's possible to dump the request that was attempted to be sent, as that might be a solid clue)
Flags: needinfo?(philringnalda)
Flags: needinfo?(gfritzsche)
Flags: needinfo?(chutten)
Flags: needinfo?(alessio.placitelli)
My concern is that testing behave the same on beta as it will on release, instead of offering up a nasty surprise similar to that nightly surprise, only starting when we merge a green beta to a suddenly orange release. Other than things that misbehave by looking at MOZ_UPDATE_CHANNEL, there's no difference between beta and release as far as tests are concerned, so unlike trunk-as-beta which gets tested on try many times before a merge, there's absolutely no testing of beta-as-release, so if you break tests only on release, it gets discovered as a sudden release blocker with wailing and tearing of hair and panicked texts and calls to your phone and your manager's phone and your manager's manager's phone. If I were you, I'd talk to relman before treating beta after EARLY_BETA_OR_EARLIER any differently than release.

The closest thing I could come up with to a possible explanation was "the reftest harness doesn't successfully turn telemetry reporting off, and reporting works by waiting for n minutes after startup, maybe 8, then scheduling a report m minutes later, perhaps 7." It doesn't really quite entirely work with the actual data, since there were a few green runs that ran for 15 minutes, and at least one orange one that only ran for 8, but in general the chunks that failed are the longest running chunks. And there are plenty of longer running chunks in other suites, so it must be specific to things run by the reftest harness which run for a while.
Flags: needinfo?(philringnalda)
Reporter

Comment 33

2 years ago
(In reply to Chris H-C :chutten from comment #31)
> The second is that toolkit.telemetry.enabled doesn't control contact to
> inbound.telemetry.mozilla.org at all (except on Android, which we
> specifically exempted from this at this time). t.t.e only affects the amount
> of data collected and sent, not whether it is collected or sent. I have no
> idea why certain reftests are trying to ping inbound all of a sudden.
> datareporting.healthreport.uploadEnabled _is_ disabled for those suites,
> right? Otherwise it's only been luck so far that it hasn't caught fire like
> this in the past.

It's worth checking if those tests are disabling upload. If not we can just disable it and carry on.
Otherwise we need to investigate a bit.

> ni?gfritzsche and Dexter for the "beta should essentially be release"
> comments. We were preserving existing behaviour and knowingly were aiming
> for a release/prerelease split. It seems as though there are good reasons to
> not do this, but I don't know how they balance the desire (need?) to have
> solid extended collection data from our broad beta population. This may
> require discussion.

We already have this behavior for Telemetry. That was a conscious decision and we need to keep that working.
I don't think we need discussion about this part.
Flags: needinfo?(gfritzsche)
Flags: needinfo?(alessio.placitelli)
Assignee

Comment 34

2 years ago
Ah-ha, so it turns out that reftests specifically disable Unified Telemetry when not on Android which is an archaic bit of kit. Upon removing that offending line, we get a much greener reftest setup on the nightly configs (see https://treeherder.mozilla.org/#/jobs?repo=try&revision=b030dbcae5ee2d3b233ff71fb7200fa140cc5218 )

So, presuming there's no other non-Android non-Unified cases lurking in the mud (No shipping version of Firefox gets this wrong), I think we're in a better setup. I'll get this loaded and pushed back onto inbound soonish.

Comment 35

2 years ago
Pushed by chutten@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/726047001b4a
Lock toolkit.telemetry.enabled based on channel r=froydnj
https://hg.mozilla.org/integration/mozilla-inbound/rev/ab441478281c
Remove toolkit.telemetry.enabled manipulation from tests r=Dexter
https://hg.mozilla.org/mozilla-central/rev/726047001b4a
https://hg.mozilla.org/mozilla-central/rev/ab441478281c
Status: REOPENED → RESOLVED
Closed: 2 years ago2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58

Comment 38

2 years ago
Verified as fixed in FF58 - Nightly and in Release and Beta channels builds provided by Allesio. 

The status of the pref was verified in the following platforms and FF versions.

Desktop  (windows 7 x64)
Release - false (locked)
Beta -  true (locked)
Nightly -  true (locked)


Desktop (MacOS 10.13)
Release - false (locked)
Beta -  true (locked)
Nightly -  true (locked)


Desktop (Ubuntu 14.04)
Release - false (locked)
Beta -  true (locked)
Nightly -  true (locked)
	

Android 7.1.2
Release - fase (default)
Beta - true (default)
Nightly - true (default)
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.