Absolventa / emarsys-rb

Ruby wrapper for the Emarsys API
MIT License
18 stars 30 forks source link

Rate limiting headers #27

Open michaelsauter opened 7 years ago

michaelsauter commented 7 years ago

In regards to #21, I was wondering how to expose the headers. Right now I see 2 issues:

  1. I don't even get the headers from Emarsys as promised in http://documentation.emarsys.com/resource/developers/getting-started/rate-limiting/. response.headers is:

    {:date=>"Wed, 01 Mar 2017 11:13:18 GMT",
    :server=>"Apache",
    :strict_transport_security=>"max-age=15552000; includeSubDomains; preload",
    :x_suite_response_time=>"53",
    :vary=>"Accept-Encoding",
    :content_encoding=>"gzip",
    :content_length=>"72",
    :content_type=>"text/html; charset=utf-8"}
  2. Emarsys::Request always returns Emarsys::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?

boogie commented 7 years ago

1) Are you using api.emarsys.com? I think that is the problem with the missing headers. You should use that domain instead of the "environment" domain, as some endpoints are only working there.

michaelsauter commented 7 years ago

@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 api.emarsys.net I do get the correct headers. api.emarsys.com does not work - I get "connection refused".

michaelsauter commented 7 years ago

@boogie Actually, the headers are a bit confusing. I made one request and got:

:x_ratelimit_limit=>"1000",
:x_ratelimit_reset=>"1488376140",
:x_ratelimit_remaining=>"999",

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?

mungler commented 7 years ago

@michaelsauter I think where the observed behaviour and the documentation differ, always favour the observed behaviour :)

boogie commented 7 years ago

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.

mungler commented 7 years ago

I stand corrected. Thanks @boogie

michaelsauter commented 7 years ago

@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 ...

mungler commented 7 years ago

@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?

dansch commented 7 years ago

From a today's point of view I'd also prefer getting the whole response object back and being able to differentiate in response.headersand response.resultor something like that. So yes, 👍 for the suggestion.

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.

mungler commented 7 years ago

Thanks @dansch - I think we're all in agreement, then. @michaelsauter please feel free to submit a PR to make the change 👍 (remember tests!)

mungler commented 7 years ago

@michaelsauter can this be closed now? Its possible to get the headers now, right?

michaelsauter commented 7 years ago

@mungler No not yet. We now return an Emarsys::Response instance which wraps the original response. However, the headers on the original response are not exposed yet.

mungler commented 7 years ago

@michaelsauter ahh, of course. Ok, no worries.