Closed Bug 860094 Opened 7 years ago Closed 6 years ago

Remove saving of payload to disk

Categories

(Firefox Health Report Graveyard :: Client: Desktop, defect)

defect
Not set

Tracking

(firefox22 fixed)

RESOLVED FIXED
Firefox 23
Tracking Status
firefox22 --- fixed

People

(Reporter: gps, Assigned: gps)

References

Details

(Whiteboard: [qa-])

Attachments

(1 file, 1 obsolete file)

We originally saved the last payload to disk to support needs of the initial version of about:healthreport and to potentially make Android FHR's life easier. Neither of which are needed any more. We can stop saving the payload to disk.
Attached patch Don't save payload to disk, v1 (obsolete) — Splinter Review
rnewman is out, so mconnor gets review.

An issue with this patch is some users may have an orphaned lastpayload.json file sitting in their profile for eternity. If this gets uplifted to Beta (which I'd recommend since the payload saving just adds unnecessary performance overhead), then the only users affected are the {Beta, Aurora, and Nightly} channels. Honestly, I'm not too concerned about that relatively small population. We can always land cleanup code later if this bothers us.
Assignee: nobody → gps
Status: NEW → ASSIGNED
Attachment #735493 - Flags: review?(mconnor)
Do we care about eliminating the ability to see what was uploaded, rather than a live snapshot?

Do we care that about:healthreport now needs to (a) keep the document in-memory across refreshes, (b) needs to do a collection each time it's displayed, or (c) needs to somehow query real-time info from the DB? Not sure what the current state of a:hr is, so maybe it's moved beyond "display some JSON"…
(In reply to Richard Newman [:rnewman] from comment #2)
> Do we care about eliminating the ability to see what was uploaded, rather
> than a live snapshot?

We aren't using it for anything now. We only care about the document ID.

> Do we care that about:healthreport now needs to (a) keep the document
> in-memory across refreshes, (b) needs to do a collection each time it's
> displayed, or (c) needs to somehow query real-time info from the DB? Not
> sure what the current state of a:hr is, so maybe it's moved beyond "display
> some JSON"…

a:hr currently collects and obtains a JSON payload with every load. It needs to collect to ensure there is *some* data to provide. Otherwise, we never generate a document if upload is enabled (we just perform a daily collection). We decided it was easier for a:hr to just perform the whole enchilada rather than bake "do I need to do X or Y" logic into a:hr. About the only downside is a:hr may take a few seconds to perform the collection before data is displayed. If this turns out to be a problem, we can always change this behavior and start caching documents again.
(In reply to Gregory Szorc [:gps] from comment #3)

> We aren't using it for anything now. We only care about the document ID.

Yeah, I kinda meant from a legal or privacy standpoint; I know *we* only care about the ID. I'm hesitant to remove functionality that might have been baked in as a requirement without checking.

> a:hr currently collects and obtains a JSON payload with every load.

Gotcha.
Rebased on top of bug 854018.
Attachment #735493 - Attachment is obsolete: true
Attachment #735493 - Flags: review?(mconnor)
Attachment #743308 - Flags: review?(rnewman)
Comment on attachment 743308 [details] [diff] [review]
Don't save payload to disk, v2

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

Kinda want this in beta, so we don't create files in a user's profile that will stick around, stale, fo'eva.

::: services/healthreport/healthreporter.jsm
@@ +101,5 @@
>    this._dbOpenHistogram = hasFirstRun ? TELEMETRY_DB_OPEN : TELEMETRY_DB_OPEN_FIRSTRUN;
>  
>    TelemetryStopwatch.start(this._initHistogram, this);
>  
> +  // As soon as we have could storage, we need to register cleanup or

could have
Attachment #743308 - Flags: review?(rnewman) → review+
https://hg.mozilla.org/services/services-central/rev/3a7209e3c338

Perhaps we should consider a follow-up to delete the payload file from disk?
Whiteboard: [fixed-in-services]
https://hg.mozilla.org/mozilla-central/rev/3a7209e3c338
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-services]
Target Milestone: --- → Firefox 23
Blocks: 867902
Comment on attachment 743308 [details] [diff] [review]
Don't save payload to disk, v2

[Approval Request Comment]
Bug caused by (feature/regressing bug #): FHR
User impact if declined: Slightly worse FHR perf, makes uplift of bug 860094 a little harder.
Testing completed (on m-c, etc.): It's baked for a few weeks. No known issues.
Risk to taking this patch (and alternatives if risky): This patch is low risk since it mostly removes code unneeded code.
String or IDL/UUID changes made by this patch: None
Attachment #743308 - Flags: approval-mozilla-beta?
Comment on attachment 743308 [details] [diff] [review]
Don't save payload to disk, v2

In support of bug 860094's success in FF22. Risk profile of the overall uplift will not change according to gps.
Attachment #743308 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Is anything QA can verify here?
(In reply to Simona B [QA] from comment #12)
> Is anything QA can verify here?

Nah.
Whiteboard: [qa-]
Product: Firefox Health Report → Firefox Health Report Graveyard
You need to log in before you can comment on or make changes to this bug.