Closed Bug 1222054 Opened 4 years ago Closed 4 years ago

Route shutdown/saved-session pings through TelemetrySend

Categories

(Toolkit :: Telemetry, defect, P1)

defect
Points:
1

Tracking

()

VERIFIED FIXED
mozilla45
Tracking Status
firefox42 --- wontfix
firefox43 --- wontfix
firefox44 --- fixed
firefox45 --- verified

People

(Reporter: Dexter, Assigned: Dexter)

References

(Blocks 1 open bug)

Details

(Whiteboard: [measurement:client])

Attachments

(2 files, 2 obsolete files)

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
Blocks: 1201022
Points: --- → 1
Priority: -- → P2
Whiteboard: [measurement:client]
(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.
(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
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.
Assignee: nobody → alessio.placitelli
Status: NEW → ASSIGNED
Attachment #8685461 - Flags: review?(gfritzsche)
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.
Attachment #8685463 - Flags: feedback?(gfritzsche)
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
Priority: P2 → P1
Attachment #8685461 - Flags: review?(gfritzsche) → review+
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.
Attachment #8685463 - Flags: feedback?(gfritzsche) → feedback+
Attachment #8685463 - Flags: review?(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.
Attachment #8685463 - Flags: review?(gfritzsche) → review+
Attachment #8685463 - Attachment is obsolete: true
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.
Attachment #8688514 - Attachment is obsolete: true
Attachment #8688943 - Flags: review+
Attachment #8688943 - Flags: feedback?(gfritzsche)
Flags: qe-verify+
QA Contact: alexandra.lucinet
Attachment #8688943 - Flags: feedback?(gfritzsche) → feedback+
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
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Can you flag this for uplift to aurora?
Flags: needinfo?(alessio.placitelli)
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
Flags: needinfo?(alessio.placitelli)
Attachment #8685461 - Flags: approval-mozilla-aurora?
Comment on attachment 8688943 [details] [diff] [review]
part 2 - Fix the broken test_TelemetrySession

As above comment.
Attachment #8688943 - Flags: approval-mozilla-aurora?
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.
Attachment #8685461 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 8688943 [details] [diff] [review]
part 2 - Fix the broken test_TelemetrySession

Automated tests are always great.
Attachment #8688943 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.