720456 - Include more startup-relevant information in telemetry pings

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect)

Description

[Nathan Froyd [:froydnj]]

+++ 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]]

Attachment: 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)]

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]]

Attachment: patch

Oh, very well then.

Comment 4

[(dormant account)]

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]]

Attachment: 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)]

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)]

(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]]

Attachment: patch

!= 0 dropped, rebased against trunk.  Carrying over r+.

Comment 9

[Dão Gottwald [:dao]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/3bd7584753ae

Comment 10

[Marco Bonardo [:mak]]

backed out for failures in test_telemetryPng.js

Comment 11

[Marco Bonardo [:mak]]

and also failing in browser_TelemetryTimestamps.js

Comment 12

[Nathan Froyd [:froydnj]]

Attachment: 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]

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]]

Huh, that's weird.  My patch applies just fine to m-i.  Why would the autoland bot have problems?

Comment 15

[Ed Morley [:emorley]]

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]

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]]

Attachment: patch

Rebasing against something sane.  Must have screwed up with the first patch somehow.

Comment 18

[Mozilla RelEng Bot]

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]

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/[e-mail]-d2f1dbb8f800

Comment 20

[Vladan Djeric (:vladan)]

Comment on attachment 596166 [details] [diff] [review]
patch

https://hg.mozilla.org/integration/mozilla-inbound/rev/34429047e2ad

Comment 21

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/34429047e2ad