i13n: Log EL events with mw.track

Currently, the mw.eventLog.Schema class samples per pageview. However,
we expect that if a user is bucketed for a session, then all
EventLogging events logged during that session are in the sample.

Moreover, loading the class in the way that we did - asynchronously,
using mw.loader#using - introduced an issue where the eventLogging
change listener would subscribe in the next tick of the JavaScript VM's
event loop and miss the "pageLoaded" event being queued (see T167273).

Changes:
* Make the schema module follow the form of the statsvInstrumentation
  module, i.e. make it expose the #isEnabled method, and add the
  associated getEventLoggingTracker function.
* Update the eventLogging change listener accept the tracker returned by
  getEventLoggingTracker.
* Update/fix related JSDoc documentation.

Bug: T167236
Bug: T167273
Change-Id: I4f653bbaf1bbc2c2f70327e338080e17cd3443d4

src/changeListeners/eventLogging.js

@@ -46,14 +46,14 @@ function fnv1a32( string ) {
  * * https://phabricator.wikimedia.org/T161769
  * * https://phabricator.wikimedia.org/T163198
  *
- * [0]: https://github.com/wikimedia/mediawiki-extensions-WikimediaEvents/blob/master/modules/ext.wikimediaEvents.statsd.js
+ * [0]: https://github.com/wikimedia/mediawiki-extensions-WikimediaEvents/blob/29c864a0/modules/ext.wikimediaEvents.statsd.js
  *
  * @param {Object} boundActions
- * @param {mw.eventLog.Schema} schema
+ * @param {EventTracker} eventLoggingTracker
- * @param {ext.popups.EventTracker} track
+ * @param {EventTracker} statsvTracker
  * @return {ext.popups.ChangeListener}
  */
-module.exports = function ( boundActions, schema, track ) {
+module.exports = function ( boundActions, eventLoggingTracker, statsvTracker ) {
 	var tokenToSeenMap = {},
 		hashToSeenMap = {};
 
@@ -71,7 +71,7 @@ module.exports = function ( boundActions, schema, track ) {
 		token = event.linkInteractionToken;
 
 		if ( tokenToSeenMap[ token ] === true ) {
-			track( 'counter.PagePreviews.EventLogging.DuplicateToken', 1 );
+			statsvTracker( 'counter.PagePreviews.EventLogging.DuplicateToken', 1 );
 
 			shouldLog = false;
 		}
@@ -89,7 +89,7 @@ module.exports = function ( boundActions, schema, track ) {
 
 		// Has the event been seen before?
 		if ( hashToSeenMap[ hash ] === true ) {
-			track( 'counter.PagePreviews.EventLogging.DuplicateEvent', 1 );
+			statsvTracker( 'counter.PagePreviews.EventLogging.DuplicateEvent', 1 );
 
 			shouldLog = false;
 		}
@@ -99,7 +99,7 @@ module.exports = function ( boundActions, schema, track ) {
 		event = $.extend( true, {}, eventLogging.baseData, event );
 
 		if ( shouldLog ) {
-			schema.log( event );
+			eventLoggingTracker( 'event.Popups', event );
 		}
 
 		// Dispatch the eventLogged action even if it was a duplicate so that the

src/changeListeners/statsv.js

@@ -4,10 +4,10 @@
  * The listener will log events to StatsD via the [the "StatsD timers and
  * counters" analytics event protocol][0].
  *
- * [0]: https://github.com/wikimedia/mediawiki-extensions-WikimediaEvents/blob/master/modules/ext.wikimediaEvents.statsd.js
+ * [0]: https://github.com/wikimedia/mediawiki-extensions-WikimediaEvents/blob/29c864a0/modules/ext.wikimediaEvents.statsd.js
  *
  * @param {Object} boundActions
- * @param {ext.popups.EventTracker} track
+ * @param {EventTracker} track
  * @return {ext.popups.ChangeListener}
  */
 module.exports = function ( boundActions, track ) {

src/index.js

@@ -10,7 +10,6 @@ var mw = mediaWiki,
 	createGateway = require( './gateway' ),
 	createUserSettings = require( './userSettings' ),
 	createPreviewBehavior = require( './previewBehavior' ),
-	createSchema = require( './schema' ),
 	createSettingsDialogRenderer = require( './settingsDialog' ),
 	registerChangeListener = require( './changeListener' ),
 	createIsEnabled = require( './isEnabled' ),
@@ -18,6 +17,7 @@ var mw = mediaWiki,
 	renderer = require( './renderer' ),
 	createExperiments = require( './experiments' ),
 	statsvInstrumentation = require( './statsvInstrumentation' ),
+	eventLoggingInstrumentation = require( './schema' ),
 
 	changeListeners = require( './changeListeners' ),
 	actions = require( './actions' ),
@@ -34,47 +34,80 @@ var mw = mediaWiki,
 	];
 
 /**
- * @typedef {Function} ext.popups.EventTracker
+ * @typedef {Function} EventTracker
  *
- * An analytics event tracker like `mw.track`.
+ * An analytics event tracker, i.e. `mw.track`.
+ *
+ * @param {String} topic
+ * @param {Object} data
+ *
+ * @global
  */
 
 /**
  * Gets the appropriate analytics event tracker for logging metrics to StatsD
- * via the [the "StatsD timers and counters" analytics event protocol][0].
+ * via [the "StatsD timers and counters" analytics event protocol][0].
  *
- * If logging metrics to StatsD is enabled for the user, then the appriopriate
+ * If logging metrics to StatsD is enabled for the duration of the user's
- * function is `mw.track`; otherwise it's `$.noop`.
+ * session, then the appriopriate function is `mw.track`; otherwise it's
+ * `$.noop`.
  *
- * [0]: https://github.com/wikimedia/mediawiki-extensions-WikimediaEvents/blob/master/modules/ext.wikimediaEvents.statsd.js
+ * [0]: https://github.com/wikimedia/mediawiki-extensions-WikimediaEvents/blob/29c864a0/modules/ext.wikimediaEvents.statsd.js
  *
  * @param {Object} user
  * @param {Object} config
- * @param {Object} experiments
+ * @param {Experiments} experiments
- * @return {ext.popups.EventTracker}
+ * @return {EventTracker}
  */
 function getStatsvTracker( user, config, experiments ) {
 	return statsvInstrumentation.isEnabled( user, config, experiments ) ? mw.track : $.noop;
 }
 
+/**
+ * Gets the appropriate analytics event tracker for logging EventLogging events
+ * via [the "EventLogging subscriber" analytics event protocol][0].
+ *
+ * If logging EventLogging events is enabled for the duration of the user's
+ * session, then the appriopriate function is `mw.track`; otherwise it's
+ * `$.noop`.
+ *
+ * [0]: https://github.com/wikimedia/mediawiki-extensions-EventLogging/blob/d1409759/modules/ext.eventLogging.subscriber.js
+ *
+ * @param {Object} user
+ * @param {Object} config
+ * @param {Experiments} experiments
+ * @param {Window} window
+ * @return {EventTracker}
+ */
+function getEventLoggingTracker( user, config, experiments, window ) {
+	return eventLoggingInstrumentation.isEnabled(
+		user,
+		config,
+		experiments,
+		window
+	) ? mw.track : $.noop;
+}
+
 /**
  * Subscribes the registered change listeners to the
  * [store](http://redux.js.org/docs/api/Store.html#store).
  *
  * @param {Redux.Store} store
  * @param {Object} actions
- * @param {ext.popups.UserSettings} userSettings
+ * @param {UserSettings} userSettings
  * @param {Function} settingsDialog
- * @param {ext.popups.PreviewBehavior} previewBehavior
+ * @param {PreviewBehavior} previewBehavior
- * @param {ext.popups.EventTracker} statsvTracker
+ * @param {EventTracker} statsvTracker
+ * @param {EventTracker} eventLoggingTracker
  */
-function registerChangeListeners( store, actions, userSettings, settingsDialog, previewBehavior, statsvTracker ) {
+function registerChangeListeners( store, actions, userSettings, settingsDialog, previewBehavior, statsvTracker, eventLoggingTracker ) {
 	registerChangeListener( store, changeListeners.footerLink( actions ) );
 	registerChangeListener( store, changeListeners.linkTitle() );
 	registerChangeListener( store, changeListeners.render( previewBehavior ) );
 	registerChangeListener( store, changeListeners.statsv( actions, statsvTracker ) );
 	registerChangeListener( store, changeListeners.syncUserSettings( userSettings ) );
 	registerChangeListener( store, changeListeners.settings( actions, settingsDialog ) );
+	registerChangeListener( store, changeListeners.eventLogging( actions, eventLoggingTracker, statsvTracker ) );
 }
 
 /*
@@ -99,14 +132,20 @@ mw.requestIdleCallback( function () {
 		settingsDialog,
 		experiments,
 		statsvTracker,
+		eventLoggingTracker,
 		isEnabled,
-		schema,
 		previewBehavior;
 
 	userSettings = createUserSettings( mw.storage );
 	settingsDialog = createSettingsDialogRenderer();
 	experiments = createExperiments( mw.experiments );
 	statsvTracker = getStatsvTracker( mw.user, mw.config, experiments );
+	eventLoggingTracker = getEventLoggingTracker(
+		mw.user,
+		mw.config,
+		experiments,
+		window
+	);
 
 	isEnabled = createIsEnabled( mw.user, userSettings, mw.config, mw.experiments );
 
@@ -128,15 +167,9 @@ mw.requestIdleCallback( function () {
 
 	registerChangeListeners(
 		store, boundActions, userSettings, settingsDialog,
-		previewBehavior, statsvTracker
+		previewBehavior, statsvTracker, eventLoggingTracker
 	);
 
-	// Load EventLogging schema if possible...
-	mw.loader.using( 'ext.eventLogging.Schema' ).done( function () {
-		schema = createSchema( mw.config, window );
-		registerChangeListener( store, changeListeners.eventLogging( boundActions, schema, statsvTracker ) );
-	} );
-
 	boundActions.boot(
 		isEnabled,
 		mw.user,

src/schema.js

@@ -2,22 +2,21 @@
  * @module schema
  */
 
-var mw = window.mediaWiki,
-	$ = jQuery;
-
 /**
- * Creates an instance of the [EventLogging Schema class][0] with a sampling
+ * Gets whether EventLogging logging is enabled for the duration of the user's
- * rate of `wgPopupsSchemaSamplingRate` if the UA supports [the Beacon API][1]
+ * session. The bucketing rate is controlled by `wgPopupsSchemaSamplingRate`.
- * or `0` if it doesn't.
+ * However, if the UA doesn't support [the Beacon API][1], then bucketing is
+ * disabled.
  *
- * [0]: https://github.com/wikimedia/mediawiki-extensions-EventLogging/blob/master/modules/ext.eventLogging.Schema.js
  * [1]: https://w3c.github.io/beacon/
  *
- * @param {mw.Map} config
+ * @param {mw.user} user The `mw.user` singleton instance
+ * @param {mw.Map} config The `mw.config` singleton instance
+ * @param {Experiments} experiments
  * @param {Window} window
- * @return {mw.eventLog.Schema}
+ * @return {Boolean}
  */
-module.exports = function ( config, window ) {
+exports.isEnabled = function isEnabled( user, config, experiments, window ) {
 	var samplingRate = config.get( 'wgPopupsSchemaSamplingRate', 0 );
 
 	if (
@@ -27,5 +26,9 @@ module.exports = function ( config, window ) {
 		samplingRate = 0;
 	}
 
-	return new mw.eventLog.Schema( 'Popups', samplingRate );
+	return experiments.weightedBoolean(
+		'ext.Popups.instrumentation.eventLogging',
+		samplingRate,
+		user.sessionId()
+	);
 };

tests/node-qunit/changeListeners/eventLogging.test.js

@@ -6,16 +6,13 @@ QUnit.module( 'ext.popups/eventLogging', {
 			eventLogged: this.sandbox.spy()
 		};
 
-		this.schema = {
+		this.eventLoggingTracker = this.sandbox.spy();
-			log: this.sandbox.spy()
+		this.statsvTracker = this.sandbox.spy();
-		};
-
-		this.track = this.sandbox.spy();
 
 		this.changeListener = eventLogging(
 			this.boundActions,
-			this.schema,
+			this.eventLoggingTracker,
-			this.track
+			this.statsvTracker
 		);
 	}
 } );
@@ -33,8 +30,6 @@ QUnit.test( 'it should log the queued event', function ( assert ) {
 	var baseData,
 		state;
 
-	assert.expect( 1 );
-
 	baseData = {
 		foo: 'bar',
 		baz: 'qux'
@@ -47,11 +42,14 @@ QUnit.test( 'it should log the queued event', function ( assert ) {
 	this.changeListener( undefined, state );
 
 	assert.ok(
-		this.schema.log.calledWith( {
+		this.eventLoggingTracker.calledWith(
-			foo: 'bar',
+			'event.Popups',
-			baz: 'qux',
+			{
-			action: 'pageLoaded'
+				foo: 'bar',
-		} ),
+				baz: 'qux',
+				action: 'pageLoaded'
+			}
+		),
 		'It should merge the event data and the accumulated base data.'
 	);
 } );
@@ -93,16 +91,16 @@ QUnit.test( 'it should handle duplicate events', function ( assert ) {
 	this.changeListener( undefined, state );
 	this.changeListener( state, nextState );
 
-	assert.ok( this.track.calledTwice );
+	assert.ok( this.statsvTracker.calledTwice );
 	assert.deepEqual(
-		this.track.getCall( 0 ).args,
+		this.statsvTracker.getCall( 0 ).args,
 		[
 			'counter.PagePreviews.EventLogging.DuplicateToken',
 			1
 		]
 	);
 	assert.deepEqual(
-		this.track.getCall( 1 ).args,
+		this.statsvTracker.getCall( 1 ).args,
 		[
 			'counter.PagePreviews.EventLogging.DuplicateEvent',
 			1
@@ -111,7 +109,7 @@ QUnit.test( 'it should handle duplicate events', function ( assert ) {
 	);
 
 	assert.notOk(
-		this.schema.log.calledTwice,
+		this.eventLoggingTracker.calledTwice,
 		'It shouldn\'t log the event.'
 	);
 
@@ -126,7 +124,7 @@ QUnit.test( 'it should handle duplicate events', function ( assert ) {
 	this.changeListener( state, nextState );
 
 	assert.notOk(
-		this.track.calledThrice,
+		this.statsvTracker.calledThrice,
 		'The counter isn\'t incremented if the event isn\'t a duplicate'
 	);
 } );
@@ -139,7 +137,7 @@ QUnit.test( 'it should handle no event being logged', function ( assert ) {
 	this.changeListener( undefined, state );
 	this.changeListener( state, state );
 
-	assert.ok( this.track.notCalled );
+	assert.ok( this.statsvTracker.notCalled );
 } );
 
 QUnit.test( 'it should handle duplicate tokens', function ( assert ) {
@@ -161,9 +159,9 @@ QUnit.test( 'it should handle duplicate tokens', function ( assert ) {
 	this.changeListener( undefined, state );
 	this.changeListener( state, nextState );
 
-	assert.ok( this.track.calledOnce );
+	assert.ok( this.statsvTracker.calledOnce );
 	assert.deepEqual(
-		this.track.getCall( 0 ).args,
+		this.statsvTracker.getCall( 0 ).args,
 		[
 			'counter.PagePreviews.EventLogging.DuplicateToken',
 			1
@@ -172,7 +170,7 @@ QUnit.test( 'it should handle duplicate tokens', function ( assert ) {
 	);
 
 	assert.ok(
-		this.schema.log.calledOnce,
+		this.eventLoggingTracker.calledOnce,
 		'It shouldn\'t log the event with the duplicate token.'
 	);
 } );

tests/node-qunit/schema.test.js

@@ -1,10 +1,9 @@
-var mw = mediaWiki,
+var isEnabled = require( '../../src/schema' ).isEnabled,
-	createSchema = require( '../../src/schema' ),
+	stubs = require( './stubs' );
-	createStubMap = require( './stubs' ).createStubMap;
 
 QUnit.module( 'ext.popups/schema', {
 	beforeEach: function () {
-		this.config = createStubMap();
+		this.config = stubs.createStubMap();
 
 		this.config.set( 'wgPopupsSchemaSamplingRate', 1 );
 
@@ -14,44 +13,82 @@ QUnit.module( 'ext.popups/schema', {
 			}
 		};
 
-		// Stub out the mw.eventLog.Schema constructor function.
+		this.experiments = {
-		mw.eventLog = { Schema: this.sandbox.stub() };
+			weightedBoolean: this.sandbox.stub()
+		};
+
+		this.user = stubs.createStubUser();
+
+		// Helper function that DRYs up the tests below.
+		this.isEnabled = function () {
+			return isEnabled(
+				this.user,
+				this.config,
+				this.experiments,
+				this.window
+			);
+		};
 	}
 } );
 
-QUnit.test( 'it should use $wgPopupsSchemaSamplingRate as the sampling rate', function ( assert ) {
+QUnit.test( 'it should use wgPopupsSchemaSamplingRate as the sampling rate', function ( assert ) {
-	assert.expect( 2 );
+	this.isEnabled();
 
-	createSchema( this.config, this.window );
+	assert.ok( this.experiments.weightedBoolean.calledOnce );
-
+	assert.deepEqual(
-	assert.ok( mw.eventLog.Schema.calledWith( 'Popups', 1 ) );
+		this.experiments.weightedBoolean.getCall( 0 ).args,
+		[
+			'ext.Popups.instrumentation.eventLogging',
+			this.config.get( 'wgPopupsSchemaSamplingRate' ),
+			this.user.sessionId()
+		]
+	);
 
 	// ---
 
-	createSchema( createStubMap(), this.window );
+	this.config.delete( 'wgPopupsSchemaSamplingRate' );
 
-	assert.ok(
+	this.isEnabled();
-		mw.eventLog.Schema.calledWith( 'Popups', 0 ),
+
-		'If $wgPopupsSchemaSamplingRate isn\'t set, then the sampling rate should be 0.'
+	assert.strictEqual(
+		this.experiments.weightedBoolean.getCall( 1 ).args[ 1 ],
+		0,
+		'The bucketing rate should be 0 by default.'
 	);
 } );
 
-QUnit.test( 'it should use a 0 sampling rate when sendBeacon isn\'t supported', function ( assert ) {
+QUnit.test( 'it should use a 0 bucketing rate when sendBeacon isn\'t supported', function ( assert ) {
-	var expectedArgs = [ 'Popups', 0 ];
+	var window = {};
 
-	assert.expect( 2 );
+	isEnabled( this.user, this.config, this.experiments, window );
 
-	createSchema( this.config, { } );
+	assert.deepEqual(
-
+		this.experiments.weightedBoolean.getCall( 0 ).args[ 1 ],
-	assert.deepEqual( mw.eventLog.Schema.getCall( 0 ).args, expectedArgs );
+		/* trueWeight = */ 0
+	);
 
 	// ---
 
-	createSchema( this.config, {
+	window.navigator = {
-		navigator: {
+		sendBeacon: 'NOT A FUNCTION'
-			sendBeacon: 'NOT A FUNCTION'
+	};
-		}
-	} );
 
-	assert.deepEqual( mw.eventLog.Schema.getCall( 1 ).args, expectedArgs );
+	isEnabled( this.user, this.config, this.experiments, window );
+
+	assert.deepEqual(
+		this.experiments.weightedBoolean.getCall( 1 ).args[ 1 ],
+		/* trueWeight = */ 0
+	);
+} );
+
+QUnit.test( 'it should return the weighted boolean', function ( assert ) {
+	this.experiments.weightedBoolean.returns( true );
+
+	assert.ok( this.isEnabled() );
+
+	// ---
+
+	this.experiments.weightedBoolean.returns( false );
+
+	assert.notOk( this.isEnabled() );
 } );