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: Don't Label Users as Quota Limited for N Days After They Exceed Their Quota #19938

Merged
merged 13 commits into from
Feb 9, 2024

Conversation

xrdt
Copy link
Contributor

@xrdt xrdt commented Jan 24, 2024

Problem

N = 7. We had discussed making N = 3 because 7 might just be too many days.

We would like to keep users' data for N days after they start to exceed their usage quotas. This will allow us to recover in a case where billing limits inadvertently become lower than current usage (INC-144). This PR introduces a new redis cache and additional metadata on the cached usage dict in posthog to record this temporary state.

Working on tests 🔨

Changes

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

How did you test this code?

@xrdt
Copy link
Contributor Author

xrdt commented Jan 26, 2024

Background and requirements:

  • We'd like to be able to toggle a data retention mechanism before we execute billing migrations. This will prevent us from inadvertently lowering billing limits and quota limiting users.
    • We should have telemetry into the following:
      • Who would have had data dropped had we not toggled this mechanism?
      • How much data would have been dropped? Aggregate and per org.
  • Pipeline team will be looking into a plugin-server based implementation of billing / quota limits in Q2. This implementation will be a temporary mechanism that builds on existing quota limiting functionality in the billing api.

Approaches:

  1. Temporarily move users into a separate redis set and mark when they should no longer be put into this set but into the default quota limiting set. This mechanism resets at the shorter of 7 days or the start of the new billing cycle.
    a. It's not clear how to support a toggling mechanism.
  2. Use feature flags to toggle the suspension of quota limiting.
    a. This should be relatively simple and will allow us to have fine-grained toggling controls (all users and arbitrary subsets).

For both approaches, we can send events to posthog marking when a user should have been quota limited and when the temporary suspension of quota limiting ended. We can then use usage reports to estimate the usage that would have been dropped had quota limiting been in effect.

This PR partially implements approach 1. Here is a PR for approach 2.

@raquelmsmith More or less sums it up? It's looking like the feature flags approach makes the most sense, unless @neilkakkar identifies an issue.

Bianca Yang added 2 commits January 30, 2024 14:31
@xrdt xrdt force-pushed the by/dont-drop-data-retention branch from 39a1668 to 38132ee Compare January 30, 2024 22:31
@xrdt
Copy link
Contributor Author

xrdt commented Jan 30, 2024

We're going to keep this strategy in place as a failsafe to complement the manual, feature-flag based retention trigger: #19996

@xrdt xrdt marked this pull request as ready for review January 30, 2024 22:32
@posthog-bot
Copy link
Contributor

📸 UI snapshots have been updated

6 snapshot changes in total. 0 added, 6 modified, 0 deleted:

  • chromium: 0 added, 6 modified, 0 deleted (wasn't pushed!)
  • webkit: 0 added, 0 modified, 0 deleted

Triggered by this commit.

👉 Review this PR's diff of snapshots.

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.

Logic looks good here, just a couple questions about code hygiene.

I wonder what is the best way to use this and keep an eye on it. Like should we have an alert of some sort when the number of teams who would have been limited goes up?

ee/billing/quota_limiting.py Outdated Show resolved Hide resolved
ee/billing/quota_limiting.py Outdated Show resolved Hide resolved
ee/billing/quota_limiting.py Outdated Show resolved Hide resolved
ee/billing/quota_limiting.py Outdated Show resolved Hide resolved
summary["retained_period_end"] = data_retained_until
needs_save = True

if billing_period_end:
Copy link
Member

Choose a reason for hiding this comment

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

When would we not have a billing period end?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can't imagine why we wouldn't have one. This is here simply to be consistent with the previous check if is_quota_limited and billing_period_end:

ee/billing/quota_limiting.py Outdated Show resolved Hide resolved
ee/billing/test/test_quota_limiting.py Outdated Show resolved Hide resolved
@xrdt
Copy link
Contributor Author

xrdt commented Feb 6, 2024

It seems like we'd be most interested in keeping tabs on whether certain orgs are incorrectly having data retained. That would imply we're not correctly inserting and removing them from the data-retention redis set. I think we should create a dashboard similar to the existing quota limiting one, or simply add graphs for data retention to that dashboard.

@xrdt xrdt requested a review from raquelmsmith February 7, 2024 00:41
@xrdt xrdt merged commit 879fb81 into master Feb 9, 2024
72 checks passed
@xrdt xrdt deleted the by/dont-drop-data-retention branch February 9, 2024 01:56
Gilbert09 pushed a commit that referenced this pull request Feb 9, 2024
… Their Quota (#19938)

* wip

* add tests

* fix up mypy

* Update query snapshots

* Update query snapshots

* address pr feedback

* small updates

* fix tests

* use better types and no key defaults

* fix typing

* fix up some test setup

---------

Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: Raquel Smith <[email protected]>
benjackwhite added a commit that referenced this pull request Feb 9, 2024
@benjackwhite benjackwhite mentioned this pull request Feb 9, 2024
benjackwhite added a commit that referenced this pull request Feb 9, 2024
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