Page MenuHomePhabricator

Performance review of WikimediaApiPortalOAuth extension
Open, MediumPublic



We are developing a publicly accessible API portal. The work is described by the API Gateway documentation plan.

As part of this project we will be launching a new wiki on which we plan to use the WikimediaApiPortalOAuth extension.

Preview environment

You can preview the software at which is the Beta API Portal wiki.

@apaskulin can facilitate account creation as needed.

Which code to review

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptJun 9 2020, 10:33 PM
Gilles claimed this task.Jun 15 2020, 7:56 PM
Gilles moved this task from Inbox to Backlog: Future Goals on the Performance-Team board.
Gilles changed the task status from Open to Stalled.Jun 17 2020, 7:00 PM

Please change the status back to open once it's ready for review. Thanks!

Thanks, will do! Per our status meeting on Tuesday, they're almost ready to go.

WDoranWMF changed the task status from Stalled to Open.Jun 25 2020, 2:49 PM
WDoranWMF added a subscriber: WDoranWMF.

@Gilles Changing status to open as this is ready for review. Thanks!

@WDoranWMF can the task description be udpated with a link to the code?

The only repo I can find is empty:

Gilles triaged this task as Medium priority.Jun 29 2020, 11:40 AM
WDoranWMF updated the task description. (Show Details)Jun 29 2020, 6:56 PM

@Gilles Updated the description the patch is still in review, we wanted to get perf review there to make changes before merging. Is that acceptable?

Gilles added a comment.EditedAug 6 2020, 9:07 AM

Nevermind my old comment, I didn't realise the bulk of it was the lock file.

Gilles added a comment.Aug 6 2020, 9:29 AM

Looking at the code, I don't see anything concerning for performance, this seems like fairly simple CRUD stuff built with OOUI. Where can I see it running on the demo portal? Do I need to log in?

@Gilles, the extension won't be running on the beta site until the patch is merged

Gilles added a comment.Aug 6 2020, 3:54 PM

OK! Please ping me here when that's the case

Gilles changed the task status from Open to Stalled.Sep 11 2020, 7:44 AM

Marking this as stalled for my own tracking of performance reviews that are currently actionable for us. Please move this back to "open" once it's running on Beta. Thanks!

WDoranWMF changed the task status from Stalled to Open.Oct 5 2020, 4:48 PM
WDoranWMF updated the task description. (Show Details)

@Gilles Thanks so much for bearing with us, the final version of the code is merged and it's up on Beta API Portal wiki. Let us know what is needed from us, if we've elided or missed important details to included in this review request.

I can't see to be able to access any page on the wiki, I get this error message:

My username on Beta is "GDubuc (WMF)"

@Gilles Could you try again? @CCicalese_WMF has upgraded the user.

Gilles added a comment.Oct 6 2020, 6:41 PM

It works now, thanks

Gilles added a comment.Oct 9 2020, 1:59 PM

I've noticed a very minor thing when testing the site out for performance. There's a 20kb image that's requested on every page and never displayed:

I suppose it comes from some CSS (as the default site logo does), as I can't find it in the HTML. There's probably some other CSS from the skin to hide the original logo, but not in a way that stops it from loading.

Other than that, the site behaves fine without JS. It seems like it's not loading any superfluous JS/CSS. In my next pass I'll take a new dive in the code.

Thank you so much, @Gilles. Will check into the image issue.

@Gilles we have identified two possible solutions to the logo issue.

  1. Fix in core with a supporting patch in the WikimediaApiPortal skin: and
  2. Fix in WikimediaApiPortal skin:

We would appreciate your review on the core patch in order to determine which of those two approaches to take.

Thank you.

T265507: 20kb project logo image requested on every page (betawiki.png) has been researched and resolved. Thanks, Cindy and Gilles!

Gilles added a comment.EditedNov 4 2020, 10:11 AM

Going to the Special:AppManagement page, I see that a progress bar appears immediately and for a very short time:

It would be preferable for a delay to be applied before the progress bar appears, as the common case will be for the content to appear quickly. Progress placeholders are useful when the expected delay is long (eg. an expensive search query that takes 5-10 seconds to complete). But for a process that is fast in the common case, they needlessly draw attention to the wait if displayed immediately, which actually gives an impression of slowness to users with fast internet connections. Simply waiting for 2-3 seconds before displaying a progress indicator would be the best compromise here.

On the topic of that progress bar, it also draws attention when it disappears, because it causes a layout shift (its vertical space is no longer used). It would be preferable for it to be placed differently so that no layout jump occurs (at least in this empty case, which is the default experience).

I was unable to create a client on that page, I get an error when I try, maybe a privileges issue still?

Anyway, what I've just described is a best practice advice and it's only affecting the special page, so not a high priority issue.

A similar loading placeholder shown immediately is seen on the Notifications page, but I assume it's coming from the skin and not something that was made as part of this project:

Aside from this, I went through all the code and didn't see anything noteworthy.

Thanks, @Gilles!

I've opened T267241 to address the issues with the progress bar on Special:AppManagement. For the progress bar on the Notifications page, I'm seeing the same behavior on, so that's probably coming from the Echo extension.

I was unable to create a client on that page, I get an error when I try, maybe a privileges issue still?

Ah yes, that's a known issue (T264637). If you log in to and refresh Special:AppManagement, you should be able to create a client.

Gilles added a comment.EditedNov 5 2020, 12:46 PM

I was actually already logged into Beta Meta. I logged out and logged back in just in case, but it didn't help.

I've just looked at the error code returned by the API call and it's more explicit: email_not_confirmed.

Maybe the user-facing error could be more precise? As it stands it suggests that trying again later might fix it, when it clearly won't if the issue is that my email address isn't confirmed:

As for the email_not_confirmed error code, it contradicts what I see in my user preferences:

Interesting. If the issue is an unconfirmed email, Special:AppManagement should prompt you to confirm your email before showing you the "Create client" button. This could be an issue with email verification on beta specifically.

Gilles added a comment.Thu, Nov 5, 8:35 PM

FYI, it doesn't:

Thanks, @Reedy. I wasn't able to reproduce this error, but I've added your experience to T267429 to continue troubleshooting. Just to confirm, did you verify that you were logged into before attempting to create a client?

Reedy added a comment.Mon, Nov 9, 6:43 PM

I didn’t. I’ll have a poke further when I get back home later

Reedy added a comment.Mon, Nov 9, 6:49 PM

Though the other thing is visiting it will automatically log you in too

There's a known issue (T264637) where we're getting errors when trying to create clients because logging into the beta portal doesn't automatically log in to beta Meta. If you make sure you're logged in to beta Meta, then refresh Special:AppManagement in the beta portal, it should allow you to create a client. (Or rather, that's the behavior I've observed.) Let me know if that fixes it for you.

Reedy added a comment.EditedMon, Nov 9, 8:57 PM

So visit, one request seemingly happens on load

Then try and create a client, get the POST error, and a second CORS error

Go to beta meta, wait for the magic autologin to take place... Refresh the meta page Refresh the api portal page... Try again and it's all good

So yeah, T264637 seems applicable. The CORS errors look odd at first glance, but I guess it actually makes sense, as beta meta is behaving like the user isn't logged in (which they're technically not on the remote wiki), and as such, won't allow the CORS requests to be done

As per Gilles, it does seem like some better/more specific error handling could be useful now and in future