Closed Bug 1348250 Opened 3 years ago Closed 3 years ago

Change TelemetryStorage to load crash pending pings outside of the profile

Categories

(Toolkit :: Telemetry, defect, P1)

defect
Points:
2

Tracking

()

VERIFIED FIXED
mozilla55
Tracking Status
firefox54 --- fixed
firefox55 --- fixed

People

(Reporter: Dexter, Assigned: Dexter)

References

Details

(Whiteboard: [measurement:client])

Attachments

(2 files)

Bug 1343504 will save pending crash pings outside of the profile directory, to prevent sending duplicate pings.

This bug is about changing TelemetryStorage.jsm so that we can load them and send them through Telemetry if the pingsender failed to send/remove them.
Points: --- → 2
Depends on: 1343504
Priority: -- → P2
Whiteboard: [measurement:client]
Depends on: 1345153
Priority: P2 → P1
Note to self:

- Pending crash pings live in the User App Data (e.g. %AppData%/Mozilla/Firefox/Pending Pings -> Services.dirsvc.get("UAppData", Ci.nsIFile).path)
- We need to check for them when Firefox starts and migrate them over the Firefox pending pings directory from TelemetryStorage.
Assignee: nobody → alessio.placitelli
Comment on attachment 8853403 [details]
Bug 1348250 - Change TelemetryStorage to load crash pending pings outside of the profile.

https://reviewboard.mozilla.org/r/125512/#review128076

So, in a multi-profile case (rare as they are) crash pings assembled in one profile will be sent by another. That's kinda neat.

::: toolkit/components/telemetry/tests/unit/test_MigratePendingPings.js:39
(Diff revision 2)
> +  yield createFakeAppDir();
> +  // Make sure we don't generate unexpected pings due to pref changes.
> +  yield setEmptyPrefWatchlist();
> +});
> +
> +add_task(function* test_migrateUsentPings() {

"Usent"?
Attachment #8853403 - Flags: review?(chutten) → review+
Pushed by alessio.placitelli@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/a9f9515d8bca
Change TelemetryStorage to load crash pending pings outside of the profile. r=chutten
https://hg.mozilla.org/mozilla-central/rev/a9f9515d8bca
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Abe, as we discussed, this bug handles the pending crash pings saved by 1343504 outside of the user profile when the pingsender fails. 

With this patch, the crash pings are removed from the "Pending Pings" directory and dispatched through the normal Telemetry workflow. This means that, after Firefox and Telemetry is initialized (60 seconds from the startup), the pings should be sent.

Would you please validate it? :-)
Flags: needinfo?(amasresha)
Whiteboard: [measurement:client] → [measurement:client][measurement:client:uplift]
Depends on: 1353192
With the exception of Bug 1353192, all are green here. 

Here is the link to test cases and runs: https://public.etherpad-mozilla.org/p/bug1341349
Flags: needinfo?(amasresha)
QA verified as fixed. Please see comment 9 for test cases.
Status: RESOLVED → VERIFIED
Comment on attachment 8853403 [details]
Bug 1348250 - Change TelemetryStorage to load crash pending pings outside of the profile.

Approval Request Comment
[Feature/Bug causing the regression]: 1310703
[User impact if declined]: If we fail to send the "crash" ping using the pingsender, this patch will migrate them to the user profile and try to send them again. If we don't uplift this, we won't be resending the pings.
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: Yes, see comment 10
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: This patch should be applied after bug 1345153. After we can apply 1345108.
[Is the change risky?]: Low risk
[Why is the change risky/not risky?]: This is making Telemetry migrate pings saved outside of the profile directory (where the crashreporter data is) to the user profile. If we fail the migration, we just won't be sending these crash pings. Nothing else should break.
[String changes made/needed]: None
Attachment #8853403 - Flags: approval-mozilla-aurora?
Comment on attachment 8853403 [details]
Bug 1348250 - Change TelemetryStorage to load crash pending pings outside of the profile.

Fix crash ping sending issue and was verified by QA. Aurora54+.
Attachment #8853403 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Holding off on this uplift until we get a rebased patch for bug 1345153.
Whiteboard: [measurement:client][measurement:client:uplift] → [measurement:client]
You need to log in before you can comment on or make changes to this bug.