-
Notifications
You must be signed in to change notification settings - Fork 30
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
Rate limiting headers #27
Comments
|
@boogie Thanks, I was using the "environment" domain indeed. I thought that's the one that should be used as the readme of this gem shows how to override the default. When using |
@boogie Actually, the headers are a bit confusing. I made one request and got:
The documentation states 200 req/min. Is the documentation outdated? I assume that the header value of remaining requests is reliable and that I would actually be able to make another 999 requests in that minute? |
@michaelsauter I think where the observed behaviour and the documentation differ, always favour the observed behaviour :) |
Officially we are supporting 200 requests/minute, you should try to calculate with this. It is not documented, but you can observe it: we slow down the response time after 200 requests, and start blocking the requests only after 1000. The observed behaviour might change, we try to not change what's documented and be backward compatible as much as possible. It was hard to communicate this setup with the headers. |
I stand corrected. Thanks @boogie |
@boogie Thanks for the clarification. In general we shouldn't get near that limit. However, when doing a batch sync every now and then of e.g. just one or two attributes of all contacts, the script does need to handle rate-limits to be as fast as possible, hence my question. @mungler What do you think regarding my 2nd point? It'd be quite a drastic change to the API, but I don't see another option at the moment ... |
@michaelsauter haven't really had a chance to look into it or think about it, to be honest. I think it probably does make sense to return the Response instance (suitably augmented with a mechanism to get header data), but obviously thats a breaking change. Interested in what @carpodaster @dansch or others might think about this? |
From a today's point of view I'd also prefer getting the whole But to be honest, I don't want to take to much of a decision on the question. We haven't used the emarsys service and thus this gem for a long time now, and I highly doubt that we ever will again. So we leave it up to the community and @mungler (who took over the gem maintenance) on how to decide. |
Thanks @dansch - I think we're all in agreement, then. @michaelsauter please feel free to submit a PR to make the change 👍 (remember tests!) |
@michaelsauter can this be closed now? Its possible to get the headers now, right? |
@mungler No not yet. We now return an |
@michaelsauter ahh, of course. Ok, no worries. |
In regards to #21, I was wondering how to expose the headers. Right now I see 2 issues:
response.headers
is:Emarsys::Request
always returnsEmarsys::Response.new(response).result
, so there is no way to access the response. I wonder if it would make sense to change this to return the response and not the data.Any thoughts?
The text was updated successfully, but these errors were encountered: