1208167 - Telemetry probe for click-to-play images

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Jonathan Almeida [:jonalmeida]]

Do we want a probe to see if this feature is being used?

If yes, will it be behind just the UI checkbox option or somewhere else?

Comment 1

[:Margaret Leibovic]

Yes, I think we should measure whether or not people use this.

We do already have UI telemetry for settings that tells us when people toggle preferences, but that doesn't tell us what % of users have a feature enabled.

I think it would also be interesting to know how often users actually click to show images. 

Barbara, are there any other metrics you think we should take into consideration here?

(PS - Jonathan, I just gave your account canconfirm and editbugs privs :)

Comment 2

[Mark Finkle (:mfinkle) (use needinfo?)]

I suggested putting the contextmenu telemetry in my review of bug 1170725. We can use this bug for any other probes we want, like the "feature enabled" probe Margaret suggests in comment 1.

Comment 3

[Barbara Bermes [:barbara] - NI please!]

Things I can think of

- Click to show images enabled (via settings): yes/no
- From the ones who have it enabled, who actually clicked on the image vs. who didn't
- Can we get info about the image sizes that were clicked to view? 
- Can we also get info of the people's locales / countries who are using this feature (I don't think so)

Comment 4

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Barbara Bermes [:barbara] from comment #3)
> Things I can think of
> 
> - Click to show images enabled (via settings): yes/no

Done

> - From the ones who have it enabled, who actually clicked on the image vs.
> who didn't

Margaret's proposal in comment 1

> - Can we get info about the image sizes that were clicked to view? 

Might be tough. If we can get the sizes, we'd likely use a histogram to track the sizes.

