975235 - Send click pings for Directory Tiles (including which link, tile, other metadata)

Status: VERIFIED FIXED

(Firefox :: New Tab Page, defect)

Description

[Jennifer Morrow [:Boriss] (UX)]

Counting of tile impressions and clicks will give us data to determine the usefulness and interaction rate of specific Tiles as well as positions of tiles.

Comment 1

[Robert Kaiser]

There's already some work being done to count those in bug 975570 to do telemetry on them.

Comment 2

[Matthew N. [:MattN]]

Mass-move to Firefox::New Tab Page.

Filter on new-tab-page-component.

Comment 3

[Ed Lee :Mardak]

Initially, we'll be sending impression data onPageFirstVisible and click data onClick to a tiles-metrics server separate from Telemetry or FHR. Both event handlers already have telemetry probes but only report back aggregates or an index without specific tile data.

We'll be including tile data such as an id (unique for a given url/title/image/bcColor(?)/type(?) tuple), index, frecency/score. We'll also want tile-independent data such as a list-id/version and locale (if it's not embedded in the list id).

A tile's id could be mozilla-1 that is updated appropriately when the url/title/image change, or it could be a hash of those values that the client could generate.

For the list-id, we'll similarly need to either maintain ids, e.g., en-US-1, or dynamically generate it by hashing the whole file.

For the hashing, we can use:
Cu.import("resource://services-crypto/utils.js");
CryptoUtils.sha1(JSON.stringify(tileObject || directoryLinksObject))

Comment 4

[maxim zhilyaev]

Following Tim's  conversation these are clarifications:

1) tile IDs should be generated by back-end and attached to each tile in the list
2) stats are collected for directory tiles only (history tiles are not touched)
3) list-id, version and locale can come with JSON directory file 
4) browser aggregates impression/click stats for each tile and ships aggregate batch to a server on directory request.
5) browser clears aggregate batch upon successful response from server
6) browser needs to timestamp each impression/click observed, and timestamp the batch before sending to the server

Comment 5

[maxim zhilyaev]

Following Ed's advice, the simpler way is to send impression/click as they happen. 
1) browser sends impressions upon newtab load
2) browser sends clicks if they happen
3) is timestamping needed?

Comment 6

[Ed Lee :Mardak]

(In reply to maxim zhilyaev from comment #4)
> 1) tile IDs should be generated by back-end and attached to each tile in the
> list
Instead of having an id explicitly attached to each tile as part of the json, we know given a unique list id and the link's index (in the json's list) which link it is.

So the impression or click data only needs to report back a list id and an array of link indices.

