Page MenuHomePhabricator

VirtualPageView schema should not use EventLogging api to send virtual page view events
Closed, ResolvedPublic5 Estimated 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

StatusSubtypeAssignedTask
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
Declined JMinor
Resolvedphuedx
Resolved Pchelolo
ResolvedJdlrobson
Declined Pchelolo
Resolvedphuedx
DeclinedJdlrobson
DuplicateNone
Resolved Fjalapeno
Resolvedphuedx
Declinedpmiazga
DeclinedNone
Resolvedphuedx
DeclinedNone
Resolved Pchelolo
Resolved bearND
Resolved Mholloway
ResolvedMSantos
Resolved Mholloway
InvalidNone
ResolvedJdlrobson
InvalidNone
DuplicateNone
ResolvedJdlrobson
ResolvedJdlrobson
ResolvedJdlrobson
ResolvedJdlrobson
Resolvedphuedx
Resolved bearND
Resolved Mholloway
DuplicateNone
ResolvedJdlrobson
ResolvedJdlrobson
Resolvedphuedx
ResolvedJdlrobson
ResolvedJdlrobson
Resolved bearND
ResolvedJdlrobson
Resolved Mholloway
Resolved Mholloway
ResolvedJdlrobson
ResolvedJdlrobson
Resolved bearND
Resolved Tbayer
ResolvedNone
Resolvedovasileva
Resolved Tbayer
ResolvedNone
Resolvedmforns
Resolvedphuedx
DeclinedNone

Event Timeline

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

@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 removed a project: Patch-For-Review.
Jdlrobson updated the task description. (Show Details)
Jdlrobson updated the task description. (Show Details)

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.

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:

Screen Shot 2018-04-05 at 3.56.18 PM.png (472×1 px, 55 KB)

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 added subscribers: ABorbaWMF, ovasileva.

Handing this off to @Tbayer for signoff.

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