> - Can we also get info of the people's locales / countries who are using
> this feature (I don't think so)

Locales are sent with telemetry. I don't think dashboards are capable of filtering by locale, so you'd need a custom analysis.

Comment 5

[:Margaret Leibovic]

Jonathan, are you interested in working on this bug? We should try to get a patch landed sooner rather than later, so if you're too busy, we should find someone else to work on this :)

Comment 6

[Jonathan Almeida [:jonalmeida]]

Yes! I'll look into it in this week. Everything seems trivial to implement, except the image sizes.

Comment 7

[Jonathan Almeida [:jonalmeida]]

Attachment: telemetry-ctp-images-bug_265659.patch

Proposed patch with a probe for the image size, and when an image is unblocked.

Getting the image size feels a bit hacky since there isn't any good way to get the size. I make a HEAD request and determine the size from the content length.

Comment 8

[Jonathan Almeida [:jonalmeida]]

Also note that I put in some fake values for the probes as a placeholder till we have a better one.

Comment 9

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 8669499 [details] [diff] [review]
telemetry-ctp-images-bug_265659.patch

># HG changeset patch
># User Jonathan Almeida [:jonalmeida] <[e-mail]>
># Date 1444013064 14400
>#      Sun Oct 04 22:44:24 2015 -0400
># Node ID 2bc92768806240ba899d8f16863e21b8a67faff4
># Parent  5f16c6c2b969f70e8da10ee34853246d593af412
>Bug 1208167 - Telemetry probe for click-to-play images r?mfinkle
>
>- A telemetry probe for when a user clicks to unblock an image.
>- A second probe for sending the size of the image being unblocked.
> Since we don't have information on the image size the best option
> was to make make an XHR HEAD request. We make an approximation
> of the image size from the content length which we send in kB.
>
>diff --git a/mobile/android/components/ImageBlockingPolicy.js b/mobile/android/components/ImageBlockingPolicy.js
>--- a/mobile/android/components/ImageBlockingPolicy.js
>+++ b/mobile/android/components/ImageBlockingPolicy.js
>@@ -34,16 +34,18 @@ ImageBlockingPolicy.prototype = {
>       // Accept any non-http(s) image URLs
>       if (!contentLocation.schemeIs("http") && !contentLocation.schemeIs("https")) {
>         return Ci.nsIContentPolicy.ACCEPT;
>       }
> 
>       if (node instanceof Ci.nsIDOMHTMLImageElement) {
>         // Accept if the user has asked to view the image
>         if (node.getAttribute("data-ctv-show") == "true") {
>+          UITelemetry.addEvent("action.1", "shouldLoad", null, "shouldLoad_show_image");

I don't think we need this one. We already capture "web_show_image"

>+          sendImageSizeTelemetry(node.getAttribute("data-ctv-src"));

Not sure if we really want this one either. Well, maybe, but see below.

>+function sendImageSizeTelemetry(src) {

>+    UITelemetry.addEvent("action.1", "sizes", null, imageSize + " kB");

Let's assume we want this probe. We really don't want to use UITelemetry for it. We want a historgram which you would add to Histograms.json and call something like this:

http://mxr.mozilla.org/mozilla-central/source/mobile/android/components/SessionStore.js?force=1#645

Also note, you did not add telemetry for Margaret's use case of "how many people have this setting enabled?" which would be a boolean (or flag) histogram.

Comment 10

[Jonathan Almeida [:jonalmeida]]

Attachment: Telemetry probe for click-to-play images

- A telemetry probe for when a user clicks to unblock an image.
- A second probe for sending the size of the image being unblocked.
 Since we don't have information on the image size the best option
 was to make make an XHR HEAD request. We make an approximation
 of the image size from the content length which we send in kB.

Comment 11

[Jonathan Almeida [:jonalmeida]]

I've made some more modifications as per the comments, but I haven't tested it - attaching it here for now. 

I'm looking for a way to manually trigger the "gather-telemetry" event to see if it works as expected.

Comment 12

[Jonathan Almeida [:jonalmeida]]

Looks like bug 1127907 is trying to remove the "gather-telemetry" event.

Comment 13

[Jonathan Almeida [:jonalmeida]]

I've been trying to force a "gather-telemetry" event by changing the gathering timeout[1] to 1 instead of 300 since this signals "idle-daily"[2] which then notifies the "gather-telemetry" observers[3]. This doesn't seem to be working, or it takes a really long time..

Anything thing else I could try? I just need to verify my patches work as expected now.


[1]: https://dxr.mozilla.org/mozilla-central/source/toolkit/components/telemetry/TelemetrySession.jsm#92
[2]: https://dxr.mozilla.org/mozilla-central/source/toolkit/components/telemetry/TelemetrySession.jsm#1733
[3]: https://dxr.mozilla.org/mozilla-central/source/toolkit/components/telemetry/TelemetrySession.jsm#1740

Comment 14

[Jonathan Almeida [:jonalmeida]]

Readings for me: https://developer.mozilla.org/en-US/docs/Mozilla/Performance/Adding_a_new_Telemetry_probe

Comment 15

[Georg Fritzsche [:gfritzsche]]

What do you need "gather-telemetry" for?
That notification was never really useful and only a few old pieces of code use it.

If you are looking to display the current state of your probe you can e.g. view it in about:telemetry and just reload the page to update the values.

Comment 16

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Georg Fritzsche [:gfritzsche] from comment #15)
> What do you need "gather-telemetry" for?
> That notification was never really useful and only a few old pieces of code
> use it.
> 
> If you are looking to display the current state of your probe you can e.g.
> view it in about:telemetry and just reload the page to update the values.

Mobile currently uses "gather-telemetry" as a trigger to send a message to Java and collect some Java-side probes:
https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/BrowserApp.java#1642

This bug is a similar type of probe, but on the JS side.

Comment 17

[:Margaret Leibovic]

(In reply to Jonathan Almeida (:jonalmeida) from comment #13)
> I've been trying to force a "gather-telemetry" event by changing the
> gathering timeout[1] to 1 instead of 300 since this signals "idle-daily"[2]
> which then notifies the "gather-telemetry" observers[3]. This doesn't seem
> to be working, or it takes a really long time..
> 
> Anything thing else I could try? I just need to verify my patches work as
> expected now.

If you're just looking to test, you can use the remote console to send a "gather-telemetry" notification:

Services.obs.notifyObservers(null, "gather-telemetry", null);

Comment 18

[Georg Fritzsche [:gfritzsche]]

(In reply to Mark Finkle (:mfinkle) from comment #16)
> (In reply to Georg Fritzsche [:gfritzsche] from comment #15)
> > What do you need "gather-telemetry" for?
> > That notification was never really useful and only a few old pieces of code
> > use it.
> > 
> > If you are looking to display the current state of your probe you can e.g.
> > view it in about:telemetry and just reload the page to update the values.
> 
> Mobile currently uses "gather-telemetry" as a trigger to send a message to
> Java and collect some Java-side probes:
> https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/
> BrowserApp.java#1642
> 
> This bug is a similar type of probe, but on the JS side.

Ah, thanks. Note that it is rather random when measurements triggered from "gather-telemetry" end up in a ping, hence bug 1127907 and the need to re-design current uses.
But i'll comment over on that bug.

Comment 19

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 8677146 [details] [diff] [review]
Telemetry probe for click-to-play images


>diff --git a/mobile/android/components/ImageBlockingPolicy.js b/mobile/android/components/ImageBlockingPolicy.js

>+////////////////////////////////////////////////////////////////////////////////
>+//// Constants
>+
>+//// SVG placeholder image for blocked image content
>+const PLACEHOLDER_IMG = "chrome://browser/skin/images/placeholder_image.svg";
>+
>+//// Telemetry
>+const TELEMETRY_TAP_TO_LOAD_ENABLED = "TAP_TO_LOAD_ENABLED";
>+const TELEMETRY_SHOW_IMAGE_SIZE = "SHOW_IMAGE_SIZE";
>+const TOPIC_GATHER_TELEMETRY = "gather-telemetry";
>+
>+//// Gecko preference
>+const PREF_IMAGEBLOCKING_ENABLED = "browser.image_blocking.enabled"
>+
> /**
>  * Content policy for blocking images
>  */
>-
>-// SVG placeholder image for blocked image content
>-var PLACEHOLDER_IMG = "chrome://browser/skin/images/placeholder_image.svg";
>-
>-function ImageBlockingPolicy() {}
>+function ImageBlockingPolicy() {
>+  Services.obs.addObserver(this, TOPIC_GATHER_TELEMETRY, false);
>+}
> 
> ImageBlockingPolicy.prototype = {
>-  QueryInterface: XPCOMUtils.generateQI([Ci.nsIContentPolicy]),
>+  QueryInterface: XPCOMUtils.generateQI([Ci.nsIContentPolicy, Ci.nsIObserver]),
>   classDescription: "Click-To-Play Image",
>   classID: Components.ID("{f55f77f9-d33d-4759-82fc-60db3ee0bb91}"),
>   contractID: "@mozilla.org/browser/blockimages-policy;1",
>   xpcom_categories: [{category: "content-policy", service: true}],
> 
>   // nsIContentPolicy interface implementation
>   shouldLoad: function(contentType, contentLocation, requestOrigin, node, mimeTypeGuess, extra) {
>     if (!getEnabled()) {
>@@ -34,16 +46,17 @@ ImageBlockingPolicy.prototype = {
>       // Accept any non-http(s) image URLs
>       if (!contentLocation.schemeIs("http") && !contentLocation.schemeIs("https")) {
>         return Ci.nsIContentPolicy.ACCEPT;
>       }
> 
>       if (node instanceof Ci.nsIDOMHTMLImageElement) {
>         // Accept if the user has asked to view the image
>         if (node.getAttribute("data-ctv-show") == "true") {
>+          sendImageSizeTelemetry(node.getAttribute("data-ctv-src"));

I think this function takes a node.

>+  observe : function (subject, topic, data) {
>+    if (topic == TOPIC_GATHER_TELEMETRY) {
>+      Services.telemetry.getHistogramById(TELEMETRY_TAP_TO_LOAD_ENABLED).add(getEnabled());
>+    }
>+  },
>+

No need for a blank

>+function sendImageSizeTelemetry(src) {
>+  let xhr = Cc["@mozilla.org/xmlextras/xmlhttprequest;1"].createInstance(Ci.nsIXMLHttpRequest);
>+  xhr.open( 'HEAD' , src , true );

Remove the extra spaces and use " instead of '

>+    contentLength = xhr.getResponseHeader('Content-Length');

Need a | let | and use "

>diff --git a/toolkit/components/telemetry/Histograms.json b/toolkit/components/telemetry/Histograms.json

>+  "SHOW_IMAGE_SIZE": {

TAP_TO_LOAD_IMAGE_SIZE

>+    "expires_in_version": "never",
>+    "kind": "linear",
>+    "n_buckets": 50,
>+    "description": "The size of the image being shown, when using tap-to-load images. (kilobytes)"

Add the bug number:  "Bug 1208167: The size of the image being shown, when using tap-to-load images. (kilobytes)"

>+  "TAP_TO_LOAD_ENABLED": {
>+    "expires_in_version": "never",
>+    "kind": "boolean",
>+    "description": "Whether or not a user has tap-to-load enabled."

Same

This looks like the right direction

Comment 20

[Jonathan Almeida [:jonalmeida]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0a39919cc8e1

Comment 21

[Jonathan Almeida [:jonalmeida]]

Attachment: MozReview Request: Bug 1208167 - Telemetry probe for click-to-play images r?mfinkle,ally

Bug 1208167 - Telemetry probe for click-to-play images r?mfinkle

- A telemetry probe for when a user clicks to unblock an image.
- A second probe for sending the size of the image being unblocked.
 Since we don't have information on the image size the best option
 was to make make an XHR HEAD request. We make an approximation
 of the image size from the content length which we send in kB.

Comment 22

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 8681595 [details]
MozReview Request: Bug 1208167 - Telemetry probe for click-to-play images r?mfinkle,ally

https://reviewboard.mozilla.org/r/23873/#review21789

::: mobile/android/components/ImageBlockingPolicy.js:102
(Diff revision 1)
> +function sendImageSizeTelemetry(imgSrc) {

nit: imgSrc -> imageURL

::: mobile/android/components/ImageBlockingPolicy.js:113
(Diff revision 1)
> +    let imageSize = contentLength / 1024;

Should we check for an empty contentLength?

::: toolkit/components/telemetry/Histograms.json:2497
(Diff revision 1)
> +    "description": "The size of the image being shown, when using tap-to-load images. (kilobytes)"

Needs a leading "Bug 1208167: "

::: toolkit/components/telemetry/Histograms.json:8327
(Diff revision 1)
> +    "description": "Whether or not a user has tap-to-load enabled."

Needs a leading "Bug 1208167: "

Looks good. For the next one, also add :ally as an approver for the Histogram.json changes (required to get a privacy peer approval)

Comment 23

[Jonathan Almeida [:jonalmeida]]

Comment on attachment 8681595 [details]
MozReview Request: Bug 1208167 - Telemetry probe for click-to-play images r?mfinkle,ally

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/23873/diff/1-2/

Comment 24

[Jonathan Almeida [:jonalmeida]]

https://reviewboard.mozilla.org/r/23871/#review21913

::: toolkit/components/telemetry/Histograms.json:8362
(Diff revision 2)
> +    "kind": "boolean",

Note that this will change to an enum when landing bug 1209293

Comment 25

[Jonathan Almeida [:jonalmeida]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d5ed080a35c5

Comment 26

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 8681595 [details]
MozReview Request: Bug 1208167 - Telemetry probe for click-to-play images r?mfinkle,ally

https://reviewboard.mozilla.org/r/23873/#review21919

::: mobile/android/components/ImageBlockingPolicy.js:103
(Diff revision 2)
> +    if (contentLength == 0) {

Let's use falsey:

if (!contentLength) {
  return;
}

Comment 27

[Jonathan Almeida [:jonalmeida]]

https://reviewboard.mozilla.org/r/23873/#review21921

::: mobile/android/components/ImageBlockingPolicy.js:103
(Diff revision 2)
> +    if (contentLength == 0) {

I think I'll go one step further since some headers may not include the content-length or the length is 0.

```
if (!contentLength || contentLength == 0) {
  return;
}
```

Comment 28

[Jonathan Almeida [:jonalmeida]]

(In reply to Jonathan Almeida (:jonalmeida) from comment #27)
> https://reviewboard.mozilla.org/r/23873/#review21921
> 
> ::: mobile/android/components/ImageBlockingPolicy.js:103
> (Diff revision 2)
> > +    if (contentLength == 0) {
> 
> I think I'll go one step further since some headers may not include the
> content-length or the length is 0.
> 
> ```
> if (!contentLength || contentLength == 0) {
>   return;
> }
> ```

I am a fool.

Comment 29

[Jonathan Almeida [:jonalmeida]]

Comment on attachment 8681595 [details]
MozReview Request: Bug 1208167 - Telemetry probe for click-to-play images r?mfinkle,ally

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/23873/diff/2-3/

Comment 30

[Allison Naaktgeboren :ally]

Comment on attachment 8681595 [details]
MozReview Request: Bug 1208167 - Telemetry probe for click-to-play images r?mfinkle,ally

https://reviewboard.mozilla.org/r/23873/#review22135

p=ally w/ changes.

Thank you for including the bug number in the description!

::: toolkit/components/telemetry/Histograms.json:2500
(Diff revision 3)
> +    "expires_in_version": "never",

We no longer accept expires_in_version: never. How about version 50?

::: toolkit/components/telemetry/Histograms.json:8376
(Diff revision 3)
> +    "expires_in_version": "never",

same

Comment 31

[Jonathan Almeida [:jonalmeida]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=47120112978d

Comment 32

[Jonathan Almeida [:jonalmeida]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ff95f73f8dda

Comment 33

[Jonathan Almeida [:jonalmeida]]

https://hg.mozilla.org/integration/fx-team/rev/11c522b12d461c20ae02841ead572af9b2bbf47d
Bug 1208167 - Telemetry probe for click-to-play images r=mfinkle,ally

Comment 34

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/11c522b12d46