Closed Bug 1390095 Opened 7 years ago Closed 7 years ago

Send a duplicate of a users first shutdown ping with pingsender

Categories

(Toolkit :: Telemetry, enhancement, P1)

enhancement

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox57 --- fixed

People

(Reporter: gfritzsche, Assigned: amiyaguchi)

References

Details

Attachments

(2 files, 5 obsolete files)

We want more data on users that churn after the first session.
Getting this data by enabling the shutdown pingsender for a users first session is risky in the short-term, because it will impact DAU/MAU and other metrics (see e.g. bug 1376292 comment 24 and bug 1376292 comment 30). This will need more careful planning.

In the short-term, we can get the same data risk-free by sending a duplicate of the first session shutdown main ping using the ping sender.
We give it a different document type (say "first-shutdown"), so no existing data job would be impacted.
As we duplicate the data, we still send the first "main" ping on a users second session.
Assignee: nobody → gfritzsche
Anthony can look into this bug, cheers.
The next merge is Sep 20, so i would say we safely have up to two weeks to land this (including testing/fallout).

Alessio, can you set this up with some pointers?
Assignee: gfritzsche → amiyaguchi
Flags: needinfo?(alessio.placitelli)
(In reply to Georg Fritzsche [:gfritzsche] from comment #1)
> Anthony can look into this bug, cheers.
> The next merge is Sep 20, so i would say we safely have up to two weeks to
> land this (including testing/fallout).
> 
> Alessio, can you set this up with some pointers?

Sure!

The code around [1] is where we gather the payload for the "shutdown main-ping" and submit it through Telemetry. Right after that function, I think we should:

1) Create a pref at [2] to turn on/off the "first-shutdown" ping. The pref could be named something like "toolkit.telemetry.firstShutdownPing.enabled" and default to true. Also add the pref to [4] and to the docs at [3].
2) Right after [1], check if both the newly added pref is true and this is a first run (|TelemetryReportingPolicy.isFirstRun();|).
3) If that's the case, submit the same payload (|shutdownPayload|) using |submitExternalPing| as done in [1]. Change the first argument to "first-shutdown".
4) Add the ping documentation in docs/data/first-shutdown.rst. It should probably just mention what's this ping and why is it needed and reference the "main-ping" for details on the content [5].
5) Add test coverage for the new ping at [6]. Please note that we already had the logic for sending the "main-ping" on the first subsession, even though it's disabled. That test covers that case anyway, so you might simply update the test case to expect both the main ping and the "first-shutdown" ping. Another (better?) option is to create a similar test-case right after this one. Please note that you also need to turn off your new ping during testing at [7] and manually turn it on in your test case.

I hope I didn't miss anything. Please let me know if you have any question!

[1] - http://searchfox.org/mozilla-central/rev/18c16ebf818abb86805ce08a6e537e4cd826f044/toolkit/components/telemetry/TelemetrySession.jsm#1717
[2] - http://searchfox.org/mozilla-central/rev/18c16ebf818abb86805ce08a6e537e4cd826f044/browser/app/profile/firefox.js#1508
[3] - http://searchfox.org/mozilla-central/source/toolkit/components/telemetry/docs/internals/preferences.rst
[4] - http://searchfox.org/mozilla-central/source/toolkit/components/telemetry/TelemetryUtils.jsm#27
[5] - http://searchfox.org/mozilla-central/source/toolkit/components/telemetry/docs/data/main-ping.rst
[6] - http://searchfox.org/mozilla-central/rev/18c16ebf818abb86805ce08a6e537e4cd826f044/toolkit/components/telemetry/tests/unit/test_TelemetrySession.js#1472-1486
[7] - http://searchfox.org/mozilla-central/rev/18c16ebf818abb86805ce08a6e537e4cd826f044/testing/profiles/prefs_general.js#262
Flags: needinfo?(alessio.placitelli)
I'm close to finishing up the patch for this bug. Surprisingly, setting the new pref-flag in [7] does not disable the ping in the unit tests; instead, I have to manually set the preference to false in the test setup function. Would you happen to know why this is the case?
Flags: needinfo?(alessio.placitelli)
(In reply to Anthony Miyaguchi [:amiyaguchi] from comment #3)
> I'm close to finishing up the patch for this bug. Surprisingly, setting the
> new pref-flag in [7] does not disable the ping in the unit tests; instead, I
> have to manually set the preference to false in the test setup function.
> Would you happen to know why this is the case?

Ha! Darn me, you're completely right. I forgot to mention that for this specific case the pref also needs to be set to false at [*] in addition to [7]. Good catch! Looking forward to seeing the patch ;-)

[*] - http://searchfox.org/mozilla-central/rev/51b3d67a5ec1758bd2fe7d7b6e75ad6b6b5da223/toolkit/components/telemetry/tests/unit/head.js#327
Flags: needinfo?(alessio.placitelli)
Attachment #8903823 - Attachment is obsolete: true
Thanks for the help with setting the preference in the testing context, now I've run into a new set of problems. I've attached the in progress, where the tests are currently failing.

I am having trouble running the tests for this new option. I define a new test section for the `first-shutdown` ping in`test_TelemetrySession.js`. In lines 1536-1546, I enable the first shutdown ping and set this to the first run.

```
  // Set conditions for general use-case of the 'first-shutdown' ping
  Preferences.set(TelemetryUtils.Preferences.FirstShutdownPingEnabled, true);
  Preferences.set(TelemetryUtils.Preferences.FirstRun, true);
  // Update preferences and clear the tests
  TelemetryReportingPolicy.testUpdateFirstRun();
  PingServer.clearRequests();
  Telemetry.clearScalars();

  await TelemetryController.testShutdown();
  pings = await PingServer.promiseNextPings();
  Assert.ok(pings.length > 0, "There should be at least one ping")
```

I expect to get at least one ping -- the first shutdown ping. However, I get an empty batch of pings here. Am I missing something from manipulating these test fixtures?
Flags: needinfo?(alessio.placitelli)
The main problem is here:
`pings = await PingServer.promiseNextPings();`
The function `promiseNexPings()` expects a count argument, you pass `undefined`, so thanks to JS rules you always get `[]` back.
You can just use `PingServer.promiseNextPing()` (singular).

Now, the next problem you'll run into is that `TelemetryController` was not setup/initialized yet.
You can call `TelemetryController.testReset()` early in your test to fix that.
Flags: needinfo?(alessio.placitelli)
Attachment #8903825 - Attachment is obsolete: true
Attachment #8904792 - Attachment is obsolete: true
Comment on attachment 8904797 [details] [diff] [review]
Send a duplicate of a users first shutdown ping with pingsender

This is ready for review!

In addition to the async bug that you pointed out, I ended up fighting with a bug that was caused by leftover pings in Telemetry storage that were being sent on shutdown.

One thing that I noticed is that the `first-shutdown` ping will get persisted to disk if it fails to send on the first session. I'm curious on seeing what the rate of forced shutdowns on the first session will look like.
Attachment #8904797 - Flags: review?(gfritzsche)
Comment on attachment 8904797 [details] [diff] [review]
Send a duplicate of a users first shutdown ping with pingsender

Moving this to Alessio.
Attachment #8904797 - Flags: review?(gfritzsche) → review?(alessio.placitelli)
Comment on attachment 8904797 [details] [diff] [review]
Send a duplicate of a users first shutdown ping with pingsender

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

Hey Anthony, that's great work. I just have a few observations, nothing major, then we're good to go.

::: testing/profiles/prefs_general.js
@@ +262,5 @@
>  user_pref("toolkit.telemetry.shutdownPingSender.enabledFirstSession", false);
> +// Don't send the 'first-shutdown' during tests, otherwise tests expecting
> +// main and subsession pings will fail.
> +user_pref("toolkit.telemetry.firstShutdownPing.enabled", false);
> +

nit: let's remove this blank line

::: toolkit/components/telemetry/TelemetrySession.jsm
@@ +1702,5 @@
> +
> +      // Send a duplicate of first-shutdown pings as a new ping type, in order to properly
> +      // evaluate first session profiles (see bug 1390095).
> +      const sendFirstShutdownPing =
> +        Services.prefs.getBoolPref(Utils.Preferences.FirstShutdownPingEnabled, false) &&

I think we should also check for |Services.prefs.getBoolPref(TelemetryUtils.Preferences.ShutdownPingSender, false)| here. User can still disable sending pings at shutdown from the pingsender: we should respect that. Let's also change the test accordingly.

::: toolkit/components/telemetry/TelemetryUtils.jsm
@@ +34,4 @@
>      Server: "toolkit.telemetry.server",
>      ShutdownPingSender: "toolkit.telemetry.shutdownPingSender.enabled",
>      ShutdownPingSenderFirstSession: "toolkit.telemetry.shutdownPingSender.enabledFirstSession",
> +    FirstShutdownPingEnabled: "toolkit.telemetry.firstShutdownPing.enabled",

nit: can you move this above? The entries here are alphabetically sorted

::: toolkit/components/telemetry/tests/unit/test_TelemetrySession.js
@@ +1486,5 @@
> +  // Assert properties about the `first-shutdown` ping. `test_sendShutdownPing` asserts
> +  // the that the `first-shutdown` ping is not sent.
> +  if (gIsAndroid ||
> +      (AppConstants.platform == "linux" && OS.Constants.Sys.bits == 32)) {
> +    // see `test_sendShutdownPing` for more detailed comments

I think we can drop the initial 2 comment lines. Let's also report/copy the comments from test_sendShutdownPing here: that test might be removed or moved around.

@@ +1506,5 @@
> +    // *not* sent. This can be combined with the state of the storage to infer
> +    // the appropriate behavior from the preference flags.
> +
> +    // Assert failure if we recive a ping.
> +    PingServer.registerPingHandler(() => Assert.ok(false, "Telemetry must not send pings if not allowed to."));

To make debugging easier, let's change this to something similar to: http://searchfox.org/mozilla-central/rev/4d8e389498a08668cce9ebf6232cc96be178c3e4/toolkit/components/telemetry/tests/unit/test_TelemetrySession.js#1894-1896

That helps in case of permanent or intermittent testing failures, enabling us to trace back the origin of the failing ping from the logs.

@@ +1511,5 @@
> +
> +    // Assert that pings are sent on first run, forcing a forced application
> +    // quit. This should be equivalent to the first test in this suite.
> +    Preferences.set(TelemetryUtils.Preferences.FirstRun, true);
> +    Preferences.set(TelemetryUtils.Preferences.FirstShutdownPingEnabled, true);

This FirstShutdownPingEnabled pref is already being set at the beginning of the test (below): let's remove it from here. You just need FirstRun to update the policy.

@@ +1515,5 @@
> +    Preferences.set(TelemetryUtils.Preferences.FirstShutdownPingEnabled, true);
> +    TelemetryReportingPolicy.testUpdateFirstRun();
> +
> +    await TelemetryController.testReset();
> +    Services.obs.notifyObservers(null, "quit-application-forced");

Let's add a comment mentioning that this is to simulate OS shutdown with the browser open.

@@ +1524,5 @@
> +    await TelemetryStorage.testClearPendingPings();
> +
> +    // Assert that it's not sent during subsequent runs
> +    Preferences.set(TelemetryUtils.Preferences.FirstRun, false);
> +    Preferences.set(TelemetryUtils.Preferences.FirstShutdownPingEnabled, true);

The FirstShutdownPingEnabled pref should still be true, no need to set it again.

@@ +1586,5 @@
> +  Preferences.reset(TelemetryUtils.Preferences.FirstRun);
> +  PingServer.resetPingHandler();
> +});
> +
> +

nit: remove the extra blank line
Attachment #8904797 - Flags: review?(alessio.placitelli) → feedback+
Attachment #8904797 - Attachment is obsolete: true
Attachment #8905172 - Attachment is obsolete: true
Comment on attachment 8905179 [details] [diff] [review]
Send a duplicate of a users first shutdown ping with pingsender

This is ready for another look.
Attachment #8905179 - Flags: review?(alessio.placitelli)
Comment on attachment 8905179 [details] [diff] [review]
Send a duplicate of a users first shutdown ping with pingsender

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

Looks great, thanks Anthony! Do you know how to push this patch to try? With just your patch applied, run the command generated by the Try Chooser [1] from your mozilla-central root directory. As a safety precaution, to not have any nasty surprise with the tests, I'd say run *all* of them.

./mach try -b do -p all -u all -t none

[1] - https://mozilla-releng.net/trychooser/
Attachment #8905179 - Flags: review?(alessio.placitelli) → review+
(In reply to Alessio Placitelli [:Dexter] from comment #17)
> Comment on attachment 8905179 [details] [diff] [review]
> Send a duplicate of a users first shutdown ping with pingsender
> 
> Review of attachment 8905179 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks great, thanks Anthony! Do you know how to push this patch to try? With
> just your patch applied, run the command generated by the Try Chooser [1]
> from your mozilla-central root directory. As a safety precaution, to not
> have any nasty surprise with the tests, I'd say run *all* of them.
> 
> ./mach try -b do -p all -u all -t none

Since bug 1397860 is still open and the 57 soft-freeze is approaching, let me push this to try for you.
Hey Anthony, the tests look good. The intermittent failures don't seem to relate to your patch. Can you please take this for a manual test-drive on a fresh profile (assuming you didn't do this already). If you already did this, feel free to land it.
Flags: needinfo?(amiyaguchi)
Attached file report1.json
I've verified that this ping was sent manually via the gzipServer and by enabling MOZILLA_OFFICIA=1 in my .mozconfig.
Flags: needinfo?(amiyaguchi)
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/cf5d96ca71f6
Send a duplicate of a users first shutdown ping with pingsender. r=Dexter
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/cf5d96ca71f6
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Depends on: 1402249
You need to log in before you can comment on or make changes to this bug.