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.

Sign up for GitHub

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

Jump to bottom

chore(api): meter rate limit on team #12006

Merged
merged 11 commits into from Oct 18, 2022
Merged

chore(api): meter rate limit on team #12006

merged 11 commits into from Oct 18, 2022

Conversation

rcmarron
Copy link
Contributor

@rcmarron rcmarron commented Sep 27, 2022 •
edited by pauldambra

Problem

Take 2 on this PR. The problem is that our rate limits should be based on team_id instead of an API key, and the last PR didn't handle views that didn't have a team_id.

Changes

  • Moves the rate_limit to be based on the team_id when possible
  • Handles views that don't have a team_id
  • Ensures the enabling/disabling of rate limits applies to all rate_limit classes (it used to only apply to the default classes)

and

  • moves enabling the rate limit to an instance setting
  • and adds a team id allow list for bypassing the rate limit
  • and a simple cache which I think stops us hammering the reading of the infrequently changing settings

👉 Stay up-to-date with PostHog coding conventions for a smoother review.

How did you test this code?

Added tests for the new cases:

  • Rate limit works across a team - not just on an individual user.
  • Rate limits work on views without a team_id
  • Disabling the rate limits works

@rcmarron rcmarron changed the title Team rate limit chore(api): meter rate limit on team Sep 27, 2022
benjackwhite
benjackwhite approved these changes Oct 13, 2022
View reviewed changes
Copy link
Contributor

@benjackwhite benjackwhite left a comment

Choose a reason for hiding this comment

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

This all looks pretty good to me 👍 Took me a little to get my head around it but whats here looks legit.

@pauldambra
Copy link
Member

pauldambra commented Oct 13, 2022 •
edited

Took me a little to get my head around it

refactoring opportunity?

@pauldambra pauldambra merged commit 258cd32 into master Oct 18, 2022
@pauldambra pauldambra deleted the team-rate-limit branch October 18, 2022 08:01
@posthog-contributions-bot posthog-contributions-bot bot mentioned this pull request Oct 18, 2022
Merged
macobo added a commit that referenced this pull request Oct 18, 2022
@macobo
Revert "chore(api): meter rate limit on team (#12006)"
945f76e 
This reverts commit 258cd32.
@macobo macobo mentioned this pull request Oct 18, 2022
Merged
macobo added a commit that referenced this pull request Oct 18, 2022
@macobo
fix: Revert "chore(api): meter rate limit on team" (#12316)
20bc4ca 
Revert "chore(api): meter rate limit on team (#12006)"

This reverts commit 258cd32.
fuziontech added a commit that referenced this pull request Oct 19, 2022
@fuziontech
Merge branch 'master' into hobby_script
f2ca3c5 
* master: (121 commits)
  fix(app-metrics): fix navigation issue (#12319)
  fix: update insight api docs about refreshing flag (2) (#12322)
  feat: highlight in legend for pie charts (#12249)
  feat: Added person properties expander to Meta area (#12294)
  chore(tests): add pytest-randomly plugin to randomize test run order (#12307)
  chore(deps): Update posthog-js to 1.33.0 (#12321)
  feat(apps): frontend urls (#12094)
  feat(plugin-server): use graphile-worker crontab (#12242)
  fix(experiments): Handle start and end dates appropriately (#12258)
  feat(apps): remove frontend apps feature flag (#12257)
  feat: Added app context to double check things (#12317)
  fix: Revert "chore(api): meter rate limit on team" (#12316)
  fix(app-metrics): Fix navigation issue (#12315)
  fix: Improvements to Organization model setup (#12309)
  fix: dashboards last refresh not updating until page refresh (#12271)
  test(plugin-server): fix exports test flakiness (#12313)
  chore(contributors): 🤖 - Add rcmarron as a contributor 🎉 (#12310)
  chore(api): meter rate limit on team (#12006)
  feat(app-metrics): Show exports progress (#12285)
  fix: custom dashboard uses insight dates (#12305)
  ...
@neilkakkar neilkakkar restored the team-rate-limit branch January 20, 2023 13:54
@neilkakkar neilkakkar deleted the team-rate-limit branch January 20, 2023 14:19
@pauldambra pauldambra mentioned this pull request Mar 3, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@benjackwhite benjackwhite benjackwhite approved these changes

@alexkim205 alexkim205 alexkim205 approved these changes

@pauldambra pauldambra Awaiting requested review from pauldambra

Assignees

@benjackwhite benjackwhite

@alexkim205 alexkim205

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@rcmarron @pauldambra @benjackwhite @alexkim205