Page MenuHomePhabricator

Measure performance of cookie-based anonymous client preferences
Closed, ResolvedPublic5 Estimated Story PointsBUG REPORT

Description

In T321498, we added an inline script that allowed the reading of a cookie to toggle classes on the body tag, effectively disabling a "feature".

We would like to expand this for other features, allowing anonymous users to toggle font-size, dark mode and to remember states such as sidebar / table of contents collapsed. To do this the right way, we will be engaging in a TDMP around various available options in this space here. As part of this TDMP, however, we should measure the performance of our cookie-based approach, which is what this ticket is meant to handle. Note that actually analyzing these logs is out of scope for now.

Background

Current usage is like so:

mw.cookie.set( 'mwclientprefs', 'vector-feature-limited-width' );

which results in the toggling of 'vector-feature-limited-width-enabled' to 'vector-feature-limited-width-disabled'

Proposal

It's proposed that this be expanded to allow the disabling of multiple "features" and to provide this functionality to consumers.

mw.util.setFeature('feature-1', false)
mw.util.setFeature('feature-2', false)
mw.util.setFeature('feature-3', false)
mw.util.setFeature('feature-3', true)

Internally this would result in the following call:

mw.cookie.set( 'mwclientprefs', 'feature-1|feature-2' );

which would result in feature-1-enabled and feature-2-enabled class when present on the HTML tag to become feature-1-disabled and feature-2-disabled.

This would require:

  • A small modification to the existing code, to introduce a delimiter and a for loop to the existing code: https://gerrit.wikimedia.org/r/c/mediawiki/core/+/883243
  • Providing a utility method to wrap the setting of the cookie and capturing these requirements in a new public and stable method disableFeature
  • Constraining the utility method as necessary

TODO

  • Add tests that can measure the performance impact of the change on key frontend metrics.

Separately (not as part of this ticket but adding it here for reference)
We will be going through the TDMP process via a separate ticket and proposal, here. The purpose of this proposal is to define a general problem statement that could have multiple means of being addressed. One of these is a cookie-based approach as outlined in this ticket, but there may be others.

Next steps are to discuss this problem statement together with Architecture, Performance, and other interested feature teams, and then based on the feedback we receive, we will be implementing various prototypes to address these concerns.

Sign off steps

For now, the purpose of this ticket is to add tests that measure the performance impact of the multi-preference cookie approach and that we can run at a later date to compare with other implementations (if needed). If the tests have been added, we can close out this ticket.

Going through the TDMP with the ticket that we create will be the most immediate next step, but is outside the scope of this ticket. We will then evaluate the different options on the table via prototypes. Based on the approach we identify, we will then have a new approach for setting these preferences, at which time we should do the following:

  • We should implement the new API. (create a new task for doing that)
  • We should document and advertise the new API. (include that as A/C)

But, notably - most of the upcoming action will take place via the to-be-created TDMP ticket.

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

A prefs string that can contain multiple prefs seems clear + easy to extend. (while allowing logic elsewhere to limit which prefs are even looked at, if that has a performance impact)

