feat: Use get-metadata-subscription to get max_api_calls #2279
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
Uffizzi Preview |
frontend/web/components/App.js
Outdated
| @@ -254,6 +258,17 @@ const App = class extends Component { | |||
| this.setState({ totalApiCalls: res[0]?.data?.totals.total }) | |||
| }) | |||
| } | |||
| const subscriptionModel = OrganisationStore.getSubscriptionMeta() | |||
There was a problem hiding this comment.
One thing that I'm concerned about here is that for each request the client makes to get-subscription-metadata, we are making a call to Chargebee's API. What that means is that:
a. This endpoint is not the most performant endpoint we have (as we're reliant on the performance of an external API). We should therefore ensure that we're not blocking anything waiting on the response from this endpoint. Maybe that's already happening here, but I'd like to confirm that.
b. For each request to this endpoint on our side, we trigger a request to Chargebee's API which has rate limits, etc. so we should be careful about abusing it.
Here are the CB API limits. We are on the 'Rise' plan, as per the highlighted section.
Based on the above, is there anything we can do to optimise the requests from the frontend? Or, perhaps we should optimise this on the BE? Store some kind of additional cache of a customer's subscription information after a successful request from CB?
I'm guessing that by adding this here, we're triggering the request on every successful load of the Flagsmith dashboard. I don't know where our current traffic is right now, but I don't imagine this number is beyond the realms of possibility.
There was a problem hiding this comment.
Can we just store this with a 24 hour TTL in our DB? I thought we were already storing this data in our database? Isn't that what organisations_organisationsubscriptioninformationcache is for?
There was a problem hiding this comment.
Can we just store this with a 24 hour TTL in our DB?
Yes, I think that's what I would suggest.
I thought we were already storing this data in our database? Isn't that what organisations_organisationsubscriptioninformationcache is for?
Currently this table is only used for the sales dashboard. We could, however, expose this to the frontend for sure. We should make sure we implement the recurring task before we do that though.
There was a problem hiding this comment.
@matthewelwell do we have in place a job to execute the update of this data? That is what you are talking about when you say "recurring task" right?
There was a problem hiding this comment.
Yep, you can see the functionality here.
There was a problem hiding this comment.
TODO:
- Use CB webhook to keep OrganisationSubscriptionInformationCache up to date with max_seats and max_api_calls
- Update the get-subscription-metadata endpoint to use the OrganisationSubscriptionInformationCache object
- Update the current task which updates the OrganisationSubscriptionInformationCache models to ONLY update influx data
To clarify, we should continue using live influx data in the frontend dashboard.
…ate update_caches function
github-actions
bot
added
api
There was a problem hiding this comment.
Some general feedback is that we should try and extract logic to well named functions / methods here too. We're cramming a lot into the chargebee webhook view for example. I know the code for that view is already a bit messy, but we shouldn't add to the mess, we should try to improve it!
api/organisations/views.py
Outdated
| @@ -278,11 +283,37 @@ def chargebee_webhook(request): | |||
| ) | |||
| logger.error(error_message) | |||
| return Response(status=status.HTTP_200_OK) | |||
| existing_organisation_subscription_information_cache = ( | |||
| OrganisationSubscriptionInformationCache.objects.filter( | |||
| organisation_id=existing_subscription.id | |||
There was a problem hiding this comment.
| organisation_id=existing_subscription.id | |
| organisation_id=existing_subscription.organisation_id |
There was a problem hiding this comment.
This could have been really bad! We'd have ended up updating the data for the wrong organisations because we were using the id of the subscription.
There was a problem hiding this comment.
Yes, horrible. Corrected
api/organisations/views.py
Outdated
|
|
||
| subscription_status = subscription_data.get("status") | ||
| if subscription_status == "active": | ||
| if subscription_data.get("plan_id") != existing_subscription.plan: | ||
| existing_subscription.update_plan(subscription_data.get("plan_id")) | ||
| if existing_organisation_subscription_information_cache: |
There was a problem hiding this comment.
This (including the above) could all be simplified by using OrganisationSubscriptionInformationCache.objects.update_or_create() I believe?
| self.payment_source.title(), | ||
| self.payment_source.title() | ||
| if self.payment_source is not None | ||
| else "no title", |
There was a problem hiding this comment.
It's not a title don't forget, the .title() is converting the string to title case, not getting a title attribute!
We should use something like "unknown payment source"
| @@ -13,7 +13,7 @@ | |||
| ] | |||
|
|
|||
|
|
|||
| def update_caches(): | |||
| def update_caches(update_influx=False): | |||
There was a problem hiding this comment.
typehinting please!
Also, this is a bit messy as we're implicitly updating chargebee if update influx is false but only updating influx if it's true. We should either just create 2 completely separate functions, or have one function that takes an attribute of update_from_chargebee: bool = False and then always update influx, but only update chargebee if the attribute is set.
There was a problem hiding this comment.
I think my preference is for having 2 separate functions and just get rid of this one entirely, unless there's a reason to keep it.
There was a problem hiding this comment.
The code for those 2 functions would be almost exactly the same. I kept one function but clarified the parameter name and type.
api/organisations/views.py
Outdated
|
|
||
| subscription_status = subscription_data.get("status") | ||
| if subscription_status == "active": | ||
| if subscription_data.get("plan_id") != existing_subscription.plan: | ||
| existing_subscription.update_plan(subscription_data.get("plan_id")) | ||
| if existing_organisation_subscription_information_cache: | ||
| metadata = get_subscription_metadata( |
There was a problem hiding this comment.
Do we need to do this? Isn't the data already available in the chargebee webhook payload? Maybe we still need to check the plan / addons against the chargebee cache, but I think that, by calling this function, we're making another outbound request to chargebee unnecessarily, right?
There was a problem hiding this comment.
Improved as per our conversation in this regard.
| @@ -1,4 +1,4 @@ | |||
| import typing | |||
| from typing import Tuple | |||
There was a problem hiding this comment.
Not really an issue, but I'm not sure why these changes have snuck into this PR.
| max_seats: int, | ||
| max_api_calls: int, | ||
| max_projects: int, | ||
| chargebee_updated_at: datetime = datetime.now(), |
There was a problem hiding this comment.
While this is probably ok in tests, it's bad practice to use function returns as default arguments as they are evaluated when the module is loaded. As such, the chargebee_updated_at will always take the value that python evaluates when the module is loaded.
| } | ||
| } | ||
|
|
||
| # When | ||
| if plan_id != "updated-plan-id": |
There was a problem hiding this comment.
Hmm... I don't like that we're basing some logic here on a hard coded string that might get passed in. Can't we just pass a boolean as an argument to this method?
There was a problem hiding this comment.
If we had moved these tests to be pytest based function tests, then we could have just used parameterisation and this could all have been much neater.
| max_projects = 3 # The maximum number of projects allowed in the current plan | ||
|
|
||
| # When | ||
| res, subscription_information_cache = self._get_mocked_subscription_plan_data( |
There was a problem hiding this comment.
My concern here (and why I think we should have used parameterisation instead) is that, when you look at the test in isolation, it looks like we're testing some mocked data which isn't very useful.
There was a problem hiding this comment.
@matthewelwell Tests changed as suggested. Can you please take a look at them?
| @mock.patch("organisations.models.get_plan_meta_data") | ||
| @mock.patch("organisations.views.extract_subscription_metadata") | ||
| def test_when_plan_is_changed_max_seats_and_max_api_calls_are_updated( | ||
| mock_get_subscription_metadata, |
There was a problem hiding this comment.
| mock_get_subscription_metadata, | |
| mock_extract_subscription_metadata, |
| assert subscription_information_cache.chargebee_updated_at | ||
| assert subscription_information_cache.influx_updated_at is None | ||
| if is_updated: | ||
| assert subscription_information_cache.chargebee_updated_at != updated_at |
There was a problem hiding this comment.
This should be >, no?
There was a problem hiding this comment.
We could also use pytest-freezegun if we want to be accurate with these timestamps.
There was a problem hiding this comment.
This should be
>, no?
Corrected.
We could also use
pytest-freezegunif we want to be accurate with these timestamps.
Added and used
| ) | ||
|
|
||
| subscription.refresh_from_db() | ||
| subscription_information_cache.refresh_from_db() |
There was a problem hiding this comment.
This is unnecessary since we're getting it from the database directly above.
There was a problem hiding this comment.
subscription_information_cache.refresh_from_db(), deleted.
…datedtest_view test
matthewelwell
deleted the
fix/use-get-metadata-subscription-for-max-api-calls
branch
Thanks for submitting a PR! Please check the boxes below:
pre-committo check lintingChanges
Get
value of max_api_callsfromget-metadata-subscriptionendpoint.How did you test this code?
In order to test locally
max_api_calls_alertfor your identityFLAGSMITH_API_URLto productionmax_api_callsortotal_api_callsreturned by the cooresponding endpoint. You can also change the hardcoded condition incommon/utils/utiils.txL58, that currently triggers the alert on 70% of usage and above in order to force the display of the alert message.