NotificationsYou must be signed in to change notification settings
Fork676
Star2.4k

fix: support RateLimit-Reset header#1895

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

nejch merged 1 commit intomainfromjlvillal/rate-limit

Mar 10, 2022

Merged

fix: support RateLimit-Reset header#1895

nejch merged 1 commit intomainfromjlvillal/rate-limit

Mar 10, 2022

Conversation

Copy link

Member

JohnVillalovos commentedFeb 17, 2022•
edited
Loading

Some endpoints are not returning theRetry-After header when
rate-limiting occurrs. In those cases use theRateLimit-Reset [1]
header, if available.

Closes:#1889

[1]https://docs.gitlab.com/ee/user/admin_area/settings/user_and_ip_rate_limits.html#response-headers

JohnVillalovos requested a review fromnejch

February 17, 2022 00:42

Copy link

codecov-commenter commentedFeb 17, 2022•
edited by codecovbot
Loading

Codecov Report

❌ Patch coverage is50.00000% with1 line in your changes missing coverage. Please review.
✅ Project coverage is 92.53%. Comparing base (85a734f) to head (4060146).
⚠️ Report is 1117 commits behind head on main.

Files with missing lines	Patch %	Lines
gitlab/client.py	50.00%	1 Missing⚠️

Additional details and impacted files

@@            Coverage Diff             @@##             main    #1895      +/-   ##==========================================- Coverage   92.55%   92.53%   -0.02%==========================================  Files          78       78                Lines        4889     4891       +2     ==========================================+ Hits         4525     4526       +1- Misses        364      365       +1

Flag	Coverage Δ
cli_func_v4	`81.68% <0.00%> (-0.04%)`	⬇️
py_func_v4	`80.20% <0.00%> (-0.04%)`	⬇️
unit	`83.41% <50.00%> (-0.02%)`	⬇️

Flags with carried forward coverage won't be shown.Click here to find out more.

Files with missing lines	Coverage Δ
gitlab/client.py	`90.36% <50.00%> (-0.20%)`	⬇️

🚀 New features to boost your workflow:

❄️Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

fix: support RateLimit-Reset header

4060146

Some endpoints are not returning the `Retry-After` header whenrate-limiting occurrs. In those cases use the `RateLimit-Reset` [1]header, if available.Closes:#1889[1]https://docs.gitlab.com/ee/user/admin_area/settings/user_and_ip_rate_limits.html#response-headers

JohnVillalovos force-pushed thejlvillal/rate-limit branch from65c7e62 to4060146Compare

February 17, 2022 01:25

Copy link

Member

nejch commentedFeb 21, 2022

Thanks@JohnVillalovos! Would it make sense to add another case to

python-gitlab/tests/functional/api/test_gitlab.py

Lines 160 to 183 in4cb7d92

	deftest_rate_limits(gl):
	settings=gl.settings.get()
	settings.throttle_authenticated_api_enabled=True
	settings.throttle_authenticated_api_requests_per_period=1
	settings.throttle_authenticated_api_period_in_seconds=3
	settings.save()

	projects= []
	foriinrange(0,20):
	projects.append(gl.projects.create({"name":f"{str(i)}ok"}))

	withpytest.raises(gitlab.GitlabCreateError)ase:
	foriinrange(20,40):
	projects.append(
	gl.projects.create(
	{"name":f"{str(i)}shouldfail"},obey_rate_limit=False
	)
	)

	assert"Retry later"instr(e.value)

	settings.throttle_authenticated_api_enabled=False
	settings.save()
	[project.delete()forprojectinprojects]

with endpoints that don't returnretry-after, e.g.user as mentioned in the issue? Although this might need 14.8 and not sure if it's enabled by default. Just wondering.https://docs.gitlab.com/ee/user/admin_area/settings/rate_limit_on_users_api.html#rate-limits-on-users-api

(If we do this, I would maybe turn the settings into a fixture to use in both tests, and do the cleanup afteryield to make it more reusable, e.g.)

@pytest.fixturedefrate_limit_settings(gl):settings=gl.settings.get()settings.throttle_authenticated_api_enabled=Truesettings.throttle_authenticated_api_requests_per_period=1settings.throttle_authenticated_api_period_in_seconds=3settings.save()yieldsettingssettings.throttle_authenticated_api_enabled=Falsesettings.save()

PS: Interesting, looks like these headers are also useful to avoid 429s altogether, looking at go-gitlab with some fancy rate-limiting code:xanzy/go-gitlab@d8bb0b4