dougdellolio / coinbasepro-csharp

The unofficial .NET/C# client library for the Coinbase Pro/GDAX API
MIT License
193 stars 90 forks source link

When requesting a large amount of historical bars, Rate Limit is exceeded #85

Closed andyb1979 closed 6 years ago

andyb1979 commented 6 years ago

Not surprising really but when I request historical data for "BTC-USD" with parameters CandleGranularity=Hourly and StartDate = 100 days ago, EndDate = today then the GDAX webservice responds with 'Rate limit exceeded'.

Should GDAXSharp throttle or retry after a short period or time? Or should I handle that in my own code?

I am already caching historical data just want the system to be robust

dougdellolio commented 6 years ago

@andyb1979 i have a feeling there is a bug. if you are only calling this endpoint once then there should be no rate limit exceeded error. however if you are making this call multiple times then I would expect this error. For this call in particular, the API recommends that you shouldn't poll it frequently and to use the websocket feed for the realtime info.

i will investigate this and get back..... a batching concept was added here and i have a feeling it may be contributing to a rate limit error

andyb1979 commented 6 years ago

Hi Doug,

I'm not polling just playing around with requesting and caching historical rates. I've made a few requests today so maybe its counting the datarate to a particular client?

chrisw000 commented 6 years ago

The rate limit is 3 calls per second to the public API endpoints... and yes, the Batching code I wrote for the Historic call can/does exceed this.... the GDAX docs say you're allowed to burst to 6 requests per second. So if the date range you supply generates >3 or maybe >6 batches of API calls to get all the candles... you will hit the limit.

I have some code (taken from blogs) which rate limits the HttpClient

While looking into this I found some projects which address this sort of issue. This seemed the cleanest. - but is obviously a dependency.

There is also the Rx Reactive Extensions - but for people who have not used this, its alot to get your head around.

@dougdellolio Im happy to look into making the rate limiting code in my branch "better" - for eventual pull request - but I can also see an argument that rate limiting should be handled by consumers of your project, and not by this project.

andyb1979 commented 6 years ago

I uh kinda 'solved' this with a Task.Delay of 333ms between batching requests. Quick + dirty but it works! :)

On Mon, Mar 12, 2018 at 5:17 PM, chrisw000 notifications@github.com wrote:

The rate limit is 3 calls per second to the public API endpoints... and yes, the Batching code I wrote for the Historic call can/does exceed this.... the GDAX docs say you're allowed to burst to 6 requests per second. So if the date range you supply generates >3 or maybe >6 batches of API calls to get all the candles... you will hit the limit.

I have some code (taken from blogs) which rate limits the HttpClient https://github.com/chrisw000/gdax-csharp/tree/feature/rateLimit/GDAXSharp/HttpClient

While looking into this I found some projects which address this sort of issue. This seemed the cleanest. https://github.com/David-Desmaisons/RateLimiter - but is obviously a dependency.

There is also the Rx Reactive Extensions - but for people who have not used this, its alot to get your head around.

@dougdellolio https://github.com/dougdellolio Im happy to look into making the rate limiting code in my branch "better" - for eventual pull request - but I can also see an argument that rate limiting should be handled by consumers of your project, and not by this project.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/dougdellolio/gdax-csharp/issues/85#issuecomment-372390758, or mute the thread https://github.com/notifications/unsubscribe-auth/ANZB1ZR5gvSARMusXTdxtz8Rb_zONQvhks5tdq2-gaJpZM4Sl4UE .

dougdellolio commented 6 years ago

@andyb1979 glad to hear it is working with a delay. @chrisw000 what was the reasoning for the batching again?

chrisw000 commented 6 years ago

GDAX fail requests where the date range passed in is > "350" candles. So if you want 1 hour candles, that's a limit of 350 hours; therefore if you wanted say 1 month of data at 1 hour granularity - somewhere along the line you've got to work out how to batch that up.

I say "350" because the documentation says 350, and when I first wrote the batching code 350 worked as a batch size... but then a few days later I found that it only worked upto 300 candles....

If you request too many candles in one go - it fails saying the requested granularity for the date range is too low.

If you generally make requests to the API too quickly - you get the rate limit error. You'd also get this by calling any of the services too quickly - so some built in rate limiting maybe useful?

dougdellolio commented 6 years ago

got it. i am of the opinion that we shouldn't be handling the rate limiting and that it should be up to the user. but i am open to being convinced otherwise. will close this ticket here unless any objections

chrisw000 commented 6 years ago

Where it gets tricky at the moment is the code doesn't allow callers to pass in or make use of a CancellationToken and there is no built in way to know which service calls are limited to 3 per second and which are limited to 5 per second - so rolling your own becomes harder.

