870104 - Add telemetry to BackgroundPageThumbs

Status: RESOLVED FIXED

(Toolkit :: General, defect)

Description

[Drew Willcoxon :adw]

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

Comment 1

[Drew Willcoxon :adw]

Attachment: patch

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.

Comment 2

[Drew Willcoxon :adw]

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.

Comment 3

[Mark Hammond [:markh] [:mhammond]]

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"?

Comment 4

[Drew Willcoxon :adw]

Attachment: patch 2

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

Comment 5

[Nathan Froyd [:froydnj]]

Comment on attachment 774143 [details] [diff] [review]
patch 2

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

I think these look sane.

Comment 6

[Drew Willcoxon :adw]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0327b814b3ff

Comment 7

[Phil Ringnalda (:philor)]

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

Comment 8

[Drew Willcoxon :adw]

https://hg.mozilla.org/integration/mozilla-inbound/rev/99415f0212a6

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/99415f0212a6