1173720 - Investigate enableTelemetryRecording logic in TelemetryController.jsm

Status: RESOLVED DUPLICATE of bug 1200164

(Toolkit :: Telemetry, defect)

Description

[Alessio Placitelli [:Dexter]]

We need to investigate the |enableTelemetryRecording| logic in [1] to understand if the |#ifdef MOZILLA_OFFICIAL| guard is still relevant or if we can just leave the recording always on.

This would simplify the logic a bit.

[1] - https://hg.mozilla.org/mozilla-central/annotate/95afddf894e3/toolkit/components/telemetry/TelemetryController.jsm#l583

Comment 1

[Georg Fritzsche [:gfritzsche]]

This would also get rid of the need to do test-specific initialization.

I think what we really want is to always record extended data if:
* MOZ_TELEMETRY_REPORTING is set and
* toolkit.telemetry.enabled is set to true

Whether ping upload is enabled is covered by other code.

Vladan, any objections/concerns?

Comment 2

[Vladan Djeric (:vladan)]

I've been away from Telemetry code so I need a refresher. Bear with me

> We need to investigate the |enableTelemetryRecording| logic in [1] to
> understand if the |#ifdef MOZILLA_OFFICIAL| guard is still relevant or if we
> can just leave the recording always on.

The MOZILLA_OFFICIAL guard in combination with Telemetry.isOfficialTelemetry checks whether this is a developer build made with MOZILLA_OFFICIAL but without MOZ_TELEMETRY_REPORTING. The MOZ_TELEMETRY_REPORTING flag is meant to prevent Telemetry submissions from developer builds built with MOZILLA_OFFICIAL, which is still a desirable properly.

I think this is the desired behaviour, correct me if i'm wrong:

- always record base Telemetry for archiving purposes, unless archival pref is false
- always initialize the Telemetry service (so archiving can run)
- record extended Telemetry depending on prefs only
- only upload telemetry if MOZ_TELEMETRY_REPORTING was defined and the prefs allow it too, OR if the regression testing flag is set and prefs allow it too

While we're working on this bug, we should clean up enableTelemetryRecording:

- enableTelemetryRecording needs to be documented better.. trying to figure out canRecordExtended's default value is a pain and it's not clear to the reader why it's done that way. The rules around canRecordBase are obscure as well. A comment header above the function would be ideal
- I'd also suggest renaming isOfficialTelemetry to isOfficialBuild. Telemetry.isOfficialBuild is clearer than Telemetry.isOfficialTelemetry