Closed
Bug 717105
Opened 12 years ago
Closed 12 years ago
Add mechanism for easily adding additional JS timestamps to telemetry ping
Categories
(Toolkit :: Telemetry, defect)
Toolkit
Telemetry
Tracking
()
RESOLVED
FIXED
mozilla12
People
(Reporter: Gavin, Assigned: Gavin)
Details
Attachments
(2 files, 3 obsolete files)
14.21 KB,
patch
|
taras.mozilla
:
review+
|
Details | Diff | Splinter Review |
2.08 KB,
patch
|
taras.mozilla
:
review+
|
Details | Diff | Splinter Review |
This will allow us to add additional timestamps to front-end code. PoC for this patch: sessionstore and delayedStartup, in Firefox.
Assignee | ||
Comment 1•12 years ago
|
||
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
Assignee | ||
Comment 2•12 years ago
|
||
Attachment #587542 -
Attachment is obsolete: true
Attachment #587561 -
Flags: review?(paul)
Assignee | ||
Updated•12 years ago
|
Attachment #587561 -
Flags: review?(taras.mozilla)
Comment 3•12 years ago
|
||
Comment on attachment 587561 [details] [diff] [review] patch+test What is the purpose of try catch in telemetryping.js?
Assignee | ||
Comment 4•12 years ago
|
||
(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•12 years ago
|
||
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-
Assignee | ||
Comment 6•12 years ago
|
||
(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•12 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•12 years ago
|
||
Gavin, do you plan to wrap this up soon?
Assignee | ||
Comment 9•12 years ago
|
||
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 10•12 years ago
|
||
Comment on attachment 590053 [details] [diff] [review] patch v2 Review of attachment 590053 [details] [diff] [review]: ----------------------------------------------------------------- Looks good.
Attachment #590053 -
Flags: review?(paul) → review+
Assignee | ||
Comment 11•12 years ago
|
||
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?
Assignee | ||
Updated•12 years ago
|
Attachment #591151 -
Flags: review? → review?(taras.mozilla)
Updated•12 years ago
|
Attachment #591151 -
Flags: review?(taras.mozilla) → review+
Assignee | ||
Comment 12•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/383712b389bc
Comment 13•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/383712b389bc
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla12
Comment 14•12 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
Assignee | ||
Comment 15•12 years ago
|
||
Assignee | ||
Updated•12 years ago
|
Attachment #592206 -
Flags: review?(taras.mozilla)
Comment 16•12 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•12 years ago
|
||
nevermind, the testcase is fine.
Assignee | ||
Comment 18•12 years ago
|
||
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.
Description
•