Page MenuHomePhabricator

Update ReadingDepth instrumentation to avoid deprecated schema module (blocks loads event)
Closed, ResolvedPublic2 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
  • 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

Details

Related Gerrit Patches:
mediawiki/extensions/WikimediaEvents : masterRemove deprecated 'schema.ReadingDepth' module.

Event Timeline

Krinkle created this task.Jan 22 2019, 10:48 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptJan 22 2019, 10:48 PM
Krinkle updated the task description. (Show Details)Jan 22 2019, 10:49 PM
Jdlrobson triaged this task as High priority.Jan 22 2019, 11:44 PM
Jdlrobson moved this task from Needs Prioritization to Upcoming on the Readers-Web-Backlog board.
Jdlrobson set the point value for this task to 2.Feb 5 2019, 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.Feb 5 2019, 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.Feb 5 2019, 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.Feb 6 2019, 11:19 AM

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)Feb 13 2019, 11:23 AM
Edtadros claimed this task.Feb 13 2019, 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 reassigned this task from Edtadros to ovasileva.Feb 14 2019, 2:49 PM
Edtadros updated the task description. (Show Details)
Edtadros updated the task description. (Show Details)
Jdlrobson added a subscriber: ovasileva.

I will sign off

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.

For the record: We also talked about this briefly in standup earlier today and determined that this task indeed doesn't need data signoff / data quality vetting. (If we already had the planned dashboard tracking average or median ReadingDepth values, it would have been worth taking a quick look there to see if those changed around the deploy date, but that is still being worked on in T209598.)

Jdlrobson closed this task as Resolved.Feb 19 2019, 10:01 PM

@Tbayer let me know if you want us to do any analysis on the period during the deploy to confirm one way or another if this impacted ReadingDepth in any way. If not, hopefully my comment https://meta.wikimedia.org/wiki/Schema_talk:ReadingDepth#Note_on_February_ResourceLoader_module_schema_deprecation will suffice retroactively.