Closed Bug 1435753 Opened 6 years ago Closed 6 years ago

canRecordExtended is false in late betas/release candidate Beta in 58+

Categories

(Toolkit :: Telemetry, defect, P1)

Product:
Toolkit
Component:
Telemetry
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla60
Tracking Flags:
Tracking Status
firefox58 --- wontfix
firefox59 --- wontfix
firefox60 --- fixed

People

(Reporter: chutten, Assigned: chutten)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

bug 1435753 - Resume collection of extended data from Release Candidate builds on beta, too
59 bytes, text/x-review-board-request
gfritzsche
: review+
froydnj
: review+
Details 
Since the pref changes in 58, canRecordExtended should be true in all betas (MOZ_UPDATE_CHANNEL=beta). I noticed in my local beta that main pings did not have childPayloads.

After I updated to 59b1, childPayloads were back.

Is this a widespread issue in the population, or was it just me? When did it start, when did it end? Could it be EARLY_BETA_OR_EARLIER rearing its head? Or does MOZ_UPDATE_CHANNEL change for RC builds?

Investigation needed.
As soon as the displayVersion switched from 58.0b14 to 58.0 (release candidate, in other words), things disappear that imply canRecordExtended was false. As soon as it switched to 59.0b3 (not sure what happened to delay me from b1/2) they were back.

So it isn't EARLY_BETA_OR_EARLIER, but it seems to be tied to release candidacy.
This is where I loop in RyanVM and he tells me that he already told me why this happens, but I was insensitive and forgot.
Flags: needinfo?(ryanvm)
+ :mreid, for visibility. tl;dr - Release Candidate beta builds are not sending pre-release telemetry. They are sending release telemetry only.
To summarize the chat I just had with chutten on IRC about this:
* RC builds are shipped off the mozilla-release tree. They are true RC builds in that they're the exact same bits we end up shipping to our release channel users once ready for GA. The updater has logic which handles not changing the update channel for beta users getting the RC builds.
* Given that, you're basically hitting the logic in Preferences.cpp where the update channel is being checked at compile time (https://searchfox.org/mozilla-central/source/modules/libpref/Preferences.cpp#3869) and the channel is "release", even though the runtime channel is going to be "beta" for users getting those builds as RCs prior to GA.
Flags: needinfo?(ryanvm)
So, to find a beta build either MOZ_UPDATE_CHANNEL is beta, or app.update.channel is beta.

