Location via proxy:   [ UP ]  
[Report a bug]   [Manage cookies]                
Page MenuHomePhabricator

VirtualPageView should use EventLogging api to send virtual page view events
Closed, ResolvedPublic3 Estimated Story Points

Description

In order to do T238138: VirtualPageView Event Platform Migration, VirtualPageView instrumentation code needs to use the EventLogging mw.eventLog.logEvent API, so that EventLogging can handle the backwards compatible conversion.

This should undo the work done for T190188: VirtualPageView schema should not use EventLogging api to send virtual page view events in https://gerrit.wikimedia.org/r/c/mediawiki/extensions/Popups/+/422261.

QA steps

  1. Start a new private browsing session
  2. Navigate to https://en.wikipedia.beta.wmflabs.org/wiki/PreviewsAppearAbove/sandbox
  3. Hover over one of the links until a preview appears
  4. Wait 1 second
  5. Observe that a VirtualPageView event has been logged
    • VirtualPageView events are still logged to the beacon URL, i.e. /event/beacon?…

QA Results - Beta

ACStatusDetails
1T279382#7053764

QA Results - Prod

ACStatusDetails
1T279382#7071181

Event Timeline

@Jdlrobson I was going to try to make a patch for this, but after briefly looking at the Popups code I can't quite find the right thing to do. Should I just create a revert of https://gerrit.wikimedia.org/r/c/mediawiki/extensions/Popups/+/422261?

fdans triaged this task as High priority.
fdans moved this task from Incoming to Event Platform on the Analytics board.
Jdlrobson added subscribers: ovasileva, phuedx.

I honestly can't remember off the top of my head. @phuedx may know better.

@ovasileva sounds like our help here is needed so if we can prioritize this based on whatever timeline analytics are working on, we will need to investigate this (analysis,) and make the changes.

Making sure T267211 is prioritized will also help with refreshing our memory on this part of the codebase.

I honestly can't remember off the top of my head. @phuedx may know better.

You've known me for long enough to understand that this is rarely the case, if ever.

At the time of writing the patch that @Ottomata mentioned, mw.eventLogging.logEvent silently discarded events to be logged if DNT was enabled and VirtualPageview events need to be logged regardless. T252438: [Spike] Should EventLogging support DNT? made this moot – though mw.eventLogging.submit still nods to DNT in its DocBlock 😉

@ovasileva sounds like our help here is needed so if we can prioritize this based on whatever timeline analytics are working on, we will need to investigate this (analysis,) and make the changes.

Making sure T267211 is prioritized will also help with refreshing our memory on this part of the codebase.

The business case here is that this would lead to:

  • A clean binding to the EventLogging API from the Page Previews codebase
  • Further removal of code from the Page Previews codebase
  • Less maintenance burden as a result of the above

FYI, @mforns has begun work on this I think.

FYI, @mforns has begun work on this I think.

@Jdlrobson, @phuedx, yes, I have.
If you're OK, I was planning to write a (very inaccurate) patch to the Popups extension and let you guys review and guide me.
If you prefer to do that yourselves, that'd be OK, of course.

If you're OK, I was planning to write a (very inaccurate) patch to the Popups extension and let you guys review and guide me.

Sure!

Change 682754 had a related patch set uploaded (by Mforns; author: Mforns):

[mediawiki/extensions/Popups@master] Move VirtualPageViews back to mw.track

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

Jdlrobson added a subscriber: mforns.

Change 682754 abandoned by Mforns:

[mediawiki/extensions/Popups@master] Move VirtualPageViews back to mw.track

Reason:

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

ovasileva set the point value for this task to 3.Apr 27 2021, 5:15 PM

Change 683297 had a related patch set uploaded (by Mforns; author: Mforns):

[mediawiki/extensions/Popups@master] Move VirtualPageViews back to mw.track

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

This should be deployed to the Beta Cluster any moment now.

Change 683297 merged by jenkins-bot:

[mediawiki/extensions/Popups@master] Move VirtualPageViews back to mw.track

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

phuedx updated the task description. (Show Details)

Test Result - Beta

Status: ✅ PASS
Environment: beta
OS: macOS Big Sur
Browser: Chrome
Device: MBP
Emulated Device: NA

Test Artifact(s):

QA Steps

