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: use feature flag to control whether to quota limit or not #19996

Merged
merged 22 commits into from
Jan 31, 2024

Conversation

xrdt
Copy link
Contributor

@xrdt xrdt commented Jan 26, 2024

Problem

Context here

Changes

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

How did you test this code?

ee/billing/quota_limiting.py Outdated Show resolved Hide resolved
@xrdt xrdt marked this pull request as ready for review January 29, 2024 17:22
@xrdt xrdt changed the title use feature flag to control whether to quota limit or not feat: use feature flag to control whether to quota limit or not Jan 29, 2024
@xrdt xrdt force-pushed the by/dont-drop-data-retention-feature-flag branch from b663f8e to 6dc8ddc Compare January 29, 2024 22:10
ee/billing/quota_limiting.py Outdated Show resolved Hide resolved
ee/billing/test/test_quota_limiting.py Outdated Show resolved Hide resolved
@@ -89,6 +92,14 @@ def org_quota_limited_until(organization: Organization, resource: QuotaResource)
if is_quota_limited and organization.never_drop_data:
return None

if is_quota_limited and posthoganalytics.feature_enabled(QUOTA_LIMIT_DATA_RETENTION_FLAG, organization.id):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Assuming this is a group flag, you need the groups parameter here, along with any potential group properties you'll use to target orgs so this can be evaluated locally quickly

Copy link
Collaborator

Choose a reason for hiding this comment

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

Not quite sure where are the $groupidentify events coming in where you put people into quota limiting / remove them from quota limiting.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It should happen here: https://github.com/PostHog/posthog/blob/master/posthog/event_usage.py#L283

That function should get called here: https://github.com/PostHog/posthog/blob/master/ee/billing/quota_limiting.py#L267

But that event, "organization quota limits changed" hasn't been seen in 9 months...


@patch("posthoganalytics.capture")
@patch("posthoganalytics.feature_enabled", return_value=True)
def test_quota_limit_feature_flag_not_on(self, patch_feature_enabled, patch_capture) -> None:
Copy link
Collaborator

Choose a reason for hiding this comment

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

unsure about this test, flag is on it seems? Is there another reason nothing is captured?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The conditional on line 95 of the quota_limiting.py file should lazy eval and fail since the org hasn't exceed their quota limit.

@xrdt xrdt requested a review from neilkakkar January 30, 2024 19:00
Copy link
Member

@raquelmsmith raquelmsmith left a comment

Choose a reason for hiding this comment

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

Let's merge and keep a close eye. I'd also like to try it out for a bit and make sure it works in prod.

@xrdt xrdt enabled auto-merge (squash) January 31, 2024 20:38
@xrdt xrdt merged commit c3623b0 into master Jan 31, 2024
72 checks passed
@xrdt xrdt deleted the by/dont-drop-data-retention-feature-flag branch January 31, 2024 21:37
Copy link

sentry-io bot commented Jan 31, 2024

Suspect Issues

This pull request was deployed and Sentry observed the following issues:

  • ‼️ TypeError: 'str' object is not a mapping posthog.tasks.tasks.update_quota_limiting View Issue
  • ‼️ TypeError: 'str' object is not a mapping /api/billing-v2/deactivate/ View Issue
  • ‼️ TypeError: 'str' object is not a mapping /api/billing-v2/ View Issue
  • ‼️ TypeError: 'str' object is not a mapping posthog.tasks.usage_report.send_report_to_billi... View Issue

Did you find this useful? React with a 👍 or 👎

thmsobrmlr pushed a commit that referenced this pull request Feb 1, 2024
* use feature flag to control whether to quota limit or not

* add simple test

* Update query snapshots

* fix up the feature flag activation logic

* confirm that the fixed feature flag logic works

* remove unnecessary change

* Update query snapshots

* Update query snapshots

* fix up tests

* another fix for tests

* Update query snapshots

* still fixing tests

* Update query snapshots

* pr feedback, more tests

* tune up ff enabled call, add capture check to test for quota limit changed

* i really need to run tests locally

---------

Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com>
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.

3 participants