feat(features) Use dataclasses for flagpole instead of pydantic #75859
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Early timing measurements are looking promising.
This schema was generated with pydantic and will be used in CI to validate that schema and dataclasses are kept compatible, and could be part of a validation flow for flagpole.
github-actions
bot
added
Scope: Frontend
|
🚨 Warning: This pull request contains Frontend and Backend changes! It's discouraged to make changes to Sentry's Frontend and Backend in a single pull request. The Frontend and Backend are not atomically deployed. If the changes are interdependent of each other, they must be separated into two pull requests and be made forward or backwards compatible, such that the Backend or Frontend can be safely deployed independently. Have questions? Please ask in the |
|
Test Failures Detected: Due to failing tests, we cannot provide coverage reports at this time. ❌ Failed Test Results:Completed 21719 tests with View the full list of failed testspytest
|
markstory
commented
Aug 8, 2024
| """The value to compare against the condition's evaluation context property.""" | ||
|
|
||
| operator: str = dataclasses.field(default="") |
There was a problem hiding this comment.
I don't love this, but it is less gross than having to always provide operator to every condition constructor throughout tests.
GabeVillalobos
approved these changes
Aug 13, 2024
| description="A brief description or identifier for the segment" | ||
| def condition_from_dict(data: Mapping[str, Any]) -> ConditionBase: | ||
| operator_kind = ConditionOperatorKind(data.get("operator", "invalid")) | ||
| condition_cls = OPERATOR_LOOKUP[operator_kind] |
There was a problem hiding this comment.
Do we want to raise explicitly here if we get an invalid operator type? Otherwise, won't this be an attribute error on NoneType?
There was a problem hiding this comment.
I can make the raise explicit. Currently it would raise KeyError which isn't as informative as it could be.
Comment on lines
+84
to
+87
| with pytest.raises(ConditionTypeMismatchException): | ||
| not_condition.match( | ||
| context=EvaluationContext({"foo": attr_val}), segment_name="test" | ||
| ) |
There was a problem hiding this comment.
Main reason I went with the assertion util function approach before was because it made it explicit which invalid input failed when modifying condition logic. This is mostly fine, although it does make debugging just a bit harder.
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
These changes switch flagpole from using pydantic as the base classes to
dataclassesfrom stdlib. During the pydantic 2 upgrade the time to parse features shot way up which was unexpected. We have also not been as impressed with feature flag match times and suspected that pydantic might be contributing overhead.The changes of this pull request re-implement flagpole with basic python dataclasses. The new implementation has reduced time to build feature flags, which should help improve the overall performance of our feature flagging. Improvements were measured both with cProfile, and local mini-benchmarks. (scripts provided below)
cProfile results
The cprofile script builds a feature 1000 times and collects profiling data from those operations.
current master (pydantic)
after (dataclasses)
While significantly more functions were called the overall runtime is much better.
flagpole-profile script
Micro benchmark
In the microbenchmark I looked at 3 feature flags (the max, approximate median and smallest features). Each feature would be parsed 10000 times and I looked at the min, mean and max duration for building a Feature object from dictionary data loaded from options seeded with the current feature flag inventory.
before (pydantic)
after (dataclasses)
flagpole-timing script
Again we see significant improvements in runtime without any impact in memory usage.
What we lose?
While moving to dataclasses gives us some gains in performance it has a few drawbacks: