Closed Bug 717105 Opened 8 years ago Closed 8 years ago

Add mechanism for easily adding additional JS timestamps to telemetry ping

Categories

(Toolkit :: Telemetry, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla12

People

(Reporter: Gavin, Assigned: Gavin)

Details

Attachments

(2 files, 3 obsolete files)

This will allow us to add additional timestamps to front-end code. PoC for this patch: sessionstore and delayedStartup, in Firefox.
Attached patch patch (obsolete) — Splinter Review
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
Attached patch patch+test (obsolete) — Splinter Review
Attachment #587542 - Attachment is obsolete: true
Attachment #587561 - Flags: review?(paul)
Attachment #587561 - Flags: review?(taras.mozilla)
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 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+
Gavin, do you plan to wrap this up soon?
Attached patch patch v2 (obsolete) — Splinter Review
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+
Attached patch patchSplinter Review
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)
Attachment #591151 - Flags: review?(taras.mozilla) → review+
https://hg.mozilla.org/mozilla-central/rev/383712b389bc
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla12
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
Attachment #592206 - Flags: review?(taras.mozilla)
Comment on attachment 592206 [details] [diff] [review]
followup fix

i think your testcase needs updating too
Attachment #592206 - Flags: review?(taras.mozilla) → review+
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
You need to log in before you can comment on or make changes to this bug.