traveltime-dev / traveltime-python-sdk

TravelTime SDK for Python programming language
https://docs.traveltime.com/
MIT License
19 stars 5 forks source link

Potential bug: Rate limiting not considerung actual number of matrices in 1 hit? #62

Closed MxMartin closed 1 year ago

MxMartin commented 1 year ago

Hi all!

https://github.com/traveltime-dev/traveltime-python-sdk/blob/6e6332c54b0bc6c2460d2abdd52875b3cbb17ee8/traveltimepy/http.py#L27-L37

Referring to line 35: If I understand the rate limiter corectly an amount parameter can be passed to the context manager which should reflect the actual units counted towards the rate limit. If I understand the API docs correctly, in the time filter API one hit can contain multiple matrices (=DepartureSearch or ArrivalSearch) each of which is counted towards the rate limit. Shouldn't line 35 therefore look somethin like this:

     async with rate_limit(amount=len(request.departure_searches)+len(request.arrival_searches)): 

All the best and keep up the good work!

danielnaumau commented 1 year ago

Hi,

it's not like that. One hit is one search everywhere, even in the time filter. https://docs.traveltime.com/api/overview/usage-limits#Hits-Per-Minute-HPM.

MxMartin commented 1 year ago

Hi! Yes, that's exactly the docs I was looking at ;)

And my understand is as described above. I built a test that seems to confirm my understanding. If you run the following, you will find that var2 will lead to a 429 error after about one third of the number of requests as var1:


sdk = TravelTimeSdk(TRAVELTIME_ID, TRAVELTIME_KEY, rate_limit=1000)

locations = [
    Location(id="London center", coords=Coordinates(lat=51.508930, lng=-0.131387)),
    Location(id="Hyde Park", coords=Coordinates(lat=51.508824, lng=-0.167093)),
    Location(id="ZSL London Zoo", coords=Coordinates(lat=51.536067, lng=-0.153596))
]
var1_search_ids = {
    "London center": ["Hyde Park", "ZSL London Zoo"],
}
var2_search_ids = {
    "ZSL London Zoo": ["Hyde Park", "London center"],
    "London center": ["Hyde Park", "ZSL London Zoo"],
    "Hyde Park": ["ZSL London Zoo", "London center"],
}
async def test_rate_limit(search_ids: dict):
    results = []
    start_time = datetime.now()
    print("start:", start_time.strftime("%H:%M:%S"))
    for _ in range(100):
        try:
            results.append((datetime.now(), await sdk.time_filter_async(
                locations=locations,
                search_ids=search_ids,
                departure_time=datetime.now(),
                travel_time=3600,
                transportation=PublicTransport(type="bus"),
                properties=[Property.TRAVEL_TIME],
                range=FullRange(enabled=True, max_results=3, width=600)
            )))
        except ApiError as e:
            print("end:", datetime.now().strftime("%H:%M:%S"))
            print(e)
            print(f"Rate limit exceeded after {len(results)} requests and {(datetime.now() - start_time).total_seconds()} seconds")
            break
    return results
var1= await test_rate_limit(var1_search_ids)
var2 = await test_rate_limit(var2_search_ids)
print(f"Var 1 failed after {len(var1)} requests, var 2 failed after {len(var2)} requests.")

Be sure to run the second test > 1 min after the first test.

danielnaumau commented 1 year ago

We know about such cases but it's not a sdk issue.

Unfortunately these rate limits are not very precise on our server side. It's happening due to some performance issues. We are trying to fix it but it will take some time.

For now please try to use lower limits