-
Notifications
You must be signed in to change notification settings - Fork 807
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
Consolidate request handling in RecordRequestsMiddleware #2013
Consolidate request handling in RecordRequestsMiddleware #2013
Conversation
Code Coverage DiffThis PR does not change the code coverage |
f907901
to
ce6324a
Compare
/lgtm |
Good news, actual api request errors are no longer also including throttled request errors (nice!) Bad news, this now significantly undercounts actual throttled requests at the sdk level. Off by factor of 100. (I believe it is only counting the FINAL RateLimitExceeded error for EC2 API Call.) Scalability 1000 pod test results:
|
Signed-off-by: torredil <[email protected]>
ce6324a
to
c943d37
Compare
Connor's Diff works correctly, well done team!
Though we should convey to customers that |
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ConnorJC3 The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Is this a bug fix or adding new feature?
Bug fix
What is this PR about? / Why do we need it?
This PR ensures that throttled requests are correctly captured in the
cloudprovider_aws_api_throttled_requests_total
metric instead ofcloudprovider_aws_api_request_errors metric
.RecordThrottledRequestsMiddleware
has been removed as it is no longer needed.What testing is done?