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)
Toolkit
Telemetry
Tracking
()
RESOLVED
FIXED
mozilla57
Tracking | Status | |
---|---|---|
firefox57 | --- | fixed |
People
(Reporter: gfritzsche, Assigned: amiyaguchi)
References
Details
Attachments
(2 files, 5 obsolete files)
11.78 KB,
patch
|
Dexter
:
review+
|
Details | Diff | Splinter Review |
234.85 KB,
application/json
|
Details |
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.
Reporter | ||
Updated•7 years ago
|
Assignee: nobody → gfritzsche
Reporter | ||
Comment 1•7 years ago
|
||
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)
Comment 2•7 years ago
|
||
(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)
Assignee | ||
Comment 3•7 years ago
|
||
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)
Comment 4•7 years ago
|
||
(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)
Assignee | ||
Comment 5•7 years ago
|
||
Assignee | ||
Comment 6•7 years ago
|
||
Assignee | ||
Updated•7 years ago
|
Attachment #8903823 -
Attachment is obsolete: true
Assignee | ||
Comment 7•7 years ago
|
||
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)
Reporter | ||
Comment 8•7 years ago
|
||
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)
Assignee | ||
Comment 9•7 years ago
|
||
Assignee | ||
Updated•7 years ago
|
Attachment #8903825 -
Attachment is obsolete: true
Assignee | ||
Comment 10•7 years ago
|
||
Assignee | ||
Updated•7 years ago
|
Attachment #8904792 -
Attachment is obsolete: true
Assignee | ||
Comment 11•7 years ago
|
||
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)
Reporter | ||
Comment 12•7 years ago
|
||
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 13•7 years ago
|
||
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+
Assignee | ||
Comment 14•7 years ago
|
||
Assignee | ||
Updated•7 years ago
|
Attachment #8904797 -
Attachment is obsolete: true
Assignee | ||
Comment 15•7 years ago
|
||
Assignee | ||
Updated•7 years ago
|
Attachment #8905172 -
Attachment is obsolete: true
Assignee | ||
Comment 16•7 years ago
|
||
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 17•7 years ago
|
||
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+
Comment 18•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=83665e3d6171259224206ec058d0da5b496a948c
Comment 19•7 years ago
|
||
(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.
Comment 20•7 years ago
|
||
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)
Assignee | ||
Comment 21•7 years ago
|
||
I've verified that this ping was sent manually via the gzipServer and by enabling MOZILLA_OFFICIA=1 in my .mozconfig.
Flags: needinfo?(amiyaguchi)
Assignee | ||
Updated•7 years ago
|
Keywords: checkin-needed
Comment 22•7 years ago
|
||
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
Comment 23•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/cf5d96ca71f6
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in
before you can comment on or make changes to this bug.
Description
•