mvisonneau / gitlab-ci-pipelines-exporter

Prometheus / OpenMetrics exporter for GitLab CI pipelines insights
Apache License 2.0
1.21k stars 239 forks source link

ratelimit: redis should retry if allowed requests exceeded #789

Closed bkylerussell closed 4 months ago

bkylerussell commented 4 months ago

Implements retry on Take() for NewRedisLimiter, and now correctly logs debug message when throttling occurs.

When configured to use redis, ratelimiting has always been least effort. While Take() may sleep for the requested RetryAfter time, NewRedisLimiter never re-checks to see if we're actually allowed to make further requests after the RetryAfter time expires. By re-checking Allow(), this actually enforces the desired ratelimit when multiple routines are scheduled simultaneously.

This is not an issue with NewLocalLimiter since Limiter.Wait() will block until an event is allowed (or error with log.Fatal(), terminating the application).

Additionally, the "throttled GitLab requests" message was never useful under its previous conditions. Both NewRedisLimiter and NewLocalLimiter are calculating start.Sub(time.Now()) from the end of the function, which always returns a negative Duration, meaning the condition was never satisfied.

Since NewLocalLimiter is already throttling correctly via Wait(), and since any non-successful return from Limiter.Wait() terminates the application via log.Fatal(); this means this "throttled" log message is likely only useful in NewRedisLimiter (when it retries).

mvisonneau commented 4 months ago

Thanks a ton for this fix @bkylerussell! Do you mind looking into the failing tests?

mvisonneau commented 4 months ago

thanks again!