Include more startup-relevant information in telemetry pings

RESOLVED FIXED in mozilla13

Status

()

Toolkit
Telemetry
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: froydnj, Assigned: froydnj)

Tracking

unspecified
mozilla13
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 5 obsolete attachments)

(Assignee)

Description

5 years ago
+++ 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.
(Assignee)

Comment 1

5 years ago
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. :)
Assignee: nobody → nfroyd
Status: NEW → ASSIGNED
Attachment #590841 - Flags: review?(taras.mozilla)

Comment 2

5 years ago
Comment on attachment 590841 [details] [diff] [review]
patch

you can use match multiple things in a regexp, ie something like /(SQLITE|HTTP)/
Attachment #590841 - Flags: review?(taras.mozilla) → review-
(Assignee)

Comment 3

5 years ago
Created attachment 590850 [details] [diff] [review]
patch

Oh, very well then.
Attachment #590841 - Attachment is obsolete: true
Attachment #590850 - Flags: review?(taras.mozilla)

Comment 4

5 years ago
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.
Attachment #590850 - Flags: review?(taras.mozilla) → review+
(Assignee)

Comment 5

5 years ago
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.
Attachment #590850 - Attachment is obsolete: true
Attachment #591074 - Flags: review?(taras.mozilla)

Comment 6

5 years ago
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
Attachment #591074 - Flags: review?(taras.mozilla) → review+

Comment 7

5 years ago
(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
(Assignee)

Comment 8

5 years ago
Created attachment 591466 [details] [diff] [review]
patch

!= 0 dropped, rebased against trunk.  Carrying over r+.
Attachment #591074 - Attachment is obsolete: true
Attachment #591466 - Flags: review+
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
http://hg.mozilla.org/integration/mozilla-inbound/rev/3bd7584753ae
Keywords: checkin-needed
backed out for failures in test_telemetryPng.js
and also failing in browser_TelemetryTimestamps.js
(Assignee)

Comment 12

5 years ago
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+.
Attachment #591466 - Attachment is obsolete: true
Attachment #595747 - Flags: review+
(Assignee)

Updated

5 years ago
Keywords: checkin-needed

Updated

5 years ago
Whiteboard: [autoland-try]

Updated

5 years ago
Whiteboard: [autoland-try] → [autoland-in-queue]

Comment 13

5 years ago
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:

Updated

5 years ago
Whiteboard: [autoland-in-queue]
(Assignee)

Comment 14

5 years ago
Huh, that's weird.  My patch applies just fine to m-i.  Why would the autoland bot have problems?
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.

Updated

5 years ago
Whiteboard: [autoland-try]

Updated

5 years ago
Whiteboard: [autoland-try] → [autoland-in-queue]

Comment 16

5 years ago
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:

Updated

5 years ago
Whiteboard: [autoland-in-queue]
(Assignee)

Comment 17

5 years ago
Created attachment 596166 [details] [diff] [review]
patch

Rebasing against something sane.  Must have screwed up with the first patch somehow.
Attachment #595747 - Attachment is obsolete: true
Attachment #596166 - Flags: review+
(Assignee)

Updated

5 years ago
Whiteboard: [autoland-try]

Updated

5 years ago
Whiteboard: [autoland-try] → [autoland-in-queue]

Comment 18

5 years ago
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

5 years ago
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

Updated

5 years ago
Whiteboard: [autoland-in-queue]
Comment on attachment 596166 [details] [diff] [review]
patch

https://hg.mozilla.org/integration/mozilla-inbound/rev/34429047e2ad
Attachment #596166 - Flags: checkin+
Keywords: checkin-needed
Target Milestone: --- → mozilla13
https://hg.mozilla.org/mozilla-central/rev/34429047e2ad
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.