Page MenuHomePhabricator

Remove use of private 'mw.eventLog.schemas' property in ReadingDepth.js
Closed, ResolvedPublic2 Story Points

Description

The parent task tracks a combined Analytics-Performance effort to standardise a lightweight means of logging events, based what we've previously done in CentralNotice, in apps, and more recently in Popups.

We found a way that is entirely backwards-compatible. However, after scanning the codebase I found this line in ReadingDepth.js that is blocking progress due to it using a property that is @private. Please migrate this code as soon as possible.

https://gerrit.wikimedia.org/g/mediawiki/extensions/WikimediaEvents/+/70f7366477780e75cc425a29253c9a599be7223f/modules/all/ext.wikimediaEvents.readingDepth.js#318

readingDepth.js
// ..
		var schema = mw.eventLog.schemas[ SCHEMA_NAME ].schema,
// ..
	function isValidSampleGroup( schema, bucket ) {
		return schema.properties[ bucket ] && /_sample$/.test( bucket );
	}
// ..

This is not supported and should not have been used in production. The status quo for producing events is for the JavaScript code to contain all required logic to produce the event object. I'd recommend following the same approach here.

This was never a public interface. If such feature is commonly needed, it should be requested through the normal means. Perhaps we can accomodate it in some generic way as part of next year's plans as part of a non-lightweight schema model. Although given that the revision-number is already controlled by the JavaScript code, it might make more sense to require the two to stay together. Perhaps a build-tool can help to semi-automatically update the script, but we shouldn't want to load schemas from Meta-Wiki during production page views.

Developer notes

Validation already happens inside EventLogging via the private mw.eventLog.validate function. If validation fails errors will be captured and logged (where?).

We chatted about this and decided the validation is not useful and should be removed.

QA steps

Test on https://reading-web-staging.wmflabs.org/w/index.php?title=Pharmacovigilance&mobileaction=toggle_view_mobile#
Verify that both PageIssues and ReadingDepth events show on page load and that the ReadingDepth events contain

"page-issues-b_sample": true,

Event Timeline

Krinkle created this task.Sep 28 2018, 4:21 PM
Krinkle triaged this task as High priority.
Krinkle updated the task description. (Show Details)Sep 28 2018, 4:29 PM

let me know if the above sounds sane.

Jdlrobson added a subscriber: phuedx.EditedOct 2 2018, 4:37 PM

@phuedx points out mw.eventLog.getSchema is a public API. We could use this to do the same check. Would replacing mw.eventLog.schemas[ SCHEMA_NAME ].schema with mw.eventLog.getSchema unblock you @Krinkle ?

Edit: Ignore, we notice this is marked as @private

@Niedzielski also has some thoughts around this being fake protection and maybe we should remove it altogether.

function isValidSampleGroup( schema, bucket ) {
	return schema.properties[ bucket ] && /_sample$/.test( bucket );
}

Can we just remove this client-side validation? This is only useful for debugging. I think(?) that client-side validation is being avoided in production for T201068.

Jdlrobson updated the task description. (Show Details)Oct 2 2018, 5:38 PM
Jdlrobson moved this task from Needs Analysis to Upcoming on the Readers-Web-Backlog board.

We had a chat and are okay with removing this validation, however we are a bit wary of changing this while an A/B test that depends on this in flight.
We'll write a patch but can we wait 2 weeks to merge this or does that an unnecessary block to your work Timo?

It's actively blocking progress on T187207, which I'd really not want to delay further, also because of limited availability from Analytics to collaborate on this, and with lots of performance regressions going on right now that T187207 would help mitigate somewhat. I wouldn't normally push that hard for technical debt.

Perhaps we can make a compromise. The validation logic can be performed as-is, but without the schema object coming from EventLogging. The following would fulfil the exact same contract and resolve this issue.

function isValidSampleGroup(  bucket ) {
	return bucket === 'default_sample' || bucket === 'page-issues-a_sample' || bucket === 'page-issues-b_sample';

	// or
	// return /^(default|page-issues-a|page-issues-b)_sample$/.test( bucket );
}

Change 465208 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/extensions/WikimediaEvents@master] Remove client side validation for schema properties

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

Change 465208 merged by jenkins-bot:
[mediawiki/extensions/WikimediaEvents@master] Remove client side validation for schema properties

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

Jdlrobson assigned this task to Ryasmeen.
Jdlrobson added a project: Audiences-QA.
Milimetric added a subscriber: Milimetric.EditedOct 29 2018, 3:20 PM

ping @Ryasmeen, just checking when you're thinking of getting to this, it's blocking an Event Logging change we'd like to merge: https://gerrit.wikimedia.org/r/#/c/mediawiki/extensions/EventLogging/+/463501/

(there's no huge rush, just checking so I know to plan)

Jdlrobson moved this task from Backlog to Done on the Audiences-QA board.Oct 31 2018, 6:45 PM

I ran the QA steps and this is done.
The change has been long deployed @Milimetric so feel free to proceed with the EventLogging patch!