Closed Bug 1148500 Opened 9 years ago Closed 9 years ago

Make sure we can turn the unified Telemetry behavior off with a pref

Categories

(Toolkit :: Telemetry, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla40
Tracking Status
firefox39 + fixed
firefox40 --- fixed

People

(Reporter: gfritzsche, Assigned: gfritzsche)

References

Details

Attachments

(7 files, 5 obsolete files)

3.61 KB, patch
Dexter
: review+
Details | Diff | Splinter Review
2.40 KB, patch
Dexter
: review+
Details | Diff | Splinter Review
3.81 KB, patch
gfritzsche
: review+
Details | Diff | Splinter Review
3.77 KB, patch
Dexter
: review+
Details | Diff | Splinter Review
1.47 KB, patch
Dexter
: review+
Details | Diff | Splinter Review
13.96 KB, patch
gfritzsche
: review+
Details | Diff | Splinter Review
17.50 KB, patch
Dexter
: review+
Details | Diff | Splinter Review
We should be able to pref off the unified Telemetry behavior with one switch.
Lets verify that.
Same for being able to disable FHR.
Blocks: 1120356
What do you mean by "the unified telemetry behavior"? I think for release management of 39, the thing we need to be able to turn off is disable this entire collection system for release users who haven't opted into telemetry.
Assignee: nobody → gfritzsche
Status: NEW → ASSIGNED
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #2)
> What do you mean by "the unified telemetry behavior"? I think for release
> management of 39, the thing we need to be able to turn off is disable this
> entire collection system for release users who haven't opted into telemetry.

For 39 this currently works fine.
Per Brendan we don't need really need the 39 beta data though at this point, so shouldn't we make sure that we don't take additional submission & storage hits like bug 1140037?
For 40 we need to introduce a switch to disable always initializing the Telemetry feature (result from bug 1137252).

I plan to support a hidden pref that can be set to disable this specific behavior (say "toolkit.telemetry.disableUnified"), then we should be good.
(In reply to Georg Fritzsche [:gfritzsche] from comment #3)
> (In reply to Benjamin Smedberg  [:bsmedberg] from comment #2)
> > What do you mean by "the unified telemetry behavior"? I think for release
> > management of 39, the thing we need to be able to turn off is disable this
> > entire collection system for release users who haven't opted into telemetry.
> 
> For 39 this currently works fine.
> Per Brendan we don't need really need the 39 beta data though at this point,
> so shouldn't we make sure that we don't take additional submission & storage
> hits like bug 1140037?
Flags: needinfo?(benjamin)
I don't understand the proposal yet. For 39 beta, what things are you planning on changing?

* disable the feature when FHR is disabled (a 39 shipping requirement)
* disable ping retention?
* disabled sending of pings other than the saved-session ping?
* other?

Presumably we have to keep sending the saved-session ping, because that's the basis for all the telemetry dashboards. But if we can keep sending the normal main pings, I think that would be helpful to have a larger basis for comparing things like search counts and other metrics.
Flags: needinfo?(benjamin)
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #6)
> I don't understand the proposal yet. For 39 beta, what things are you
> planning on changing?
> 
> * disable the feature when FHR is disabled (a 39 shipping requirement)
> * disable ping retention?
> * disabled sending of pings other than the saved-session ping?
> * other?
> 
> Presumably we have to keep sending the saved-session ping, because that's
> the basis for all the telemetry dashboards. But if we can keep sending the
> normal main pings, I think that would be helpful to have a larger basis for
> comparing things like search counts and other metrics.

Yes, we need to keep sending the saved-session ping. I plan to:
* disabled sending of pings other than the saved-session ping

Ping retention/archiving is not on 39 at this point.
Telemetry recording is not happening on 39 at this point if the Telemetry pref is off.

If we want to collect the main pings on 39 beta we will need further work on 39 (at least bug 1140037) and need to clear this up with release management.
Either way, i want to be able to turn the unified features off in beta (either immediately or in reaction to issues).
We can cover bug 1153252 here too.
Blocks: 1153252
Yes, please fix bug 1140037 before beta. The spikes may cause trouble on the server with Beta request volume.
Attachment #8597258 - Flags: feedback?(alessio.placitelli)
The above patches introduce a toolkit.telemetry.unified pref on Aurora and use it for:
* only initializing the Telemetry modules if toolkit.telemetry.enabled==true or .unified==true
* only sending/triggering the unified "main" pings if .unified==true
* only enabling the aborted-session feature if .unified==true

For the patch stack context see the try push (based on Aurora):
https://treeherder.mozilla.org/#/jobs?repo=try&revision=74074cdae872

Upcoming patches will address 40.
Comment on attachment 8597259 [details] [diff] [review]
Part 2 (Fx 39): Honor toolkit.telemetry.unified pref in Telemetry modules

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

::: toolkit/components/telemetry/TelemetryPing.jsm
@@ +693,5 @@
> +    const enabled = Preferences.get(PREF_ENABLED, false);
> +
> +    // Enable base Telemetry recording.
> +    // We always record base data if unified behavior is on.
> +    Telemetry.canRecordBase = enabled || IS_UNIFIED_TELEMETRY;

Does all.js govern the behaviour of IS_UNIFIED_TELEMETRY also in tests (Mochitests, reftests, etc.)? If that's the case, and tests are crashing, you could need to stub out the telemetry server in runreftest.py and profile.py as done by bug 1137252.
Attachment #8597259 - Flags: feedback?(alessio.placitelli) → feedback+
Attachment #8597258 - Flags: feedback?(alessio.placitelli) → feedback+
Depends on: 1149980
Blocks: 1158317
Attachment #8597258 - Attachment is obsolete: true
Attachment #8597259 - Attachment is obsolete: true
Attachment #8598036 - Flags: review?(alessio.placitelli)
Attachment #8597345 - Attachment is obsolete: true
Cleared up an oversight from friday, now try looks sane:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5bb29b99b645
Attachment #8598034 - Flags: review?(alessio.placitelli) → review+
Attachment #8598036 - Flags: review?(alessio.placitelli) → review+
Comment on attachment 8598035 [details] [diff] [review]
Part 2 (Fx 39): Honor toolkit.telemetry.unified pref in Telemetry modules

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

::: toolkit/components/telemetry/TelemetrySession.jsm
@@ +1590,5 @@
> +            yield this._saveAbortedSessionPing();
> +          }
> +
> +          // Start the scheduler.
> +          TelemetryScheduler.init();

In your previous patch, you had a comment describing why the scheduler init was being skipped. Do you think it could be worth keeping it to explain that this turns off the new unified pings but keeps sending the one in the old format?
Attachment #8598035 - Flags: review?(alessio.placitelli) → review+
Attachment #8598034 - Attachment is obsolete: true
Attachment #8598035 - Attachment is obsolete: true
Attachment #8598036 - Attachment is obsolete: true
Comment on attachment 8598067 [details] [diff] [review]
Part 1 (Fx 40): Introduce a toolkit.telemetry.unified pref

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

Nevermind, this part doesn't really differ from the 39 version.
Attachment #8598067 - Flags: review?(alessio.placitelli) → review+
Addressed review comment and fixed not disabling 'shutdown' ping.
Attachment #8598071 - Attachment is obsolete: true
Attachment #8598071 - Flags: review?(alessio.placitelli)
Attachment #8598505 - Flags: review+
Attachment #8598071 - Attachment is obsolete: false
Attachment #8598036 - Attachment is obsolete: false
Attachment #8598034 - Attachment is obsolete: false
Updated to disable "shutdown" ping too.

(Sorry about the above patch noise, bzexport doesn't seem to work that great for obsoleting)
Attachment #8598071 - Attachment is obsolete: true
Attachment #8598515 - Flags: review?(alessio.placitelli)
Attachment #8598068 - Flags: review?(alessio.placitelli) → review+
Comment on attachment 8598515 [details] [diff] [review]
Part 3 (Fx 40): Honor toolkit.telemetry.unified pref in Telemetry modules

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

It looks good, I just have this doubt:

::: toolkit/components/telemetry/TelemetryController.jsm
@@ -882,5 @@
>  
> -    // Initialize some probes that are kept in their own modules
> -    this._thirdPartyCookies = new ThirdPartyCookieProbe();
> -    this._thirdPartyCookies.init();
> -

Why are you removing this?
Attachment #8598515 - Flags: review?(alessio.placitelli) → feedback+
Comment on attachment 8598072 [details] [diff] [review]
Part 4 (Fx 40): Disable Telemetry recording in reftests

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

::: layout/tools/reftest/runreftest.py
@@ +205,5 @@
>  
>      # Ensure that telemetry is disabled, so we don't connect to the telemetry
>      # server in the middle of the tests.
>      prefs['toolkit.telemetry.enabled'] = False
> +    prefs['toolkit.telemetry.unified'] = False

Do we need to do the same in profile.py [1]?

[1] - https://hg.mozilla.org/mozilla-central/annotate/caf25344f73e/testing/mozbase/mozprofile/mozprofile/profile.py#l372
Attachment #8598072 - Flags: review?(alessio.placitelli) → review+
(In reply to Alessio Placitelli [:Dexter] from comment #28)
> ::: toolkit/components/telemetry/TelemetryController.jsm
> @@ -882,5 @@
> >  
> > -    // Initialize some probes that are kept in their own modules
> > -    this._thirdPartyCookies = new ThirdPartyCookieProbe();
> > -    this._thirdPartyCookies.init();
> > -
> 
> Why are you removing this?

We initialize that in TelemetrySession.jsm already. That module only pushes histograms, so it should be enough to initialize it there.
Attachment #8598515 - Flags: feedback+ → review+
(In reply to Alessio Placitelli [:Dexter] from comment #29)
> ::: layout/tools/reftest/runreftest.py
> @@ +205,5 @@
> >  
> >      # Ensure that telemetry is disabled, so we don't connect to the telemetry
> >      # server in the middle of the tests.
> >      prefs['toolkit.telemetry.enabled'] = False
> > +    prefs['toolkit.telemetry.unified'] = False
> 
> Do we need to do the same in profile.py [1]?

Looks like we should. FWIW, i cleared up what profile.py affects and added it here:
https://developer.mozilla.org/en-US/docs/Mozilla/QA/Automated_testing#Need_to_set_preferences_for_test-suites.3F
Repushed to try (with some other bugs) with minor fix:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9ebe645e2d2f
Minor Android xpcshell issue around the archiving API. Repushed:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=58c713a66be2
Georg, does this need uplift to aurora?
Flags: needinfo?(gfritzsche)
We are currently discussing that on bug 1158317.
Flags: needinfo?(gfritzsche)
No longer blocks: 1158317
You need to log in before you can comment on or make changes to this bug.