Page MenuHomePhabricator

Performance review of TheWikipediaLibrary extension
Closed, ResolvedPublic

Description

Description

This extension checks to see if a logged in editor qualifies for access to The Wikipedia Library by checking their account age and global edit count. The first time they become eligible, it sends them a notification about it.

Preview environment

https://en.wikipedia.beta.wmflabs.org/wiki/Main_Page

This has been deployed to beta cluster. It is currently configured to notify editors when they reach 100 edits with an account that is at least one day old.

Which code to review

https://gerrit.wikimedia.org/r/admin/repos/mediawiki/extensions/TheWikipediaLibrary

Performance assessment

Please initiate the performance assessment by answering the below:

  • What work has been done to ensure the best possible performance of the feature?
    • None, will update with details when we have done so.
  • What are likely to be the weak areas (e.g. bottlenecks) of the code in terms of performance?
    • We're doing an extra read + write to global preferences on save in a pretty common case. This was originally sending duplicate notifications on beta do to silent failures to read/write global prefs, and this workaround ensures that we only send if we're really sure that globalprefs is working correctly.
  • Are there potential optimisations that haven't been performed yet?
    • We could potentially do a more efficient globalprefs check if we were ever able to reproduce the duplicate notifications outside of beta
  • Please list which performance measurements are in place for the feature and/or what you've measured ad-hoc so far. If you are unsure what to measure, ask the Performance Team for advice: performance-team@wikimedia.org.
    • We have emailed the performance team as recommended, but have not heard back yet

Event Timeline

Aklapper changed the task status from Open to Stalled.Mar 5 2021, 11:22 AM

@jsn.sherman Setting task status to stalled as title says "Placeholder". Please reset task status to open and edit task summary, once this task is ready. Thanks.

Hi all!

Please see https://www.mediawiki.org/wiki/Writing_an_extension_for_deployment for the list of things to do to get a new extension deployed to WMF production.

Hi all!

Please see https://www.mediawiki.org/wiki/Writing_an_extension_for_deployment for the list of things to do to get a new extension deployed to WMF production.

I took this to mean that we haven't done all the steps we should have done before requesting a performance review, so having read through, here are the things in this list that we don't have, as far as I can tell:

  • puppet role for mediawiki-vagrant - that seems quite straightforward, but optional. If this needs done, I'll absolutely get on it
  • a user guide - is this a thing we should actually have since this sends a one-time notification, with no other user interaction?
  • a separate project on phabricator - This seems like it would result in a mostly inactive component project because of the nature of the extension - could we just reuse our existing phab group and one of its components?

Is there an easier way to test this on Beta than making 100 edits manually?

The link to the code in the task description is broken for me: https://gerrit.wikimedia.org/r/mediawiki/extensions/TheWikipediaLibrary (Not found)

In terms of what to measure, I presume that the notification is a standard Echo one? How/when is it created?

Is there an easier way to test this on Beta than making 100 edits manually?

Not to my knowledge. We could lower the edit threshold, though that would mean a lot more folks would get the notification. I'm very open to suggestions on how best to make this less of a chore.

The link to the code in the task description is broken for me: https://gerrit.wikimedia.org/r/mediawiki/extensions/TheWikipediaLibrary (Not found)

Updated task description

In terms of what to measure, I presume that the notification is a standard Echo one?

Yes, it creates a notification with the following attributes:

'canNotifyAgent' => true,
'category' => 'system',
'group' => 'positive',
'section' => 'message',

How/when is it created?
We fire the message in the PageSaveComplete hook if the editor is eligible.

I took this to mean that we haven't done all the steps we should have done before requesting a performance review.

Mostly just generally and I can't find a tracking task that has all of these as blockers so it was hard for me to verify quickly :)

Also, another call out is the listing of the extension on the Dev/Maintainers page for stewardship of the extension in production.

Mostly just generally and I can't find a tracking task that has all of these as blockers so it was hard for me to verify quickly :)

T132084 is our tracking task, I'll go make sure we have all those blockers (like security review, etc) to it. Sorry for making life difficult!

Also, another call out is the listing of the extension on the Dev/Maintainers page for stewardship of the extension in production.

👍 Thanks!

I've looked at the code where the notification is created. Since you're correctly doing this in a DeferredUpdate, there shouldn't be any measurable impact on Save Timing. I don't think this extension required new metrics to be measured.

The icon (twl-eligible.svg) can be optimised. Running it through ImageOptim I see at least 15% possible savings on file weight.

@Gilles thanks for that feedback! I don't have access to ImageOptim specifically, but I was able to trim that image down by about 15% as you suggested and opened up a change request for it (T283527).

@greg I think we've got the tracking task (T132084) and dev/maintainers page populated with all the relevant info we have now.

jsn.sherman renamed this task from Placeholder - Performance review of TheWikipediaLibrary extension to Performance review of TheWikipediaLibrary extension.Wed, May 26, 2:44 PM

Okay, we're just waiting on a +2 on that change request.
We have a few other blocking tasks that are not in the extension itself that we want to take care of before deploying and sending the notifications, but we've done all the stuff that we know needs done on the extension. @Gilles, is there anything else we need to do to move through the performance review gate?

Gilles claimed this task.

No, that's all I could find. It's not a gate, by the way, it's just best practice to have it done before deploying to production. Thank you!