feat(Segment Membership): Skip traitless identities when seeding - #8455
feat(Segment Membership): Skip traitless identities when seeding#8455matthewelwell wants to merge 10 commits into
Conversation
An identity carrying no traits at all can only match a segment by percentage split or `is not set`, and some environments hold millions of them. Leave them out of the ClickHouse mirror, matching the Edge CDC filter that now drops them at the stream. System traits count as traits here, so cohort members are kept. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 8 included reviews per hour; 6 remain after this review. 📝 WalkthroughWalkthroughThe identity seeding task excludes identities with neither identity traits nor system traits. It skips empty batches before inserting data into ClickHouse. Unit tests cover traitless identities and identities with cohort membership in Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to Identity seeding now omits traitless identities while preserving identities with system traits, preventing unnecessary ClickHouse mirror rows without affecting cohort membership. The change is ready to merge. 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 |
Docker builds report
|
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Team
Run ID: 85a2210c-9809-44c8-8865-117e9e467499
📒 Files selected for processing (2)
api/segment_membership/tasks.pyapi/tests/unit/segment_membership/test_unit_segment_membership_tasks.py
Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review.
✅ private-cloud · depot-ubuntu-latest-arm-16 — run #20302 (attempt 2)Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)Details
🗂️ Previous results❌ private-cloud · depot-ubuntu-latest-arm-16 — run #20302 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)Details
Failed testsfirefox › tests/environment-permission-test.pw.ts › Environment Permission Tests › Environment-level permissions control access to features, identities, and segments @enterprise ✅ private-cloud · depot-ubuntu-latest-16 — run #20302 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-16)Details
✅ oss · depot-ubuntu-latest-arm-16 — run #20302 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-arm-16)Details
✅ oss · depot-ubuntu-latest-16 — run #20302 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-16)Details
✅ private-cloud · depot-ubuntu-latest-arm-16 — run #20298 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)Details
✅ private-cloud · depot-ubuntu-latest-16 — run #20298 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-16)Details
✅ oss · depot-ubuntu-latest-16 — run #20299 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-16)Details
✅ oss · depot-ubuntu-latest-16 — run #20298 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-16)Details
✅ oss · depot-ubuntu-latest-arm-16 — run #20298 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-arm-16)Details
✅ private-cloud · depot-ubuntu-latest-16 — run #20275 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-16)Details
✅ private-cloud · depot-ubuntu-latest-arm-16 — run #20275 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)Details
|
Visual Regression19 screenshots compared. See report for details. |
One identity per batch, so the traitless one forms a batch with nothing left to write, and assert no INSERT is issued with an empty row list. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
emyller
left a comment
There was a problem hiding this comment.
Ran
tests/unit/segment_membership— 48 passed. The ClickHouse-marked tests were deselected; I don't have a local ClickHouse to run them against.🤖 Generated with Claude Code
I'm pretty sure you do. ClickHouse related tests seem to fail.
Also, a couple comments.
Minor code refactor for readability
Thanks for the review (and the slop tolerance!) I've fixed the test, and actioned the comments. I'll be honest, I'm still not 100% confident on the compromise, but I think it's the right decision. Given the implementation here though (where we're skipping empty identities for all environments), we might want to update the CDC side to only send non-empty identities for all environments too? What do you think? |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #8455 +/- ##
========================================
Coverage 98.81% 98.82%
========================================
Files 1621 1622 +1
Lines 66300 66726 +426
========================================
+ Hits 65515 65941 +426
Misses 785 785 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
emyller
left a comment
There was a problem hiding this comment.
The code looks like what we need, but I'd suggest another anti-slop pass for the sake of our better understanding of this decision, and why we made it, in the future.
Co-authored-by: Evandro Myller <22429+emyller@users.noreply.github.com>
If I understand all this correctly: this PR caters for historical identities, while https://github.com/Flagsmith/edge-api/pull/714 caters for live identities, so yes, I guess they should go together. |
…eat/seed-skip-traitless
I agree. I've added this PR stacked on top of Flagsmith/edge-api#714 which adds support for |
There was a problem hiding this comment.
Reviewing the other pull request led me to think we're potentially missing on an opportunity to apply the filter at DDB query-level, and save on resources/cost/time.
Given we're already dragging this and the above is a late finding, I'm ready to approve as to unblock the goal, but would love a follow up.
docs/if required so people know about the feature.Changes
Contributes to https://github.com/Flagsmith/edge-api/pull/714
Ignores empty1 identities when seeding identity data for segment membership purposes.
This comes with a slight compromise in that percentage split segments and segments containing only 'Is not set' operators may report inaccurate counts, but is an accepted compromise in order to reduce the amount of data in ClickHouse for now.
1 an empty identity is an identity with
identity_traits == [] and system_traits == []How did you test this code?
Added
test_seed_organisation_identities__traitless_identities__are_not_mirrored, covering an identity with no traits (dropped) and one carrying only a system trait (kept).