902729 - [Session Restore] Use sessionStartTime to send a telemetry ping for session life times

Status: RESOLVED FIXED

(Firefox :: Session Restore, defect)

Description

[Tim Taubert [:ttaubert] (inactive)]

So I just stumbled upon bug 665260. Turns out that we actually store the time the session was started. I think that's quite an interesting metric and we should implement the telemetry ping for it, after all that's why it's been added.

Comment 1

[Jesse Fox [:jfox]]

Hi Tim,
I'd like to work on adding this functionality. Could you point me in the right direction to help me get started? Thanks.

Comment 2

[Tim Taubert [:ttaubert] (inactive)]

Hi Jesse, that's great to hear! What you need is a checkout of the Firefox source code and you should've already built Firefox on your computer at least once. Here's a good guide on how to get there:

https://developer.mozilla.org/en-US/docs/Simple_Firefox_build

All changes will be made to browser/components/sessionstore/src/SessionStore.jsm. There are two methods that need be changed. The first is initSession():

http://mxr.mozilla.org/mozilla-central/source/browser/components/sessionstore/src/SessionStore.jsm#464

The second is restoreLastSession():

http://mxr.mozilla.org/mozilla-central/source/browser/components/sessionstore/src/SessionStore.jsm#1836

When restoring the session in one of those two methods we want to report the length of the restored session. We can do this like so:

Cc["@mozilla.org/base/telemetry;1"]
  .getService(Ci.nsITelemetry)
  .getHistogramById("FX_SESSION_RESTORE_SESSION_LENGTH_MS")
  .add(Date.now() - this._sessionStartTime);

The only thing left now is to create the histogram "FX_SESSION_RESTORE_SESSION_LENGTH_MS" so that we can add values to it. This needs to be done in toolkit/components/telemetry/Histograms.json.

Feel free to ask me here or on IRC (my nickname is ttaubert) should you have any questions.

Comment 3

[Tim Taubert [:ttaubert] (inactive)]

I'm assigning the bug to you to let others know that you're working on it.

Comment 4

[Jesse Fox [:jfox]]

Yes, I have the latest source and I'm able to build it successfully. This is great info; thank you for your guidance.

Comment 5

[Jesse Fox [:jfox]]

Hi Tim,

What do we want to do if there is nothing to restore or if there is no timestamp? In the below code snippet, it is possible that this._sessionStartTime will be re-assigned its original Date.now() value.

this._sessionStartTime = this._initialState.session &&
                         this._initialState.session.startTime ||
                         this._sessionStartTime;

My assumption is that we should not report session length for this scenerio, but I want to make sure that it should be ignored and not handled in a special way.

Also, do we need a test for this? If so, can you tell me where the test should be written?

And one last thing...is there a good way to debug these JavaScript modules, such as SessionStore.jsm?

Comment 6

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Jesse Fox from comment #5)
> My assumption is that we should not report session length for this scenerio,
> but I want to make sure that it should be ignored and not handled in a
> special way.

Correct, if the session to be restored doesn't have any .startTime property (for whatever reason) we don't want to submit a telemetry measurement.

> Also, do we need a test for this? If so, can you tell me where the test
> should be written?

AFAIK, we usually don't do any testing of telemetry submissions. I think this should be good enough without a test but it's definitely great that you thought of that!

> And one last thing...is there a good way to debug these JavaScript modules,
> such as SessionStore.jsm?

We do have a debugger that in mysterious ways can be enabled to debug chrome code as well. OTOH you can just rely on default error reporting if you set browser.dom.window.dump.enabled=true in about:config.

Also, if you use the debug() method defined in SessionStore.jsm those message are printed to the error console.

Comment 7

[Jesse Fox [:jfox]]

Tim, the last thing I need to know before I submit a patch for your review is what kind of histogram you would like? What range of session lengths do you want to capture, how many buckets, etc? Thanks.

Comment 8

[Tim Taubert [:ttaubert] (inactive)]

Talking about restored session and their lifetimes, I think it would be a good idea to measure in days instead of hours or less. We should use 15 buckets and a high of 365.

Comment 9

[Jesse Fox [:jfox]]

Attachment: bug-902729-fix.patch

Tim: Here is a patch for your review. Thanks.

Comment 10

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 790879 [details] [diff] [review]
bug-902729-fix.patch

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

Thank you, Jesse. This looks great! A couple of things:

::: browser/components/sessionstore/src/SessionStore.jsm
@@ +468,5 @@
> +          if (state.session && state.session.startTime) {
> +            this._sessionStartTime = state.session.startTime;
> +
> +            // ms to days
> +            let sessionLength = (Date.now() - this._sessionStartTime) / 1000.0 / 60.0 / 60.0 / 24.0;

You could define a constant at the top of the file (or right before you use it) like:

// Number of milliseconds in a day.
const MS_PER_DAY = 1000.0 / 60.0 / 60.0 / 24.0;

and then do:

// ms to days
let sessionLength = (Date.now() - this._sessionStartTime) / MS_PER_DAY;

Additionally we should guard against malicious .startTime values. A simple if (sessionLength > 0) would do, I think.

@@ +472,5 @@
> +            let sessionLength = (Date.now() - this._sessionStartTime) / 1000.0 / 60.0 / 60.0 / 24.0;
> +
> +            // Submit the session length telemetry measurement
> +            Cc["@mozilla.org/base/telemetry;1"]
> +              .getService(Ci.nsITelemetry)

You can use Services.telemetry.getHistogramById(...).

@@ +1811,5 @@
>                            lastSessionState.session.recentCrashes || 0;
> +
> +    // Attempt to load the session start time from the previous state
> +    if (lastSessionState.session && lastSessionState.session.startTime) {
> +      this._sessionStartTime = lastSessionState.session.startTime;

In order to not repeat ourselves too much, let's move all this to a separate function like '_updateSessionStartTime()'. That can take a session state as an argument, update the _sessionStartTime field and record the telemetry measurement.

Comment 11

[Jesse Fox [:jfox]]

Attachment: bug-902729-fix.patch

Tim: Updated the patch based on your feedback. Thanks.

Comment 12

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 792198 [details] [diff] [review]
bug-902729-fix.patch

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

Thank you, this looks good, has comments and doesn't break any tests. Great work, Jesse!

Please mark this patch as checkin-needed so that someone can check it in.

Comment 13

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 792198 [details] [diff] [review]
bug-902729-fix.patch

Jesse, "checkin" is a field belonging to the attachment that tells whether is has already been checked in. To request a checkin for your patch you need to use the "Keywords" field at the top of the bug and add the "checkin-needed" keyword. Thanks!

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9fb952a6e85e

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9fb952a6e85e

Comment 16

[Tim Taubert [:ttaubert] (inactive)]

Thanks a lot, Jesse! Congratulations on your first patch shipping in Firefox 26 :) I hope to see you around for another mentored bug or on IRC!