Page MenuHomePhabricator

Update related pages schema to allow for current A/B test
Closed, ResolvedPublic3 Estimated Story Points

Description

Background

Follow up to T154681
We would like to compare the experiences of 10% of users who have related pages disabled with the 90% of users who have it enabled to see whether the 10% of users who have it disabled read less pages than those that have it.

Acceptance Criteria

To do this, we need to use the schema revision 16352530
Add to the existing schema:

  1. Log an event feature-enabled for users where the feature is enabled if allowed by EventLogging sampling
  2. Log an event feature-disabled for users where the feature is disabled if allowed by EventLogging sampling
  3. Every existing event should also send an isEnabled flag which says whether it's possible for the user to see related pages.

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
ovasileva updated the task description. (Show Details)
ovasileva updated the task description. (Show Details)

@Jdlrobson - do we know if lazy loading affects the "ready" state in https://meta.wikimedia.org/wiki/Schema:RelatedArticles ? If not we can count ready as pageloaded and wouldn't need separate loaded events to look at session depth

Looking at the code we cannot currently A/B test session length with/without related pages:

  • The ext.relatedArticles.readMore.bootstrap module is loaded by default. This decides whether the user is in the 10% of users not getting related pages. If the feature is enabled and the user scrolls close to read more then a mediawiki event is triggered that results in the loading of the articles and the commencement of EventLogging events being sent...
  • The description in the schema is confusing ("ready - when the read more code is ready") - The ready event is only sent when the user scrolls close to the footer (not at the start of the page load).

You'll probably want to add a new event to the schema "code-loaded'/'page-loaded' as well as an "isEnabled" flag.

Right now with the current schema we log no events for the 10% who are opted out.

Jdlrobson set the point value for this task to 3.Feb 7 2017, 7:25 PM
Jdlrobson updated the task description. (Show Details)

@Jdlrobson - would eventName be the same as pageLoaded?

Jdlrobson updated the task description. (Show Details)
ovasileva moved this task from Product Owner Backlog to Upcoming on the Web-Team-Backlog board.

also, if there's space, might make sense to pull this into the current sprint

@ovasileva copying and pasting from https://phabricator.wikimedia.org/T157372#3011403:

