Last Comment Bug 717105 - Add mechanism for easily adding additional JS timestamps to telemetry ping
: Add mechanism for easily adding additional JS timestamps to telemetry ping
Status: RESOLVED FIXED
:
Product: Toolkit
Classification: Components
Component: Telemetry (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla12
Assigned To: :Gavin Sharp [email: gavin@gavinsharp.com]
:
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-01-10 16:59 PST by :Gavin Sharp [email: gavin@gavinsharp.com]
Modified: 2012-02-01 07:09 PST (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (8.34 KB, patch)
2012-01-10 17:02 PST, :Gavin Sharp [email: gavin@gavinsharp.com]
no flags Details | Diff | Splinter Review
patch+test (14.41 KB, patch)
2012-01-10 18:12 PST, :Gavin Sharp [email: gavin@gavinsharp.com]
paul: review-
taras.mozilla: review+
Details | Diff | Splinter Review
patch v2 (12.99 KB, patch)
2012-01-19 17:08 PST, :Gavin Sharp [email: gavin@gavinsharp.com]
paul: review+
Details | Diff | Splinter Review
patch (14.21 KB, patch)
2012-01-24 10:11 PST, :Gavin Sharp [email: gavin@gavinsharp.com]
taras.mozilla: review+
Details | Diff | Splinter Review
followup fix (2.08 KB, patch)
2012-01-27 12:09 PST, :Gavin Sharp [email: gavin@gavinsharp.com]
taras.mozilla: review+
Details | Diff | Splinter Review

Description :Gavin Sharp [email: gavin@gavinsharp.com] 2012-01-10 16:59:01 PST
This will allow us to add additional timestamps to front-end code. PoC for this patch: sessionstore and delayedStartup, in Firefox.
Comment 1 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-01-10 17:02:10 PST
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.
Comment 2 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-01-10 18:12:11 PST
Created attachment 587561 [details] [diff] [review]
patch+test
Comment 3 (dormant account) 2012-01-10 19:01:37 PST
Comment on attachment 587561 [details] [diff] [review]
patch+test

What is the purpose of try catch in telemetryping.js?
Comment 4 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-01-11 12:01:57 PST
(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 5 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2012-01-11 12:17:03 PST
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.
Comment 6 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-01-11 13:51:10 PST
(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 (dormant account) 2012-01-12 15:57:43 PST
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.
Comment 8 (dormant account) 2012-01-17 15:24:24 PST
Gavin, do you plan to wrap this up soon?
Comment 9 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-01-19 17:08:00 PST
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.
Comment 10 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2012-01-20 17:08:50 PST
Comment on attachment 590053 [details] [diff] [review]
patch v2

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

Looks good.
Comment 11 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-01-24 10:11:31 PST
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?
Comment 12 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-01-24 15:15:34 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/383712b389bc
Comment 13 :Ms2ger (⌚ UTC+1/+2) 2012-01-25 07:19:42 PST
https://hg.mozilla.org/mozilla-central/rev/383712b389bc
Comment 14 (dormant account) 2012-01-27 10:32:35 PST
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
Comment 15 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-01-27 12:09:57 PST
Created attachment 592206 [details] [diff] [review]
followup fix
Comment 16 (dormant account) 2012-01-27 12:13:24 PST
Comment on attachment 592206 [details] [diff] [review]
followup fix

i think your testcase needs updating too
Comment 17 (dormant account) 2012-01-27 12:14:27 PST
nevermind, the testcase is fine.
Comment 18 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-01-27 12:43:41 PST
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
Comment 19 Joe Drew (not getting mail) 2012-01-28 19:03:46 PST
https://hg.mozilla.org/mozilla-central/rev/28c8e1836673

Note You need to log in before you can comment on or make changes to this bug.