1222054 - Route shutdown/saved-session pings through TelemetrySend

Status: VERIFIED FIXED

(Toolkit :: Telemetry, defect, P1)

Description

[Alessio Placitelli [:Dexter]]

Both shutdown and saved-session pings get directly saved as pending pings when closing Firefox [0]. Due to this, shutdown/saved-session pings get saved when shutting down after FHR gets disabled. They are eventually sent when enabling FHR again, which is undesired.

We should change [0] (and a couple of line below as well) by using |TelemetryController.submitExternalPing| instead of |TelemetryController.addPendingPing|.

Then in TelemetrySend, special-case shutdown and saved-session to be saved to disk instead of trying to send them. We should investigate if this special case is required as TelemetrySend already serializes to disk pings submitted when shutting down [1].

[0] - https://dxr.mozilla.org/mozilla-central/rev/6077f51254c69a1e14e1b61acba4af451bf1783e/toolkit/components/telemetry/TelemetrySession.jsm#1595
[1] - https://dxr.mozilla.org/mozilla-central/rev/6077f51254c69a1e14e1b61acba4af451bf1783e/toolkit/components/telemetry/TelemetrySend.jsm#704

Comment 1

[Georg Fritzsche [:gfritzsche]]

(In reply to Alessio Placitelli [:Dexter] from comment #0)
> Then in TelemetrySend, special-case shutdown and saved-session to be saved
> to disk instead of trying to send them. We should investigate if this
> special case is required as TelemetrySend already serializes to disk pings
> submitted when shutting down [1].
> 
> [1] -
> https://dxr.mozilla.org/mozilla-central/rev/
> 6077f51254c69a1e14e1b61acba4af451bf1783e/toolkit/components/telemetry/
> TelemetrySend.jsm#704

If TelemetrySend gets shut down before TelemetrySession, then [1] should already be hit correctly.

Comment 2

[Alessio Placitelli [:Dexter]]

(In reply to Georg Fritzsche [:gfritzsche] from comment #1)
> If TelemetrySend gets shut down before TelemetrySession, then [1] should
> already be hit correctly.

It looks like it does. We wait on the shutdown barrier and trigger TelemetrySession.shutdownChromeProcess [1] after TelemetrySend.shutdown() [0]. 

[0] - https://dxr.mozilla.org/mozilla-central/rev/e2a910c048dc82fc3be53475f18e7f81f03e377b/toolkit/components/telemetry/TelemetryController.jsm#817
[1] - https://dxr.mozilla.org/mozilla-central/rev/6077f51254c69a1e14e1b61acba4af451bf1783e/toolkit/components/telemetry/TelemetrySession.jsm#1425

Comment 3

[Alessio Placitelli [:Dexter]]

Attachment: part 1 - Make TelemetrySession use submitExternalPing

This patch changes TelemetrySession to use TelemetryController.submitExternalPing instead of TelemetryController.addPendingPing.

I've manually tested it and got the expected results:

- With both Base and Extended Telemetry enabled, the saved-session and main/shutdown pings get correctly saved to disk when shutting down for sessions >= 1 min.
- Same as above, for short sessions (< 1 min)
- Flipping Base Telemetry off (FHR Data upload) during a session and then shutting down does not persist a shutdown nor a saved-session ping.

Comment 4

[Alessio Placitelli [:Dexter]]

Attachment: part 2 - Fix the broken test_TelemetrySession

Our xpcshell tests do not properly shut down TelemetrySend and, in general, it's tricky to simulate a proper browser restart. This is going to be dealt with by bug 1145188 and bug 1156358.

To work around this problem in test_TelemetrySend, we need to make sure that TelemetrySend is shut down before shutting down TelemetrySession, otherwise the shutdown pings will be sent and not persisted.

This patch manually does that, but I'm not sure that's the right approach.

Comment 5

[Alessio Placitelli [:Dexter]]

The test patch could probably be easier by making [0] also reset _sendingEnabled to true.

[0] - https://dxr.mozilla.org/mozilla-central/rev/4a7526d26bd47ce2e01f938702b91c95424026ed/toolkit/components/telemetry/TelemetrySend.jsm#661

Comment 6

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8685463 [details] [diff] [review]
part 2 - Fix the broken test_TelemetrySession

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

The work-arounds here are unfortunate, but we plan to settle the testing situation for the Telemetry modules setup/shutdown/reset in the next weeks - this part will be cleaned up as a result.

Comment 7

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8685463 [details] [diff] [review]
part 2 - Fix the broken test_TelemetrySession

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

The work-arounds here are unfortunate, but we plan to settle the testing situation for the Telemetry modules setup/shutdown/reset in the next weeks - this part will be cleaned up as a result.

Comment 8

[Alessio Placitelli [:Dexter]]

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

Comment 9

[Alessio Placitelli [:Dexter]]

Attachment: part 2 - Fix the broken test_TelemetrySession

Comment 10

[Alessio Placitelli [:Dexter]]

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

Comment 11

[Alessio Placitelli [:Dexter]]

Attachment: part 2 - Fix the broken test_TelemetrySession

There was some fallout in the tests only happening on the try server. This fixes the fallout by adding a couple of missing TelemetrySend.shutdown/setup combos.

Comment 12

[Alessio Placitelli [:Dexter]]

https://hg.mozilla.org/integration/fx-team/rev/b3efb38024ca2a0eec15f915692c2f8417fd2e55
Bug 1222054 - Route shutdown/saved-session pings through TelemetrySend. r=gfritzsche

https://hg.mozilla.org/integration/fx-team/rev/83112749ad274d0db90432f26f042d9df30eed91
Bug 1222054 - Fix broken tests. r=gfritzsche

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/b3efb38024ca
https://hg.mozilla.org/mozilla-central/rev/83112749ad27

Comment 14

[Ada [:adalucinet]]

Verified fixed with latest Nightly 45.0a1, across platforms [1], with STR via comment 3.

[1] Windows 7 64-bit, Mac OS X 10.11 and Ubuntu 14.04 32-bit

Comment 15

[Georg Fritzsche [:gfritzsche]]

Can you flag this for uplift to aurora?

Comment 16

[Alessio Placitelli [:Dexter]]

Comment on attachment 8685461 [details] [diff] [review]
part 1 - Make TelemetrySession use submitExternalPing

Approval Request Comment
[Feature/regressing bug #]: UT generating shutdown/saved-session pings.
[User impact if declined]: As soon as upload is turned back on, we would send saved-session/shutdown pings even though user turned off sending before saving those pings.
[Describe test coverage new/current, TreeHerder]: xpcshell test coverage and manual QA verification performed.
[Risks and why]: Low, as there are no user-facing changes involved. The changes are limited to the way pings are saved during the shutdown process.
[String/UUID change made/needed]: None

Comment 17

[Alessio Placitelli [:Dexter]]

Comment on attachment 8688943 [details] [diff] [review]
part 2 - Fix the broken test_TelemetrySession

As above comment.

Comment 18

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8685461 [details] [diff] [review]
part 1 - Make TelemetrySession use submitExternalPing

This has been on Nightly for a few days already and seems like a safe fix. Let's uplift to Aurora44.

Comment 19

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8688943 [details] [diff] [review]
part 2 - Fix the broken test_TelemetrySession

Automated tests are always great.

Comment 20

[Carsten Book [:Tomcat]]

landed by dexter as:
https://hg.mozilla.org/releases/mozilla-aurora/rev/1e0a87fb06a3
https://hg.mozilla.org/releases/mozilla-aurora/rev/6ad162acd38b