Last Comment Bug 720456 - Include more startup-relevant information in telemetry pings
: Include more startup-relevant information in telemetry pings
Status: RESOLVED FIXED
:
Product: Toolkit
Classification: Components
Component: Telemetry (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla13
Assigned To: Nathan Froyd [:froydnj]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-01-23 11:43 PST by Nathan Froyd [:froydnj]
Modified: 2012-02-14 02:28 PST (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (3.92 KB, patch)
2012-01-23 13:07 PST, Nathan Froyd [:froydnj]
taras.mozilla: review-
Details | Diff | Review
patch (3.84 KB, patch)
2012-01-23 13:34 PST, Nathan Froyd [:froydnj]
taras.mozilla: review+
Details | Diff | Review
patch (4.21 KB, patch)
2012-01-24 06:40 PST, Nathan Froyd [:froydnj]
taras.mozilla: review+
Details | Diff | Review
patch (4.25 KB, patch)
2012-01-25 07:44 PST, Nathan Froyd [:froydnj]
nfroyd: review+
Details | Diff | Review
patch (4.25 KB, patch)
2012-02-09 08:04 PST, Nathan Froyd [:froydnj]
nfroyd: review+
Details | Diff | Review
patch (5.47 KB, patch)
2012-02-10 13:54 PST, Nathan Froyd [:froydnj]
nfroyd: review+
vladan.bugzilla: checkin+
Details | Diff | Review

Description Nathan Froyd [:froydnj] 2012-01-23 11:43:29 PST
+++ This bug was initially created as a clone of Bug #717069 +++

We should also include more histograms in copying into STARTUP_* ones. All of the disk cache ones, http calls.

Should also add a startupslowsql category to slow sql stuff.
Comment 1 Nathan Froyd [:froydnj] 2012-01-23 13:07:53 PST
Created attachment 590841 [details] [diff] [review]
patch

This patch:

1) Adds several new histograms to the STARTUP_* histograms.  I realize regexes are a poor-man's solution to this problem.  I have ideas about providing histogram categories (also useful for a user-friendly about:telemetry), but this is not the patch to introduce all that;
2) Adds a slowSQLStartup field to the ping;
3) Restores old behavior of only cloning STARTUP_* histograms if there was data in the originals.

Apologies for squishing so many things into one patch, but they're all one-liners by themselves...  Besides, you filed the fix-all-the-things bug. :)
Comment 2 (dormant account) 2012-01-23 13:10:27 PST
Comment on attachment 590841 [details] [diff] [review]
patch

you can use match multiple things in a regexp, ie something like /(SQLITE|HTTP)/
Comment 3 Nathan Froyd [:froydnj] 2012-01-23 13:34:38 PST
Created attachment 590850 [details] [diff] [review]
patch

Oh, very well then.
Comment 4 (dormant account) 2012-01-23 13:52:11 PST
Comment on attachment 590850 [details] [diff] [review]
patch

this._slowSQLStartup = Telemetry.slowSQL; should be conditional on 
slowSQL being non-empty.Ie we shouldn't send empty fields when possible.

r+ with above fixed.
Comment 5 Nathan Froyd [:froydnj] 2012-01-24 06:40:02 PST
Created attachment 591074 [details] [diff] [review]
patch

If we shouldn't send empty fields, then shouldn't we be checking when we stick this._slowSQLStartup into the ping payload?  Otherwise, we'd be sending payload.slowSQLStartup as either useful information or a null object; the latter doesn't seem very useful.
Comment 6 (dormant account) 2012-01-24 14:56:17 PST
Comment on attachment 591074 [details] [diff] [review]
patch

