Closed Bug 975235 Opened 7 years ago Closed 6 years ago

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

Categories

(Firefox :: New Tab Page, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
Firefox 33

People

(Reporter: jboriss, Assigned: mzhilyaev)

References

()

Details

(Whiteboard: [tiles] p=5 s=33.1 [qa-])

Attachments

(1 file, 3 obsolete files)

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.
Depends on: 975228
Blocks: 975236
There's already some work being done to count those in bug 975570 to do telemetry on them.
Whiteboard: [tiles] → [tiles] p=0
No longer blocks: fxdesktopbacklog
Flags: firefox-backlog+
Blocks: 995262
Blocks: 974474
Blocks: tiles-dev
Mass-move to Firefox::New Tab Page.

Filter on new-tab-page-component.
Component: Tabbed Browser → New Tab Page
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))
Blocks: 972933
Assignee: nobody → tspurway
Assignee: tspurway → mzhilyaev
Summary: Add counting of Tile impressions, clicks (inc. which tile and position) → Send impression and click pings for Directory Tiles (inc. which tile and position)
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
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?
(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.
With bug 1019298 covering impressions through telemetry for non-Release, let's focus this bug on just sending click pings.
Summary: Send impression and click pings for Directory Tiles (inc. which tile and position) → Send click pings for Directory Tiles (including which link, tile, other metadata)
Depends on: 1020626
Attachment #8435395 - Flags: review?(adw)
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 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
Attachment #8435395 - Flags: review?(adw)
Attached patch v2 (obsolete) — Splinter Review
Report click actions including link, block, pin, unpin, sponsored
Attachment #8435395 - Attachment is obsolete: true
Attachment #8436198 - Flags: review?(adw)
Attachment #8436198 - Attachment description: wip v2 (no json) → v2
Attached patch v3 (obsolete) — Splinter Review
Fixes an issue with tests starting too soon before links populate on the hidden page
Attachment #8436198 - Attachment is obsolete: true
Attachment #8436198 - Flags: review?(adw)
Attachment #8436277 - Flags: review?(adw)
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
Attachment #8436277 - Flags: review?(adw)
Attached patch v4Splinter Review
https://tbpl.mozilla.org/?tree=Try&rev=55426de7dde2
Attachment #8436277 - Attachment is obsolete: true
Attachment #8436317 - Flags: review?(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.
Attachment #8436317 - Flags: review?(adw) → review+
https://hg.mozilla.org/mozilla-central/rev/410f509e9913
Status: NEW → RESOLVED
Closed: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 33
Whiteboard: [tiles] p=0 → [tiles] p=0 s=33.1 [qa?]
marking qa-, since this is flagged in-testsuite+.  If anyone feels there should be manual QA here, feel free to change to qa+
Whiteboard: [tiles] p=0 s=33.1 [qa?] → [tiles] p=0 s=33.1 [qa-]
Status: RESOLVED → VERIFIED
Were privacy issues discussed/approved wrt such telemetry pings?
Flags: needinfo?(edilee)
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.
Flags: needinfo?(edilee)
Whiteboard: [tiles] p=0 s=33.1 [qa-] → [tiles] p=5 s=33.1 [qa-]
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.
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?
(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?
(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).
(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?
(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)
Blocks: 1043669
You need to log in before you can comment on or make changes to this bug.