Page MenuHomePhabricator

Deprecate/delete the mw.eventLog.Schema class
Open, Needs TriagePublic

Description

Background

mw.eventLog.Schema was introduced in rEEVL306f4c1ac384: Add Schema class. IIRC it was used by various instruments owned by Readers Web. However, at the time of writing (2022/04/05), it is not used by any codebase:

TODO

  • Decide whether the class should be deprecated or deleted immediately
  • If we decide to deprecate the class, announce the deprecation on wikitech-l
  • Delete the modules/ext.eventLogging/Schema.js and tidy up

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald Transcript

I'm down to delete it. My understanding is we introduced it back in a time when WikimediaEvents was not the defacto location for where instrumentation happened and classes were fashionable. I think we've outlived this evidenced by the lack of usage.

Were you thinking of deprecating it in 1.39 and removing it in 1.40? Could also backport a deprecation patch to 1.38 branch given the lack of usage and remove it now, or skip the deprecation process altogether.

I believe the bulk of usage was removed over time by me, due to it encouraging performance anti-patterns. It made it hard to do the right thing (and practice, this meant it wasn't done), and very easy to do something suboptimal. In particular, by abstracting the sampling it meant the caller would, unless they do it directly as well, not know what the state of the instrumentation is and thus incur all the costs of DOM instrumentation, Crypto API generation for tokens, cookie I/O, and any other values needed for the event objects; even if the instrumentation wasn't currently active for this page/user/wiki, etc. It also encouraged the event object to be build up in a distributed fashion where it was difficult to confirm in code review or debugging whether it was matching the schema. And this then also had the side-effect of offering this convenience, again, by computing it all upfront as it had to satify the injection pattern of the class.

Since we have mw.track() the need for the class has largely gone away. And even more so with the utility methods for inSample() we added since from a single lightweight EL bundle. Anything expensive can be computed after the inSample check, and used from local variables re-used when creating the actual event object.

I understand the metrics-platform will likely do some of this differently, which is fine. These were the motivations for phasing it out. Thanks for filing the task.

+1 to delete outright, deprecation would be nice but I think this API is pretty tightly controlled and used by a small set of us, with very low probability that we're missing something when we decide to delete.

I'm happy to remove this class without deprecation for the reasons that @Milimetric stated above.

I'll submit the patch, email wikitech-l with a "proposal for removal without deprecation" email, wait two weeks, and then bump this task.

Change 780882 had a related patch set uploaded (by Phuedx; author: Phuedx):

[mediawiki/extensions/EventLogging@master] ext.eventLogging: Remove mw.eventLog.Schema without deprecation

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

I can review and merge the above patch. I have set a reminder for 2 weeks time.

Your codesearch is missing things -- it's used in MobileFrontend in schemaEditAttemptStep.js and schemaVisualEditorFeatureUse.js, and also in DiscussionTools.

Try this instead: https://codesearch.wmcloud.org/search/?q=mw.eventLog.Schema&i=nope&files=js&excludeFiles=&repos=

@phuedx do you need help moving this one along presumably those usages would be straightforward to address ?

@phuedx do you need help moving this one along presumably those usages would be straightforward to address ?

Agreed. The changes might conflict with/confuse current work in progress on the instruments that use them (see T309013: EditAttemptStep Migration to (monoschema) MP and T309602: VisualEditorFeatureUse Migration to MP). After those tasks are Done™, I'd be happy to revisit this.

Edit

Changed "conflict with" to "might conflict with/confuse".