Closed Bug 847499 Opened 9 years ago Closed 9 years ago

Make sure telemetry knows to write out saved-session data when fennec goes in background

Categories

(Firefox for Android Graveyard :: General, defect)

All
Android
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 22

People

(Reporter: taras.mozilla, Assigned: kats)

References

Details

Attachments

(1 file, 2 obsolete files)

10:30 <taras> so atm telemetry reports on idle-daily
10:30 <taras> which many android sessions dont live long enough for
10:30 <taras> the other time it saves data
10:30 <taras> is on app shutdown
10:30 <taras> which also doesnt happen
10:30 <taras> so we need to trigger telemetry flush
10:31 <taras> on when app gets pushed into background

This should get us bursty saved-session submissions. Follow up would be to have the fennec background service send telemetry in.
We might have similar issues in Firefox for Metro...
OS: Windows 8 → Android
Product: Fennec → Firefox for Android
Hardware: x86_64 → All
Duplicate of this bug: 852081
Assignee: nobody → bugmail.mozilla
Attached patch WIP (obsolete) — Splinter Review
Just an outline of how this could be implemented. I think the current telemetry code assumes the pings are either in _pendingPings or saved to file (but not both), and they are only flushed to file on exit. If we flush to disk on going to background we'll be violating this assumption and so we have to deal with the fallout by re-loading them into memory before we send it out. I'll flesh this out more and test it.
I think you should be able to get away with:

1) Flush the current session to disk when backgrounding the application;
2) When resuming from background, delete the ping file corresponding to the current session.  There's no need to send that ping file now, since we're still running the current session.

I'm assuming backgrounding/resuming doesn't create new session IDs (seems logical).  I don't think there's any need to load all the saved pings every time we try to send.
Updated patch. Still needs testing; will request review once that's done.
Attachment #727218 - Attachment is obsolete: true
This seems to do the right thing, although in order to test it I had to hack up the code a bunch so I'm not 100% sure I tested it correctly.

Also, if I'm reading the code correctly, it seems that when you quit desktop firefox, the quit-application-granted and profile-before-change2 handlers will get run, and will unconditionally save the ping file for the session, even if that ping has already been sent. This means the ping will get re-sent in the next session. Is that correct? I added the _pingsSubmitted flag to guard against this scenario before I realized it affected desktop as well, so maybe it doesn't need to be there.
Attachment #727760 - Attachment is obsolete: true
Attachment #729329 - Flags: review?(nfroyd)
Comment on attachment 729329 [details] [diff] [review]
Save session data on backgrounding

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

You are reading the code correctly, and it's OK that a sequence like:

send ping at idle
save ping
<time elapses, browser restarts>
load saved ping from previous session, send that

happens, because the last (saved) ping should contain at least as much data as the ping sent out at idle.  In fact, the telemetry server only stores one ping per session anyway.

r=me if you take out the code dealing with _pingsSubmitted.
Attachment #729329 - Flags: review?(nfroyd) → review+
That makes sense, thanks. Took out _pingsSubmitted and landed:

https://hg.mozilla.org/integration/mozilla-inbound/rev/eed15a51c2d8
https://hg.mozilla.org/mozilla-central/rev/eed15a51c2d8
Status: NEW → RESOLVED
Closed: 9 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Target Milestone: --- → Firefox 22
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.