Create Task
Maniphest T214444

Update ReadingDepth instrumentation to avoid deprecated schema module (blocks loads event)
Open, HighPublic2 Story Points
Actions

Description

Most page views currently have the following deprecation notice emitted in the console;

This page is using the deprecated ResourceLoader module "schema.ReadingDepth".
See https://phabricator.wikimedia.org/T205744 for migration info.

There is also a dedicated network request created for this one, which is a roundtrip holding back the window load event with a full roundtrip, only to log a deprecation message.

https://en.wikipedia.org/w/load.php?debug=false&lang=en&modules=schema.ReadingDepth&skin=vector&version=0uitdtj

mw.loader.implement("schema.ReadingDepth@1wjvpdp",..
  mw.log.warn("This page is using the deprecated ResourceLoader module \"schema.ReadingDepth\".\nSee https://phabricator.wikimedia.org/T205744 for migration info.");
});

The migration guide for this is at T205744, which contains code samples as well.

I note though, that @phuedx was ahead of this and already migrated the instrumentation to mw.track a while back (f28f1f5f141a9). However, that change left the schema module behind, which is normally ELs responsibility to load when using mw.track(). Since then, I've worked with Analytics to optimise away the need for these modules, hence the request is now empty (T187207).

Acceptance criteria

  • No such deprecation message appears in the JavaScript console
    • i.e. the ReadingDepth instrumentation no longer loads the deprecated schema.ReadingDepth RL module
  • The ReadingDepth schema's talk page is updated with a note about this change
    • Since we're removing the dedicated network request which blocks the logging of the first ReadingDepth event (because it blocks the window load event), this may impact future analysis.

QA steps

NOTE: The code is enabled on the staging server as well as on the Beta Cluster,
  • Open the developer console and tag "preserve logs" in the network tab.
  • Clear the list of network requests.
  • Type "ReadingDepth" in the filter
  • Scroll the page and click a link
  • Verify that requests to http://reading-web-staging.wmflabs.org/event.gif that contain ReadingDepth are showing up (if you are using the filter they should be the only ones you see)
  • Open the developer console and observe that there are no deprecation warnings like the following:

This page is using the deprecated ResourceLoader module "schema.ReadingDepth"

QA Results

StatusDetails
✅ PassedT214444#4954313

Related Objects

Krinkle created this task.Tue, Jan 22, 10:48 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptTue, Jan 22, 10:48 PM
Krinkle updated the task description. (Show Details)Tue, Jan 22, 10:49 PM
Jdlrobson triaged this task as High priority.Tue, Jan 22, 11:44 PM
Jdlrobson moved this task from Needs Analysis to Upcoming on the Readers-Web-Backlog board.
Jdlrobson set the point value for this task to 2.Tue, Feb 5, 5:26 PM
Jdlrobson added a subscriber: Jdlrobson.

Should be straight forward but we recognise this is a slight context switch by people not familiar with this code.

phuedx claimed this task.Tue, Feb 5, 5:28 PM
phuedx added a subscriber: Edtadros.

I'll add a note as to why my proposed change should work and testing instructions for reviewing engineers and @Edtadros.

phuedx claimed this task.Tue, Feb 5, 7:00 PM
phuedx added a comment.Wed, Feb 6, 11:19 AM
In T214444#4905420, @phuedx wrote:

This should only involve changing https://github.com/wikimedia/mediawiki-extensions-WikimediaEvents/blob/db66eac96fb29beffd89cc40edda1aae99eaee0e/modules/all/ext.wikimediaEvents.readingDepth.js#L21 from dependencies = [ 'schema.' + SCHEMA_NAME ], to dependencies = [],.

The dependencies variable is used to track which additional RL modules that need to be loaded before any ReadingDepth logging can take place. We used this pattern (an RL "init" module that loads dynamic dependencies) because we wanted to avoid:

  1. Loading the schema.ReadingDepth RL module unconditionally when ReadingDepth logging is enabled on 1% (or fewer) of pageviews; and
  2. A race condition between the ReadingDepth logging and some experiments that run on MFE/MN.

Since the schema.* modules are no longer required to be loaded before EventLogging events can be logged, we no longer need to load the schema.ReadingDepth module. However, we still need to use this pattern to avoid #2 above.

phuedx removed phuedx as the assignee of this task.Wed, Feb 6, 11:19 AM
Tbayer added a subscriber: Tbayer.Wed, Feb 6, 6:03 PM
gerritbot added a subscriber: gerritbot.Mon, Feb 11, 3:05 PM

Change 489710 had a related patch set uploaded (by Jdrewniak; owner: Jdrewniak):
[mediawiki/extensions/WikimediaEvents@master] Remove deprecated 'schema.ReadingDepth' module.

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

gerritbot added a comment.Mon, Feb 11, 9:14 PM

Change 489710 merged by jenkins-bot:
[mediawiki/extensions/WikimediaEvents@master] Remove deprecated 'schema.ReadingDepth' module.

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

Edtadros added a comment.Tue, Feb 12, 6:07 AM

@Jdlrobson I just want to be sure of the following:

  1. Are you expecting an HTTP 204 status in the last QA step for the requests that appear after filtering?
  2. Would be good (possibly redundant) to verify that ReadingDepth doesn't appear as a warning in the console?

Thanks!

phuedx added a comment.Tue, Feb 12, 6:11 PM
In T214444#4946376, @Edtadros wrote:
  1. Are you expecting an HTTP 204 status in the last QA step for the requests that appear after filtering?

Yes. In this case, the server MUST respond with 204 No Content.

Tbayer added a comment.Tue, Feb 12, 8:36 PM

I understand this shouldn't have any impact on the logged events and their data (except maybe as consequence of the performance improvement in general), but please flag it in case that assumption turns out to be wrong.

phuedx updated the task description. (Show Details)Wed, Feb 13, 11:23 AM
Edtadros claimed this task.Wed, Feb 13, 6:13 PM
Edtadros added a comment.Thu, Feb 14, 2:47 PM

Test Result

Status: ✅ PASS
OS: macOS Mojave
Browser: Chrome DevTools Device Emulator (iPhone X)

Test Artifact(s):

The following message does not appear in the Console:

This page is using the deprecated ResourceLoader module "schema.ReadingDepth".

When filtered for ReadingDepth only two items appear with a 204 status.

Edtadros updated the task description. (Show Details)Thu, Feb 14, 2:49 PM
Edtadros updated the task description. (Show Details)
Edtadros reassigned this task from Edtadros to ovasileva.
Edtadros moved this task from Needs QA to Ready for Signoff on the Readers-Web-Backlog (Readers-Web-Kanbanana-Board-2018-19-Q3) board.
Content licensed under Creative Commons Attribution-ShareAlike 3.0 (CC-BY-SA) unless otherwise noted; code licensed under GNU General Public License (GPL) or other open source licenses. By using this site, you agree to the Terms of Use, Privacy Policy, and Code of Conduct. · Wikimedia Foundation · Privacy Policy · Code of Conduct · Terms of Use · Disclaimer · CC-BY-SA · GPL