Closed Bug 902729 Opened 6 years ago Closed 6 years ago

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

Categories

(Firefox :: Session Restore, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 26

People

(Reporter: ttaubert, Assigned: jfox)

References

Details

(Whiteboard: [good first bug][mentor=ttaubert][lang=js])

Attachments

(1 file, 1 obsolete file)

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.
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.
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.
I'm assigning the bug to you to let others know that you're working on it.
Assignee: nobody → jfox.mozilla
Status: NEW → ASSIGNED
Yes, I have the latest source and I'm able to build it successfully. This is great info; thank you for your guidance.
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?
(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.
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.
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.
Attached patch bug-902729-fix.patch (obsolete) — Splinter Review
Tim: Here is a patch for your review. Thanks.
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.
Attachment #790879 - Flags: feedback+
Tim: Updated the patch based on your feedback. Thanks.
Attachment #790879 - Attachment is obsolete: true
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.
Attachment #792198 - Flags: review+
Attachment #792198 - Flags: checkin+
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!
Attachment #792198 - Flags: checkin+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/9fb952a6e85e
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 26
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!
You need to log in before you can comment on or make changes to this bug.