1405490 - Fennec telemetry service doesn't honor the telemetry.toolkit.send.overrideOfficialCheck preference

Status: RESOLVED INCOMPLETE

(Firefox for Android Graveyard :: Metrics, enhancement, P3)

Description

[Gabriele Svelto [:gsvelto] (PTO)]

On desktop Firefox the telemetry.toolkit.send.overrideOfficialCheck can be used to override the compile-time switch used to disable telemetry on non-official builds. This is both useful for manual testing and a requirement for writing and running local unit tests.

Comment 1

[Gabriele Svelto [:gsvelto] (PTO)]

See:

https://dxr.mozilla.org/mozilla-central/rev/11fe0a2895aab26c57bcfe61b3041d7837e954cd/toolkit/components/telemetry/docs/internals/preferences.rst#149

Comment 2

[Gabriele Svelto [:gsvelto] (PTO)]

Attachment: Bug 1405490 - Make Fennec honor the manual override to send telemetry from non-official builds;

Review commit: https://reviewboard.mozilla.org/r/186176/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/186176/

Comment 3

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8914931 [details]
Bug 1405490 - Make Fennec honor the manual override to send telemetry from non-official builds;

I think this looks fine, but I'd like to get mcomella's opinion.

mcomella: is it true that TelemetryUploadService always has access to a running Gecko, so that it can ask for Gecko prefs?  (I see other Gecko prefs requests in the relevant code, but I'm not certain the TelemetryUploadService lifecycle is guaranteed to be encapsulated by the Gecko lifecycle.)  Can you confirm?

Comment 4

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8914931 [details]
Bug 1405490 - Make Fennec honor the manual override to send telemetry from non-official builds;

https://reviewboard.mozilla.org/r/186176/#review192438

Gabriele, what are you trying to do with `telemetry.toolkit.send.overrideOfficialCheck`? We have several different telemetry systems (that I don't fully understand) which may be causing some confusion:
* TelemetryUploadService, which you're trying to change, is our Java based telemetry. This handles the Core ping and the Sync ping and afaik the settings are detached from Gecko preferences and I don't see a particular reason to change that
* Gecko uploads a few pings, including the crash ping, and I don't really know how this works. I assume this would honor gecko-based preferences.

Feel free to ping me on IRC/Slack to help figure out what you're trying to do. For context, I wrote the original Java telemetry & the core ping.

In any case, I don't think this patch is correctly reading Gecko preferences (see below).

::: mobile/android/base/java/org/mozilla/gecko/preferences/GeckoPreferences.java:125
(Diff revision 1)
>  
>      // These match keys in resources/xml*/preferences*.xml
>      private static final String PREFS_SEARCH_RESTORE_DEFAULTS = NON_PREF_PREFIX + "search.restore_defaults";
>      private static final String PREFS_DATA_REPORTING_PREFERENCES = NON_PREF_PREFIX + "datareporting.preferences";
>      private static final String PREFS_TELEMETRY_ENABLED = "toolkit.telemetry.enabled";
> +    public static final String PREFS_TELEMETRY_SEND_OVERRIDE_OFFICIAL_CHECK = "toolkit.telemetry.send.overrideOfficialCheck";

nit: this preference should not appear in this block of keys because it doesn't match a key in the preference file (as the comment at the top of this block specifies).

The preference files are used to create preferences in the UI settings that are backed by gecko preferences.

Perhaps this should be up by PREFS_HEALTHREPORT_UPLOAD_ENABLED?

::: mobile/android/base/java/org/mozilla/gecko/preferences/GeckoPreferences.java:673
(Diff revision 1)
>                          preferences.removePreference(pref);
>                          i--;
>                          continue;
>                      }
> -                } else if (PREFS_TELEMETRY_ENABLED.equals(key)) {
> +                } else if (PREFS_TELEMETRY_ENABLED.equals(key) ||
> +                           PREFS_TELEMETRY_SEND_OVERRIDE_OFFICIAL_CHECK.equals(key)) {

This loop is meant to configure the UI preferences from the xml and since PREFS_TELEMETRY_SEND... doesn't appear in the preferences xml, it's key will never appear and this line is unnecessary.

::: mobile/android/base/java/org/mozilla/gecko/telemetry/TelemetryUploadService.java:187
(Diff revision 1)
>       *
>       * Note that this method logs debug statements when upload is disabled.
>       */
>      public static boolean isUploadEnabledByAppConfig(final Context context) {
> -        if (!TelemetryConstants.UPLOAD_ENABLED) {
> +        if (!TelemetryConstants.UPLOAD_ENABLED &&
> +            !GeckoPreferences.getBooleanPref(context, GeckoPreferences.PREFS_TELEMETRY_SEND_OVERRIDE_OFFICIAL_CHECK, false)) {

I think this is retrieving the value from SharedPreferences, not from Gecko: `getBooleanPref` just gets SharedPreferences and retrieves the value from there and as far as I can tell, we never sync these prefs to Gecko.

I think what you actually want is to make an async query to gecko to get the preference and then cache the preference. It looks like we already do this for the `TelemetryUploadService` in `TelemetryPreferences`, so I think you should add your preference/caching there: http://searchfox.org/mozilla-central/rev/8efd128b48cdae3a6c6862795ce618aa1ca1c0b4/mobile/android/base/java/org/mozilla/gecko/telemetry/TelemetryPreferences.java#16

Note that since the uploader runs before Gecko is available and we cache the values after Gecko is first available, these values will only be available the second time gecko is run, which is not ideal for automated testing (we have other checks to disable telemetry if we're in automation).

---

Notes:
- `GeckoPreferences` is intended to be used as an Android Activity and it might be non-trivial to separate out its gecko syncing parts from its Ui parts
- `GeckoSharedPrefs` is a misnomer: for legacy reasons, we namespace our classes with `Gecko` even if they're not really `Gecko` persay, e.g. `GeckoSharedPrefs` is just SharedPreferences with profiles.

Comment 5

[Nick Alexander :nalexander [he/him]]

(In reply to Michael Comella (:mcomella) from comment #4)
> Comment on attachment 8914931 [details]
> Bug 1405490 - Make Fennec honor the manual override to send telemetry from
> non-official builds;
> 
> https://reviewboard.mozilla.org/r/186176/#review192438

<snip> Great review, mcomella!  Glad I asked -- I'd completely forgotten GeckoSharedPrefs was not GeckoPrefs.  Thanks!

Comment 6

[Gabriele Svelto [:gsvelto] (PTO)]

Comment on attachment 8914931 [details]
Bug 1405490 - Make Fennec honor the manual override to send telemetry from non-official builds;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/186176/diff/1-2/

Comment 7

[Gabriele Svelto [:gsvelto] (PTO)]

Sorry for the long hiatus, this work was sidelined by P1 stuff but now I should be back on track. I've addressed all the review comments and reading the preference this way seems to work correctly. I've manually verified that flipping the pref controls the ping as it's supposed to do and the general behavior is identical with what we have on desktop.

Comment 8

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Hey Gabriele, I just got back from PTO – I'll likely get time tomorrow to look at this.

Comment 9

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8914931 [details]
Bug 1405490 - Make Fennec honor the manual override to send telemetry from non-official builds;

https://reviewboard.mozilla.org/r/186176/#review209164

Nice – I like how straightforward these changes are.

I think we should move where the new preference is accessed from for correctness so with that change, r+ (or if you can convince me otherwise :).

::: mobile/android/base/java/org/mozilla/gecko/telemetry/TelemetryPreferences.java:26
(Diff revision 2)
>   */
>  public class TelemetryPreferences {
>      private TelemetryPreferences() {}
>  
> +    private static final String GECKO_PREF_SEND_OVERRIDE = "toolkit.telemetry.send.overrideOfficialCheck";
> +    private static final String SHARED_PREF_SEND_OVERRIDE = "toolkit-sendOverrideOfficialCheck";

nit: the shared preferences telemetry keys should be namespaced to the same value, i.e. `telemetry-sendOverrideOfficialCheck`

::: mobile/android/base/java/org/mozilla/gecko/telemetry/TelemetryUploadService.java:188
(Diff revision 2)
>       *
>       * Note that this method logs debug statements when upload is disabled.
>       */
> -    public static boolean isUploadEnabledByAppConfig(final Context context) {
> -        if (!TelemetryConstants.UPLOAD_ENABLED) {
> +    public static boolean isUploadEnabledByAppConfig(final Context context, final String profileName) {
> +        if (!TelemetryConstants.UPLOAD_ENABLED &&
> +            !TelemetryPreferences.getSendOverrideOfficialCheck(context, profileName)) {

Will this still work if we move this to `isUploadEnabledByProfileConfig`? I don't like that the added profileName parameter breaks the app/profile level abstraction.

Note that the log statement needs to change for sendOverride too – it's not really compile-time disabled.

`isUploadEnabledByProfileConfig` will be called before the UploadService is requested to run so the service should never run if the profile config doesn't return true. The `isUploadEnabledByAppConfig` call in the service is just a safeguard in case something goes wrong (e.g. someone launches the service from elsewhere).

For historical context: it was probably an oversight in the original code that I could access the profile name from here but didn't. However, it'd require research whether accessing the full profile from here is acceptable.

Comment 10

[Gabriele Svelto [:gsvelto] (PTO)]

(In reply to Michael Comella (:mcomella) from comment #9)
> Comment on attachment 8914931 [details]
> [...]
> Will this still work if we move this to `isUploadEnabledByProfileConfig`? I
> don't like that the added profileName parameter breaks the app/profile level
> abstraction.

That shouldn't be a problem. I'll change it to move the test into `isUploadEnabledByProfileConfig` and re-test just in case.

Comment 11

[Gabriele Svelto [:gsvelto] (PTO)]

Comment on attachment 8914931 [details]
Bug 1405490 - Make Fennec honor the manual override to send telemetry from non-official builds;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/186176/diff/2-3/

Comment 12

[Gabriele Svelto [:gsvelto] (PTO)]

Patch updated with review comments addressed, everything seems to be working fine.

Comment 13

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8914931 [details]
Bug 1405490 - Make Fennec honor the manual override to send telemetry from non-official builds;

https://reviewboard.mozilla.org/r/186176/#review210836

Looks good if UPLOAD_ENABLED gets moved back to where it was.

::: mobile/android/base/java/org/mozilla/gecko/telemetry/TelemetryUploadService.java
(Diff revision 3)
>       * You may wish to also check if the network is connected when calling this method.
>       *
>       * Note that this method logs debug statements when upload is disabled.
>       */
>      public static boolean isUploadEnabledByAppConfig(final Context context) {
> -        if (!TelemetryConstants.UPLOAD_ENABLED) {

UPLOAD_ENABLED should stay in the `isUploadEnabledByAppConfig` – is there a reason you moved it too?

Comment 14

[BMO Automation]

We have completed our launch of our new Firefox on Android. The development of the new versions use GitHub for issue tracking. If the bug report still reproduces in a current version of [Firefox on Android nightly](https://play.google.com/store/apps/details?id=org.mozilla.fenix) an issue can be reported at the [Fenix GitHub project](https://github.com/mozilla-mobile/fenix/). If you want to discuss your report please use [Mozilla's chat](https://wiki.mozilla.org/Matrix#Connect_to_Matrix) server https://chat.mozilla.org and join the [#fenix](https://chat.mozilla.org/#/room/#fenix:mozilla.org) channel.