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

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

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

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

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

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

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

  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.

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

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.