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

feat: custom /query rate limit when using personal API key #28184

Open
wants to merge 24 commits into
base: master
Choose a base branch
from

Conversation

orian
Copy link
Contributor

@orian orian commented Feb 2, 2025

Problem

Some users want to send more than 120 queries per hour.

Changes

  1. Add new team property api_query_rate_limit
  2. Use the new rate limit when HogQLQueryThrottle in action

image

Does this work well for both Cloud and self-hosted?

Yes.

How did you test this code?

Added test and check locally.

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR Summary

This PR adds a new team property api_query_rate_limit to enable customizable rate limits for API queries when using personal API keys. Here are the key changes:

  • Added api_query_rate_limit CharField to Team model with validation for rate limit formats like '100/hour', '5/min'
  • Modified PersonalApiKeyRateThrottle class to support custom rate limits through new load_team_rate_limit method
  • Added validation function validate_rate_limit in utils.py to ensure proper rate limit format using regex pattern
  • Created migration 0559 to add the new nullable field with max length of 32 characters
  • Added test cases to verify rate limit format validation rejects invalid formats like '1/week' while accepting valid ones like '1/s'

The implementation looks solid but there are a few potential concerns:

  • Consider adding documentation on supported rate limit formats and validation rules
  • The commented-out test exception in query.py should be removed
  • May want to add validation to prevent extremely high rate limits that could impact system stability

10 file(s) reviewed, 10 comment(s)
Edit PR Review Bot Settings | Greptile

posthog/api/query.py Outdated Show resolved Hide resolved
posthog/migrations/0559_team_api_query_rate_limit.py Outdated Show resolved Hide resolved
posthog/admin/admins/team_admin.py Show resolved Hide resolved
posthog/migrations/0559_team_api_query_rate_limit.py Outdated Show resolved Hide resolved
posthog/models/test/test_utils.py Outdated Show resolved Hide resolved
posthog/rate_limit.py Outdated Show resolved Hide resolved
posthog/rate_limit.py Outdated Show resolved Hide resolved
"set custom rate limit", extra={"rate_limit_cache_key": rate_limit_cache_key, "rate_limit": self.rate}
)

self.num_requests, self.duration = self.parse_rate(self.rate)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

logic: parse_rate() could raise ValueError for invalid rate string formats. Add error handling here.

posthog/models/utils.py Outdated Show resolved Hide resolved
posthog/models/utils.py Show resolved Hide resolved
@orian orian force-pushed the pawel/feat/team-rate-limit branch 3 times, most recently from 1121456 to a1e3353 Compare February 3, 2025 14:00
@orian orian changed the title Feat: custom /query rate limit when using personal API key feat: custom /query rate limit when using personal API key Feb 3, 2025
@orian orian force-pushed the pawel/feat/team-rate-limit branch from ebbf909 to db97a9c Compare February 3, 2025 15:19
Copy link
Contributor

@Daesgar Daesgar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm!

@orian orian force-pushed the pawel/feat/team-rate-limit branch from 83184fe to 5d6fab2 Compare February 3, 2025 16:11
@orian orian enabled auto-merge (squash) February 3, 2025 20:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants