Shared: Migrate to Plan / Tier Tables #479

ajay-sentry · 2025-01-16T00:00:24Z

Description

This PR aims to migrate off the plan consts in shared in favor of the Plan data model. The integration can be summed up as doing DB calls throughout where previously we were referencing a constant value.

Typically a query for a plan will use the plan name (identified as "plan") on the owner model to first retrieve a user's plan. At the same time, if the tier is needed, we will also prefetch that object as well to streamline the number of DB queries we have.

As much as possible we default to either using the plan "cached" from a higher level query, as in the Plan Service; or we build up a query and execute in the case of availablePlans. Overall this will result in slightly higher load on our database, if there are any charts and/or metrics anyone has readily available with those higher level monitoring specs I'd love to take a look.

We will look to clean up those consts and any lingering references (only in tests at this point) in an upcoming PR.

This PR relates to the API/Worker versions here:

Notion with some more info about the motivation and where we are in the process: https://www.notion.so/sentry/Streamlining-Plan-Creation-with-the-Plan-Configuration-Table-1428b10e4b5d808ca5b2d4f5b66493a1

Closes codecov/engineering-team#3251

Legal Boilerplate

Look, I get it. The entity doing business as "Sentry" was incorporated in the State of Delaware in 2015 as Functional Software, Inc. In 2022 this entity acquired Codecov and as result Sentry is going to need some rights from me in order to utilize my contributions in this PR. So here's the deal: I retain all rights, title and interest in and to my contributions, and by keeping this boilerplate intact I confirm that Sentry can use, modify, copy, and redistribute my contributions, under Sentry's choice of terms.

…lans

codecov · 2025-01-17T18:49:32Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.98%. Comparing base (8db93ad) to head (5c8398a).

❗ Current head 5c8398a differs from pull request most recent head 7804571

Please upload reports for the commit 7804571 to get more accurate results.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #479      +/-   ##
==========================================
- Coverage   90.32%   89.98%   -0.34%     
==========================================
  Files         435      324     -111     
  Lines       12999     9137    -3862     
  Branches     2112     1624     -488     
==========================================
- Hits        11741     8222    -3519     
+ Misses       1143      854     -289     
+ Partials      115       61      -54

Flag	Coverage Δ
shared-docker-uploader	`?`

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

…xing tests

tests/unit/plan/test_plan.py

shared/plan/constants.py

…boarding the consts

adrian-codecov · 2025-01-22T23:36:35Z

shared/django_apps/codecov_auth/models.py

-            return plan_details
+        plan_details = Plan.objects.get(name=self.plan)
+        if plan_details:
+            return {


Could we make this a class method, like serialized plan or something

class Plan(BaseModel): ... def serialized_plan(self): -> <you could also type this into another dataclass or something if you want brownie points) return {...} # this is where you'd put this "pretty plan"

adrian-codecov · 2025-01-22T23:38:22Z

shared/plan/constants.py

@@ -109,6 +110,24 @@ def convert_to_DTO(self) -> dict:
        }


+def convert_to_DTO(plan) -> dict:


Thoughts on typing this?

I was getting a circular dependency trying to import the Plan class and type it that way, is there another way we can do it?

So why is this a circular dependency? I suppose cause you'd import Plan, and plan imports from constants.

So, this being the only thing that imports from Plan, could this belong in plan instead? You'd have to make an instance of Plan to use this wherever you do though

yeah exactly, I kinda wanted to avoid having to bind this function to a class when it's really just a "transformer" from our plan object to the plan object that the planService and subsequently GQL expects

I see. I'll leave the implementation up to you, I think though that not typing a function because of circular imports feels a bit odd, but you're getting of the fn soon right? If so I wouldn't say it's a big deal

adrian-codecov · 2025-01-22T23:39:47Z

shared/plan/service.py

@@ -55,19 +47,20 @@ def __init__(self, current_org: Owner):
            self.current_org = current_org.root_organization
        else:
            self.current_org = current_org
-        if self.current_org.plan not in USER_PLAN_REPRESENTATIONS:
+
+        if not Plan.objects.filter(name=self.current_org.plan).exists():


Remind me, are we porting all plans, including "obsolete" plans like the super super old ones that we don't use? If so, changing those "if name not in USER_PLAN_REPRESENTATION" to this wouldn't be 100% equivalent

The full plan list we're porting over can be found here: https://www.notion.so/sentry/Plan-and-Tier-Table-Mapping-1758b10e4b5d801aa2d7e30198bcfc68

It's basically all the plans that were in USER_PLAN_REPRESENTATION, and seemingly the check should hold true even if the plans are no longer active in the future too, which is why I didn't add the is_active=True filter

adrian-codecov · 2025-01-22T23:42:44Z

shared/plan/service.py

@@ -89,25 +82,25 @@ def has_account(self) -> bool:
        return self.current_org.account is not None

    @property
-    def plan_data(self) -> PlanData:
+    def plan_data(self) -> Plan:


Have you confirmed this implementation effectively acts as cacheing the field? Since this is now a db query, we'd want this to be cached - perhaps look into cached_property decorator

How can I confirm if its cached or not? Is there a way to check locally / on stage if we do a DB hit vs. use the cache with the new property? In any case I was able to update using that property in 7804571

Setting the decorator should suffice, there's probably a way to check it programatically as well but can't think off the top of my head

adrian-codecov · 2025-01-22T23:43:27Z

shared/plan/service.py

        """Returns the tier name of the plan."""
-        return self.plan_data.tier_name
+        return self.plan_data.tier.tier_name


Since this has a relationship to tier, might be worth in the query of plan_data to prefetch this relationship, otherwise you'll have an n+1

Added to all relevant touchpoints! Good call

adrian-codecov · 2025-01-22T23:44:28Z

shared/plan/service.py


-        if self.plan_name == FREE_PLAN.value:
-            available_plans.append(FREE_PLAN)
+        curr_plan = Plan.objects.get(name=self.plan_name)


Should this be the same as plan_data? If not, how does the has_account affect this logic?

oh yeah nice catch, saves a query too

adrian-codecov · 2025-01-22T23:46:52Z

shared/plan/service.py


-        return [plan.convert_to_DTO() for plan in available_plans]
+        available_plans.update(
+            Plan.objects.filter(tier__tier_name__in=available_tiers, is_active=True)


If we're going to always do this at least for the PRO value, would it make sense for the first available_plans to include that parameter, and if available_tiers doesn't change, you save yourself 1 query every time

I'd also look into cacheing this one for perf

Looked into this one a bit, @cached_property is unavailable to use because we have an additional input parameter (owner), and while LRU_cache does work, if we don't end up evicting the value for an owner when other aspects of that owner change we could end up with some stale / incorrect values

adrian-codecov · 2025-01-22T23:51:58Z

shared/plan/service.py

@@ -236,10 +235,12 @@ def start_trial_manually(self, current_owner: Owner, end_date: datetime) -> None
            No value
        """
        # Start a new trial plan for free users currently not on trial
-        if self.plan_name in FREE_PLAN_REPRESENTATIONS:
+
+        plan = Plan.objects.get(name=self.plan_name)


Could this be self.plan_data?

I'm seeing a lot of Plan.objects.get(name=self.plan_name), so this comment applies to all the instances that behave the same way

Could we use plan_data instead? Not sure how the account part of this works, but I'm not a fan of doing many DB queries in different properties when you could have made one. If we can't use plan_data, I'd make another field called "plan_db_representation" or something (please something shorter 😂), make that a cache property, and do that instead (this is effectively plan_data but idk how the accounts play a role).

Prefetch other relationships you have (tier below). When you do plan.tier, if you haven't prefetched that, you basically do another DB call to tier cause it's a fk, so prefetching here means "ask for it when you do the initial fetch cause it will be 1 query"

Yeah those are great points, I was rushing through on the original implementation just to get us started on fixing tests but going back we can totally use plan_data instead, I think the account stuff will be needed as well so the stuff previously was wrong anyway.

2 birds with one stone: optimizations and bug fixing 😂

I also fixed all the instances of plan.tier to now prefetch tier if we are gonna need it too

adrian-codecov · 2025-01-22T23:52:17Z

tests/unit/django_apps/codecov_auth/test_codecov_auth_models.py

@@ -396,6 +398,12 @@ def test_fields_that_account_overrides(self):
            ENTERPRISE_CLOUD_USER_PLAN_REPRESENTATIONS[self.owner.account.plan]
        )
        account_pretty_plan.update({"quantity": 0})
+        print(


Do we still want this print here or typo?

adrian-codecov · 2025-01-22T23:53:32Z

tests/unit/django_apps/codecov_auth/test_codecov_auth_models.py



 class TestOwnerModel(TransactionTestCase):
    def setUp(self):
        self.owner = OwnerFactory(username="codecov_name", email="[email protected]")
+        mock_all_plans_and_tiers()


Remind me, does setup run once per test or per class? If so, I'd try to put it in a method that does it once, no need to recreate it every time. You'd just need to make sure it persists across different tests.

Same for the other test

That's a great point, apparently there's a setUpClass() function we might be able to use here. Not only for this setup but probably a lot of other tests as well

yep, i like that

adrian-codecov · 2025-01-22T23:56:02Z

tests/unit/upload/test_utils.py



 class CoverageMeasurement(TestCase):
+    def setUp(self):
+        mock_all_plans_and_tiers()


Why do we need that here?

bc of this:

shared/shared/upload/utils.py

Line 35 in b186b3c

monthly_limit = plan_service.monthly_uploads_limit

admittedly could probably be more precise with the mock here

ajay-sentry · 2025-01-23T22:51:54Z

shared/plan/constants.py

@@ -189,7 +208,7 @@ def convert_to_DTO(self) -> dict:
            "Unlimited private repositories",
            "Priority Support",
        ],
-        tier_name=TierName.PRO.value,
+        tier_name=TierName.SENTRY.value,


note: These consts are all disappearing soon and aren't / shouldn't be a SOT for anything anymore

ajay-sentry · 2025-01-23T22:53:46Z

tests/helper.py

@@ -46,3 +54,166 @@ def _sessions(sessions):
        "totals": report.get("totals", {}),
        "chunks": chunks,
    }
+
+
+def mock_all_plans_and_tiers():


Note: Eventually we want to migrate these off of using any enums at all; they really should be agnostic to whatever we have in production

Yep we could do that as part of the clean up bonus ticket

ajay-sentry · 2025-01-24T23:23:43Z

shared/plan/constants.py

+    TRIAL = "trial"
+
+
+def convert_to_DTO(plan) -> dict:


@Swatinem this guy

adrian-codecov · 2025-01-27T22:14:19Z

shared/django_apps/codecov_auth/models.py

@@ -605,7 +605,8 @@ def pretty_plan(self):
        if self.account:
            return self.account.pretty_plan

-        plan_details = Plan.objects.get(name=self.plan)
+        print("@@@@@@@@@@@@@@@@@@@@", self.plan)


Another print 🖨️

ajay-sentry added 2 commits January 15, 2025 16:00

shared stuff should be migrated with this, first try with available_p…

fbbb396

…lans

Merge branch 'main' into Ajay/milestone-3-migration

52fdc2d

ajay-sentry and others added 6 commits January 17, 2025 13:16

boilerplate updates, better way to do the plan searches

33d98f6

update logic a bit to work with new plan, and types as well, start fi…

ba68a8b

…xing tests

43 tests failing locally

3b1deb6

fix some tests:

7eb00e4

uncomment stuff

4fd5090

more tests fixing

7473f20

github-advanced-security bot found potential problems Jan 21, 2025

View reviewed changes

tests/unit/plan/test_plan.py Fixed Show fixed Hide fixed

RulaKhaled added 3 commits January 21, 2025 13:40

5 more to go

ccb28ab

wrap up tests in test_plan

208eb10

resolve all tests

a9a5eeb

ajay-sentry commented Jan 21, 2025

View reviewed changes

tests/unit/plan/test_plan.py Outdated Show resolved Hide resolved

ajay-sentry commented Jan 21, 2025

View reviewed changes

shared/plan/constants.py Outdated Show resolved Hide resolved

ajay-sentry and others added 3 commits January 21, 2025 11:30

update test imports and helper, remove trial_days from DTO, start off…

3dcd8ef

…boarding the consts

lint

6b1fabc

Update with pretty plan changes

906e7bc

adrian-codecov reviewed Jan 22, 2025

View reviewed changes

ajay-sentry changed the title ~~[WIP] Plan Milestone 3 migration~~ Migrate to Plan / Tier Tables in Shared Jan 23, 2025

ajay-sentry changed the title ~~Migrate to Plan / Tier Tables in Shared~~ Shared: Migrate to Plan / Tier Tables Jan 23, 2025

ajay-sentry added 3 commits January 23, 2025 11:41

setupClass from setUp and clean up a bunch of the available plan tests

2cd7436

remove ttestcase if not needed

8277f6d

print

5c8398a

ajay-sentry commented Jan 23, 2025

View reviewed changes

This was referenced Jan 24, 2025

Worker: Migrate to Plan / Tier Tables codecov/worker#1007

Open

API: Migrate to Plan / Tier Tables codecov/codecov-api#1099

Open

ajay-sentry added 2 commits January 24, 2025 10:37

Merge branch 'main' into Ajay/milestone-3-migration

d2c0af3

use cached property, remove setter

7804571

ajay-sentry commented Jan 24, 2025

View reviewed changes

shared/plan/constants.py

TRIAL = "trial"

def convert_to_DTO(plan) -> dict:

Copy link

Contributor Author

ajay-sentry Jan 24, 2025

Choose a reason for hiding this comment

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

@Swatinem this guy

ajay-sentry added 2 commits January 27, 2025 11:06

Merge branch 'main' into Ajay/milestone-3-migration

0e2207e

missed a couple prefetch references

6e1418a

adrian-codecov reviewed Jan 27, 2025

View reviewed changes

adrian-codecov approved these changes Jan 28, 2025

View reviewed changes

nora-codecov approved these changes Jan 28, 2025

View reviewed changes

Merge branch 'main' into Ajay/milestone-3-migration

a4bd8f7

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Shared: Migrate to Plan / Tier Tables #479

Shared: Migrate to Plan / Tier Tables #479

ajay-sentry commented Jan 16, 2025 •

edited

Loading

codecov bot commented Jan 17, 2025 •

edited

Loading

adrian-codecov Jan 22, 2025

adrian-codecov Jan 22, 2025

ajay-sentry Jan 22, 2025

adrian-codecov Jan 23, 2025

ajay-sentry Jan 23, 2025

adrian-codecov Jan 27, 2025

adrian-codecov Jan 22, 2025

ajay-sentry Jan 23, 2025

adrian-codecov Jan 22, 2025

ajay-sentry Jan 24, 2025

adrian-codecov Jan 27, 2025

adrian-codecov Jan 22, 2025

ajay-sentry Jan 23, 2025

adrian-codecov Jan 22, 2025

ajay-sentry Jan 23, 2025

adrian-codecov Jan 22, 2025

ajay-sentry Jan 24, 2025

adrian-codecov Jan 22, 2025

ajay-sentry Jan 23, 2025

adrian-codecov Jan 22, 2025

adrian-codecov Jan 27, 2025

adrian-codecov Jan 22, 2025 •

edited

Loading

ajay-sentry Jan 23, 2025

RulaKhaled Jan 23, 2025

adrian-codecov Jan 22, 2025 •

edited

Loading

ajay-sentry Jan 23, 2025

ajay-sentry Jan 23, 2025

ajay-sentry Jan 23, 2025

RulaKhaled Jan 24, 2025

ajay-sentry Jan 24, 2025

adrian-codecov Jan 27, 2025

		@@ -109,6 +110,24 @@ def convert_to_DTO(self) -> dict:
		}


		def convert_to_DTO(plan) -> dict:

Shared: Migrate to Plan / Tier Tables #479

Are you sure you want to change the base?

Shared: Migrate to Plan / Tier Tables #479

Conversation

ajay-sentry commented Jan 16, 2025 • edited Loading

Description

Legal Boilerplate

codecov bot commented Jan 17, 2025 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adrian-codecov Jan 22, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adrian-codecov Jan 22, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ajay-sentry commented Jan 16, 2025 •

edited

Loading

codecov bot commented Jan 17, 2025 •

edited

Loading

adrian-codecov Jan 22, 2025 •

edited

Loading

adrian-codecov Jan 22, 2025 •

edited

Loading