- Notifications
You must be signed in to change notification settings - Fork1k
chore: drop github per user rate limit tracking#12286
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
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Rate limits for authenticated requests are per user.This would be an excessive number of prometheus labels,so we only track the unauthorized limit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I think this makes sense, and I'm down for dropping unnecessary prom metrics 👍
Just need to make CI happy.
Forgot to fix the tests for the rate limits. On it 👍 |
| # TYPE coderd_oauth2_external_requests_rate_limit_total gauge | ||
| coderd_oauth2_external_requests_rate_limit_total{name="primary-github",resource="core"} 5000 | ||
| coderd_oauth2_external_requests_rate_limit_total{name="secondary-github",resource="core"} 5000 | ||
| coderd_oauth2_external_requests_rate_limit_total{name="primary-github",resource="core-unauthorized"} 5000 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Should this be 60 instead of 5000?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
It should be, but it's only used to generate docs. The values are ignored, but the labels are included.
https://coder.com/docs/v2/latest/admin/prometheus#prometheus-configuration
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
So it is fine
Uh oh!
There was an error while loading.Please reload this page.
Rate limits for authenticated requests are per user. This would be an excessive number of prometheus labels, so we only track the unauthorized limit.
The rate limit labels without a user label make no sense.
Closes:#10853
Unauthenticated Rate Limit
We used to hit the unauthenticated rate limit as our ValidateToken always required an external auth request. With#11830, we no longer waste an external auth request if we know the token is invalid.
Additionally, if we have an invalid token (say it was revoked),#11598 prevents us from constantly making an external auth request with the same invalid token in the "listen" loop. So when requiring a re authentication, we only trigger 1 request.
There is still improvement to be made, as an invalid token can trigger multiple requests if the user hits the same endpoint multiple times. The caching of "invalid" is only in the scope of 1 request atm. We could either persist the cached "invalid" to the db, or delete the link when we get a 401 back?