Page MenuHomePhabricator

VirtualPageView schema should not use EventLogging api to send virtual page view events
Closed, ResolvedPublic5 Story Points

Description

Events fired via EventLogging inspects the DNT header and does not send events when it is present.
For the VirtualPageView schema, in order to have consistency with how we track page views we will need to send the event regardless of whether that header is present.

Per Sam's suggestion this should be done solely in the Popups extension, not EventLogging.

Further background:
https://phabricator.wikimedia.org/T187277#4052896

Acceptance criteria

  • Readers Web will opt for #3: we'll use parts of the core EventLogging client-side API in order to construct the correct URL to request and make the request using sendBeacon inside of the Page Previews codebase, e.g.
[0]
const eventData = {
  // ...
};
const payload = mw.eventLog.prepare( 'VirtualPageview', eventData);
const url = mw.eventLog.makeBeaconUrl( payload );

navigator.sendBeacon( url );

Testing criteria

Test on the beta cluster. Hover over a link and hold the mouse there for at least a second. An EventLogging event should fire for all requests (in incognito window and new window)

Verify that events are sent to the VirtualPageView schema even if Chrome is configured to send the do not track header.

Verify the same for Firefox.

Signoff criteria

  • Ensure acceptance criteria is implemented

Related Objects

StatusAssignedTask
OpenNone
Resolvedmforns
ResolvedDereckson
ResolvedJdlrobson
Resolvedovasileva
DuplicateNone
Resolvedovasileva
Resolvedovasileva
ResolvedJdlrobson
DuplicateNone
DuplicateNone
Resolvedovasileva
Resolvedovasileva
Resolvedovasileva
Resolvedphuedx
Resolvedphuedx
DuplicateNone
ResolvedJdlrobson
ResolvedJdlrobson
DuplicateNone
Duplicateovasileva
Resolvedovasileva
DuplicateNone
DeclinedNone
DuplicateJdlrobson
ResolvedMhurd
DeclinedJMinor
Resolvedphuedx
ResolvedPchelolo
ResolvedJdlrobson
DeclinedPchelolo
DeclinedNone
OpenNone
Resolvedphuedx
DeclinedJdlrobson
DuplicateNone
ResolvedFjalapeno
Resolvedphuedx
Declinedpmiazga
DeclinedNone
Resolvedphuedx
DeclinedNone
ResolvedPchelolo
ResolvedbearND
ResolvedMholloway
ResolvedMSantos
ResolvedMholloway
InvalidNone
ResolvedJdlrobson
InvalidNone
DuplicateNone
ResolvedJdlrobson
ResolvedJdlrobson
ResolvedJdlrobson
ResolvedJdlrobson
Resolvedphuedx
ResolvedbearND
ResolvedMholloway
DuplicateNone
ResolvedJdlrobson
ResolvedJdlrobson
Resolvedphuedx
ResolvedJdlrobson
ResolvedJdlrobson
ResolvedbearND
ResolvedJdlrobson
ResolvedMholloway
ResolvedMholloway
ResolvedJdlrobson
ResolvedJdlrobson
ResolvedbearND
Resolved Tbayer
ResolvedNone
Resolvedovasileva
Resolved Tbayer
ResolvedNone
Resolvedmforns
Resolvedphuedx
DeclinedNone

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptMar 20 2018, 5:28 PM
Jhernandez updated the task description. (Show Details)Mar 20 2018, 5:53 PM
ovasileva set the point value for this task to 5.Mar 20 2018, 6:01 PM
Jdlrobson moved this task from To Do to Doing on the Readers-Web-Kanbanana-Board-Old board.

Change 422261 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/extensions/Popups@master] VirtualPageViews bypass EventLogging for logging virtual events

https://gerrit.wikimedia.org/r/422261

Jdlrobson added a comment.EditedMar 27 2018, 9:31 PM

@Tbayer what should we do in the case of VirtualPageViews if navigator.sendBeacon is used? I see EventLogging falls back to an img with href attribute. I'm not sure if we want to do this here. What do you think? (Note that Schema:Popups will only trigger an event if sendBeacon is present - do we want to be consistent with that?)

@Jdlrobson You mean in case the browser doesn't support sendBeacon? Assuming I understand the situation correctly, we should adapt the behavior of EventLogging including the fallback, which also seems to be more consistent with the overall goal of counting seen previews compatible with the way we count pageviews. Also, conversely, consistency with Schema:Popups isn't very relevant in the long run.

@Tbayer yeh, if the browser doesn't support sendBeacon what happens? We are not using EventLogging here so we'd need to copy/pasta the fallback.

@Tbayer yeh, if the browser doesn't support sendBeacon what happens? We are not using EventLogging here so we'd need to copy/pasta the fallback.

Yes, that looks like the way to go here. In general, it seems prudent to emulate the tried and tested approach of EL as much as possible.

Change 422261 merged by jenkins-bot:
[mediawiki/extensions/Popups@master] VirtualPageViews bypass EventLogging for logging virtual events

https://gerrit.wikimedia.org/r/422261

Jdlrobson reassigned this task from Jdlrobson to ABorbaWMF.Mar 29 2018, 10:12 PM
Jdlrobson removed a project: Patch-For-Review.
Jdlrobson updated the task description. (Show Details)
Jdlrobson updated the task description. (Show Details)
Jdlrobson updated the task description. (Show Details)Mar 29 2018, 10:14 PM

Over to you @ABorbaWMF please allow 30 minutes for the change to be on the beta cluster. We'll want to verify that events are sending with and without the DNT header.

@Niedzielski @Jdlrobson to do a little more QAing.

I confirm VirtualPageView events are received on deployment-eventlog05 from the beta cluster regardless of DNT settings for Chrome v65.0.3325.181 on Ubuntu v17.10 64b and Firefox v59.0.2.

Note, since we are no longer using EventLogging @ABorbaWMF cannot sign this off using the technique he was using before.
I'm seeing VirtualPageViews on Firefox 59.0.2 now (I wasn't before) and Chrome with DNT enabled.
For bonus points I've tested on Android device (running desktop, Samsung Galaxy S6) and I'm also seeing the same behaviour there.

According to IRC the deploy corresponded with a spike in events on the schema highlighted in this graph so fairly confident this is working given the above:

Let me know if there is any specific devices we need to verify for. The above were easy to verify given I can view outgoing network requests but it will be a lot trickier for browsers that do not.

ovasileva reassigned this task from ABorbaWMF to Tbayer.Apr 9 2018, 11:24 AM
ovasileva added subscribers: ABorbaWMF, ovasileva.

Handing this off to @Tbayer for signoff.

Restricted Application edited projects, added Readers-Web-Backlog; removed Readers-Web-Backlog (Tracking). · View Herald TranscriptApr 11 2018, 7:01 PM
ovasileva updated the task description. (Show Details)Apr 11 2018, 7:13 PM
Tbayer closed this task as Resolved.Apr 23 2018, 7:57 PM

@Niedzielski already tested this successfully on the beta cluster (T190188#4105897 ) and @Jdlrobson then tested it successfully in production (at least that's my reading of T190188#4110569 - let me know in case I misunderstood). Based on that, I assume that the signoff criteria are met.

Tbayer updated the task description. (Show Details)Apr 23 2018, 7:57 PM