Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Translation] Make http requests synchronous when reading the Loco API #44416

Merged
merged 1 commit into from Dec 6, 2021
Merged

[Translation] Make http requests synchronous when reading the Loco API #44416

merged 1 commit into from Dec 6, 2021

Conversation

Kocal
Copy link
Contributor

@Kocal Kocal commented Dec 2, 2021

Q A
Branch? 5.3
Bug fix? yes
New feature? no
Deprecations? no
Tickets Fix #...
License MIT
Doc PR symfony/symfony-docs#...

Hi!

Since we migrated from the PHP Translation's Loco Adapter to the Symfony's Loco Translation Provider, we started to have 429 errors from the Loco API when running our CI or deploying our code on review apps/staging/production environments:

Some screenshots

image

image

We have asked to the Loco support and this is their response:

Hello.

You will never get a 429 if you make single threaded requests. i.e. waiting for a response before dispatching another requests. As noted in the API usage limits: parallel requests burst the response speed limit. This is currently the only thing that would invoke a 429. I can provide more detail on the implementation if you would like.

This measure is in lieu of quota based rate limiting which will one day be added, but currently no other limits are in place in terms of number of requests.

Which makes sense. AFAIK, the PHP Translation's Loco Adapter makes synchronous requests to get translations from Loco.

Some Blackfire traces, it does not impact performances:

I consider this PR as a bug-fix and not a feature, that's why I've targeted 5.3.

WDYT?

ping @welcoMattic

@carsonbot carsonbot added this to the 5.3 milestone Dec 2, 2021
@carsonbot carsonbot changed the title [Translation][Loco] Make http requests synchronous when reading the Loco API [Translation] [Loco] Make http requests synchronous when reading the Loco API Dec 2, 2021
@Kocal
Copy link
Contributor Author

Kocal commented Dec 2, 2021

Checks failures are unrelated I think.

Copy link
Member

@welcoMattic welcoMattic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is legitimate, as you profiled it and there is no big difference I'm ok to merge it. Thank you @Kocal

@nicolas-grekas
Copy link
Member

nicolas-grekas commented Dec 2, 2021

BTW, is anyone in touch with Loco, because so far they didn't answer to our ping to back the bridge.

(Crowdin and Lokalise did, so you know which bridge you'd better use ;) )

@Kocal
Copy link
Contributor Author

Kocal commented Dec 2, 2021

The two foreach have been merged!

@nicolas-grekas nicolas-grekas changed the title [Translation] [Loco] Make http requests synchronous when reading the Loco API [Translation] Make http requests synchronous when reading the Loco API Dec 2, 2021
@stof
Copy link
Member

stof commented Dec 2, 2021

I suggest adding a comment in the code mentioning that the Loco API forbids concurrent requests, to explain why this provider does not take advantage of the request concurrency allowed by http-client (so that it does not get refactored in the opposite way by someone not knowing about this API limitation).

@Kern046
Copy link
Contributor

Kern046 commented Dec 2, 2021

BTW, is anyone in touch with Loco, because so far they didn't answer to our ping to back the bridge.

They answered us quite quickly about this issue, I contacted them via their contact form 🤷

@Kocal
Copy link
Contributor Author

Kocal commented Dec 3, 2021

A comment has been added, thanks for your reviews.

@fabpot
Copy link
Member

fabpot commented Dec 6, 2021

Thank you @Kocal.

@fabpot fabpot merged commit e4870b0 into symfony:5.3 Dec 6, 2021
@Kocal Kocal deleted the fix/translation-loco-read-sync-http-requests branch December 6, 2021 13:21
This was referenced Dec 9, 2021
@fabpot fabpot mentioned this pull request Dec 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants