Add mechanism for easily adding additional JS timestamps to telemetry ping

RESOLVED FIXED in mozilla12

Status

()

Toolkit
Telemetry
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Gavin, Assigned: Gavin)

Tracking

Trunk
mozilla12
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 3 obsolete attachments)

This will allow us to add additional timestamps to front-end code. PoC for this patch: sessionstore and delayedStartup, in Firefox.
Created attachment 587542 [details] [diff] [review]
patch

Apps can add a TelemetryTimestamps module that gets loaded by TelemetryPing, who then adds the properties to its simpleMeasurements thinger. I still need to write a test for this.
Assignee: nobody → gavin.sharp
Status: NEW → ASSIGNED
Created attachment 587561 [details] [diff] [review]
patch+test
Attachment #587542 - Attachment is obsolete: true
Attachment #587561 - Flags: review?(paul)
Attachment #587561 - Flags: review?(taras.mozilla)

Comment 3

5 years ago
Comment on attachment 587561 [details] [diff] [review]
patch+test

What is the purpose of try catch in telemetryping.js?
(In reply to Taras Glek (:taras) from comment #3)
> What is the purpose of try catch in telemetryping.js?

A telemetry-using app may not have a TelemetryTimestamps.jsm.
Comment on attachment 587561 [details] [diff] [review]
patch+test

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

The sessionstore usage is on its way, but I think it needs to be a bit more fine tuned. Apart from the comments below, we're going to miss some points for on demand restores (restore previous session), and probably some restores going through the API (typically extensions go through here, though maybe not TMP).

::: browser/base/content/browser.js
@@ +1505,5 @@
>  }
>  
>  function delayedStartup(isLoadingBlank, mustLoadSidebar) {
> +  Cu.import("resource:///modules/TelemetryTimestamps.jsm");
> +  TelemetryTimestamps.add("delayedStartupStarted");

Isn't this just going to invalidate the value once a second window is opened?

::: browser/components/sessionstore/src/nsSessionStore.js
@@ +829,3 @@
>      // perform additional initialization when the first window is loading
>      if (this._loadState == STATE_STOPPED) {
> +      TelemetryTimestamps.add("sessionRestoreRestoring");

I think you'd actually want this under if (_initialState) (and it's not going to distinguish between session showing about:sessionrestore vs actual restore).

@@ +3219,5 @@
>        // want to attempt to restore the next tab.
>        if (!didStartLoad)
>          this.restoreNextTab();
> +    } else {
> +      TelemetryTimestamps.add("sessionRestoreTabsEnd");

For people with restore_on_demand = true, this may never happen.

Also, for sessions that only have a single tab in each window (even 1 window), we won't ever get here.

You should be able to piggyback on the SSTabRestored event (_sendTabRestoredNotification) - keep track of how many tabs should be restored.
Attachment #587561 - Flags: review?(paul) → review-
(In reply to Paul O'Shannessy [:zpao] from comment #5)
> The sessionstore usage is on its way, but I think it needs to be a bit more
> fine tuned. Apart from the comments below, we're going to miss some points
> for on demand restores (restore previous session), and probably some
> restores going through the API (typically extensions go through here, though
> maybe not TMP).

For the moment, we really just care about restoring on startup. But you're right that we need to be careful that we're not invalidating timestamps by adding them multiple times.

> Isn't this just going to invalidate the value once a second window is opened?

Yes indeed it will! Good catch.

> I think you'd actually want this under if (_initialState) (and it's not
> going to distinguish between session showing about:sessionrestore vs actual
> restore).

I think that's fine.

> For people with restore_on_demand = true, this may never happen.

That's OK, but good to note when interpreting the data. I may have gone overboard with the initial timestamps. It may also be useful to make add() just ignore subsequent additions, rather than treating them as invalid, so we don't need to worry about that at the callsites.

Comment 7

5 years ago
Comment on attachment 587561 [details] [diff] [review]
patch+test


> 
>+  // Look for app-specific timestamps
>+  try {
>+    Cu.import("resource:///modules/TelemetryTimestamps.jsm");
>+    let obj = TelemetryTimestamps.get();
>+
>+    for (let p in obj) {
>+      if (!(p in ret) && obj[p])
>+        ret[p] = obj[p];
>+    }
>+  } catch (ex) {}

move obj and for loop outside of try. We've had problems with exceptions due to syntax errors, etc getting swallowed by try.

r+ for telemetry/ bits with that.

Sorry for slow review, I reviewed this once and forgot to commit.
Attachment #587561 - Flags: review?(taras.mozilla) → review+

Comment 8

5 years ago
Gavin, do you plan to wrap this up soon?
Created attachment 590053 [details] [diff] [review]
patch v2

Comments addressed. Trimmed down some of the sessionRestore timestamps, and made the module ignore subsequent timestamp additions, rather than invalidating the saved value.
Attachment #587561 - Attachment is obsolete: true
Attachment #590053 - Flags: review?(paul)
Comment on attachment 590053 [details] [diff] [review]
patch v2

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

Looks good.
Attachment #590053 - Flags: review?(paul) → review+
Created attachment 591151 [details] [diff] [review]
patch

I got bitrotted by bug 707320, had to make some changes to the patch - please re-review the TelemetryPing portions?
Attachment #590053 - Attachment is obsolete: true
Attachment #591151 - Flags: review?
Attachment #591151 - Flags: review? → review?(taras.mozilla)

Updated

5 years ago
Attachment #591151 - Flags: review?(taras.mozilla) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/383712b389bc
https://hg.mozilla.org/mozilla-central/rev/383712b389bc
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla12

Comment 14

5 years ago
Comment on attachment 591151 [details] [diff] [review]
patch

+  for (let p in appTimestamps) {
+    if (!(p in ret) && appTimestamps[p])
+      ret[p] = appTimestamps[p];
+  }

I just realized this makes the patch useless as it sends in localtime stamps. Note how all of the other timestamps are just sent as milliseconds since startup via -process
http://mxr.mozilla.org/mozilla-central/source/toolkit/components/telemetry/TelemetryPing.js#115
Created attachment 592206 [details] [diff] [review]
followup fix
Attachment #592206 - Flags: review?(taras.mozilla)

Comment 16

5 years ago
Comment on attachment 592206 [details] [diff] [review]
followup fix

i think your testcase needs updating too
Attachment #592206 - Flags: review?(taras.mozilla) → review+

Comment 17

5 years ago
nevermind, the testcase is fine.
I did have to tweak the test a little bit, since it also checks the TelemetryPing'ed values:
https://hg.mozilla.org/integration/mozilla-inbound/rev/28c8e1836673
https://hg.mozilla.org/mozilla-central/rev/28c8e1836673
You need to log in before you can comment on or make changes to this bug.