Maybe a solution is to have some ability for the consumer of the code to chose, or supply their own strategy? The HTTPClient implements an interface, so you could allow someone to pass in their own IHTTPClient in while constructing the GDAXClient?

Could leave the default to how it is today, using the simple GDAXSharp.HttpClient.HTTPClient and provide code for an example rate limited client GDAXSharp.HttpClient.RateLimitedHTTPClient --> based on what I put in my branch above?

Might need to allow users to pass in 2 clients - one for 3 per second and one for 5 per second? Or have some way of knowing if the Endpoint being called is Public or Private?

dougdellolio commented 6 years ago

hm interesting idea. i just worry about making it more complicated than it needs to be. although it may allow for more flexibility. i need to think about this more

chrisw000 commented 6 years ago

Have a look at the change to GDAXClient and IHttpClient here:

https://github.com/chrisw000/gdax-csharp/commit/74f1503451ae1055b810f53a034ceb1bbeef32a0#diff-c9ba790961311e6999a6a22e87b9713c

The default behavior is as its always been.

The RateLimitedHTTPClient can live outside the project if needed.

dougdellolio commented 6 years ago

@chrisw000 to make sure we are on the same page. you are proposing to allow the user to pass in their own HttpClient which could give them the ability to decide the time in between calls to prevent exceeding the rate limit?

i am looking at the RateLimitedHttpClient in your code and trying to understand how it works with the TimeSpanSemaphore. What is the limit in this case? is that the amount of times that it can be called?

also does this rate limit occur between all calls? so if the user wanted to not rate limit certain calls and limit others how does that work? i imagine people use this library to place orders and may not want limit the times between say retrieving data and placing an order

let me know if I am missing the point of this.... just trying to wrap my head around it

chrisw000 commented 6 years ago

If the user could pass in an IHttpClient, they can then code up something to take greater control of the pipeline of requests being made.

I'm not suggesting the RateLimitedHttpClient solves all problems - I put it together specifically with the Historic data batching in mind; for this it works well... It's using a SimaphoreSlim to restrict the number of threads that can proceed on to actually call into the .SendAsync. Because we want the restriction based on time - it also keeps track on when the last calls were made.... Therefore delaying subsequent calls for the minimum possible time.

EG: If calls were made (times in seconds) at: 0.0, 0.1., 0.2, 1.0 -> Then there would be no delays at all. If calls were made at 0.0, 0.4, 0.5, 0.6 --> Then the first 3 calls would fire off right away... but the 4th call would be held for 0.4 seconds (which is the time required until the 0.0 call is outside the 1 second window)

If 30 calls were fired off in quick succession - then 27 would be held and released over time. And it's in this situation that the RateLimitedHttpClient isn't great - because you start to get quite a delay between making a call and it proceeding.... which is why it's useful to also have access to the cancellation token

What I'm really suggesting is the changes to GDAXClient, IHTTPClient (and therefore the new overload on HttpClient) only.

RateLimitedHttpClient and TimeSpanSemaphore don't need to be put into the library - I put them there so you can see them... tweaking GDAXClient to allow passing something in sidesteps the issue in allowing the consumer the flexibility to do whatever they need.

In the example you give of PlaceOrders - I can see myself coding something up that allows these calls to jump any queue and go in as soon as possible.... whereas maybe Historic candles are lowest priority and only fire off if nothing else is waiting.

Main thing is that it's likely that people would have differing needs or priorities - and therefore allowing them to insert the code lets them sort it out themselves.

And just another thought; I've seen other libraries organise the service end point data into classes - holding info like if it's a GET / PUT / DELETE, if it needs signing, rate limits, url, potentially url construction etc. --> IE the endpoint object takes in the request data.... then the IEndPoint gets passed down into where the RequestMessage is constructed... if this were still available at the point .SendAsync is called... we'd be able to manage the pipeline of incoming requests in one place.

dougdellolio commented 6 years ago

gotcha. i think this is a good idea and I see the value in it. if you feel up to adding it then lets do it 😄

chrisw000 commented 6 years ago

OK... do you mean the just basic changes to the GDAXClient constructors...

or the whole thing... refactoring endpoints etc. ?

Happy to give the whole thing a crack.... but would probably do it in stages of pull requests:

PR1) GDAXClient constructor, interface changes PR2) Change just one end point and put in the plumbing to allow that to get passed around PR3) update the rest of the end points to use new plumbing PR4) remove any refactored away code

Something like that?

dougdellolio commented 6 years ago

i would say just the basic changes to the constructor. meaning give the user ability to pass in their own httpclient. if we see a greater need to refactor all the endpoints then we can take it on at that point. thoughts?

chrisw000 commented 6 years ago

Thats cool.... less is more and all that!