diff --git a/resources/dist/index.js b/resources/dist/index.js index 95cc2f6fe..ee3684cb6 100644 Binary files a/resources/dist/index.js and b/resources/dist/index.js differ diff --git a/resources/dist/index.js.map b/resources/dist/index.js.map index 2eeb2c7d1..062e80bf7 100644 Binary files a/resources/dist/index.js.map and b/resources/dist/index.js.map differ diff --git a/src/changeListeners/eventLogging.js b/src/changeListeners/eventLogging.js index 06077fe25..072ffae04 100644 --- a/src/changeListeners/eventLogging.js +++ b/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 {ext.popups.EventTracker} track + * @param {EventTracker} eventLoggingTracker + * @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 diff --git a/src/changeListeners/statsv.js b/src/changeListeners/statsv.js index 74991b76d..4f0e01364 100644 --- a/src/changeListeners/statsv.js +++ b/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 ) { diff --git a/src/index.js b/src/index.js index c5a1c2b80..35dac3f57 100644 --- a/src/index.js +++ b/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 - * function is `mw.track`; otherwise it's `$.noop`. + * If logging metrics to StatsD 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-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 - * @return {ext.popups.EventTracker} + * @param {Experiments} experiments + * @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 {ext.popups.EventTracker} statsvTracker + * @param {PreviewBehavior} previewBehavior + * @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, diff --git a/src/schema.js b/src/schema.js index cdacddbd6..338439d95 100644 --- a/src/schema.js +++ b/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 - * rate of `wgPopupsSchemaSamplingRate` if the UA supports [the Beacon API][1] - * or `0` if it doesn't. + * Gets whether EventLogging logging is enabled for the duration of the user's + * session. The bucketing rate is controlled by `wgPopupsSchemaSamplingRate`. + * 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() + ); }; diff --git a/tests/node-qunit/changeListeners/eventLogging.test.js b/tests/node-qunit/changeListeners/eventLogging.test.js index 90ebe5218..0e307d26a 100644 --- a/tests/node-qunit/changeListeners/eventLogging.test.js +++ b/tests/node-qunit/changeListeners/eventLogging.test.js @@ -6,16 +6,13 @@ QUnit.module( 'ext.popups/eventLogging', { eventLogged: this.sandbox.spy() }; - this.schema = { - log: this.sandbox.spy() - }; - - this.track = this.sandbox.spy(); + this.eventLoggingTracker = this.sandbox.spy(); + this.statsvTracker = this.sandbox.spy(); this.changeListener = eventLogging( this.boundActions, - this.schema, - this.track + this.eventLoggingTracker, + 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( { - foo: 'bar', - baz: 'qux', - action: 'pageLoaded' - } ), + this.eventLoggingTracker.calledWith( + 'event.Popups', + { + 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.' ); } ); diff --git a/tests/node-qunit/schema.test.js b/tests/node-qunit/schema.test.js index aea9c15fa..cbab44eed 100644 --- a/tests/node-qunit/schema.test.js +++ b/tests/node-qunit/schema.test.js @@ -1,10 +1,9 @@ -var mw = mediaWiki, - createSchema = require( '../../src/schema' ), - createStubMap = require( './stubs' ).createStubMap; +var isEnabled = require( '../../src/schema' ).isEnabled, + stubs = require( './stubs' ); 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. - mw.eventLog = { Schema: this.sandbox.stub() }; + this.experiments = { + 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 ) { - assert.expect( 2 ); +QUnit.test( 'it should use wgPopupsSchemaSamplingRate as the sampling rate', function ( assert ) { + this.isEnabled(); - createSchema( this.config, this.window ); - - assert.ok( mw.eventLog.Schema.calledWith( 'Popups', 1 ) ); + assert.ok( this.experiments.weightedBoolean.calledOnce ); + assert.deepEqual( + 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( - mw.eventLog.Schema.calledWith( 'Popups', 0 ), - 'If $wgPopupsSchemaSamplingRate isn\'t set, then the sampling rate should be 0.' + this.isEnabled(); + + 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 ) { - var expectedArgs = [ 'Popups', 0 ]; +QUnit.test( 'it should use a 0 bucketing rate when sendBeacon isn\'t supported', function ( assert ) { + var window = {}; - assert.expect( 2 ); + isEnabled( this.user, this.config, this.experiments, window ); - createSchema( this.config, { } ); - - assert.deepEqual( mw.eventLog.Schema.getCall( 0 ).args, expectedArgs ); + assert.deepEqual( + this.experiments.weightedBoolean.getCall( 0 ).args[ 1 ], + /* trueWeight = */ 0 + ); // --- - createSchema( this.config, { - navigator: { - sendBeacon: 'NOT A FUNCTION' - } - } ); + window.navigator = { + 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() ); } );