Jump to content

Extension:CentralNotice/Notes/Banner controller refactoring

From mediawiki.org

Here's a place to keep some rough notes about reorganizing CentralNotice banner controller code. See also Campaign-associated mixins and banner history. Work should go on the campaign_mixins branch.

Goals

[edit]
  • Improve modularity and separation of concerns in banner controller code.
  • Adapt the client-side API as needed for new campaign-associated mixins.
  • Improve site performance when banners are shown and when they're not.

Objects, architecture and rollout

[edit]

Requirements

[edit]
  • Separate concerns in a way that makes sense for selective loading of components.
  • Follow typical and best-practice MW patterns.
  • Remove cruft.
  • Prepare to move out code that has uses beyond CentralNotice (GeoIP cookie stuff, maybe mobile device detection).

Proposal

[edit]

Following are modules, objects and their responsibilities. Format is: RLModule/object(where applicable).

ext.centralNotice.startup Checks choiceData and the presence of a banner URL param, and starts the appropriate processes. This module is added via the HTML, and brings in choiceData and GeoIP as dependencies.

ext.centralNotice.choiceData JSON with data needed to choose a campaign and a banner on the client. If there are possible campaigns available, this module will depend on display, as well as any mixin modules required by the campaings, and dependencies of those modules.

ext.centralNotice.display/mw.centralNotice Coordinates other objects, displays banners and provides an API for outside code, including banners and campaign-associated mixins.

ext.centralNotice.display/mw.centralNotice.internal.state Stores data for campaign/banner processing and data related to the state of that processing.

ext.centralNotice.display/mw.centralNotice.internal.chooser Selects a campaign and a banner based on choiceData.

ext.centralNotice.display/mw.centralNotice.internal.bucketer Retrieves, stores, chooses and processes buckets.

ext.centralNotice.display/mw.centralNotice.internal.hide Retrieve, process and store 'hide' cookies, which prevent banners from showing in certain circumstances.

ext.centralNotice.kvStore/mw.centralNotice.internal.kvStore Stores and retrieves items for different contexts (campaign, category, global). Should be declared as a dependency by mixin modules, as needed. See also the existing KV store patch.

ext.centralNotice.geoIP/mw.geoIP Sets window.Geo. May be moved to a different extension or to MW core. (See the related Phabricator task.)

We'll use dynamic RL dependencies of ext.centralNotice.choiceData to select the RL modules needed.

Considerations: mobile-only code

[edit]

Currently the code for checking the device is in a mobile-only module, so it's only sent to mobile clients. However, it's only a small bit of code, and it by putting it in bannerManager we're able to send it only when a client has possible campaigns in choiceData. This will benefit mobile users more than the current setup and will not affect desktop users noticeably. It would be possible to keep it mobile-only and make it available only when there are campaign choices by creating a mobile-only version of CNBannerChoiceDataResourceLoaderModule; however, this added complication does not seems worth it, especially if, in the longer term, we can work towards having device data available server-side.

Rollout proposal: RL module deprecation and queue changes

[edit]

Calls to load ResourceLoader modules are embedded <script> tags in the main HTML. This HTML can be cached for up to one month for anonymous users. As a result, both deprecating modules and moving them to a different queue can take a while.

In addition, given the size of the current change, we need to keep open the option of reverting the main code rollout.

Here is a multi-stage proposal to deprecate current RL modules, move new modules to a different queue, and separate GeoIP code from CentralNotice.

Modules as currently deployed

[edit]

Summary: Everything is top-loaded. GeoIP data is processed right away, and everything else waits for the DOM to be ready.

Module Included in site HTML Contents Dependencies
ext.centralNotice.bannerController Desktop Reams o' code jquery.cookie, json, mediawiki.Uri, ext.centralNotice.bannerChoiceData, ext.centralNotice.bannerController.lib
ext.centralNotice.bannerController.mobile Mobile Reams o' code (same as bannerController) jquery.cookie, json, mediawiki.Uri, ext.centralNotice.bannerChoiceData, ext.centralNotice.bannerController.lib,

ext.centralNotice.bannerController.mobiledevice

