1381490 - Enable the 'shutdown' pingsender on the first session

Status: RESOLVED FIXED

(Toolkit :: Telemetry, enhancement, P1)

Description

[Alessio Placitelli [:Dexter]]

Sending the shutdown ping using the pingsender was disabled from the first Firefox session due to bot profile concerns (see bug 1354482).

After investigating the impact of the 'new-profile' ping on Release, if we find that sending the 'shutdown' ping from the first session is a good idea, we should revisit the topic and remove this first session special case.

Comment 1

[Alessio Placitelli [:Dexter]]

Attachment: bug1381490_firstShutdown.patch

I will not be landing this until we settle the discussion on the mailing list.

Comment 2

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8889865 [details] [diff] [review]
bug1381490_firstShutdown.patch

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

r=me for the code change.
Landing has to wait until we cleared the discussion (or land it with the pref set to false).

::: toolkit/components/telemetry/TelemetrySession.jsm
@@ +1808,5 @@
>        let shutdownPayload = this.getSessionPayload(REASON_SHUTDOWN, false);
>  
>        // Only send the shutdown ping using the pingsender from the second
>        // browsing session on, to mitigate issues with "bot" profiles (see bug 1354482).
> +      const SEND_ON_THIS_SESSION =

Nit: This is not a global constant, lets use normal camel-case here.

::: toolkit/components/telemetry/tests/unit/test_TelemetrySession.js
@@ +1482,5 @@
> +  TelemetryReportingPolicy.testUpdateFirstRun();
> +
> +  // Restart/shutdown telemetry and wait for an incoming ping.
> +  await TelemetryController.testReset();
> +  nextPing = PingServer.promiseNextPing();

Nit: `pingPromise` or so.

@@ +1484,5 @@
> +  // Restart/shutdown telemetry and wait for an incoming ping.
> +  await TelemetryController.testReset();
> +  nextPing = PingServer.promiseNextPing();
> +  await TelemetryController.testShutdown();
> +  ping = await nextPing;

Why not just use `ping = PingServer.promiseNextPing()` here directly?

Comment 3

[Alessio Placitelli [:Dexter]]

(In reply to Georg Fritzsche [away Jul 19 - 24] [:gfritzsche] from comment #2)
> @@ +1484,5 @@
> > +  // Restart/shutdown telemetry and wait for an incoming ping.
> > +  await TelemetryController.testReset();
> > +  nextPing = PingServer.promiseNextPing();
> > +  await TelemetryController.testShutdown();
> > +  ping = await nextPing;
> 
> Why not just use `ping = PingServer.promiseNextPing()` here directly?

Do you mean like this?

> await TelemetryController.testShutdown();
> ping = await PingServer.promiseNextPing();

Wouldn't we risk missing the 'shutdown' ping/introducing oranges on try, with promiseNextPing being executed after the ping is generated? That should be unlikely, though

Comment 4

[Alessio Placitelli [:Dexter]]

Attachment: bug1381490_firstShutdown.patch

Comment 5

[Alessio Placitelli [:Dexter]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e27ba55afa5bac3e0c5d0d653d023cf4b3cc2c37

Comment 6

[Georg Fritzsche [:gfritzsche]]

(In reply to Alessio Placitelli [:Dexter] from comment #3)
> > await TelemetryController.testShutdown();
> > ping = await PingServer.promiseNextPing();
> 
> Wouldn't we risk missing the 'shutdown' ping/introducing oranges on try,
> with promiseNextPing being executed after the ping is generated? That should
> be unlikely, though

No, you can't miss pings with our PingServer.
You get either 1) the most recent ping or 2) wait for the next ping to arrive.

Comment 7

[Alessio Placitelli [:Dexter]]

Attachment: bug1381490_firstShutdown.patch

Comment 8

[Alessio Placitelli [:Dexter]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=26768f21e5292e3fe7b69dca48774e98160eae37

Comment 9

[Alessio Placitelli [:Dexter]]

Attachment: bug1381490_firstShutdown.patch

As discussed over IRC; let's land this initially preff'd off.

Comment 10

[Alessio Placitelli [:Dexter]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/34617f86887ebb1fa5cba44d6e339c02ffec785f
Bug 1381490 - Enable sending the shutdown ping with the pingsender on the first session. r=gfritzsche

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/34617f86887e