868445 - Provider: sessions and startup times

Status: RESOLVED FIXED

(Firefox Health Report Graveyard :: Client: Android, defect, P2)

Comment 1

[Richard Newman [:rnewman]]

Attachment: Part 1: allow recording of JSON objects. v1 (git)

https://github.com/mozilla-services/android-sync/pull/316

Comment 2

[Richard Newman [:rnewman]]

Attachment: Part 2: sessions provider for FHR on Android. v1

Throwing this at Wes. Wes, if you don't think you have enough context, please redirect to nalexander!

Comment 3

[Richard Newman [:rnewman]]

Attachment: Part 1: allow recording of JSON objects. v2 (git)

Comment 4

[Richard Newman [:rnewman]]

Attachment: Part 2: sessions provider for FHR on Android. v2

Comment 5

[Richard Newman [:rnewman]]

Here's the current format.

http://rnewman.pastebin.mozilla.org/2485763

   "days": {
      "2013-06-04": {
        "MJ5k3KUY2H\/yDHbn1ZrNdRWiRPk=": {
          "org.mozilla.appSessions": {
            "abnormal": [
              {
                "stopped": 0,
                "r": "A",
                "oom": 0
              }
            ],
            "normal": [
              {
                "r": "P",
                "sg": 2752,
                "d": 17,
                "sj": 574
              }
            ],
            "_v": "4"
          }
        }
      }

Comment 6

[Richard Newman [:rnewman]]

Attachment: Part 2: sessions provider for FHR on Android. v3

Changes:

* Splitting and reformatting payload for compactness and sanity.
* Now records Gecko and Java startup times.

Comment 7

[Richard Newman [:rnewman]]

Note that when a user switches rapidly between other apps and Firefox, the activity will stay alive, and thus sessions won't need to init Java or Gecko. Those sessions will look like the second (five-second) one here:

{
  "org.mozilla.appSessions": {
    "normal": [
      {
        "r": "P",
        "sg": 2593,
        "d": 7,
        "sj": 412
      },
      {
        "r": "P",
        "d": 5
      }
    ],
    "_v": "4"
  }
}

Comment 8

[Richard Newman [:rnewman]]

Additional note: until the UI rework arrives (currently on fig), *using the Awesomebar* counts as switching away from the browser. We'll have multiple brief sessions recorded as a result.

Comment 9

[Richard Newman [:rnewman]]

Yes, those "r":"P" values are *probably* redundant right now, but eventually we'll record a session transition when your environment changes, and then we'll need a different reason for termination.

Comment 10

[Nick Alexander :nalexander [he/him]]

Comment on attachment 758178 [details] [diff] [review]
Part 2: sessions provider for FHR on Android. v3

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

LGTM, nits and one startup time issue.

::: mobile/android/base/GeckoApp.java
@@ +540,5 @@
>                  mGeckoReadyStartupTimer.stop();
> +
> +                // handleMessage runs on the background thread, so we don't
> +                // need to schedule a new runnable.
> +                mHealthRecorder.recordGeckoStartupTime(mGeckoReadyStartupTimer.getElapsed());

I think this will regress startup time.  The |geckoConnected| is the path to initializing the layer view, so we can't render until this is done.  Seems like we don't /need/ to schedule a new runnable but we want to.

@@ +1249,5 @@
>                  SharedPreferences.Editor editor = prefs.edit();
>                  editor.putBoolean(GeckoApp.PREFS_OOM_EXCEPTION, false);
>  
>                  // Put a flag to check if we got a normal onSaveInstanceState
>                  // on exit, or if we were suddenly killed (crash or native OOM)

nit: style this comment while you're here :)

@@ +1469,5 @@
>                      mShouldReportGeoData = false;
>              }
>          });
>  
> +        // Make a note of the startup of our Java App.

nit: improve.

@@ +1843,4 @@
>                  SharedPreferences.Editor editor = prefs.edit();
>                  editor.putBoolean(GeckoApp.PREFS_WAS_STOPPED, true);
> +                if (rec != null) {
> +                    rec.recordSessionEnd("P", editor);

Note about recording an FHR error if |rec == null|?

@@ +1945,2 @@
>          }
> +        mHealthRecorder = null;

nit: move this before the block.

::: mobile/android/base/health/BrowserHealthRecorder.java
@@ +128,5 @@
> +            boolean wasOOM = prefs.getBoolean(GeckoApp.PREFS_OOM_EXCEPTION, false);
> +            boolean wasStopped = prefs.getBoolean(GeckoApp.PREFS_WAS_STOPPED, true);
> +            long wallStartTime = prefs.getLong(PREFS_SESSION_START, 0L);
> +
> +            Log.d(LOG_TAG, "Building SessionInformation from prefs. " + wallStartTime + ", " + wasStopped + ", " + wasOOM);

nit: include the 0 argument, and maybe prefs: instead of prefs.

@@ +144,5 @@
> +            Log.d(LOG_TAG, "Recording start of session: " + this.wallStartTime);
> +            editor.putLong(PREFS_SESSION_START, this.wallStartTime);
> +        }
> +
> +        public void recordCompletion(SharedPreferences.Editor editor) {

nit: repeat "Does not commit."

@@ +152,5 @@
> +
> +        /**
> +         * Return the JSON that we'll put in the DB for this session.
> +         */
> +        public JSONObject getCompletionJSON(String reason, long realEndTime) throws JSONException {

I get that this is expedient, but I'd like |SessionInformation| to stand alone, with a |realEndTime| member.

@@ +166,5 @@
> +            }
> +            return out;
> +        }
> +
> +        public JSONObject getCrashedJSON() throws JSONException {

I'd rather "toString()" do the right thin, and |SessionInformation| have an API for querying status.  But it's nbd.

@@ +168,5 @@
> +        }
> +
> +        public JSONObject getCrashedJSON() throws JSONException {
> +            JSONObject out = new JSONObject();
> +            // We use ints here instead of booleans, because saving bytes in

This is a little odd -- since in most DBs, ints take more space than bytes.  It's only that you're serializing to JSON that this makes sense.  Reword?

@@ +193,5 @@
> +    public void recordGeckoStartupTime(long duration) {
> +        if (this.session == null) {
> +            return;
> +        }
> +        this.session.timedGeckoStartup = duration;

Hmm, your use of |session| as a whiteboard colours the above comments differently.  As you were.

@@ +804,5 @@
> +     * "d": duration. Value in seconds.
> +     * "sg": Gecko startup time. Present if this is a clean launch.
> +     * "sj": Java startup time. Present if this is a clean launch.
> +     *
> +     * Abnormal terminations will be missing a duration and will feature these keys.

How does this fit as a 5. above?

@@ +845,5 @@
> +        }
> +    }
> +
> +    /**
> +     * Call this *once* for each BrowserHealthRecorder, or double-writes will occur.

Why not flag this and throw?

@@ +855,5 @@
> +        }
> +        if (this.previousSession.wallStartTime == 0) {
> +            return;
> +        }
> +        

nit: whitespace.

@@ +872,5 @@
> +        }
> +    }
> +
> +    /**
> +     * Record that the current session ended.

Does not commit.

@@ +893,5 @@
> +            Log.e(LOG_TAG, "Session start " + session.wallStartTime + " isn't valid! Can't record end.");
> +            return;
> +        }
> +
> +        this.session = null;        // So it can't be double-recorded.

nit: move this right after the assignment.

Comment 11

[Richard Newman [:rnewman]]

(In reply to Nick Alexander :nalexander from comment #10)

> I think this will regress startup time.  The |geckoConnected| is the path to
> initializing the layer view, so we can't render until this is done.  Seems
> like we don't /need/ to schedule a new runnable but we want to.

As mentioned on IRC, this doesn't do much work (just assigns a member). The main point is to explain why this call doesn't occur in a background runnable: because this code is *already* being called on the background thread, and so we know mHealthRecorder will have been initialized.

I've reworked the comment to explain.

> Note about recording an FHR error if |rec == null|?

We wouldn't be able to... rec is the health recorder :P

Comments addressed, thanks!

Comment 12

[Richard Newman [:rnewman]]

https://hg.mozilla.org/services/services-central/rev/79088e422daf
https://hg.mozilla.org/services/services-central/rev/530fbc32771b

Comment 13

[Nick Alexander :nalexander [he/him]]

Comment on attachment 758097 [details] [diff] [review]
Part 1: allow recording of JSON objects. v2 (git)

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

Reviewed on github: https://github.com/mozilla-services/android-sync/pull/316.

Comment 14

[Richard Newman [:rnewman]]

Follow-up for missing import (not sure how that didn't make it):

https://hg.mozilla.org/services/services-central/raw-rev/168173d49c87

Comment 15

[Richard Newman [:rnewman]]

https://hg.mozilla.org/mozilla-central/rev/79088e422daf
https://hg.mozilla.org/mozilla-central/rev/530fbc32771b
https://hg.mozilla.org/mozilla-central/rev/168173d49c87

Comment 16

[Richard Newman [:rnewman]]

Comment on attachment 758097 [details] [diff] [review]
Part 1: allow recording of JSON objects. v2 (git)

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
  New work.

User impact if declined: 
  No recording of sessions => no FHR.

Testing completed (on m-c, etc.): 
  Baking on m-c, extensive manual testing.

Risk to taking this patch (and alternatives if risky): 
  Slim.

String or IDL/UUID changes made by this patch:
  None.

Comment 17

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 758178 [details] [diff] [review]
Part 2: sessions provider for FHR on Android. v3

Approving for uplift as part of the body of work for FHR's first revision on Android.

Comment 18

[Richard Newman [:rnewman]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/01d641ffb1fe
https://hg.mozilla.org/releases/mozilla-aurora/rev/a04d6f9d1131