ext.centralNotice.bannerController.mobiledevice Mobile Device categorization
ext.centralNotice.bannerChoiceData Mobile JSON with available campaign and banner choices
ext.centralNotice.bannerController.lib Mobile Reams o' code

Stage 1: New modules only as dependencies, rollback is possible

[edit]

Summary: Old modules are emptied of code and only bring in new modules as dependencies. The old modules are still added to the HTML; this should make it easier to roll back this step if necessary. Everything executes as soon as possible (unless it accesses the DOM).

Module Included in site HTML Contents Dependencies
ext.centralNotice.bannerController Desktop empty ext.centralNotice.startUp, ext.CentralNotice.geoIP
ext.centralNotice.bannerController.mobile Mobile empty ext.centralNotice.startUp, ext.CentralNotice.geoIP
ext.centralNotice.bannerController.mobiledevice Mobile empty
ext.centralNotice.bannerChoiceData Mobile empty
ext.centralNotice.bannerController.lib Mobile empty
ext.centralNotice.startUp Minimal startup logic ext.centralNotice.choiceData, mediawiki.util
ext.centralNotice.goeIP CN-independent GeoIP processing
ext.centralNotice.choiceData JSON with available campaign and banner choices Only as needed: ext.centralNotice.display, campaign-associated mixin modules, and the mixin modules' dependencies dependencies.
ext.centralNotice.display All campaign- and banner-selection logic ext.centralNotice.geoIP, jquery.cookie, json, mediawiki.Uri

Stage 2: New modules added to cached HTML

[edit]

Summary: Old modules are no longer added to the HTML, but will still be loaded from cached HTML. As in the previous stage, everything executes as soon as possible (unless it accesses the DOM).

Module Included in site HTML Contents Dependencies
ext.centralNotice.bannerController empty ext.centralNotice.startUp, ext.CentralNotice.geoIP
ext.centralNotice.bannerController.mobile empty ext.centralNotice.startUp, ext.CentralNotice.geoIP
ext.centralNotice.bannerController.mobiledevice empty
ext.centralNotice.bannerChoiceData empty
ext.centralNotice.bannerController.lib empty
ext.centralNotice.startUp Desktop and mobile Minimal startup logic ext.centralNotice.choiceData, mediawiki.util
ext.centralNotice.goeIP Desktop and mobile CN-independent GeoIP processing
ext.centralNotice.choiceData JSON with available campaign and banner choices Only as needed: ext.centralNotice.display, campaign-associated mixin modules, and their dependencies.
ext.centralNotice.display All campaign- and banner-selection logic ext.centralNotice.geoIP, jquery.cookie, json, mediawiki.Uri

Stage 3: Improve performance for banners display, old modules ready to be removed

[edit]

Summary: One month after Stage 2 is deployed, we should be able to completely remove the old RL modules.

Module Included in site HTML Contents Dependencies
ext.centralNotice.bannerController
ext.centralNotice.bannerController.mobile
ext.centralNotice.bannerController.mobiledevice
ext.centralNotice.bannerChoiceData
ext.centralNotice.bannerController.lib
ext.centralNotice.startUp Desktop and mobile Minimal startup logic ext.centralNotice.choiceData, mediawiki.util
ext.centralNotice.goeIP Desktop and mobile CN-independent GeoIP processing
ext.centralNotice.choiceData JSON with available campaign and banner choices Only as needed: ext.centralNotice.display, campaign-associated mixin modules, and their dependencies.
ext.centralNotice.display All campaign- and banner-selection logic ext.centralNotice.geoIP, jquery.cookie, json, mediawiki.Uri

Expected future changes

[edit]
  • GeoIP processing should move to a different extension or to Mediawiki core.
  • We may make other changes to improve performance and/or mitigate the "banner bump".

API

[edit]

Requirements

[edit]
  • Remain compatible with existing banners. Certain methods, properties and hooks should remain available. We can use JS get to warn when deprecate properties are accessed. The following should stick around:
    • mw.centralNotice.data (though it'll now be read-only)
    • mw.centralNotice.bannerData
    • mw.centralNotice.bannerData.alterImpressionData
    • mw.centralNotice.events.bannerLoaded
    • mw.centralNotice.insertBanner()
    • mw.centralNotice.hideBanner()
  • Here are things that we don't need to keep publicly accessible outside CentralNotice:
    • mw.centralNotice.recordImpression()
    • mw.centralNotice.impressionData
    • mw.centralNotice.alreadyRan
    • mw.centralNotice.deferredObjs
    • mw.centralNotice.loadBanner()
    • mw.centralNotice.chooseRandomBanner()
    • mw.centralNotice.toggleNotice()
    • mw.centralNotice.getBucket() (already deprecated)
    • mw.centralNotice.storeBucket() (already deprecated)
    • mw.centralNotice.initialize()
  • New stuff the API will offer for campaign-associated mixins:
    • Retrieve bucket.
    • Set bucket.
    • A flag that can be set by a mixin to signal that banners in a campaign are guaranteed to display to the user if loaded. This will disable Special:RecordImpression for such campaigns and add a parameter to the call to Special:BannerLoader to confirm that impressions may be counted using logs to that endpoint.
    • A flag that can be set by a mixin to cancel the display of banners from this campaign.
    • Access to the Key-Value store methods.
  • Everything should work smoothly with existing cached HTML.

Proposal

[edit]

See also the WIP patch.

mw.centralNotice.registerCampaginMixin( mixin )

mw.centralNotice.getBucket() Get the bucket for this campaign. Guaranteed to be available in the Mixin.setPreBannerHandler() hook.

mw.centralNotice.setBucket( bucket ) Will influence which banner is selected when called from the Mixin.setPreBannerHandler() hook.

mw.centralNotice.setBannerNotGuaranteedToDisplay() Sets a boolean flag; default false.

mw.centralNotice.cancelBanner( reason ) Sets boolean flag and string. Should be called from the Mixin.setPreBannerHandler() hook.

mw.centralNotice.setBannerLoadedButHidden( reason ) Sets boolean flag and string.

mw.centralNotice.setRecordImpressionSampleRate( rate ) Set the sample rate for calling Special:RecordImpression. Default is wgCentralNoticeSampleRate.

mw.centralNotice.getKVStorageContexts() Available key-value storage contexts

mw.centralNotice.isKVStorageAvailable()

mw.centralNotice.setKVStorageItem( key, value, context )

mw.centralNotice.getKVStorageItem( key, context )

mw.centralNotice.removeKVStorageItem( key, context )

mw.centralNotice.getKVStorageErrors()

Smoke and unit testing

[edit]

Following are bits of functionality that we need to verify in the new code. The "PS..." columns are for patchs set in the Gerrit change. Functionality covered by unit tests should be marked as such.

Component Functionality PS44
geoIP When a GeoIP cookie is present and contains valid data, the data is parsed and placed in a window.Geo object, no background requests are made, and the deferred object is resolved right away.
geoIP When a GeoIP cookie is present, but it contains invalid data of any kind, a background request is made that sets window.Geo, a cookie is set, and the deferred object is resolved.
geoIP If a background geoIP lookup request returns an error, appropriately flagged data is stored in the cookie (an object with the value "vx" for the af property), and the deferred object is rejected. The flagged cookie data is in the right format to cause another background lookup on the next pageview.
display A banner is injected into the DOM by mw.centralNotice.insertBanner() ✓QUnit
display Calling mw.centralNotice.displayTestingBanner() fetches the banner indicated by the banner URL param. ✓QUnit
display The randomcampaign URL param determines campaign selection when more than one campaign is allocated. ✓QUnit
display The randombanner URL param determines banner selection when more than one banner is allocated. ✓QUnit
display chooseAndMaybeDisplay() runs pre-banner mixin hook handlers with the correct parameters, after a campaign and bucket are chosen.
display chooseAndMaybeDisplay() runs post-banner mixin hook handlers with the correct parameters,if a banner is canceled, or if the chosen campaign doesn't have a banner for this user.
display chooseAndMaybeDisplay() calls the alterImpressionData function if it exists and if bannerNotGuaranteedToDisplay is set, and that function is able to cancel the banner and provide a reason. Note: this should be smoke tested, but unit tests are probably not worth the effort, since the feature may well be deprecated.
display chooseAndMaybeDisplay() calls Special:RecordImpression at the designated sample rate in all cases if a banner is chosen. Note: this should be smoke tested, but unit tests are probably not worth the effort, since the feature may well be deprecated.
display Calls to Special:RecordImpresion provide the correct legacy params as per the documentation. Note: many parameters there are out of scope for this code, since they're currently provided by in-banner JS. Also note: this should be smoke tested, but unit tests are probably not worth the effort, since the feature may well be deprecated.
display.state setUp() pulls in initial data and makes it available from getData() as appropriate.
display.state setUp() sets device class correctly based on userAgent.
display.bucketer process() recognizes when a bucket is available from a cookie and doesn't change it.
display.bucketer process() chooses a random bucket if none was available in the cookie, and saves it to the cookie.
display.bucketer process() updates a campaign's end date in the cookie if necessary.
display.bucketer process() purges expired buckets from the cookie.
display.bucketer loadBuckets() deals correctly with the absence of a cookie and the presence of a corrupted one.
display.bucketer getBucket() retrieves the bucket correctly.
display.bucketer setBucket() sets the bucket and stores it in the cookie.
display.bucketer storeBucket() sets the cookie expiry date correctly.
display.hide If a hide cookie is present for the category indicated by setCategory(), and the duration for the hide reason indicated by the cookie has not yet elapsed, then after processCookie() is called, shouldHide() will return true.
display.hide If a hide cookie is present for the category indicated by setCategory(), and the duration for the hide reason indicated by the cookie has elapsed, then after processCookie() is called, shouldHide() will return false.
display.hide setHideWithCloseButtonCookies() sets a cookie with the appropriate values.
display.hide setHideWithCloseButtonCookies() adds img tags to the DOM to bring on the hide cookie swarm, as appropriate.
display.chooser Campaigns and banners correctly allocated for diverse targeting scenarios and contexts. ✓QUnit
kvStore setItem() correctly stores an item with the right key for the context indicated.
kvStore setItem() adds an error to the error log when something goes awry.
kvStore The error log stores errors as a FIFO queue.
kvStore getErrorLog() retrieves the error log.
kvStore getItem() gets an item correctly for the context indicated.
kvStore removeItem() removes an item correctly for the context indicated.

Issuez and considerations

[edit]

Reducing the size of top-loading RL modules

[edit]

Does the fact that RL modules are cached in localStorage somewhat mitigate performance gains from doing this?

[edit]

Why should any CentralNotice code be top-loaded?

[edit]

Discussion

The sooner the bannerController runs, the less of a page bump there will be when a banner is shown. Also, completely eliminating CentralNotice top-loaded modules would increase the number of round trips across the board. —AGreen (WMF) (talk)

I guess this only applies to traditional banners. Banners that overlay the content, and animate wouldn't have this issue. I think ideally you want to minimise what is loaded on top to just be the function - doINeedToRenderABanner, this can decide the answer and load a banner placeholder at the top of the page with a nice ajax loader bar (a bit like the ones they have in VE). Then the code at the bottom takes over when it can and says whichBannerDoINeed and howDoIRenderThatBanner and hey presto top loaded RL JavaScript is much tinier. —Jdlrobson (talk) 19 June 2015‎
Thanks! I agree that if banners were required to always overlay page content, CentralNotice could be more performant pretty easily. However, that seems like a broad proposal that would need more discussion elsewhere.
I think there's a lot of room to improve the performance of the current system, without moving to banners that must overlay content. As it stands, the bulk of the processing is to answer precisely the question you mentioned, doINeedToRenderABanner. If we can start to answer this question on the server for a larger percentage of users (as explained in this Phabricator task, then a lot less users will receive the banner selection code.
Maybe some benchmarking of the newly refactored code would help set priorities.
Regarding the idea of showing a placeholder while the actual banner loads (I think that suggestion is for a scenario in which banners don't overlay content?), the code that decides whether or not to inject the placeholder, and determines its size, would still have to run as early as possible, again to avoid a page bump, no?
Also, remember that it's not possible to fully determine on the server whether a banner should be shown to a user, since some of the criteria are only available in the user's browser. —AGreen (WMF) (talk) 03:00, 9 July 2015 (UTC)