Enable the 'shutdown' pingsender on the first session

RESOLVED FIXED in Firefox 56

Status

()

P1
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: Dexter, Assigned: Dexter)

Tracking

Trunk
mozilla56
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox56 fixed)

Details

(Whiteboard: [measurement:client])

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

2 years ago
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.
(Assignee)

Updated

2 years ago
Blocks: 1343277
Depends on: 1381487
Priority: -- → P3
Whiteboard: [measurement:client]
(Assignee)

Updated

2 years ago
Priority: P3 → P1
(Assignee)

Updated

2 years ago
Assignee: nobody → alessio.placitelli
(Assignee)

Comment 1

2 years ago
Created attachment 8889865 [details] [diff] [review]
bug1381490_firstShutdown.patch

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+
(Assignee)

Comment 3

2 years ago
(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
(Assignee)

Comment 4

2 years ago
Created attachment 8889911 [details] [diff] [review]
bug1381490_firstShutdown.patch
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.
(Assignee)

Comment 7

2 years ago
Created attachment 8889955 [details] [diff] [review]
bug1381490_firstShutdown.patch
Attachment #8889911 - Attachment is obsolete: true
Attachment #8889955 - Flags: review+
(Assignee)

Comment 9

2 years ago
Created attachment 8890339 [details] [diff] [review]
bug1381490_firstShutdown.patch

As discussed over IRC; let's land this initially preff'd off.
Attachment #8889955 - Attachment is obsolete: true
Attachment #8890339 - Flags: review+
(Assignee)

Comment 10

2 years ago
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

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/34617f86887e
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.