Requirements: * Abstract away SQL so we can switch out storage later. * Background everything. * Cheap storage of session time: perhaps not even in the CP to avoid init cost. * Buffer writes if necessary (depends on sqlite configuration -- synchronous=NORMAL, WAL). * Measure IO + CPU in background storage layer. Report via telemetry (if opted in). See ANRReporter for example. This will allow us to optimize.

Hasn't got as many tests as I'd like (but see pull request). Isn't perfect. Still ought to land. Doesn't do anything until later patches make it accessible.

Trivial include.

Consider this r?, with a note that there's a lot of TODO in environment generation. But I'd be happy to just fix those as we go.

Comment on attachment 747632 [details] [diff] [review] Part 3: wiring into Fennec. v1 Should BrowserHealthRecorder be "attached" to GeckoApp? This activity can be killed and recreated a lot of times. Maybe add to GeckoApplication? If we really only care about FHR for "when you're a browser", and we decide GeckoApp is better than GeckoApplication, should we move to BrowserApp? That is the "when you're a browser" activity.

Either. The important part is that BHR is alive when the user is tapping around in something they think of as "Firefox" -- anywhere it might record data (displaying UI for the first time, searches, installing add-ons, restoring a session). There is a small but non-zero cost to BHR initialization, because it has to grab a handle on the content provider and compute the environment, but if the content provider is still alive there will be no database access. The two avenues, then, are: * We reinit BHR every time a new browser activity runs, but we have a total handle on that, and BHR === a browser 'session' for FHR's purposes. * We put a BHR into GeckoApplication, and we watch for all of the activity lifecycle changes. Background footprint of Fennec would be slightly larger, but there would be less init work. Do you care about that background footprint? Is there anything material that can change between BrowserApp activity launches that would require us to recompute an environment? (E.g., add-on installs, profile switches?)

