Closed Bug 1343504 Opened 3 years ago Closed 3 years ago

Save the crash pings outside of the user profile directory so pingSender can delete them

Categories

(Toolkit :: Telemetry, enhancement, P3)

enhancement

Tracking

()

RESOLVED DUPLICATE of bug 1345153
Tracking Status
firefox54 --- affected

People

(Reporter: Dexter, Unassigned)

References

Details

(Whiteboard: [measurement:client])

To send the crash ping from the crash reporter (bug 1310703) using the PingSender, we currently use pipes: if something goes wrong, the generated crash ping doesn't get sent.

For the shutdown ping we're taking a different approach: we persist a ping to the disk and then try to send it using the PingSender, which eventually removes it from the disk on success. We can't use the "shutdown" ping approach for the crashreporter as we don't have the profile information there (since crashes can happen before a profile is chosen).

One solution could be to store the crash pings generated from the crashreporter in a directory similar to the one crash reports are stored, outside of the user profiles.
Priority: -- → P3
Whiteboard: [measurement:client]
Blocks: 1310703
See Also: → 1345153
Blocks: 1336360
Because this is done to allow pingSender to remove the ping when successfully sent, modifying the summary accordingly.
Summary: Save the crash pings outside of the user profile directory → Save the crash pings outside of the user profile directory so pingSender can delete them
Blocks: 1348250
Let's make this bug only about changing the crashreporter and the pingsender. I filed bug 1348250 (and will take it) to pick up the pending pings from Telemetry and send them again in Firefox.
I don't think telemetry changes are needed, here's why: we already flag the event files for a crash with the UUID of the crash ping. When we launch Firefox the CrashManager scans the pending folder for event files and parses. Them at this stage my intention was to check for stored pings still in the pending folder and send them. This would require only a very small modification to the CrashManager whose job is already to assemble and send the crash pings and wouldn't require intrusive changes to the telemetry code.
(In reply to Gabriele Svelto [:gsvelto] from comment #3)
> I don't think telemetry changes are needed, here's why: we already flag the
> event files for a crash with the UUID of the crash ping. When we launch
> Firefox the CrashManager scans the pending folder for event files and
> parses. Them at this stage my intention was to check for stored pings still
> in the pending folder and send them. This would require only a very small
> modification to the CrashManager whose job is already to assemble and send
> the crash pings and wouldn't require intrusive changes to the telemetry code.

While that would work, wouldn't it duplicate the logic from TelemetryStorage.jsm/TelemetrySend.jsm?

I think it would be cleaner to have the pending ping handling logic in one place, so that it's easier to reason about it and maintain. We already have a similar handling for "aborted-session" pings, which are not saved by default to the "pending-pings" directory, but rather migrated there if needed.
(In reply to Alessio Placitelli [:Dexter] from comment #4)
> While that would work, wouldn't it duplicate the logic from
> TelemetryStorage.jsm/TelemetrySend.jsm?

The crash manager already has logic to look up stuff in that folder, and besides if I find an unsend ping what I would do is just this:

https://dxr.mozilla.org/mozilla-central/rev/1b9293be51637f841275541d8991314ca56561a5/toolkit/components/crashes/CrashManager.jsm#650

> I think it would be cleaner to have the pending ping handling logic in one
> place, so that it's easier to reason about it and maintain.

My problem is that the crash manager code needs to know that a ping has already been sent or it will send a ping of its own. I will not duplicate any of the sending logic, just call submitExternalPing() as the code already does.
(In reply to Gabriele Svelto [:gsvelto] from comment #5)
> > I think it would be cleaner to have the pending ping handling logic in one
> > place, so that it's easier to reason about it and maintain.
> 
> My problem is that the crash manager code needs to know that a ping has
> already been sent or it will send a ping of its own. I will not duplicate
> any of the sending logic, just call submitExternalPing() as the code already
> does.

Let's mark the event file as "processed" in that case so that the CrashManager doesn't send twice.
Yep, cleared this up on IRC: the leftover pings will be handled directly by the telemetry code, this was the bit I was missing. The crash manager will never send a ping if it finds the ping UUID annotation in the event file. It will send one only if the annotation is entirely missing (which would mean that the crashreporter client hasn't even generated a ping).
Depends on: 1345153
Marking this as a duplicate, as Gabriele is working on this in bug 1345153.
Status: NEW → RESOLVED
Closed: 3 years ago
No longer depends on: 1345153
Resolution: --- → DUPLICATE
Duplicate of bug: 1345153
You need to log in before you can comment on or make changes to this bug.