Skip to content

Conversation

@JohnVillalovos
Copy link
Member

@JohnVillalovos JohnVillalovos commented Feb 17, 2022

Some endpoints are not returning the Retry-After header when
rate-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 JohnVillalovos requested a review from nejch February 17, 2022 00:42
@codecov-commenter
Copy link

codecov-commenter commented Feb 17, 2022

Codecov Report

❌ Patch coverage is 50.00000% with 1 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.

Some endpoints are not returning the `Retry-After` header when
rate-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
@nejch
Copy link
Member

nejch commented Feb 21, 2022

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

def test_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 = []
for i in range(0, 20):
projects.append(gl.projects.create({"name": f"{str(i)}ok"}))
with pytest.raises(gitlab.GitlabCreateError) as e:
for i in range(20, 40):
projects.append(
gl.projects.create(
{"name": f"{str(i)}shouldfail"}, obey_rate_limit=False
)
)
assert "Retry later" in str(e.value)
settings.throttle_authenticated_api_enabled = False
settings.save()
[project.delete() for project in projects]
with endpoints that don't return retry-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 after yield to make it more reusable, e.g.)

@pytest.fixture
def rate_limit_settings(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()

    yield settings

    settings.throttle_authenticated_api_enabled = False
    settings.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

@nejch nejch merged commit 114958e into main Mar 10, 2022
@nejch nejch deleted the jlvillal/rate-limit branch March 10, 2022 03:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Default value for max_retries is too short to "clear" all rate limit windows

4 participants