Restore telemetry measurements that were removed when transitioning to the SessionWorker

RESOLVED FIXED in Firefox 25

Status

()

Firefox
Session Restore
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: ttaubert, Assigned: smacleod)

Tracking

(Blocks: 1 bug)

Trunk
Firefox 25
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

4 years ago
The patch that introduced the SessionWorker removed a couple of telemetry measurements:

https://hg.mozilla.org/mozilla-central/rev/242b07a68acf

We should re-add them. Those are the timestamps that aren't recorded anymore:

FX_SESSION_RESTORE_READ_FILE_MS
FX_SESSION_RESTORE_BACKUP_FILE_MS (the time to move the file)

FX_SESSION_RESTORE_WRITE_FILE_MS should probably be moved to the Worker as well to get a real timing without any event queue hiccups.

As the operations are running on the worker synchronously, we can use Date.now() to measure the running time and return those numbers to the chrome process via messages. The chrome process can then submit those values to Telemetry like here:

http://mxr.mozilla.org/mozilla-central/source/toolkit/components/telemetry/TelemetryStopwatch.jsm#108
Seems like a good companion to smacleod's other telemetry bug!
Assignee: nobody → smacleod
(Assignee)

Comment 2

4 years ago
Created attachment 783435 [details] [diff] [review]
Patch - Restore telemetry measurements

The worker now returns a telemetry object which can contain measurements taken inside. It is the callers responsibilities to record the measurements.

FX_SESSION_RESTORE_READ_FILE_MS and FX_SESSION_RESTORE_BACKUP_FILE_MS have been restored, and FX_SESSION_RESTORE_WRITE_FILE_MS now measures just the write inside the worker.

Try: https://tbpl.mozilla.org/?tree=Try&rev=bca5db918ebd
Attachment #783435 - Flags: review?(ttaubert)
(Assignee)

Updated

4 years ago
Status: NEW → ASSIGNED
(Assignee)

Updated

4 years ago
Blocks: 899831
(Reporter)

Comment 3

4 years ago
Comment on attachment 783435 [details] [diff] [review]
Patch - Restore telemetry measurements

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

Great patch! A few things:

::: browser/components/sessionstore/src/SessionWorker.js
@@ +25,1 @@
>   *                                id: custom_id}

Nit: can we maybe put that 'fail' object on the next line?

@@ +124,5 @@
>    /**
>     * Write the session to disk.
>     */
>    write: function (stateString, options) {
> +    let telemetry = {}

Nit: missing semicolon.

@@ +143,5 @@
>      let bytes = Encoder.encode(stateString);
> +    let startMs = Date.now();
> +    let result = File.writeAtomic(this.path, bytes, {tmpPath: this.path + ".tmp"});
> +    telemetry.FX_SESSION_RESTORE_WRITE_FILE_MS =  Date.now() - startMs;
> +    return {result: result, telemetry: telemetry};

We could move those lines into a separate _write() method maybe? So write() and writeLoadStateOnceAfterStartup() could share code as long as the latter still exists.

@@ +178,5 @@
> +    let bytes = Encoder.encode(stateString);
> +    let telemetry = {};
> +    let options = {tmpPath: this.path + ".tmp", outExecutionDuration: null};
> +    let result = File.writeAtomic(this.path, bytes, options);
> +    telemetry.FX_SESSION_RESTORE_WRITE_FILE_MS = options.outExecutionDuration;

I don't think that outExecutionDuration is supported?
Attachment #783435 - Flags: review?(ttaubert) → feedback+
(Assignee)

Comment 4

4 years ago
Created attachment 783849 [details] [diff] [review]
Patch - Restore telemetry measurements v2

Updated patch based on Tim's feedback. Also fixed a bug causing writeLoadStateOnceAfterStartup to fail.
Attachment #783435 - Attachment is obsolete: true
Attachment #783849 - Flags: review?(ttaubert)
(Assignee)

Comment 5

4 years ago
try: https://tbpl.mozilla.org/?tree=Try&rev=f42132e58c99
(Reporter)

Comment 6

4 years ago
Comment on attachment 783849 [details] [diff] [review]
Patch - Restore telemetry measurements v2

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

I think we can make this code a little easier even:

::: browser/components/sessionstore/src/SessionWorker.js
@@ +141,5 @@
>      }
>  
> +    let writeResult = this._write(stateString);
> +    telemetry.FX_SESSION_RESTORE_WRITE_FILE_MS = writeResult.durationMs;
> +    return {result: writeResult.result, telemetry: telemetry};

return this._write(stateString, telemetry);

@@ +176,5 @@
> +    let writeResult = this._write(JSON.stringify(state));
> +    return {
> +      result: writeResult.result,
> +      telemetry: {FX_SESSION_RESTORE_WRITE_FILE_MS: writeResult.durationMs}
> +    };

return this._write(JSON.stringify(state));

@@ +182,5 @@
> +
> +  /**
> +   * Write a stateString to disk
> +   */
> +  _write: function (stateString) {

_write: function (stateString, telemetry = {}) {

@@ +186,5 @@
> +  _write: function (stateString) {
> +    let bytes = Encoder.encode(stateString);
> +    let durationMs = Date.now();
> +    let result = File.writeAtomic(this.path, bytes, {tmpPath: this.path + ".tmp"});
> +    durationMs = Date.now() - durationMs;

telemetry.FX_SESSION_RESTORE_WRITE_FILE_MS = Date.now() - durationMs;

@@ +187,5 @@
> +    let bytes = Encoder.encode(stateString);
> +    let durationMs = Date.now();
> +    let result = File.writeAtomic(this.path, bytes, {tmpPath: this.path + ".tmp"});
> +    durationMs = Date.now() - durationMs;
> +    return {result: result, durationMs: durationMs};

return {result: result, telemetry: telemetry};
Attachment #783849 - Flags: review?(ttaubert)
(Assignee)

Comment 7

4 years ago
Created attachment 783899 [details] [diff] [review]
Patch - Restore telemetry measurements v3

Updated patch based on Tim's review. Much nicer :D
Attachment #783849 - Attachment is obsolete: true
Attachment #783899 - Flags: review?(ttaubert)
(Reporter)

Comment 8

4 years ago
Comment on attachment 783899 [details] [diff] [review]
Patch - Restore telemetry measurements v3

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

Cool, thanks!
Attachment #783899 - Flags: review?(ttaubert) → review+
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/bb8539a50e37
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/bb8539a50e37
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 25
You need to log in before you can comment on or make changes to this bug.