Start a new private browsing session
Navigate to https://en.wikipedia.beta.wmflabs.org/wiki/PreviewsAppearAbove/sandbox
Hover over one of the links until a preview appears
Wait 1 second
✅ AC1: Observe that a VirtualPageView event has been logged
VirtualPageView events are still logged to the beacon URL, i.e. /event/beacon?…

{
   "event":{
      "source_page_id":184661,
      "source_namespace":0,
      "source_title":"PreviewsAppearAbove/sandbox",
      "source_url":"https://en.wikipedia.beta.wmflabs.org/wiki/PreviewsAppearAbove/sandbox",
      "page_id":181766,
      "page_namespace":0,
      "page_title":"With_image"
   },
   "schema":"VirtualPageView",
   "webHost":"en.wikipedia.beta.wmflabs.org",
   "wiki":"enwiki",
   "revision":17780078
}

Hi! I just realized that, when this change is deployed to prod, we'll be missing a functionality until we migrate to Event Platform.
Namely, the change I wrote removed a snippet that was cropping the event payload to fit in the URL.
And this is not a problem for the Event Platform client, because it sends the events via POST, and hence doesn't have any size restrictions.
But the legacy client does not do that. So, after the deploy and until we migrate this schema to Event Platform, events with very large payloads will fail validation.
Not sure how big of an issue is this, but if we sync, I can make sure that this get migrated as soon as it gets deployed to prod.

And this is not a problem for the Event Platform client, because it sends the events via POST, and hence doesn't have any size restrictions.
But the legacy client does not do that. So, after the deploy and until we migrate this schema to Event Platform, events with very large payloads will fail validation.

Will these validation errors be visible on Grafana or Logstash (or both?). If so, then we can keep an eye on it and, if necessary, deploy a partial revert of your change to re-introduce the URL length limiting code.

Not sure how big of an issue is this, but if we sync, I can make sure that this get migrated as soon as it gets deployed to prod.

Let's make sure we migrate it ASAP. This seems the best possible way to avoid this issue. Is there anything that we can help with?

deploy a partial revert of your change to re-introduce the URL length limiting code.

Maybe it wouldn't be difficult to forward port this code into EventLogging, even though we will stop using it asap?

deploy a partial revert of your change to re-introduce the URL length limiting code.

Maybe it wouldn't be difficult to forward port this code into EventLogging, even though we will stop using it asap?

Sorry, both. I should've been clearer. The URL length limiting code in the VirtualPageview instrument was designed to limit the lengths of the two URLs that are logged as part of the event. We did this because ye olde EventLogging wouldn't log an event of over 2048 characters in length. It wouldn't make sense to extract such a specific part of the instrument into EventLogging, especially when we're about to make the issue moot anyway.

I see, so forward port only if we have to?

@phuedx Is the new code in production already?

@Ottomata What do you mean with forwarding port only if we have to?

@phuedx Is the new code in production already?

It'll be live on all Wikipedias by EOD today.

@mforns I guess I mean what Sam was saying, if we get a ton of errors related to url too long (before we migrate), we can try and copy/paste the url shortening code into EventLogging.

It'll be live on all Wikipedias by EOD today.

Oh my, we should prep the migration!

Test Result - Prod

Status: ✅ PASS
Environment: enwiki
OS: macOS Big Sur
Browser: Chrome
Device: MBP
Emulated Device: NA

Test Artifact(s):

QA Steps

Start a new private browsing session
Navigate to https://en.wikipedia.org/wiki/Dog
Hover over one of the links until a preview appears
Wait 1 second
✅ AC1: Observe that a VirtualPageView event has been logged
VirtualPageView events are still logged to the beacon URL, i.e. /event/beacon?…

{
  "event": {
    "source_page_id": 4269567,
    "source_namespace": 0,
    "source_title": "Dog",
    "source_url": "https://en.wikipedia.org/wiki/Dog",
    "page_id": 210098,
    "page_namespace": 0,
    "page_title": "Hunter-gatherer"
  },
  "schema": "VirtualPageView",
  "webHost": "en.wikipedia.org",
  "wiki": "enwiki",
  "revision": 17780078
}

Being bold and signing this off. There's been no observable drop in logged VirtualPageview events no observable drop in VirtualPageview events logged nor a spike in validation errors