flexmeasures icon indicating copy to clipboard operation
flexmeasures copied to clipboard

Rate limiting for scheduling triggers

Open Flix6x opened this issue 1 year ago • 10 comments

I completed my tech spike.

I couldn't (figure out how to) decorate only the trigger method of our FlaskView, so I worked around that by decorating the entire SensorAPI and using the cost function to only count towards the limit for the trigger method. It's not the most elegant solution, but it does look easily extensible this way.

We also discussed offline on what to apply the limit (using key_func): users or accounts. Here I decided to combine the account and the sensor id, just to let a test pass (test_trigger_and_get_schedule first schedules a battery and then schedules a charging station). But it might also make sense to apply a limit per sensor.

Flix6x avatar Jan 18 '23 21:01 Flix6x

In this solution (also using a syntactic sugar on top of Flask), they found a way to add decorators per method. Not sure if Flask-Classful has that.

I'd definitely prefer that, but did they find a way though? I don't see any comment claiming they were able to add a decorator for a distinct class method. Only for all of the methods in the class, by adding the limiter.limit() to the list of method_decorators (which is applied to all class methods). In flask-classful this list variable is called decorators instead, and it is precisely that which I'm appending the limiter.limit() to. (I didn't add it to the list in the class itself, because the app context is not known there.)

What is possible is to tell limiter.limit() to only apply itself to class methods that support a certain http method (e.g. POST). I guess we could split off the schedule triggering part from the SensorAPI class, so both new classes will only contain a single POST method. That might also be more RESTful in the end, since I've come to think about the trigger endpoint as POSTing a scheduling job.

Flix6x avatar Jan 19 '23 18:01 Flix6x

Have you tried yet to decorate an endpoint outside of Flask-Classful?

nhoening avatar Jan 19 '23 21:01 nhoening

I have. That had the same problem of not having the app context available. Applying it to a whole blueprint did work, though. I had accomplished the latter in the register function of __init__.py of API v2, where the app context is available.

Flix6x avatar Jan 19 '23 22:01 Flix6x

I don't recall why the app context is needed for...

nhoening avatar Jan 19 '23 22:01 nhoening

I don't recall why the app context is needed for...

It's needed to set up the Limiter (https://flask-limiter.readthedocs.io/en/stable/api.html#flask_limiter.Limiter.params.app), so all of these config variables can be read in, including the redis connection.

Flix6x avatar Jan 23 '23 09:01 Flix6x

I'm stuck here because it seems weird that such core functionality (adding the app context) would not work in the main use case (an endpoint).

It's passed to the Limiter in the main examples. Sometimes, the limiter decorator cannot access things from the app but the docs propose a solution which might work?

nhoening avatar Jan 26 '23 11:01 nhoening

On this branch you'll find the rest of my tech spike where I test out limiting a non-FlaskView endpoint by decorating it directly, and also limiting an entire blueprint. The former doesn't work (the decorator seems to be ignored), the latter does work.

To test out, run pytest -k test_post_prognosis_2_0. (To test out the blueprint approach, comment out the endpoint's decorator, and uncomment my edits in v2's __init__.py.

Sometimes, the limiter decorator cannot access things from the app but the docs propose a solution which might work?

In my branch, the issue with the decorated endpoint approach seems not to be in getting the limit string from the app (I just hardcoded a string), but setting up the limiter to work with the app.

I guess you could carry on my tech spike, or we should come to a decision on how I can move forward. Otherwise, I'm stuck, too. I've made two suggestions:

  • Using the cost function as a way of assigning the limit to a specific endpoint, or
  • Splitting off the triggering functionality from SensorAPI.

Flix6x avatar Jan 27 '23 10:01 Flix6x

I believe I'm willing to pursue the cost function approach. I did not yet understand at which (accumulated) costs the rate limiter stops access. Is that simply 1? Or can this be some setting? And are costs accumulated per endpoint? Maybe you can point me to the place in the rate-limiter docs where I can learn this. I could not find it.

In any case, I'd still want to:

  • move the cost function to a central place
  • make sure the costs (per endpoint I guess) apply to an account
  • maybe adjust the error message

I'm willing to work on that.

nhoening avatar Feb 27 '23 16:02 nhoening

I did not yet understand at which (accumulated) costs the rate limiter stops access. Is that simply 1? Or can this be some setting? And are costs accumulated per endpoint? Maybe you can point me to the place in the rate-limiter docs where I can learn this. I could not find it.

For limiter.limit("10/second") the limit cost would be 10.

Docs here.

Flix6x avatar Feb 28 '23 11:02 Flix6x

Current list of ideas of TODOs:

  • [ ] make other tests pass, as well
  • [ ] base rate limiting for non-resource endpoints (e.g. ping, login)
  • [ ] separate base rate limiting for resource from account-based limiting (limit only "smart" endpoints, add attribute to accounts)

nhoening avatar Mar 02 '23 21:03 nhoening