Page MenuHomePhabricator

ULS makes too many action=ulslocalization API calls
Closed, ResolvedPublic

Description

action=ulslocalization is the most common API request on the cluster at the moment, amounting to ~41% of all requests. You shouldn't call the API on every page load. Find an alternative to using the API and/or don't load the language localization data until a user clicks on the cog icon.

Please get in touch with Greg G. to schedule a deployment for a fix -- this should be done ASAP.


Version: unspecified
Severity: major

Details

Reference
bz56509

Event Timeline

bzimport raised the priority of this task from to Unbreak Now!.Nov 22 2014, 2:32 AM
bzimport set Reference to bz56509.
bzimport added a subscriber: Unknown Object (MLST).
ori created this task.Nov 2 2013, 8:36 AM

A few comments:

  1. UI strings are needed for the tooltip of the clickable elements, so (some) strings must be available.
  2. I thought this was cached <somehow>. If not, should be.

Question:

  1. Is this new behavior, or has this behavior changed recently? If it is not new, it should have been like this since we implemented L10n loading through the API instead if directly loading the JSON.

Removed "see also" to bug 50391, as that was duplicate requests in the same page load. This is not about that. This issue is about repeated requests, where repeated loads of the same data between page view should probably be avoided.

I also notice that the "action=ulslocalization" API call goes to the site (nl.wikipedia.org) instead of bits.wikimedia.org. Not sure is we can/should make that call to come from bits.

(In reply to comment #2)

I also notice that the "action=ulslocalization" API call goes to the site
(nl.wikipedia.org) instead of bits.wikimedia.org. Not sure is we can/should
make that call to come from bits.

bits.wikimedia.org can't serve API calls.

brion added a comment.Nov 2 2013, 11:40 AM

Couple quick drive-by notes:

  • If most page views only need one tooltip, that should probably be bundled as a message in the main ResourceLoader module...
  • Can the whole thing be redone as a ResourceLoader module (which *would* be served from bits), or is the format of the API calls locked in in some way that prevents that? It looks like it doesn't rely on anything user- or session-specific.
  • I do see some public caching headers being set in the source:
		$this->getMain()->setCacheMode( 'public' );
		$this->getMain()->setCacheMaxAge( 300 );

however I don't know if all those make it through to the outside.

(In reply to comment #1)

  1. Is this new behavior, or has this behavior changed recently? If it is not

new, it should have been like this since we implemented L10n loading through
the API instead if directly loading the JSON.

I don't think it's new. I pointed it out before on other bugs about ULS being slow, but no one was bothered.

(In particular I pointed it out in bug 49935 comment 17.)

Please read bug 41489 for more information why we ended up with the current solution.

Points to consider:

  • In jquery.uls we cannot have dependencies on MediaWiki components.
  • In jquery.uls (via jquery.i18n) we have a custom, json-based i18n file format.
  • Serving that file directly from bits is not possible because of cross-origin restrictions.
  • We are not going to change the file format from json to anything executable.
  • We implemented API module which just servers the files as-is with some caching.
  • The API module does not have version based cache invalidation.
  • Doing this via resource loader module would mean that we need to wrap or reformat the json data into JavaScript on the fly and implement support for that in jquery.i18n.

It's not obvious to me which action(s) should be taken here. Some options:

  1. Delay the api call until ULS trigger is clicked. This needs some refactoring moving the trigger related tooltip message*s* from json to i18n.php file.
  2. Improve the caching of the api call
  3. Use resource loader. I currently have no idea how this would look in practice.

Hopefully we will have clarity on this on Monday. Have a great weekend.

(In reply to comment #6)

(In particular I pointed it out in bug 49935 comment 17.)

This is a different thing, and like stated on that bug, the code you pointed out is not executed in our environment. Instead of "I told you so" I'd like to see us all work together to design and implement a good solution.

(In reply to comment #9)

This is a different thing, and like stated on that bug, the code you pointed
out is not executed in our environment.

It is not now. It used to be until I pointed it out.

I did not mean to be all "told you so", I just pointed to an earlier discussion about the same topic (we're now talking partially about the code written to fix that bug, as far as I understand it).

(In reply to comment #8)

  • In jquery.uls we cannot have dependencies on MediaWiki components.

I'll dare to say that yes, you can and should if it means making the performance more acceptable, even if it means maintaining a fork. (And to be honest, the division of ULS into various modules has looked artificial to me ever since I learned it exists.)

  • Serving that file directly from bits is not possible because of

cross-origin restrictions.

Can't you serve it as JSONP? Can't you have CORS set up so that the restrictions go away?

ori added a comment.Nov 2 2013, 6:28 PM

(In reply to comment #8)

Please read bug 41489 for more information why we ended up with the current
solution.
Points to consider:

  • In jquery.uls we cannot have dependencies on MediaWiki components.
  • In jquery.uls (via jquery.i18n) we have a custom, json-based i18n file

format.

  • Serving that file directly from bits is not possible because of

cross-origin
restrictions.

  • We are not going to change the file format from json to anything

executable.

  • We implemented API module which just servers the files as-is with some

caching.

  • The API module does not have version based cache invalidation.
  • Doing this via resource loader module would mean that we need to wrap or

reformat the json data into JavaScript on the fly and implement support for
that in jquery.i18n.

This is what EventLogging does (schemas are JSON); see includes/ResourceLoaderSchemaModule.php in the EventLogging repository. JSON is already JavaScript, so to convert a JSON blob into something executable all you need to do is to wrap it in a function invocation. See the getScript module in that file.

It's not obvious to me which action(s) should be taken here. Some options:

  1. Delay the api call until ULS trigger is clicked. This needs some

refactoring moving the trigger related tooltip message*s* from json to
i18n.php file.

  • Wrap the code in an RL module, a la EventLogging.
  • Defer loading until the page has finished loading OR the hover event on the cog is triggered. You don't have to wait for a user click, but you don't have to block the page either.
  1. Improve the caching of the api call

Not an option, IMO. I don't have hard numbers, but if I recall correctly Yahoo! found that 30-40% of requests were always uncached, even with aggressive caching, presumably due to people resetting their browsers, using public kiosks, etc. A full HTTP request is really expensive. If you use ResourceLoader, you can fetch the JSON data and JS / CSS resources in a single request (which I think we'll want to do -- it's not just the messages that can be deferred but some of the JS/CSS too), and you'll benefit from localStorage when that gets rolled out.

  1. Use resource loader. I currently have no idea how this would look in

practice.

Something like this:

class JsonMessagesModule extends ResourceLoaderModule {

		function getDependencies() {
			return array( 'ext.uls.moduleWithInitFunc' );
		}

		function getModifiedTime( ResourceLoaderContext $context ) {
			return filemtime('/path/to/json');
		}

		function getScript( ResourceLoaderContext $context ) {
			$json = file_get_contents('/path/to/json');
			return Xml::encodeJsCall( 'ext.uls.initFunc', array( $json ) );
		}

}

Hopefully we will have clarity on this on Monday. Have a great weekend.

Yep! I think this is very solvable, so no sweat. Have a great weekend all.

ori added a comment.Nov 2 2013, 6:41 PM

(In reply to comment #4)

$this->getMain()->setCacheMode( 'public' );
$this->getMain()->setCacheMaxAge( 300 );

As a quick fix, this should be set to 31536000 (one year), not 300 (five minutes). Don't worry about invalidation since we'll move this to ResourceLoader anyway.

Change 93175 had a related patch set uploaded by Ori.livneh:
Set max-age to 2419200 (one month) for action=ulslocalization API calls

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

Change 93175 merged by jenkins-bot:
Set max-age to 2419200 (one month) for action=ulslocalization API calls

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

Change 93447 had a related patch set uploaded by Nikerabbit:
ResourceLoader Module for serving json based localization messages

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

Change 93447 merged by jenkins-bot:
ResourceLoader Module for serving json based localization messages

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

Given comment 16, what's the status of this bug?

The above patch was merged and already deployed in mediawiki.org. Does not show any api hits.

Change 95484 had a related patch set uploaded by Ori.livneh:
Update UniversalLanguageSelector to b2f9e4211e

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

Change 95484 merged by Ori.livneh:
Update UniversalLanguageSelector to b2f9e4211e

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