Skip to content
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

Validation: Daily Limit (lowers) #138

Open
11 tasks
k-macmillan opened this issue Jan 10, 2025 · 0 comments
Open
11 tasks

Validation: Daily Limit (lowers) #138

k-macmillan opened this issue Jan 10, 2025 · 0 comments
Labels
Dev Reviewed Reviewed by Tech Lead Notify Board trigger PM Reviewed Reviewed by Product Manager QA Reviewed Reviewed by Quality Assurance QA Issue requires QA collaboration Ready for Refinement

Comments

@k-macmillan
Copy link
Member

k-macmillan commented Jan 10, 2025

User Story - Business Need

We will use redis to handle daily limits.

  • Ticket is understood, and QA has been contacted (if the ticket has a QA label).

User Story(ies)

As a VA Notify reliability dev
I want to limit API requests in the lowers
So that one client is not capable of affecting another

Additional Info and Resources

We likely want to use:

pool = redis.ConnectionPool.from_url(<url>)
EnpState.redis_client = redis.Redis.from_pool(pool)

and set that as part of our EnpState with lifespan (setup/shutdown), making sure to close it with the shutdown.

Redis Keys should be remaining-daily-limit-<service_id>-<api_key_id> of an authenticated request because some clients have sub-groups using different keys.

Example taken (and tweaked) from Redis' Youtube account:

async def is_over_daily_limit(redis_client: Redis, key: str, daily_limit: int) -> bool:
    """Decrements the value of a given key and returns if the key is over the daily limit or not.

       Key expires at midnight UTC to allow for daily limits, regardless of when they started that day.
    """
    limited: bool = True
    # Set the key and value if the key does not exist
    if await redis_client.setnx(key, daily_limit):
        # Expire at midnight using a time delta
        await redis_client.expire(key, midnight UTC - now())
    request_count = await redis_client.get(key)
    # Each call reduces the count by one, resets after the key is expired
    if request_count and int(request_count) > 0:
        await redis_client.decrby(key, 1)
        limited = False
    return limited

Acceptance Criteria

This AC is based on the suggestion in the Additional Info. If the dev would like to move in a different direction please discuss with @k-macmillan so we can update the AC.

  • redis.asyncio used and added to the project dependencies (pypi page)
  • Environment variable for the redis url. Defaults to local
  • Environment variable to set the daily_limit (value for keys). Default = 100
  • key is remaining-daily-limit-<service_id>-<api_key_id>
  • Redis added to docker-compose file
  • Prod does not contact redis (always returns False or the method is not called in the first place)
  • Environment variables for docker-compose set in ci/.env.local
  • Used as a Depends on sms/email/ POST routes and notification GET route (for any that are implemented)
  • Runs after auth is verified. If necessary, group the two Depends into a single method and call each within that method
  • This work is added to the sprint review slide deck (key win bullet point and demo slide)

QA Considerations

Multiple services and api keys should all work as expected. This does not have the same race condition concerns that rate limiting has and should be much more straight-forward to test.

Potential Dependencies

@k-macmillan k-macmillan added Dev Reviewed Reviewed by Tech Lead Notify Board trigger QA Issue requires QA collaboration labels Jan 10, 2025
@cris-oddball cris-oddball added the QA Reviewed Reviewed by Quality Assurance label Jan 13, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Dev Reviewed Reviewed by Tech Lead Notify Board trigger PM Reviewed Reviewed by Product Manager QA Reviewed Reviewed by Quality Assurance QA Issue requires QA collaboration Ready for Refinement
Projects
None yet
Development

No branches or pull requests

3 participants