E.g., impression ping: {
  listId: 44b2b82fde95c,
  gridCols: 3,
  gridRows: 3,
  shownTiles: [{
    listIndex: 0,
    tilePosition: 2,
    score: 1000
  }, {
    listIndex: 4,
    tilePosition: 3,
    score: 1000
  }, {
    listIndex: 10,
    tilePosition: 0,
    score: 1234
  }
}

where listIndex is the original index of the link in the json, tilePosition is where it's shown on the grid, score is the computed frecency.

The same thing can be used for click ping except "shownTiles" array could be just clickedTile object.

Comment 7

[Ed Lee :Mardak]

With bug 1019298 covering impressions through telemetry for non-Release, let's focus this bug on just sending click pings.

Comment 8

[maxim zhilyaev]

Attachment: v1. report directory tiles clicks

Comment 9

[Ed Lee :Mardak]

maksik: https://tbpl.mozilla.org/?tree=Try&rev=00177db435d9 seems to be good

Comment 10

[Ed Lee :Mardak]

mconnor points to a jsapi that can send a ping in a smarter way:
https://developer.mozilla.org/en-US/docs/Web/API/navigator.sendBeacon

We could use navigator.sendBeach from the newtab page context although the current patch sends the ping from DirectoryLinksProvider.jsm

Comment 11

[Ed Lee :Mardak]

Comment on attachment 8435395 [details] [diff] [review]
v1. report directory tiles clicks

mconnor suggested we include other actions other than link clicks: e.g., block, pin

Comment 12

[Ed Lee :Mardak]

Attachment: v2

Report click actions including link, block, pin, unpin, sponsored

Comment 13

[Ed Lee :Mardak]

Attachment: v3

Fixes an issue with tests starting too soon before links populate on the hidden page

Comment 14

[Ed Lee :Mardak]

Comment on attachment 8436277 [details] [diff] [review]
v3

Ah nope. Seems like the directory links aren't being loaded before the tab is shown still. Might be simplest to turn off preload. https://tbpl.mozilla.org/?tree=Try&rev=45048ed2409c

Comment 15

[Ed Lee :Mardak]

Attachment: v4

https://tbpl.mozilla.org/?tree=Try&rev=55426de7dde2

Comment 16

[Drew Willcoxon :adw]

Comment on attachment 8436317 [details] [diff] [review]
v4

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

::: toolkit/modules/DirectoryLinksProvider.jsm
@@ +264,5 @@
> +      reportClickEndPoint = Services.prefs.getCharPref(PREF_DIRECTORY_REPORT_CLICK_ENDPOINT);
> +      telemetryEnabled = Services.prefs.getBoolPref(PREF_TELEMETRY_ENABLED);
> +    }
> +    catch (ex) {
> +      Cu.reportError("Error accessing preferences for click report: " + ex);

I don't think reportError is necessary.

@@ +268,5 @@
> +      Cu.reportError("Error accessing preferences for click report: " + ex);
> +      return;
> +    }
> +
> +    if (telemetryEnabled == false) {

Nit: if (!telemetryEnabled)

@@ +280,5 @@
> +      ["link", link.directoryIndex],
> +      ["action", action],
> +      ["tile", tileIndex],
> +      ["score", link.frecency],
> +      ["pin", +pinned],

Whoa, I didn't know there is a unary + operator.  Cool.

Comment 17

[:Gijs (he/him)]

https://hg.mozilla.org/mozilla-central/rev/410f509e9913

Comment 18

[Tracy Walker [:tracy]]

marking qa-, since this is flagged in-testsuite+.  If anyone feels there should be manual QA here, feel free to change to qa+

Comment 19

[Avi Halachmi (:avih)]

Were privacy issues discussed/approved wrt such telemetry pings?

Comment 20

[Ed Lee :Mardak]

We confirmed that these click pings cannot be sent to telemetry servers, so that's why there's a separate endpoint. The sending is conditional of the user having telemetry turned on. Right now this behavior is only turned on for Nightly and Aurora.

Comment 21

[Robert Kaiser]

Comment on attachment 8436317 [details] [diff] [review]
v4

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

::: browser/app/profile/firefox.js
@@ +1471,4 @@
>  pref("browser.newtabpage.directory.source", "chrome://global/content/directoryLinks.json");
>  
> +// endpoint to send newtab click reports
> +pref("browser.newtabpage.directory.reportClickEndPoint", "https://tiles.up.mozillalabs.com/ping/click");

Given that this is sending another bit of private data out, even to a non-mozilla.org/com address, is there a switch for users to turn this off in "Data Choices" prefs?
I guess that request will probably come up soon if we don't have a plan for it.

Comment 22

[Vladan Djeric (:vladan)]

So am I correct in understanding that in this bug we're reporting users' clicks on specific directory tiles to Mozilla's non-Telemetry servers, but only if the Telemetry reporting pref is set to true?

Since it's using Telemetry's consent mechanism, it seems like it should follow the same privacy rules (no URIs, no tracking of user browsing) as Telemetry, no? Could we ask the Privacy team for their blessing?

Comment 23

[Mike Hommey [:glandium]]

(In reply to Ed Lee :Mardak from comment #20)
> Right now this behavior is only turned on for Nightly and Aurora.

But will be on beta in 6 weeks, right?

Comment 24

[Avi Halachmi (:avih)]

(In reply to Mike Hommey [:glandium] from comment #23)
> (In reply to Ed Lee :Mardak from comment #20)
> > Right now this behavior is only turned on for Nightly and Aurora.
> 
> But will be on beta in 6 weeks, right?

If there's a real privacy concern here, then we should be concerned also for nightly and aurora users who, while willing to try experimental builds, didn't sign up for sending their advertising clicks back to us (even if to some degree it might be expected from such system).

Comment 25

[Mike Hommey [:glandium]]

(In reply to Ed Lee :Mardak from comment #20)
> The sending is conditional of the user having telemetry turned on. Right
> now this behavior is only turned on for Nightly and Aurora.

Ah, I got what you meant after looking at the code, but it's not exactly clear. So here is the clearer explanation: The sending is conditional of the user having telemetry turned on. Telemetry is on by default on nightly and aurora. But don't we prompt users on the first run on beta/release?

Comment 26

[Mike Hommey [:glandium]]

(In reply to Avi Halachmi (:avih) from comment #24)
> If there's a real privacy concern here, then we should be concerned also for
> nightly and aurora users who, while willing to try experimental builds,
> didn't sign up for sending their advertising clicks back to us (even if to
> some degree it might be expected from such system).

Note I wasn't assessing anything about privacy, I was trying to understand the assumption behind that sentence (as in, was it meaning that it would stick to nightly/aurora, or ride the trains. Code says the latter)