Page MenuHomePhabricator

[SUBTASK] RelatedArticlesEnabledSamplingRate breaks RelatedArticles browser tests
Closed, ResolvedPublic

Description

In T156039 we enable RelatedArticles for only some subset of users but browser tests do not verify RelatedArticlesEnabledSamplingRate configuration value.

As a result, some browser tests fail because RelatedArticles feature is not visible due to default 0.9 sampling rate (they get bucketed as 0.1 - do not show RelatedArticles feature)

For development purposes you can replicate this by setting $wgRelatedArticlesEnabledSamplingRate to 0 and opting into the beta feature and viewing on desktop.

Event Timeline

pmiazga created this task.Feb 6 2017, 7:26 PM
Restricted Application added a subscriber: Aklapper. Β· View Herald TranscriptFeb 6 2017, 7:26 PM
pmiazga triaged this task as High priority.Feb 6 2017, 7:26 PM
pmiazga updated the task description. (Show Details)Feb 6 2017, 7:28 PM

Change 336348 had a related patch set uploaded (by Jdlrobson):
Always show RelatedArticles to users who opted into beta feature

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

Change 336554 had a related patch set uploaded (by Jdlrobson):
Always show RelatedArticles to users who opted into beta feature

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

No SP needed as this is a subtask πŸ‘Œ

Change 336554 merged by jenkins-bot:
Always show RelatedArticles to users who opted into beta feature

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

(β•―Β°β–‘Β°οΌ‰β•―οΈ΅ ┻━┻

To unconfuse you I can explain what happened here :)

  • I wrote patch 1. While doing that I was reminded of T146436 and felt this would make the patch simpler.
  • Baha didn't like the approach so I wrote a patch without that dependency with the line I was trying to avoid which seemed like a bad code smell to me:
!class_exists( 'BetaFeatures' ) || ( class_exists( 'BetaFeatures' ) && self::isUserOptedIntoBetaFeature( $user ) );
  • You merged the 2nd patch thus removing the dependency on T146436
  • Thus, this work is complete.

Change 336348 abandoned by Jdlrobson:
Always show RelatedArticles to users who opted into beta feature

Reason:
https://gerrit.wikimedia.org/r/336554 has been merged

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

MBinder_WMF renamed this task from RelatedArticlesEnabledSamplingRate breaks RelatedArticles browser tests to [SUBTASK] RelatedArticlesEnabledSamplingRate breaks RelatedArticles browser tests.Feb 8 2017, 6:13 PM
bmansurov removed bmansurov as the assignee of this task.EditedFeb 8 2017, 9:41 PM
bmansurov added a subscriber: bmansurov.

Tests #302 through #304 are passing. Strange given the patch has been merged today. (The browser tests have been updated in the parent task.)

I set $wgRelatedArticlesEnabledSamplingRate = 0; locally and observed that I could still see related articles in Vector when I was opted into the beta feature. However, I also was able to see related articles in Minerva, which I should not have been to as the enabled sampling rate was set to 0 and Minerva doesn't care about BetaFeatures. Only when I disabled the beta feature by switching to Vector, I was not able to see related articles in Minerva.

I think this task needs more work as visibility of related articles in Minerva should only depend on the config variable above not and on the value of beta features.

@bmansurov Thanks for pointing out the Minerva situation... yes if you've opted into the beta feature on desktop then you will bypass the bucketing on the mobile site. This wasn't specified and seemed the correct behaviour to me as to bucketing those users would lead to even more technical debt similar to that mentioned in T146436. We'd have to add another if skin == 'minerva' line.

@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.

Back to browser tests:
I should point out the browser test builds will still fail 10% of the time if the user is bucketed into the situation where related pages is disabled. This will continue until we drop the 90% roll out from English Wikipedia or update the beta cluster to not respect bucketing. This will add a little noise to our browser tests which I think is fine if it's only expected for a 2 week period.

If we want to SWAT a config change to remove the temporary 10% failure rate we should consider this closed otherwise we should move back to "needs more work".

As for browser tests, can we not set the value of wgRelatedArticlesEnabledSamplingRate to 1 in LocalSettings.php that's inside the browser tests' folder?

Change 336732 had a related patch set uploaded (by Jdlrobson):
Beta cluster should show related pages 100% of time

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

It defaults to 1. LocalSettings.php is not run on the beta cluster for security reasons. I'll see if I can get the above patch swatted.

Change 336732 merged by jenkins-bot:
Beta cluster should show related pages 100% of time

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

β€’ Jdlrobson closed this task as Resolved.Feb 9 2017, 12:54 AM
β€’ Jdlrobson claimed this task.

The change has been SWATed so the beta cluster will now always show related pages. I've done some testing and can verify mw.config.values.wgRelatedArticlesEnabledSamplingRate is always 1 on the beta cluster.
I've tracked the issue with sampling in https://phabricator.wikimedia.org/T157375#3011769 which I consider out of the scope for getting our browser tests passing again.

phuedx reopened this task as Open.Feb 9 2017, 6:54 AM

I think we've knowingly introduced a regression in the behaviour of Related Articles – for my part, I should've spotted this during code review and testing – so I agree with @bmansurov that this needs more work.

@ovasileva: To reiterate @Jdlrobson's question:

[If] a logged in user [has] 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?

@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.

No, it's such a small subset of users, it will be fine. Question though - will we track these users as "off" in the schema?

phuedx closed this task as Resolved.Feb 9 2017, 2:31 PM

Per T157372#3012653.

No sampling will take place on either the desktop or mobile site if the user has enabled the Related Pages beta feature.