+      if (Object.keys(this._slowSQLStartup.mainThread).length != 0
+	  || Object.keys(this._slowSQLStartup.otherThreads).length != 0) {

Drop the != 0
Comment 7 (dormant account) 2012-01-24 14:57:41 PST
(In reply to Nathan Froyd (:froydnj) from comment #5)
> Created attachment 591074 [details] [diff] [review]
> patch
> 
> If we shouldn't send empty fields, then shouldn't we be checking when we
> stick this._slowSQLStartup into the ping payload?  Otherwise, we'd be
> sending payload.slowSQLStartup as either useful information or a null
> object; the latter doesn't seem very useful.

yeah, that was an oversight. I'll r+ a patch to avoid sending emptyness
Comment 8 Nathan Froyd [:froydnj] 2012-01-25 07:44:04 PST
Created attachment 591466 [details] [diff] [review]
patch

!= 0 dropped, rebased against trunk.  Carrying over r+.
Comment 10 Marco Bonardo [::mak] 2012-01-30 03:50:41 PST
backed out for failures in test_telemetryPng.js
Comment 11 Marco Bonardo [::mak] 2012-01-30 05:50:53 PST
and also failing in browser_TelemetryTimestamps.js
Comment 12 Nathan Froyd [:froydnj] 2012-02-09 08:04:02 PST
Created attachment 595747 [details] [diff] [review]
patch

Rebased patch that fixes tests.  The required changes were:

1) We need to observe sessionstore-windows-restored in test_TelemetryPing.js before sending our first test ping; this ensures we set _slowSQLStartup appropriately.  And this change just makes sense insofar as it makes the tests reflect what actually goes on in the browser.

2) browser_TelemetryTimestamps.js was requesting a payload before going through things like sessionstore-windows-restored, which meant that _slowSQLStartup was not being set.  Given that the get-payload topic was added solely for the benefit of that test, I think it's reasonable to call gatherStartupInformation in the handling of that topic.

I think these changes are trivial enough, even if the explanation is non-trivial, that I'm going to carry over r+.
Comment 13 Mozilla RelEng Bot 2012-02-09 08:51:59 PST
Autoland Patchset:
	Patches: 595747
	Branch: mozilla-central => try
Error applying patch 595747 to mozilla-central.
patching file toolkit/components/telemetry/TelemetryPing.js
Hunk #1 FAILED at 141
Hunk #2 FAILED at 338
Hunk #3 FAILED at 392
3 out of 4 hunks FAILED -- saving rejects to file toolkit/components/telemetry/TelemetryPing.js.rej
abort: patch failed to apply

Could not apply and push patchset:
Comment 14 Nathan Froyd [:froydnj] 2012-02-09 09:12:16 PST
Huh, that's weird.  My patch applies just fine to m-i.  Why would the autoland bot have problems?
Comment 15 Ed Morley [:emorley] 2012-02-09 09:20:05 PST
It pulls from m-c, which is currently 84 csets behind inbound.

I'm waiting on a PGO green before I merge (a number of bustages on inbound in the last 24 hours has limited the windows where a merge can be cut), shouldn't be too much longer.
Comment 16 Mozilla RelEng Bot 2012-02-10 13:28:42 PST
Autoland Patchset:
	Patches: 595747
	Branch: mozilla-central => try
Error applying patch 595747 to mozilla-central.
patching file toolkit/components/telemetry/TelemetryPing.js
Hunk #1 FAILED at 141
Hunk #2 FAILED at 338
Hunk #3 FAILED at 392
3 out of 4 hunks FAILED -- saving rejects to file toolkit/components/telemetry/TelemetryPing.js.rej
abort: patch failed to apply

Could not apply and push patchset:
Comment 17 Nathan Froyd [:froydnj] 2012-02-10 13:54:43 PST
Created attachment 596166 [details] [diff] [review]
patch

Rebasing against something sane.  Must have screwed up with the first patch somehow.
Comment 18 Mozilla RelEng Bot 2012-02-10 13:58:15 PST
Autoland Patchset:
	Patches: 596166
	Branch: mozilla-central => try
	Destination: http://hg.mozilla.org/try/rev/d2f1dbb8f800
Try run started, revision d2f1dbb8f800. To cancel or monitor the job, see: https://tbpl.mozilla.org/?tree=Try&rev=d2f1dbb8f800
Comment 19 Mozilla RelEng Bot 2012-02-10 20:05:59 PST
Try run for d2f1dbb8f800 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=d2f1dbb8f800
Results (out of 210 total builds):
    exception: 3
    success: 173
    warnings: 20
    failure: 14
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/autolanduser@mozilla.com-d2f1dbb8f800
Comment 21 Marco Bonardo [::mak] 2012-02-14 02:28:25 PST
https://hg.mozilla.org/mozilla-central/rev/34429047e2ad

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