This complicate things somewhat. We can quickly augment the logic to include RC beta build data, but this has repercussions for any hopeful future we had where we could determine release/pre-release at compile time (because we can't).
It also makes life more difficult for QA to test anything involving Telemetry collection because it means entirely new builds need creation instead of being able to simply edit channel-prefs.js locally instead.
Comment on attachment 8953218 [details]
bug 1435753 - Resume collection of extended data from Release Candidate builds on beta, too

https://reviewboard.mozilla.org/r/222510/#review228664

Since we're trying to rely on a pref we may add some new test [here](https://searchfox.org/mozilla-central/rev/bd05e3853c6e982e2a35c1cc404b987b2bc914d6/toolkit/components/telemetry/tests/unit/test_TelemetryController.js#606) to switch the channel through the pref, restart `TelemetryController` and make sure that the correct state is reached. Turns out `libpref` also has [gtests](https://searchfox.org/mozilla-central/source/modules/libpref/test/gtest): maybe we can test this behaviour in there?

::: modules/libpref/Preferences.cpp:3901
(Diff revision 1)
>    bool developerBuild = false;
>  #ifndef MOZILLA_OFFICIAL
>    developerBuild = !strcmp(NS_STRINGIFY(MOZ_UPDATE_CHANNEL), "default");
>  #endif
>  
> -  if (!strcmp(NS_STRINGIFY(MOZ_UPDATE_CHANNEL), "nightly") ||
> +  nsAutoCString updateChannelPrefValue;

nit: I think we should add a comment explaining why we're doing this, before we stumble on this again in the future :(

::: modules/libpref/Preferences.cpp:3902
(Diff revision 1)
>  #ifndef MOZILLA_OFFICIAL
>    developerBuild = !strcmp(NS_STRINGIFY(MOZ_UPDATE_CHANNEL), "default");
>  #endif
>  
> -  if (!strcmp(NS_STRINGIFY(MOZ_UPDATE_CHANNEL), "nightly") ||
> -      !strcmp(NS_STRINGIFY(MOZ_UPDATE_CHANNEL), "aurora") ||
> +  nsAutoCString updateChannelPrefValue;
> +  Preferences::GetCString(kChannelPref, updateChannelPrefValue,

Should we *only* trust the pref? Maybe we can just double check the pref if MOZ_UPDATE_CHANNEL is Beta?

::: modules/libpref/Preferences.cpp:3905
(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") || developerBuild) {
> +  nsAutoCString updateChannelPrefValue;
> +  Preferences::GetCString(kChannelPref, updateChannelPrefValue,
> +                          PrefValueKind::Default);
> +
> +  if (updateChannelPrefValue.EqualsLiteral("nightly") ||

Shouldn't we be also changing (or maybe only changing?) [this](https://searchfox.org/mozilla-central/rev/bd05e3853c6e982e2a35c1cc404b987b2bc914d6/toolkit/components/telemetry/TelemetryController.jsm#639-643)?
Attachment #8953218 - Flags: review?(alessio.placitelli)
Hey, :RyanVM, is it possible to catch "We're not sending pre-release telemetry from this release candidate build running on a pre-release user's machine" in CI? What is the RC test environment... does it test pretending to be full-release, or does something, somewhere (like the profile's app.update.channel?) know it's actually going to be served to beta users?
Flags: needinfo?(ryanvm)
Our CI would use whatever update channel the build is set to use by default. Given that the RC builds come from mozilla-release, they'd just use that update channel.
Flags: needinfo?(ryanvm)
Comment on attachment 8953218 [details]
bug 1435753 - Resume collection of extended data from Release Candidate builds on beta, too

https://reviewboard.mozilla.org/r/222510/#review228664

> Should we *only* trust the pref? Maybe we can just double check the pref if MOZ_UPDATE_CHANNEL is Beta?

Do you mean if MOZ_UPDATE_CHANNEL is "release"? Then yeah, I considered it. 

If we can trust app.update.channel, though, it makes testing easier. Fewer custom builds. Instead, just some custom prefs.

And the code is more straightforward this way, though that is of less importance.

> Shouldn't we be also changing (or maybe only changing?) [this](https://searchfox.org/mozilla-central/rev/bd05e3853c6e982e2a35c1cc404b987b2bc914d6/toolkit/components/telemetry/TelemetryController.jsm#639-643)?

Oh right, we definitely should be changing the JS that changes canRecordExtended... it's in the name of the bug and everything. 

toolkit.telemetry.enabled should agree with canRecordExtended, though, so we need to change both.
(In reply to Ryan VanderMeulen [:RyanVM] from comment #10)
> Our CI would use whatever update channel the build is set to use by default.
> Given that the RC builds come from mozilla-release, they'd just use that
> update channel.

This means it is impossible to write an automated test to ensure we don't do this in future. *le sigh*
Comment on attachment 8953218 [details]
bug 1435753 - Resume collection of extended data from Release Candidate builds on beta, too

I'm not qualified to review this.
Attachment #8953218 - Flags: review?(ryanvm)
Comment on attachment 8953218 [details]
bug 1435753 - Resume collection of extended data from Release Candidate builds on beta, too

https://reviewboard.mozilla.org/r/222510/#review228664

> Do you mean if MOZ_UPDATE_CHANNEL is "release"? Then yeah, I considered it. 
> 
> If we can trust app.update.channel, though, it makes testing easier. Fewer custom builds. Instead, just some custom prefs.
> 
> And the code is more straightforward this way, though that is of less importance.

I see. It makes sense your way, but it exposes to some randomness coming from people setting the update channel to weird things (even keeping telemetry enabled) because they don't want to update. I'm not sure how frequent that happens, but probably worth checking if that's a problem.
Comment on attachment 8953218 [details]
bug 1435753 - Resume collection of extended data from Release Candidate builds on beta, too

The patch is currently getting adjusted to feedback, cancelling review in the meantime.
Attachment #8953218 - Flags: review?(gfritzsche)
Attachment #8953218 - Flags: review?(alessio.placitelli)
Comment on attachment 8953218 [details]
bug 1435753 - Resume collection of extended data from Release Candidate builds on beta, too

https://reviewboard.mozilla.org/r/222510/#review229664

I think this makes sense, but it also seems like a decently-sized footgun waiting to hurt other people...do we have any plans to address that?
Attachment #8953218 - Flags: review?(nfroyd) → review+
As far as I know Telemetry is the only system grudgingly permitted to do this trickery. Everyone else uses relman-sanctioned defines to figure out whether something should be on or off: https://wiki.mozilla.org/Platform/Channel-specific_build_defines

+RyanVM for better context than my paraphrasing, and any suggestions for documenting how release candidate builds work.
Flags: needinfo?(ryanvm)
Comment on attachment 8953218 [details]
bug 1435753 - Resume collection of extended data from Release Candidate builds on beta, too

https://reviewboard.mozilla.org/r/222510/#review229876

This looks good to me, cheers.

We should document the behavior (in a separate patch here or in a follow-up).
The missing automation/testing is sad, but unavoidable with the current state.

We'll need to line up QA for the different build configs to make sure that this behaves as expected.

::: modules/libpref/Preferences.cpp:3913
(Diff revision 2)
> -      !strcmp(NS_STRINGIFY(MOZ_UPDATE_CHANNEL), "beta") || developerBuild) {
> +      !strcmp(NS_STRINGIFY(MOZ_UPDATE_CHANNEL), "beta") || developerBuild ||
> +      releaseCandidateOnBeta) {

Nit: Could we put `|| developerBuild` be on a newline for more clear readability?
Attachment #8953218 - Flags: review?(gfritzsche) → review+
Summary: canRecordExtended was false in late betas/release candidate Beta58 → canRecordExtended is false in late betas/release candidate Beta in 58+
(In reply to Chris H-C :chutten from comment #18)
> As far as I know Telemetry is the only system grudgingly permitted to do
> this trickery. Everyone else uses relman-sanctioned defines to figure out
> whether something should be on or off:
> https://wiki.mozilla.org/Platform/Channel-specific_build_defines
> 
> +RyanVM for better context than my paraphrasing, and any suggestions for
> documenting how release candidate builds work.

As far as I know, this is the only instance of compile-time update channel checking we have/had. At least that I know of. As far as documentation goes, I think we just need to make clear that RC builds are just regular release builds that happen to get shipped to the Beta channel. But they're intentionally built as release-ready.
Flags: needinfo?(ryanvm)
Pushed by chutten@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/10bd2edbcc9f
Resume collection of extended data from Release Candidate builds on beta, too r=froydnj,gfritzsche
https://hg.mozilla.org/mozilla-central/rev/10bd2edbcc9f
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox60: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: