feat(Segments): Write rules as plain JSON - #8245
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 3 Skipped Deployments
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe change adds typed, persisted Estimated code review effort: 4 (Complex) | ~60 minutes Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
e997681 to
d1b4523
Compare
Docker builds report
|
There was a problem hiding this comment.
Actionable comments posted: 5
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 405482df-5f69-4b7d-b284-074ab96f0eb7
📒 Files selected for processing (15)
api/segments/migrations/0031_add_segment_rules_data.pyapi/segments/models.pyapi/segments/serializers.pyapi/segments/services.pyapi/segments/types.pyapi/segments/validators.pyapi/tests/conftest.pyapi/tests/types.pyapi/tests/unit/segments/conftest.pyapi/tests/unit/segments/test_unit_segments_models.pyapi/tests/unit/segments/test_unit_segments_services.pyapi/tests/unit/segments/test_unit_segments_views.pydocs/docs/deployment-self-hosting/observability/_events-catalogue.mdmcp/src/flagsmith_mcp/openapi.jsonopenapi.yaml
✅ private-cloud · depot-ubuntu-latest-arm-16 — run #19227 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)Details
🗂️ Previous results✅ private-cloud · depot-ubuntu-latest-16 — run #19227 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-16)Details
✅ oss · depot-ubuntu-latest-arm-16 — run #19227 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-arm-16)Details
✅ oss · depot-ubuntu-latest-16 — run #19227 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-16)Details
✅ private-cloud · depot-ubuntu-latest-16 — run #19225 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-16)Details
✅ private-cloud · depot-ubuntu-latest-arm-16 — run #19225 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)Details
✅ oss · depot-ubuntu-latest-arm-16 — run #19226 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-arm-16)Details
✅ oss · depot-ubuntu-latest-arm-16 — run #19225 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-arm-16)Details
✅ oss · depot-ubuntu-latest-16 — run #19226 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-16)Details
✅ oss · depot-ubuntu-latest-16 — run #19225 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-16)Details
✅ private-cloud · depot-ubuntu-latest-arm-16 — run #19223 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)Details
✅ private-cloud · depot-ubuntu-latest-16 — run #19223 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-16)Details
Skipped testsfirefox › tests/onboarding-tests.pw.ts › Onboarding › New user connects via the single-page onboarding flow @oss |
Visual Regression19 screenshots compared. See report for details. |
ef93dd7 to
04b05a6
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 8f8878c4-fd1a-4a42-af9c-99b117204a52
📒 Files selected for processing (7)
api/integrations/launch_darkly/services.pyapi/segments/migrations/0031_add_segment_rules_data.pyapi/segments/serializers.pyapi/segments/types.pyapi/tests/conftest.pyapi/tests/unit/segments/test_unit_segments_migrations.pydocs/docs/deployment-self-hosting/observability/_events-catalogue.md
💤 Files with no reviewable changes (1)
- api/tests/conftest.py
9eff8a3 to
2904c84
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
api/integrations/launch_darkly/services.py (1)
1195-1198: 🗄️ Data Integrity & Integration | 🔴 Critical | ⚡ Quick winPersist
root_ruleinstead of undefinedsubrules.Line 1196 references
subrules, but_create_segments_from_ldonly definesroot_rule. Every imported non-deleted LaunchDarkly segment reaches this statement and raisesNameError. The import can then leave legacy rows withoutrules_data.Proposed fix
- segment.rules_data = [ - {"type": constants.ALL_RULE, "conditions": [], "rules": subrules} - ] + segment.rules_data = [root_rule]
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: ea285ce1-bc5a-44bb-9fdd-b9ceeba0171a
📒 Files selected for processing (7)
api/experimentation/services.pyapi/integrations/launch_darkly/services.pyapi/segments/types.pyapi/tests/unit/experimentation/test_services.pyapi/tests/unit/integrations/launch_darkly/test_services.pyapi/tests/unit/segments/test_unit_segments_views.pydocs/docs/deployment-self-hosting/observability/_events-catalogue.md
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: a1f34a67-e61b-4ecc-89b7-61014629105c
📒 Files selected for processing (5)
api/integrations/launch_darkly/services.pyapi/segments/migrations/0031_add_segment_rules_data.pyapi/tests/unit/integrations/launch_darkly/snapshots/test_process_import_request__large_segments__correctly_imported__rules_data.jsonapi/tests/unit/integrations/launch_darkly/test_services.pyapi/tests/unit/segments/test_unit_segments_migrations.py
💤 Files with no reviewable changes (2)
- api/tests/unit/segments/test_unit_segments_migrations.py
- api/tests/unit/integrations/launch_darkly/snapshots/test_process_import_request__large_segments__correctly_imported__rules_data.json
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
api/segments/migrations/0031_add_segment_rules_data.py (1)
56-60: 🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy liftAvoid per-rule queries below the prefetch depth.
When a rule is below the two prefetched rule levels,
_rasterise_segment_rule()executes one query forconditionsand one query forrules. The existing three-level test already triggers both fallback queries. Deeper trees add two queries per rule, andBATCH_SIZElimits segments, not descendant rules.Load active rules and conditions once per batch, or prefetch the supported legacy depth. Add query-count coverage for three- and four-level trees.
Source: Learnings
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 3626a794-8918-439f-9ead-8038f03087f4
📒 Files selected for processing (9)
api/integrations/launch_darkly/services.pyapi/segments/migrations/0031_add_segment_rules_data.pyapi/segments/serializers.pyapi/tests/unit/integrations/launch_darkly/snapshots/test_process_import_request__large_segments__correctly_imported__rules_data.jsonapi/tests/unit/integrations/launch_darkly/test_services.pyapi/tests/unit/segments/test_unit_segments_migrations.pydocs/docs/deployment-self-hosting/observability/_events-catalogue.mdmcp/src/flagsmith_mcp/openapi.jsonopenapi.yaml
💤 Files with no reviewable changes (3)
- api/tests/unit/segments/test_unit_segments_migrations.py
- api/tests/unit/integrations/launch_darkly/snapshots/test_process_import_request__large_segments__correctly_imported__rules_data.json
- api/segments/serializers.py
I have added information todocs/if required so people know about the feature.Changes
Closes #7815
This is the first step towards getting rid of
SegmentRuleandConditionmodels, and the tree complexity they have brought upon us thus far.We achieve:
Segmentmodel earns a newrules_dataattributerules_datarules_data.rules_data.Unplanned things we also achieve:
Chose not to achieve:
rulesshould need no special treatment as a JSON field, properly validated by the interface.Deleted LoC will look great in #7818!
How did you test this code?
Exhaustive unit testing. The migration was also benchmarked and optimised.
Important
This includes a data migration to backfill active segments with JSON rules. The migration code was exhaustively tested and tweaked. On a very limited Postgres local container (1 CPU, 2 GB memory), it took ~16s to rasterise rules of 100K segments with varying depth of rules.