1369844 - Add a "kill-switch" switchboard flag for Sync Ping

Status: RESOLVED FIXED

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

Description

[:Grisha Kruglov]

I anticipate that if anything's going to give us grief with Bug 1308337 landing, it's the "bundle, persist and upload" bits.

This bug tracks putting that functionality behind a flipped-on switchboard flag, so that we're on a safe side of landing that patch series.

Comment 1

[:Grisha Kruglov]

Attachment: Bug 1369844 - Add switchboard experiment for background telemetry processing

Intended use is a "kill-switch" for processing/upload of background telemetry.

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

Comment 2

[:Grisha Kruglov]

I've considered adding multiple flags for more granular control - processing, storage, upload - but I think this is "good enough" and doesn't pose a threat of creating a dangerous combination of flags (e.g. process, store but never upload).

Comment 3

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8875502 [details]
Bug 1369844 - Add switchboard experiment for background telemetry processing

https://reviewboard.mozilla.org/r/146932/#review151356

I'd like to see this again after clarifying how it's really going to work: enabled/disabled by default?  Is it a "kill-switch" that you flip "on" for "kill" or that you flip "off" to "kill"?

::: mobile/android/base/java/org/mozilla/gecko/Experiments.java:80
(Diff revision 1)
>      public static final String FULL_BOOKMARK_MANAGEMENT = "full-bookmark-management";
>  
>      // Enable Leanplum SDK
>      public static final String LEANPLUM = "leanplum-start";
>  
> +    // Telemetry Background Ping Processing.

We should follow suit with the file and use "enable" or "disable" in the comment consider.

I think we should use the same word in the name and the string, too -- if the default is "enable" and this is a kill-switch, let's call it "disable" throughout.

::: mobile/android/base/java/org/mozilla/gecko/telemetry/TelemetryBackgroundReceiver.java:90
(Diff revision 1)
>  
>      @Override
>      public void onReceive(final Context context, final Intent intent) {
>          Log.i(LOG_TAG, "Handling background telemetry broadcast");
>  
> +        if (!SwitchBoard.isInExperiment(context, Experiments.TELEMETRY_PROCESSING_BACKGROUND_PINGS)) {

So, clearly something needs to be communicated.  Is the default state "enabled" (from the Switchboard server)?

Comment 4

[:Grisha Kruglov]

(In reply to Nick Alexander :nalexander from comment #3)
> I think we should use the same word in the name and the string, too -- if
> the default is "enable" and this is a kill-switch, let's call it "disable"
> throughout.

I'm fine with either approach - default enabled for 100% of users, or default disabled for 0% of users. I slightly prefer to not have double negations ("not enabled for noone"), but I don't feel strongly about it.

Comment 5

[:Grisha Kruglov]

Comment on attachment 8875502 [details]
Bug 1369844 - Add switchboard experiment for background telemetry processing

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

Comment 6

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8875502 [details]
Bug 1369844 - Add switchboard experiment for background telemetry processing

https://reviewboard.mozilla.org/r/146932/#review151546

::: mobile/android/base/java/org/mozilla/gecko/Experiments.java:81
(Diff revision 2)
>  
>      // Enable Leanplum SDK
>      public static final String LEANPLUM = "leanplum-start";
>  
> +    // Enable processing of background telemetry.
> +    public static final String ENABLE_PROCESSING_BACKGROUND_TELEMETRY = "process-background-telemetry";

nit: I'd prefer FOO_BAR_BAZ to agree with "foo-bar-baz", for ease of grep/dxr/searchfox.  But if you're already committed upstream, that's OK.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a43b9bebbc13
Add switchboard experiment for background telemetry processing r=nalexander

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a43b9bebbc13