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

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.

In T279382#7026277, @Jdlrobson wrote:

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.

In T279382#7030279, @mforns wrote:

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

Change 682754 abandoned by Mforns:

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

Reason:

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

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

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.

In T279382#7062430, @mforns wrote:

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?

In T279382#7065711, @Ottomata wrote:

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?

In T279382#7066066, @mforns wrote:

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