Specialize rate limiting errors

[dblock]

See https://developers.strava.com/docs/#client-code

Strava can respond with a rate limit error providing the time when the next request can be made.

[JamesChevalier]

Apologies if I'm misunderstanding ... or just wrong. 😳 😬

It looks like this has since changed (in https://developers.strava.com/docs/#rate-limiting) so that X-RateLimit-Limit and X-RateLimit-Usage headers are included on each response. Passing these values along to the client implementation might be suitable for most use cases (it definitely would be for mine 😄 ).

Instead of only returning the body, would it be possible to also include the limit and usage values? Something like this?

response.body.merge!(
  strava_api_limit: response.headers["X-RateLimit-Limit"], 
  strava_api_usage: response.headers["X-RateLimit-Usage"]
)

I can't increase the size of the 🤷‍♂ I'd like to attach to this suggestion/question. 😄 There are probably better ways to go about this, but I was trying to think of a way to start the conversation with the least-breaking change. 🤔

[dblock]

This issue is about raising a RateLimitedError (or similar name) when Strava returns 403 Forbidden because of rate limiting along with enough information to inform the client about when to retry.

Returning rate limiting information as part of every response now that it's available would be a different work-item. What's the scenario in which you want to use that information on a successful response?

I definitely don't think we should be moving data from headers to the body of the response, however I think it should be OK to refactor the library such as instead of returning body the entire response is returned and the response is available in its raw form from every object constructed from the response, maybe something like:

activity = client.activity(1982980795)

activity.name # => 'Afternoon Run'

activity._request # http request
activity._response # http response

I would wrap Response into a new class and instead of reaching into headers have first class methods like activity._response.rate_limit.limit and activity._response.rate_limit.usage.

[JamesChevalier]

Ah, yeah, I can see how raising on rate limit vs returning rate limit info are two separate pieces of work. 👍

My use case on having access to these two values is to keep myself under the limit & to have a sense of where I stand. I run a site that can have sudden spikes of API usage (many webhooks, many new signups, etc) and I'm currently relying on the 403 responses as a signal that I have to kick the workload out by 15+ minutes. It's a really messy system, and I think if I knew I had X of Y requests left I could more gracefully handle the work and do a better job of letting my users know if there's a delay.

[dblock]

Looking forward to some PRs :)

[dblock]

~Closing via #40.~ that adds the rate limit info but doesn't specialize the error

[dblock]

@simonneutert This should be fairly easy on top of #40 now, WDYT?

[simonneutert]

I skimmed the Readme and the section Error would provide all info for what's needed to a proper client-side evaluation of the Error/Fault.

The header contains data about ratelimits already and we documented it, every project using this would need custom code to react to errors anyways. Digging the header hash, will allow for the precise identification.

And rereading through the issue, the reporter wanted to be able to check rate limits at any point in time and #40 (#64) does.

We can definitely add a specific error, too, but I wanted to bring this to discussion first ✌️ @dblock

[dblock]

@simonneutert This is about being able to rescue RateLimit => e and then the user can wait for some e.time_needed_to_wait_for_retry and then retry. Right now I believe we throw a generic error in that case and you'd need to rescue all errors then check what type of error it is (try writing a unit test). We can also then build a retry into the library as an optional feature next.