Closed
Bug 1381490
Opened 7 years ago
Closed 7 years ago
Enable the 'shutdown' pingsender on the first session
Categories
(Toolkit :: Telemetry, enhancement, P1)
Toolkit
Telemetry
Tracking
()
RESOLVED
FIXED
mozilla56
Tracking | Status | |
---|---|---|
firefox56 | --- | fixed |
People
(Reporter: Dexter, Assigned: Dexter)
References
Details
(Whiteboard: [measurement:client])
Attachments
(1 file, 3 obsolete files)
11.23 KB,
patch
|
Dexter
:
review+
|
Details | Diff | Splinter Review |
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•7 years ago
|
Assignee | ||
Updated•7 years ago
|
Priority: P3 → P1
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → alessio.placitelli
Assignee | ||
Comment 1•7 years ago
|
||
I will not be landing this until we settle the discussion on the mailing list.
Attachment #8889865 -
Flags: review?(gfritzsche)
Comment 2•7 years ago
|
||
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•7 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•7 years ago
|
||
Attachment #8889865 -
Attachment is obsolete: true
Attachment #8889911 -
Flags: review+
Assignee | ||
Comment 5•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e27ba55afa5bac3e0c5d0d653d023cf4b3cc2c37
Comment 6•7 years ago
|
||
(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•7 years ago
|
||
Attachment #8889911 -
Attachment is obsolete: true
Attachment #8889955 -
Flags: review+
Assignee | ||
Comment 8•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=26768f21e5292e3fe7b69dca48774e98160eae37
Assignee | ||
Comment 9•7 years ago
|
||
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•7 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•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/34617f86887e
Status: NEW → RESOLVED
Closed: 7 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.
Description
•