@ovasileva if a logged in user is opted into related pages on the desktop site, they will unconditionally see related pages on the mobile site (even if they are in the 10% of users that shouldn't). Is this a problem we need to fix? If it is, I recommend we prioritise T146436 for next sprint and fold this new requirement into T157375.

@Jdlrobson - this is fine. what would this look like in terms of the metrics?

Well, it means the bucket of 10% users who are not meant to have page previews enabled will not exactly be 10%. Their status will however be reflected in isEnabled.

Thus it would be a 10% - # of beta feature users roll out.
Not significant enough to care about imo

@Jdlrobson - I agree - beta mode page says 30,000 people with related pages right now

@ovasileva we are not limiting tracking to Minerva beta so I have removed the addition "skin: Minerva (currently we're only tracking Minerva beta)" Skin logged for minerva stable related pages users should be minerva-stable (not minerva)

@bmansurov flagged some really good points with the new schema.

  1. the isEnabled flag on all events other than loaded doesn't make sense for events such as clicked and seen as at this point we know the code is enabled.
  2. There is confusion about the difference between ready and loaded (ready refers to what happens when the related pages widget is injected into the page)

I think we can rename ready to rendered to help this make more sense.
I'm not sure what to do about the isEnabled flag. It may be redundant but it doesn't seem to cause much harm.

Any other ideas for how to clean this up?

This might be messier, but could we remove is enabled and have two loaded events - loaded_enabled and loaded_disabled for example?

ovasileva renamed this task from Add session length to related pages schema to Update related pages schema to allow for current A/B test.Feb 14 2017, 4:41 PM
ovasileva updated the task description. (Show Details)

Change 339565 had a related patch set uploaded (by Bmansurov):
Hygiene: lazy-load event logging code

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

@ovasileva can this point be removed?

Every existing event should also send an isEnabled flag which says whether it's possible for the user to see related pages.

If the feature is disabled, the user won't be able to see or click, thus by definition, for these events the feature is considered enabled. The only event that will benefit from the isEnabled flag will be the 'ready' event, which can safely be replaced by 'feature-enabled' or 'feature-disabled' events.

@ovasileva, as noted in T156039#3007175 we currently don't have a mechanism to allow the feature to the subset of anonymous users if they are not using the minerva skin. Should we re-open the task?

Change 340648 had a related patch set uploaded (by Bmansurov):
[mediawiki/extensions/RelatedArticles] WIP: Schema:RelatedArticles revision update

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

@bmansurov - as long we we're tracking feature-enabled and feature-disabled, this is fine. Also, tracking minerva-only is fine. We'll be disabling related pages on vector soon.

phuedx subscribed.

^ rERARa40286c800c3: Hygiene: lazy-load event logging code needs a handful of changes to support the RelatedArticles extension's soft dependency on the EventLogging extension. rERAR5672c43e5799: Schema:RelatedArticles revision update LGTM but I need to test it locally.

Change 339565 merged by jenkins-bot:
[mediawiki/extensions/RelatedArticles] Hygiene: lazy-load event logging code

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

I've +2'd rERARa40286c800c3: Hygiene: lazy-load event logging code and it'll be merged soonish – /me stares at Zuul.

I've reviewed, tested, and +1'd rERAR5672c43e5799: Schema:RelatedArticles revision update. The notes from my testing can be found below.

I'd like to talk about the relationship between the statically defined ext.relatedArticles.readMore.bootstrap and the dynamically defined ext.relatedArticles.readMore.eventLogging RL modules. As the latter is always defined, it might be enough to define the former as depending on it.


Scenario 1: Beta feature enabled; load short (stub) page.
Expected events: [feature-enabled, ready, seen]
Actual events: [feature-enabled, ready, seen]

Scenario 2: Beta feature enabled; load long page and scroll to bottom.
Expected events before scrolling: [feature-enabled, ready]
Actual events before scrolling: [feature-enabled, ready]
Expected events after scrolling: [seen]
Actual events after scrolling: [seen]

Scenario 3: Beta feature disabled; not in bucket ($wgRealtedArticlesEnabledSamplingRate = 0; in LocalSettings.php); load short (stub) page
Expected events: [feature-disabled]
Actual events: [feature-disabled]

Scenario 4: Beta feature disabled; not in bucket ($wgRealtedArticlesEnabledSamplingRate = 0; in LocalSettings.php); load long page and scroll to bottom.
Expected events: [feature-disabled]
Actual events: [feature-disabled]

Scenario 5: Beta feature disabled; in bucket ($wgRealtedArticlesEnabledSamplingRate = 1; in LocalSettings.php); load long page and scroll to bottom.
Expected events before scrolling: [feature-enabled, ready]
Actual events before scrolling: [feature-enabled, ready]
Expected events after scrolling: [seen]
Actual events after scrolling: [seen]

Let's discuss ^ that later. I've mergerd. Great QA steps. I'm moving this to signoff, this is a technical task, should we consider it QAd?

BTW if we decide to make the files dependencies explicitly then this should go back to needs more work for that change.

I'd like to talk about the relationship between the statically defined ext.relatedArticles.readMore.bootstrap and the dynamically defined ext.relatedArticles.readMore.eventLogging RL modules. As the latter is always defined, it might be enough to define the former as depending on it.

I had a reservation regarding this. After some investigation I think I ended up concluding that we may have a dependency error, because modules defined inside a hook may not be available at the time extension.json is parsed.

If anyone knows that it's not true, I'm happy to make that dependency in extension.json.

Change 340648 merged by jenkins-bot:
[mediawiki/extensions/RelatedArticles] Schema:RelatedArticles revision update

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

Change 341364 had a related patch set uploaded (by bmansurov):
[mediawiki/extensions/RelatedArticles] Do not lazy-load an event logging module

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

Change 341364 merged by jenkins-bot:
[mediawiki/extensions/RelatedArticles] Do not lazy-load an event logging module

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

Related Articles shows up on the Beta Cluster both when enabled as a Beta Feature and when using the Minerva skin (m.).

^ Because QA'ing instrumentation would be hard. Maybe another engineer could run through the scenarios in T157375#3075094?

I wanted to attempt QAing this today but I realised I'm not sure how to do so other than locally. Is there a way to force myself into the sample bucket for related pages?

I'm also a little confused about the scenarios relating to beta features given mobile stable is where we require these events to show up or not show up.

@Jdlrobson You can set the value of $wgRelatedArticlesLoggingSamplingRate to 1 in the beta cluster to test.

As for the comments regarding the beta features, it was not clear from the task description that the sampling had to happen on minerva only. Those were clarifying comments.

Since the behaviour is needed on mobile I modified Sam's test scenarios:

Scenario 1: Mobile stable; 90% bucket; load short (stub) page.
https://en.m.wikipedia.beta.wmflabs.org/wiki/Related_Articles_1?debug=true
Expected events: [feature-enabled, ready, seen]
Actual events: [feature-enabled, ready, seen]

Scenario 2: Mobile stable; 90% bucket; load long page and scroll to bottom.
https://en.m.wikipedia.beta.wmflabs.org/wiki/Barack_Obama?debug=true
Expected events before scrolling: [feature-enabled, ready]
Actual events before scrolling: [feature-enabled, ready]
Expected events after scrolling: [seen]
Actual events after scrolling: [seen]

Scenario 3: Mobile; 10% bucket; load short (stub) page
https://en.m.wikipedia.beta.wmflabs.org/wiki/Related_Articles_1?debug=true
Expected events: [feature-disabled]
Actual events: [feature-disabled]

Scenario 4: Mobile; 10% bucket; load long page and scroll to bottom.
https://en.m.wikipedia.beta.wmflabs.org/wiki/Barack_Obama?debug=true
Expected events: [feature-disabled]
Actual events: [feature-disabled]

Scenario 5: Mobile beta; 10% bucket; in bucket ($wgRealtedArticlesEnabledSamplingRate = 1; in LocalSettings.php); load long page and scroll to bottom.
Expected events before scrolling: [feature-enabled, ready]
Actual events before scrolling: [feature-enabled, ready]
Expected events after scrolling: [seen]
Actual events after scrolling: [seen]

To enable event logging for scenarios 1-5 I added the following to my user js file at User:Jdlrobson/minerva.js
I ran scenarios using debug=true

mw.config.set( 'wgRelatedArticlesLoggingSamplingRate', 1 )
 mw.config.set( 'wgRelatedArticlesEnabledSamplingRate', 0.9 )

To disable related pages for scenarios 3-5, rather than doing a SWAT, I am going to run the following code in the JS console:

mw.storage.session.set( 'mwuser-sessionId', 'a5')

where 'a5' is calculated by running:

var key, i = 0;
while ( !key ) {
  bucket = mw.experiments.getBucket( {
			name: 'ext.relatedArticles.visibility',
			enabled: true,
			buckets: {
				control: 0.9,
				A: 0.1
			}
		}, 'a' + i );
	if ( bucket !== 'control' ) {
		key = 'a' + i;
	} else {
		i++;
	}
}

Bummer. I can't test using the minerva.js method and we also probably shouldn't do a SWAT deploy as the sampling rate is 1 so it will not break our browser tests.

I've tested scenario 1-2 and they work fine but scenarios 3-5 cannot be tested on the beta cluster this way.
If we can wait, we'll be able to test this on group1 wikis (Hebrew Wikipedia) as early as Wednesday (since the patch hasn't ridden the train yet).

So I guess we're waiting till tomorrow on Hebrew wiki..

52311788.jpg (400×400 px, 84 KB)

@Jdlrobson - it seems to be running right now, feature disabled events are being logged only for enwiki, as of March 9

@Jdlrobson - it seems to be running right now, feature disabled events are being logged only for enwiki, as of March 9

@bmansurov's change was deployed to the group2 wikis on Thursday, 9th March so 👍