Closed Bug 1381490 Opened 3 years ago Closed 3 years ago

Enable the 'shutdown' pingsender on the first session

Categories

(Toolkit :: Telemetry, enhancement, P1)

enhancement

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox56 --- fixed

People

(Reporter: Dexter, Assigned: Dexter)

References

Details

(Whiteboard: [measurement:client])

Attachments

(1 file, 3 obsolete files)

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.
Blocks: 1343277
Depends on: 1381487
Priority: -- → P3
Whiteboard: [measurement:client]
Priority: P3 → P1
Assignee: nobody → alessio.placitelli
Attached patch bug1381490_firstShutdown.patch (obsolete) — Splinter Review
I will not be landing this until we settle the discussion on the mailing list.
Attachment #8889865 - Flags: review?(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?
Attachment #8889865 - Flags: review?(gfritzsche) → review+
(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
Attached patch bug1381490_firstShutdown.patch (obsolete) — Splinter Review
Attachment #8889865 - Attachment is obsolete: true
Attachment #8889911 - Flags: review+
(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.
Attached patch bug1381490_firstShutdown.patch (obsolete) — Splinter Review
Attachment #8889911 - Attachment is obsolete: true
Attachment #8889955 - Flags: review+
As discussed over IRC; let's land this initially preff'd off.
Attachment #8889955 - Attachment is obsolete: true
Attachment #8890339 - Flags: review+
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
https://hg.mozilla.org/mozilla-central/rev/34617f86887e
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.