1200164 - Only do isOfficialTelemetry checks for Telemetry upload

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect, P1)

Description

[Alessio Placitelli [:Dexter]]

Throughout the Telemetry code we disable some behaviours (e.g. the creation of shutdown pings and the deletion of aborted session pings [1]) if we're not running an official build (both MOZILLA_OFFICIAL and MOZ_TELEMETRY_REPORTING are set).

This could cause some confusion during debugging sessions: I think we should restrict those checks only to ping uploads.

[1] - https://dxr.mozilla.org/mozilla-central/rev/f2518b8a7b97b5bb477e94bc9131584007aac887/toolkit/components/telemetry/TelemetrySession.jsm#1792

Comment 2

[Alessio Placitelli [:Dexter]]

More info about this can be found in bug 1173720 comment 2.

Comment 3

[Georg Fritzsche [:gfritzsche]]

Attachment: Only do isOfficialTelemetry checks for Telemetry uploads

Comment 4

[Georg Fritzsche [:gfritzsche]]

Attachment: Make developer builds always default to Telemetry enabled

Comment 5

[Alessio Placitelli [:Dexter]]

Comment on attachment 8698909 [details] [diff] [review]
Only do isOfficialTelemetry checks for Telemetry uploads

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

This looks good and... much appreciated!

::: toolkit/components/telemetry/docs/preferences.rst
@@ +9,1 @@
>  Sending only happens on official builds with ``MOZ_TELEMETRY_REPORTING`` defined.

Nit: Maybe we should keep/mention that official means "defining MOZILLA_OFFICIAL" here.

Comment 6

[Georg Fritzsche [:gfritzsche]]

Attachment: Only do isOfficialTelemetry checks for Telemetry uploads

Comment 7

[Benjamin Smedberg]

Comment on attachment 8698910 [details] [diff] [review]
Make developer builds always default to Telemetry enabled

I don't think this is a good idea. There are many other uses for MOZILLA_OFFICIAL beyond developer builds: I suspect this will affect various linux distros, for example.

I also don't see why it's especially important to have extended-set telemetry on by default for developer builds.

Comment 8

[Georg Fritzsche [:gfritzsche]]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #7)
> I don't think this is a good idea. There are many other uses for
> MOZILLA_OFFICIAL beyond developer builds: I suspect this will affect various
> linux distros, for example.
> 
> I also don't see why it's especially important to have extended-set
> telemetry on by default for developer builds.

I think it would be much better to match the Nightly Telemetry behavior in Firefox dev builds (i.e. default to Telemetry on), some points here are:
* it avoids confusion ("oh, this hasn't recorded yet because Telemetry isn't on!")
* it gives dev builds the same behavior here
* it avoids issues with tests (test blowing up on CI that worked fine in local build due to different defaults)

Do you agree with this if we have a better solution without the side-effects you brought up?

Comment 9

[Georg Fritzsche [:gfritzsche]]

Part 1 pushed to try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e288cbadf1ea

Comment 10

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/762e4af55ba2

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/762e4af55ba2

Comment 12

[Benjamin Smedberg]

In theory if you could have nightly developer builds turn extended collection on (without any sending), while at the same time not change the defaults for external distributors building releases from tarballs, that would be fine. I don't know how specifically you'd accomplish that though.

Comment 13

[Georg Fritzsche [:gfritzsche]]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #12)
> In theory if you could have nightly developer builds turn extended
> collection on (without any sending), while at the same time not change the
> defaults for external distributors building releases from tarballs, that
> would be fine. I don't know how specifically you'd accomplish that though.

Right, ok - that doesn't sound easily doable, moved this to bug 1237954 for consideration.