Closed Bug 870104 Opened 8 years ago Closed 7 years ago

Add telemetry to BackgroundPageThumbs

Categories

(Toolkit :: General, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla25

People

(Reporter: adw, Assigned: adw)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

Add telemetry to BackgroundPageThumbs.  Also, modify PageThumbs's telemetry where necessary so that BackgroundPageThumbs's and PageThumbs's telemetries can be compared where it makes sense to compare them.  (Or maybe it never makes sense to compare them.  Haven't really thought about it.  Just pointing out the possibility.)
Assignee: nobody → adw
Status: NEW → ASSIGNED
Attached patch patch (obsolete) — Splinter Review
The top of this patch shows the 7 new histograms.  Telemetry silently fails in the content process (bug 680508), so the content script has to ship the data for its two histograms along with the capture data.  The FX_THUMBNAILS_ naming prefix follows the existing thumbnail histogram names.
Attachment #773733 - Flags: review?(mhammond)
Gavin and I decided yesterday that the only telemetry comparable between PageThumbs and BackgroundPageThumbs is the time it takes to draw the canvas, which is already measured, so removing the PageThumbs part from the summary.
Summary: Add telemetry to BackgroundPageThumbs, and modify PageThumbs's so that the two are comparable → Add telemetry to BackgroundPageThumbs
Comment on attachment 773733 [details] [diff] [review]
patch

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

::: toolkit/components/telemetry/Histograms.json
@@ +3643,5 @@
>    "MIXED_CONTENT_PAGE_LOAD": {
>      "kind": "enumerated",
>      "n_values": 4,
>      "description": "Accumulates type of content (mixed, mixed passive, unmixed) per page load"
> +  },

I'm not experienced enough with telemetry to know if the individual attributes for each of these counters is appropriate - please make sure you get someone who is experienced to check these (or you may already have!)

::: toolkit/components/thumbnails/BackgroundPageThumbs.jsm
@@ +54,4 @@
>      // we want to avoid duplicate captures for the same URL.  If there is an
>      // existing one, we just add the callback to that one and we are done.
>      let existing = this._captureQueueByUrl.get(url);
> +    tel("CAPTURE_ALREADY_QUEUED", Number(existing));

TBH I don't really see why we are recording this.  It seems the bar should be "would the data from a counter cause us to change things" and I can't see how this qualifies for that.  (Unless maybe we are thinking of ripping out the dupe detection code if it comes back very low?)

@@ +246,5 @@
>     * @param messageManager  The nsIMessageSender of the thumbnail browser.
>     */
>    start: function (messageManager) {
> +    tel("CAPTURE_QUEUE_TIME_MS", new Date() - this.creationDate);
> +    this.startDate = new Date();

probably makes sense to swap these lines, and just use this.startDate in both lines to avoid the extra "new Date()"

@@ +331,5 @@
>  
>      this.captureCallback(this);
>      this.destroy();
>  
> +    if (data && data.telemetry) {

I think it's probably worth a quick comment here mentioning bug 680508.

@@ +350,5 @@
>      }.bind(this);
>  
>      if (!data || this._privateWinOpenedDuringCapture) {
> +      if (this._privateWinOpenedDuringCapture)
> +        tel("CAPTURE_DONE_REASON", TEL_CAPTURE_DONE_PB_AFTER_START);

is there really value in splitting "PB_BEFORE" and "PB_AFTER"?
Attachment #773733 - Flags: review?(mhammond) → review+
Attached patch patch 2Splinter Review
(In reply to Mark Hammond (:markh) from comment #3)
> ::: toolkit/components/telemetry/Histograms.json
> @@ +3643,5 @@
> >    "MIXED_CONTENT_PAGE_LOAD": {
> >      "kind": "enumerated",
> >      "n_values": 4,
> >      "description": "Accumulates type of content (mixed, mixed passive, unmixed) per page load"
> > +  },
> 
> I'm not experienced enough with telemetry to know if the individual
> attributes for each of these counters is appropriate - please make sure you
> get someone who is experienced to check these (or you may already have!)

Sure, Nathan, you added Histograms.json, so would you mind verifying that the Histograms.json additions in this patch look OK?

> ::: toolkit/components/thumbnails/BackgroundPageThumbs.jsm
> @@ +54,4 @@
> >      // we want to avoid duplicate captures for the same URL.  If there is an
> >      // existing one, we just add the callback to that one and we are done.
> >      let existing = this._captureQueueByUrl.get(url);
> > +    tel("CAPTURE_ALREADY_QUEUED", Number(existing));
> 
> TBH I don't really see why we are recording this.  It seems the bar should
> be "would the data from a counter cause us to change things" and I can't see
> how this qualifies for that.  (Unless maybe we are thinking of ripping out
> the dupe detection code if it comes back very low?)

I'm erring on the side of capturing data that may turn out to be useless, because (a) I'm curious even if it is useless, and (b) it's hard to know beforehand if it's actually useless.  But you're probably right about this one, and I removed it.

> @@ +350,5 @@
> >      }.bind(this);
> >  
> >      if (!data || this._privateWinOpenedDuringCapture) {
> > +      if (this._privateWinOpenedDuringCapture)
> > +        tel("CAPTURE_DONE_REASON", TEL_CAPTURE_DONE_PB_AFTER_START);
> 
> is there really value in splitting "PB_BEFORE" and "PB_AFTER"?

In this case I really am curious to see how often PB_AFTER happens, so if it's OK I'd like to leave them separated.
Attachment #773733 - Attachment is obsolete: true
Attachment #774143 - Flags: review?(nfroyd)
Comment on attachment 774143 [details] [diff] [review]
patch 2

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

I think these look sane.
Attachment #774143 - Flags: review?(nfroyd) → review+
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/de8a514dcdc5 because I didn't know whether it was this or bug 891169 that caused the oddly 10.8 opt only timeouts like https://tbpl.mozilla.org/php/getParsedLog.php?id=25256458&tree=Mozilla-Inbound
https://hg.mozilla.org/mozilla-central/rev/99415f0212a6
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
You need to log in before you can comment on or make changes to this bug.