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

feat(limits): Add limits to features, segments and segment overrides #2480

Merged
merged 15 commits into from
Jul 21, 2023
Merged

feat(limits): Add limits to features, segments and segment overrides #2480

merged 15 commits into from
Jul 21, 2023

Conversation

gagantrivedi
Copy link
Member

@gagantrivedi gagantrivedi commented Jul 20, 2023 •
edited

Thanks for submitting a PR! Please check the boxes below:

  • I have run pre-commit to check linting
  • I have filled in the "Changes" section below?
  • I have filled in the "How did you test this code" section below?
  • I have used a Conventional Commit title for this Pull Request

Changes

Limit number of segments, segment override and feature using environment variables

How did you test this code?

Adds unit test cases

@vercel
Copy link

vercel bot commented Jul 20, 2023 •
edited

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jul 21, 2023 10:16am
flagsmith-frontend-preview ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jul 21, 2023 10:16am
flagsmith-frontend-staging ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jul 21, 2023 10:16am

@github-actions github-actions bot added the api Issue related to the REST API label Jul 20, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Jul 20, 2023 •
edited

Uffizzi Preview deployment-31365 was deleted.

@gagantrivedi gagantrivedi linked an issue Jul 20, 2023 that may be closed by this pull request
Implement limits on the number of project and environment entities #2477
Closed
@gagantrivedi gagantrivedi marked this pull request as ready for review July 20, 2023 06:34
@matthewelwell
Copy link
Contributor

Check that the limit makes sense

A few things here:

  • Do we want to optimise for the dynamodb limit?
  • Do we want to optimise for memory usage in the task processor containers?
  • What happens to the size of the document as we max out a 'regular' environment?
  • We need to add validation to migrating to edge and possibly to even generating the environment document from the Core API - we have a customer that has ~1700 flags that is still using our core API. If they were to migrate to the Edge API, it may cause us some further issues.

Here are some statistics from some of our larger environments:

image

... and the statistics from the top users of segments:

Screenshot 2023-07-20 at 09 42 55

... and the statistics from the top users of features:

Screenshot 2023-07-20 at 09 44 48

... and the statistics from the top users of segment overrides

Screenshot 2023-07-20 at 10 08 15

Based on this, I would say that we can increase the limits you have mentioned here to perhaps:

  • 100 segments.
  • 300 features
  • 100 segment overrides

Obviously these statistics all depend heavily on what the values for these features are. Large JSON documents will of course cause an issue.

Thoughts please @dabeeeenster @gagantrivedi

matthewelwell
matthewelwell reviewed Jul 20, 2023
View reviewed changes
api/segments/serializers.py Show resolved Hide resolved
api/features/serializers.py Outdated
Comment on lines 450 to 461
def validate_environment_segment_override_limit(
self, environment: Environment
) -> None:
if (
environment.feature_segments.count()
>= settings.MAX_SEGMENT_OVERRIDE_ALLOWED
):
raise serializers.ValidationError(
{
"environment": "The environment has reached the maximum allowed segments overrides limit."
}
)
Copy link
Contributor

Choose a reason for hiding this comment

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

I have a feeling we may also need to add this to the SimpleFeatureStateSerializer?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think that deal with segment override, right? Unless we want to limit identity overrides?

Copy link
Contributor

Choose a reason for hiding this comment

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

It does, yeah - it's how the UI used to manage segment overrides before we added the endpoint to do it in a single call.

Copy link
Contributor

Choose a reason for hiding this comment

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

We can do this in another PR, I'd like to test this in staging and release it asap.

@gagantrivedi
Copy link
Member Author

Check that the limit makes sense

A few things here:

* Do we want to optimise for the dynamodb limit?

* Do we want to optimise for memory usage in the task processor containers?

* What happens to the size of the document as we max out a 'regular' environment?

* We need to add validation to migrating to edge and possibly to even generating the environment document from the Core API - we have a customer that has ~1700 flags that is still using our core API. If they were to migrate to the Edge API, it may cause us some further issues.

Here are some statistics from some of our larger environments:
image

... and the statistics from the top users of segments:
Screenshot 2023-07-20 at 09 42 55

... and the statistics from the top users of features:
Screenshot 2023-07-20 at 09 44 48

... and the statistics from the top users of segment overrides
Screenshot 2023-07-20 at 10 08 15

Based on this, I would say that we can increase the limits you have mentioned here to perhaps:

* 100 segments.

* 300 features

* 100 segment overrides

Obviously these statistics all depend heavily on what the values for these features are. Large JSON documents will of course cause an issue.

Thoughts please @dabeeeenster @gagantrivedi

I think in an ideal world I would want to build the document on every update (for edge project) and see if it still under the dynamo limit

@matthewelwell
Copy link
Contributor

I think in an ideal world I would want to build the document on every update (for edge project) and see if it still under the dynamo limit

Hmm... yes, that's going to be way too resource intensive though so I think we need to just implement sensible limits here and deal with the edge cases as they happen. This should at least prevent anyone from generating an environment document that exceeds the memory limits of the task processor instances.

@matthewelwell matthewelwell mentioned this pull request Jul 21, 2023
Closed
@matthewelwell matthewelwell merged commit d150c7f into main Jul 21, 2023
16 checks passed
@matthewelwell matthewelwell deleted the feat/limit-objects branch July 21, 2023 10:29
@dabeeeenster dabeeeenster mentioned this pull request Jul 21, 2023
Merged
@matthewelwell matthewelwell mentioned this pull request Jul 24, 2023
Closed
6 tasks
@dabeeeenster dabeeeenster mentioned this pull request Aug 22, 2023
Closed
@dabeeeenster dabeeeenster mentioned this pull request Aug 29, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@matthewelwell matthewelwell matthewelwell approved these changes

Assignees
No one assigned
Labels
api Issue related to the REST API
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Implement limits on the number of project and environment entities
2 participants
@gagantrivedi @matthewelwell