feat: Add option to prevent hitting the rate limit

erezrokah commented 3 months ago

Summary

Mostly opening for feedback, happy to add tests and docs if this approach makes sense. This PR sleeps until the rate limit reset time in case the next request will hit the limit. This should prevent sending requests that are known to cause 429 errors.

I updated the auto generated code for simplicity but will update the generation templates if this change makes sense.

Fixes https://github.com/okta/okta-sdk-golang/issues/434

Type of PR

[ ] Bug Fix (non-breaking fixes to existing functionality)
[x] New Feature (non-breaking changes that add new functionality)
[ ] Documentation update
[x] Test Updates
[ ] Other (Please describe the type)

Test Information

[ ] My PR required test updates

Go Version: Os Version: OpenAPI Spec Version:

Signoff

[x] I have submitted a CLA for this PR
[x] Each commit message explains what the commit does
[ ] I have updated documentation to explain what my PR does
[x] My code is covered by tests if required
[x] I ran make fmt on my code
[x] I did not edit any automatically generated files

github-actions[bot] commented 2 months ago

This PR has been marked stale because there has been no activity within the last 28 days. To keep this PR active, remove the stale label.

erezrokah commented 2 months ago

Not stale

duytiennguyen-okta commented 1 month ago

@erezrokah This make sense, can you add the test for it? Thank you

erezrokah commented 1 month ago

@erezrokah This make sense, can you add the test for it? Thank you

Sure 💯 Added in https://github.com/okta/okta-sdk-golang/pull/450/commits/0cf4f9a41bb17391a928c9220cde8b7153c18c85

rickb-monad commented 1 month ago

Hi @erezrokah, I believe your test is not correctly formed.

The existing func Mock429Response() *http.Response and new MockRateLimitedResponse(requestId int, reset time.Time) *http.Response functions are duplicate, and should be merged/combined and updated as follows:

func Mock429RateLimitedResponse() *http.Response {
    zulu := time.Now().UTC()
    header := http.Header{}
    header.Add("Date", zulu.Format(time.RFC1123))
    header.Add("Content-Type", "application/json")
    header.Add("X-Okta-Request-id", "a-request-id")
    header.Add("X-Rate-Limit-Limit", strconv.Itoa(50))
    header.Add("X-Rate-Limit-Remaining", strconv.Itoa(0))
    header.Add("X-Rate-Limit-Reset", strconv.Itoa(int(zulu.Add(time.Second).Unix())))

    errBody := `{"errorCode":"E0000047",`+
        `"errorSummary":"API call exceeded rate limit due to too many requests.",`+
        `"errorLink":E0000047,`+
        `"errorId":"sampleVWFhy6K7rS9-IWcjsk_",`+
        `"errorCauses":[]}`

    return &http.Response{
        Status:        http.StatusText(http.StatusTooManyRequests),
        StatusCode:    http.StatusTooManyRequests,
        Body:          httpmock.NewRespBodyFromString(errBody),
        Header:        header,
        ContentLength: int64(len(errBody)),
    }
}

[!NOTE] Okta does not include an HTTP response header of X-Okta-Now as was in the previous Mock429Response(), and the go standard library time.RFC1123 format constant is correct layout for the Date header.

Per the documentation:

The HTTP status code for Rate Rimited API responses is HTTP 429 (not HTTP 200), and the HTTP response body is the JSON encoded form of the okta.Error{} struct. The Okta SDK Request Executor will return an error (which is of type okta.Error{}) via the CheckResponseForError(resp *http.Response) error function.

CheckResponseForError(resp *http.Response) error (click to expand...)

https://github.com/okta/okta-sdk-golang/blob/6739ea37f882db247fcd6bfc60c32dfcd87f7574/okta/requestExecutor.go#L635-L666

The CheckResponseForError() function effectively decodes the Error JSON response into and returns an okta.Error{} struct, with a few specific and minor exceptions.

okta.Error{} (click to expand...)

https://github.com/okta/okta-sdk-golang/blob/master/okta/error.go#L24-L32

So, when the HTTP Response Header includes X-Rate-Limit-Remaining: 0, the HTTP Status Code must also be HTTP 429, and the response body will be one of the example HTTP 429 error responses, which for most APIs will be E0000047: Too many requests exception:

The E0000047 example verbatim (click to expand...)

```http HTTP/1.1 429 Too Many Requests Content-Type: application/json { "errorCode": "E0000047", "errorSummary": "API call exceeded rate limit due to too many requests.", "errorLink": E0000047, "errorId": "sampleVWFhy6K7rS9-IWcjsk_", "errorCauses": [] } ```

A more accurate example of E0000047, including the appropriate headers (click to expand...)

summary> ```http HTTP/1.1 429 Too Many Requests Content-Type: "application/json" X-Rate-Limit-Limit: 20 X-Rate-Limit-Remaining: 0 X-Rate-Limit-Reset: 1717958209 Content-Length: 204 { "errorCode": "E0000047", "errorSummary": "API call exceeded rate limit due to too many requests.", "errorLink": E0000047, "errorId": "sampleVWFhy6K7rS9-IWcjsk_", "errorCauses": [] } ```

An example of a non-rate limited (HTTP 200) response (click to expand...)

```http HTTP/1.1 200 OK Content-Type: "application/json" X-Rate-Limit-Limit: 20 X-Rate-Limit-Remaining: 19 X-Rate-Limit-Reset: 1717958209 Content-Length: 46 [{"id": "example-id-1"},{"id":"example-id-2"}] ```

erezrokah commented 1 month ago

Hi @erezrokah, I believe your test is not correctly formed.

Thanks for the detailed response @rickb-monad ❤️ I'll take a look this week and update the PR

github-actions[bot] commented 1 week ago

This PR has been marked stale because there has been no activity within the last 28 days. To keep this PR active, remove the stale label.

erezrokah commented 1 week ago

Not stale, will try to address the comments this week

okta / okta-sdk-golang