(In reply to Richard Newman [:rnewman] from comment #6) > Do you care about that background footprint? By background footprint, you mean the the times when we are a webapp and BHR is not strictly needed? GeckoApp and BrowserApp are heavily trampled areas. It might be nicer to just move BHR out of that area and into GeckoApp for pure cleanliness and maintainability. > Is there anything material that can change between BrowserApp activity > launches that would require us to recompute an environment? (E.g., add-on > installs, profile switches?) Currently, add-on changes and profile switches require full app restart. So all activities and GeckoApplication are killed and restarted. I think you recompute the environment on startup, which should be fine.

Comment on attachment 747632 [details] [diff] [review] Part 3: wiring into Fennec. v1 We can figure out the best place to init BHR in a followup

Comment on attachment 747632 [details] [diff] [review] Part 3: wiring into Fennec. v1 Meant to r+

This (and a pile of other bugs) look good on Try, including talos: https://tbpl.mozilla.org/?tree=Try&rev=23da1157d6d2

Nick reviewed the storage parts, IIRC.

Comment on attachment 747629 [details] [diff] [review] Part 2: manifest. v1 This is trivial.

Bringing this up to date. Carrying forward review.

Comment on attachment 751923 [details] [diff] [review] Part 3: wiring into Fennec. v2 Nick, I'd like another set of eyes on BrowserHealthRecorder. Note that the TODOs will be taken care of before this whole project is complete, so don't get snagged on those. And see also Bug 868449, which has the blocklist etc. parts.

(In reply to Richard Newman [:rnewman] from comment #14) > Note that the TODOs will be taken care of before this whole project is > complete, so don't get snagged on those. And see also Bug 868449, which has > the blocklist etc. parts. And Bug 868445 is for session recording, which is one of the TODOs.

Comment on attachment 751923 [details] [diff] [review] Part 3: wiring into Fennec. v2 Review of attachment 751923 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/base/GeckoApp.java @@ +1317,5 @@ > return; > } > > if (sGeckoThread != null) { > + // this happens when the GeckoApp activity is destroyed by Android nit: capitalize This and Bug. @@ +1392,5 @@ > editor.commit(); > + > + // The lifecycle of mHealthRecorder is "shortly after onCreate" > + // through "onDestroy" -- essentially the same as the lifecycle > + // of the activity itself. IIRC, the dispatcher you fetch has the same life cycle as GeckoAppShell, which is strictly longer than GeckoApp (and therefore mHealthRecord). Please verify. @@ +2040,5 @@ > > super.onDestroy(); > > Tabs.unregisterOnTabsChangedListener(this); > + BrowserHealthRecorder rec = mHealthRecorder; This little dance is to avoid a synchronized block, no? Please assert that it is okay to call BrowserHealthRecorder.close() twice. ::: mobile/android/base/GeckoProfile.java @@ +2,2 @@ > * This Source Code Form is subject to the terms of the Mozilla Public > + * License, v. 2.0. If a copy of the MPL was not distributed with this Wrong patch? ::: mobile/android/base/health/BrowserHealthRecorder.java @@ +22,5 @@ > +import org.mozilla.gecko.util.GeckoEventListener; > +import org.mozilla.gecko.util.ThreadUtils; > + > +/** > + * Keep an instance of this class around. nit: topic sentence explaining what this class is for. non-nit: link to relevant API functions for each of the phases you describe in the comment. non-nit: the code assumes that new BHR instances are created exactly once by GeckoApp; the comment is rather generic. If it's a singleton, make it a singleton in GeckoApp; if it's not, get rid of |instance| entirely. @@ +54,5 @@ > + > + /** > + * This constructor does IO. Run it on a background thread. > + */ > + public BrowserHealthRecorder(final Context context, final String profilePath, final EventDispatcher dispatcher) { An admirable attempt to decouple GeckoApp from BHR, but I'm not sure it's successful. See above comment about GeckoApp and singleton. @@ +80,5 @@ > + > + // TODO: record session start and end? > + } > + > + public void close(final EventDispatcher dispatcher) { nit: Fennec consistently names these operations init/uninit. @@ +81,5 @@ > + // TODO: record session start and end? > + } > + > + public void close(final EventDispatcher dispatcher) { > + Log.i(LOG_TAG, "Closing Health Report client."); Please add a comment about uninitializing providers here, and duplicate it in the class comment. (Like your initializing comment below.) @@ +92,5 @@ > + } > + > + /** > + * Call this when a material change has occurred in the running environment, > + * such that a new chapter should be recorded in FHR's logbook. chapter? logbook? I get that you're trying to introduce FHR to Fennec devs, but using different terms here will violate the principle of least surprise in the future. @@ +101,5 @@ > + this.env = -1; > + // TODO: update profileCache accordingly. > + } > + > + public synchronized int ensureEnvironment() { Why is this public? It's called internally and |this.env| is private. @@ +117,5 @@ > + this.profileCache.setTelemetryEnabled(true); > + this.profileCache.setProfileCreationTime(0L); > + this.profileCache.completeInitialization(); > + > + Log.d(LOG_TAG, "Done initializing profile cache. Beginning storage init."); nit: guard |Log| calls with isLoggable. @@ +128,5 @@ > + return; > + } > + > + try { > + // Initialize each provider here. This is great -- duplicate it in the class comment.

(In reply to Nick Alexander :nalexander from comment #16) > > + // this happens when the GeckoApp activity is destroyed by Android > > nit: capitalize This and Bug. You want me to fix all the errors in GeckoApp? ;) > IIRC, the dispatcher you fetch has the same life cycle as GeckoAppShell, > which is strictly longer than GeckoApp (and therefore mHealthRecord). > Please verify. GeckoAppShell.sEventDispatcher. It's static, so it has a really long lifecycle :) > This little dance is to avoid a synchronized block, no? Please assert that > it is okay to call BrowserHealthRecorder.close() twice. Nope, just being weird. This is in onDestroy, which is called once. Reworked to remove variable. > ::: mobile/android/base/GeckoProfile.java > @@ +2,2 @@ > > * This Source Code Form is subject to the terms of the Mozilla Public > > + * License, v. 2.0. If a copy of the MPL was not distributed with this > > Wrong patch? No, just cleaning up jank I saw along the way. > nit: Fennec consistently names these operations init/uninit. Bah. Java names it "close", even to the point of having an AutoClosable interface. I'll change this if I can face the pain of touching all the patches! > > + Log.d(LOG_TAG, "Done initializing profile cache. Beginning storage init."); > > nit: guard |Log| calls with isLoggable. isLoggable is more expensive than logging, unless you're doing a bunch of work to compute the string. This is one reason we wrote Logger.

(In reply to Richard Newman [:rnewman] from comment #17) > (In reply to Nick Alexander :nalexander from comment #16) > > > > + // this happens when the GeckoApp activity is destroyed by Android > > > > nit: capitalize This and Bug. > > You want me to fix all the errors in GeckoApp? ;) No, but if you make me review a diff hunk, I'm going to nit you. To death! > > nit: Fennec consistently names these operations init/uninit. > > Bah. Java names it "close", even to the point of having an AutoClosable > interface. > > I'll change this if I can face the pain of touching all the patches! > > > > + Log.d(LOG_TAG, "Done initializing profile cache. Beginning storage init."); > > > > nit: guard |Log| calls with isLoggable. > > isLoggable is more expensive than logging, unless you're doing a bunch of > work to compute the string. This is one reason we wrote Logger. I'm aware. This isn't speed, this is hygiene: Fennec (mostly) doesn't spew to the log unless asked.

(In reply to Nick Alexander :nalexander from comment #18) > I'm aware. This isn't speed, this is hygiene: Fennec (mostly) doesn't spew > to the log unless asked. I'd prefer to keep some of these one-time debug outputs live until we hit GA. We can prune in a couple of releases if things look happy and stable. Deal?

(In reply to Richard Newman [:rnewman] from comment #19) > (In reply to Nick Alexander :nalexander from comment #18) > > > I'm aware. This isn't speed, this is hygiene: Fennec (mostly) doesn't spew > > to the log unless asked. > > I'd prefer to keep some of these one-time debug outputs live until we hit > GA. We can prune in a couple of releases if things look happy and stable. > Deal? Deal!

remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/374417ab7178 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/eb43acde496e and part of remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/c32533bf03f3

https://hg.mozilla.org/mozilla-central/rev/374417ab7178 https://hg.mozilla.org/mozilla-central/rev/eb43acde496e https://hg.mozilla.org/mozilla-central/rev/c32533bf03f3

Comment on attachment 751918 [details] [diff] [review] Part 1: storage (from Git) v2 [Approval Request Comment] Bug caused by (feature/regressing bug #): New work. User impact if declined: No FHR in 23. Testing completed (on m-c, etc.): Baking on m-c for some time. QA overview written up; waiting for QA to execute. Manual testing and automated unit tests. Risk to taking this patch (and alternatives if risky): None: new work. String or IDL/UUID changes made by this patch: None.

Comment on attachment 751923 [details] [diff] [review] Part 3: wiring into Fennec. v2 Approving for uplift as part of the body of work for FHR's first revision on Android.

https://hg.mozilla.org/releases/mozilla-aurora/rev/6ec42b44b114 https://hg.mozilla.org/releases/mozilla-aurora/rev/b4716fe1ccbe https://hg.mozilla.org/releases/mozilla-aurora/rev/82e9de310632