Measuring performance: is this a separate issue? [I see it mentioned in the parent epic but w/ no #]

Measuring performance impact would be part of this change per the TODO " We should measure the performance impact of the change on key frontend metrics."

NBaca-WMF updated the task description. (Show Details)
LGoto set the point value for this task to 8.Mar 16 2023, 5:29 PM
Jdlrobson renamed this task from Support disabling multiple features in client preferences to Make a proposal for supportitheng disabling multiple features in client preferences.Mar 27 2023, 6:38 PM
Jdlrobson renamed this task from Make a proposal for supportitheng disabling multiple features in client preferences to Make a proposal for supporting the disabling of multiple features in client preferences.

Change 903835 had a related patch set uploaded (by Nray; author: Nray):

[operations/mediawiki-config@master] Update "United States" static page to facilitate synthetic testing of T331681

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

Change 903836 had a related patch set uploaded (by Nray; author: Nray):

[performance/mobile-synthetic-monitoring-tests@master] Add mobile user journey synthetic tests for multiple client preferences

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

We are going a slightly different direction with this proposal, and will formalize according to the template laid out in this doc. I can link to the issue shortly when it's available.

What will be different:
Using cookies in this manner is one possible solution to the general problem of client preference persistence, but not necessarily the only one. In moving toward using a TDMP for this process, we are moving to abstract away from the *solution* of "should we use cookies for this?" toward the more general *problem* of "how do we store preferences for anonymous users?".

For now I would say we should still measure potential performance impact, in that this will still be important for the prototype phase of the TDMP, but for now the proposal itself will be covered by a separate ticket.

Change 903835 merged by jenkins-bot:

[operations/mediawiki-config@master] Update "United States" static page to facilitate synthetic testing of T331681

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

Mentioned in SAL (#wikimedia-operations) [2023-03-29T20:05:46Z] <taavi@deploy2002> Started scap: Backport for [[gerrit:903835|Update "United States" static page to facilitate synthetic testing of T331681 (T331681)]]

Mentioned in SAL (#wikimedia-operations) [2023-03-29T20:07:16Z] <taavi@deploy2002> nray and taavi: Backport for [[gerrit:903835|Update "United States" static page to facilitate synthetic testing of T331681 (T331681)]] synced to the testservers: mwdebug2001.codfw.wmnet, mwdebug2002.codfw.wmnet, mwdebug1001.eqiad.wmnet, mwdebug1002.eqiad.wmnet

Mentioned in SAL (#wikimedia-operations) [2023-03-29T20:15:30Z] <taavi@deploy2002> Finished scap: Backport for [[gerrit:903835|Update "United States" static page to facilitate synthetic testing of T331681 (T331681)]] (duration: 09m 45s)

Change 903836 merged by jenkins-bot:

[performance/mobile-synthetic-monitoring-tests@master] Add mobile user journey synthetic tests for multiple client preferences

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

ovasileva changed the point value for this task from 8 to 5.Mar 30 2023, 4:42 PM

Change 904621 had a related patch set uploaded (by Nray; author: Nray):

[operations/mediawiki-config@master] Remove inline script from United States static page

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

Change 904621 merged by jenkins-bot:

[operations/mediawiki-config@master] Remove inline script from United States static page

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

Mentioned in SAL (#wikimedia-operations) [2023-03-30T20:11:12Z] <thcipriani@deploy2002> Started scap: Backport for [[gerrit:904621|Remove inline script from United States static page (T331681)]]

Mentioned in SAL (#wikimedia-operations) [2023-03-30T20:12:34Z] <thcipriani@deploy2002> nray and thcipriani: Backport for [[gerrit:904621|Remove inline script from United States static page (T331681)]] synced to the testservers: mwdebug2001.codfw.wmnet, mwdebug1001.eqiad.wmnet, mwdebug1002.eqiad.wmnet, mwdebug2002.codfw.wmnet

Mentioned in SAL (#wikimedia-operations) [2023-03-30T20:20:54Z] <thcipriani@deploy2002> Finished scap: Backport for [[gerrit:904621|Remove inline script from United States static page (T331681)]] (duration: 09m 42s)

nray subscribed.

Static pages have been deployed and tests have been merged. I'm not seeing anything on the graphs yet — I'm asking Peter about this now, but I'm leaving this ticket in code review so that someone else on the web team can become a bit more familiar with the synthetic testing process. I'm happy to meet and discuss these changes!

Data has started to roll in from the synthetic tests:

With this data, I think this ticket has met the acceptance criteria, but I'm leaving it here in code review so that I can go over this work with another member from the Web team

Jdlrobson updated Other Assignee, added: nray.

Change 904852 had a related patch set uploaded (by Nray; author: Nray):

[performance/mobile-synthetic-monitoring-tests@master] Set `useformat=desktop` query param for client preferences script

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

Change 904852 merged by jenkins-bot:

[performance/mobile-synthetic-monitoring-tests@master] Set `useformat=desktop` query param for client preferences script

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

Scheduled a meeting for this Wednesday with @Mabualruz and @Edtadros to go over the synthetic testing

NBaca-WMF renamed this task from Make a proposal for supporting the disabling of multiple features in client preferences to Measure performance of cookie-based anonymous client preferences.Apr 3 2023, 8:25 PM
NBaca-WMF updated the task description. (Show Details)
NBaca-WMF updated the task description. (Show Details)

I made some changes just now to reflect how this will fit into our Technical Decision Making Proposal process:

  • Updating title to "Measure performance of cookie-based anonymous client preferences" to cover current scope, which is measuring -- but not yet analyzing -- the performance of the cookie-based approach
  • Making a subtask of https://phabricator.wikimedia.org/T333878 , which is the TDMP phase at which we will be measuring the performance of various approaches. This better reflects that the cookie-based approach as currently outlined is one of multiple possible proposals in this space, and we will need to measure this as we measure these other approaches, then compare performance against the metrics we have identified.

Test wiki created on Patch demo by Jdlrobson using patch(es) linked to this task:
https://patchdemo.wmflabs.org/wikis/57794a59f1/w

Test wiki created on Patch demo by Jdlrobson using patch(es) linked to this task:
https://patchdemo.wmflabs.org/wikis/194cdc7871/w

Test wiki created on Patch demo by Jdlrobson using patch(es) linked to this task:
https://patchdemo.wmflabs.org/wikis/b593afb509/w

Change 905329 had a related patch set uploaded (by Nray; author: Nray):

[performance/mobile-synthetic-monitoring-tests@master] Turn page white before navigation in clientPreferences tests

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

Change 905329 merged by jenkins-bot:

[performance/mobile-synthetic-monitoring-tests@master] Turn page white before navigation in clientPreferences tests

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

Change 905769 had a related patch set uploaded (by Nray; author: Nray):

[operations/mediawiki-config@master] Add static mobile United_States page to facilitate synthetic testing of T331681

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

Change 905771 had a related patch set uploaded (by Nray; author: Nray):

[performance/mobile-synthetic-monitoring-tests@master] Revise mobile client preferences synthetic tests

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

Change 905777 had a related patch set uploaded (by Nray; author: Nray):

[performance/synthetic-monitoring-tests@master] Add desktop client preference user journey tests

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

Change 905769 merged by jenkins-bot:

[operations/mediawiki-config@master] Add static mobile United_States page to facilitate synthetic testing of T331681

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

Mentioned in SAL (#wikimedia-operations) [2023-04-05T21:00:40Z] <cjming@deploy2002> Started scap: Backport for [[gerrit:905769|Add static mobile United_States page to facilitate synthetic testing of T331681 (T331681)]]

Mentioned in SAL (#wikimedia-operations) [2023-04-05T21:02:06Z] <cjming@deploy2002> cjming and nray: Backport for [[gerrit:905769|Add static mobile United_States page to facilitate synthetic testing of T331681 (T331681)]] synced to the testservers: mwdebug1001.eqiad.wmnet, mwdebug1002.eqiad.wmnet, mwdebug2001.codfw.wmnet, mwdebug2002.codfw.wmnet

Mentioned in SAL (#wikimedia-operations) [2023-04-05T21:10:46Z] <cjming@deploy2002> Finished scap: Backport for [[gerrit:905769|Add static mobile United_States page to facilitate synthetic testing of T331681 (T331681)]] (duration: 10m 06s)

Change 905777 merged by jenkins-bot:

[performance/synthetic-monitoring-tests@master] Add desktop client preference user journey tests

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

Change 905771 merged by jenkins-bot:

[performance/mobile-synthetic-monitoring-tests@master] Revise mobile client preferences synthetic tests

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

Change 906597 had a related patch set uploaded (by Nray; author: Nray):

[performance/synthetic-monitoring-tests@master] Correct client preferences config

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

Change 906597 merged by jenkins-bot:

[performance/synthetic-monitoring-tests@master] Correct client preferences config

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

nray removed ovasileva as the assignee of this task.
nray added a subscriber: Mabualruz.

Great to have these! Signing off for now, as we've done the appropriate measurements. When we pick up other alternatives we can compare this output to other potential implementations as a next step, but we will have a different ticket for these.

Test wiki created on Patch demo by Jdlrobson using patch(es) linked to this task:
https://patchdemo.wmflabs.org/wikis/1165364832/w

Change 883243 abandoned by Jdlrobson:

[mediawiki/core@master] [POC] Support enabling/disabling multiple features in client preferences

Reason:

Superceded by https://gerrit.wikimedia.org/r/c/mediawiki/core/+/932819

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

Test wiki on Patch demo by Jdlrobson using patch(es) linked to this task was deleted:

https://patchdemo.wmflabs.org/wikis/57794a59f1/w/

Test wiki on Patch demo by Jdlrobson using patch(es) linked to this task was deleted:

https://patchdemo.wmflabs.org/wikis/194cdc7871/w/

Test wiki on Patch demo by Jdlrobson using patch(es) linked to this task was deleted:

https://patchdemo.wmflabs.org/wikis/b593afb509/w/