Enable the 'shutdown' pingsender on the first session

RESOLVED FIXED in Firefox 56

Status

()

Toolkit
Telemetry
P1
normal
RESOLVED FIXED
a month ago
27 days ago

People

(Reporter: Dexter, Assigned: Dexter)

Tracking

(Depends on: 1 bug)

Trunk
mozilla56
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox56 fixed)

Details

(Whiteboard: [measurement:client])

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

a month 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

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

Updated

a month ago
Priority: P3 → P1
(Assignee)

Updated

a month ago
Assignee: nobody → alessio.placitelli
(Assignee)

Comment 1

29 days 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

29 days 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

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

Comment 5

29 days ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e27ba55afa5bac3e0c5d0d653d023cf4b3cc2c37
(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

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

Comment 8

28 days ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=26768f21e5292e3fe7b69dca48774e98160eae37
(Assignee)

Comment 9

28 days 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

28 days 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

27 days ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/34617f86887e
Status: NEW → RESOLVED
Last Resolved: 27 days 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.