feat(Segment Membership): Seed identities on Beta opt-in#7899
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 3 Skipped Deployments
|
Run seed tests against live ClickHouse with a mocked Dynamo source, inline the pending-task check, assert the whole skip event, use the ClickHouseIdentityRow type, and replace the management command with a Django admin re-seed action that clears the seed marker. beep boop Claude-Session: https://claude.ai/code/session_01EgZ5iHpDASZzCapiHRxHLB
for more information, see https://pre-commit.ci
…t-in Replace the daily all-org backfill with an on-demand seed_organisation_identities(organisation_id) task and a lightweight reconcile_segment_membership_seeds tick that fires it once per opted-in org, tracked by a SegmentMembershipSeed marker. Seeded rows are versioned at scan start so a CDC write landing mid-scan wins ReplacingMergeTree dedup. Keep a count-refresh safety-net as a separate recurring refresh_all_segment_counts task (cadence via SEGMENT_MEMBERSHIP_REFRESH_INTERVAL_HOURS, default 6) so cached counts track CDC identity churn between segment edits. All refresh enqueues - edit-triggered, seed fan-out, and the recurring sweep - route through enqueue_membership_refresh, the single flag and debounce gate, so a project never has duplicate refreshes queued. Add a Django admin action to force a re-seed by clearing the marker. beep boop Claude-Session: https://claude.ai/code/session_01EgZ5iHpDASZzCapiHRxHLB
Docker builds report
|
Playwright Test Results (oss - depot-ubuntu-latest-16)Details
Failed testsfirefox › tests/flag-tests.pw.ts › Flag Tests › Feature flags can be created, toggled, edited, and deleted across environments @oss Details
Playwright Test Results (private-cloud - depot-ubuntu-latest-16)Details
Failed testsfirefox › tests/environment-permission-test.pw.ts › Environment Permission Tests › Environment-level permissions control access to features, identities, and segments @enterprise Details
Playwright Test Results (oss - depot-ubuntu-latest-arm-16)Details
Playwright Test Results (private-cloud - depot-ubuntu-latest-16)Details
Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)Details
Playwright Test Results (oss - depot-ubuntu-latest-16)Details
Playwright Test Results (oss - depot-ubuntu-latest-arm-16)Details
Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)Details
Playwright Test Results (private-cloud - depot-ubuntu-latest-16)Details
Playwright Test Results (oss - depot-ubuntu-latest-16)Details
Playwright Test Results (oss - depot-ubuntu-latest-arm-16)Details
Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)Details
Playwright Test Results (private-cloud - depot-ubuntu-latest-16)Details
|
Visual Regression17 screenshots compared. See report for details. |
…t touch Prometheus Admin modules load during Django's admin autodiscovery on every manage.py startup. Importing tasks (→ metrics) at module top instantiated Prometheus collectors at import time, writing to PROMETHEUS_MULTIPROC_DIR in a context without write access and crashing container boot. Import the task lazily in the action, matching the pattern in services.py. beep boop Claude-Session: https://claude.ai/code/session_01EgZ5iHpDASZzCapiHRxHLB
ed910aa to
6c9bb4c
Compare
…ATION_LOGGERS The seed, reconcile and refresh tasks log under the `segment_membership` logger, which wasn't in the default APPLICATION_LOGGERS list, so their structlog events were suppressed in deployed environments. beep boop Claude-Session: https://claude.ai/code/session_01EgZ5iHpDASZzCapiHRxHLB
emyller
left a comment
There was a problem hiding this comment.
Looks good overall. Just a few topics to cover.
| log = logger.bind(organisation__id=organisation_id) | ||
| if not settings.CLICKHOUSE_ENABLED: | ||
| logger.info("backfill.skipped", reason="clickhouse_not_configured") | ||
| log.info("seed.skipped", reason="clickhouse_not_configured") |
There was a problem hiding this comment.
I think log.info might not be the best reaction here. Given seed_organisation_identities is only called when it's intended, being unable to continue should raise, or at least log.warning if we need it not to raise.
| return | ||
|
|
||
| organisation = Organisation.objects.get(pk=organisation_id) | ||
| if not is_membership_enabled(organisation): |
There was a problem hiding this comment.
It seems the only path calling this task already owns this responsibility and has organisations filtered by this flag. I also believe this feature check doesn't belong here, but in the caller, unless the live state — i.e. when the task runs — is relevant.
| project_ids = Segment.live_objects.filter( | ||
| project__organisation=organisation | ||
| ).values_list("project_id", flat=True) | ||
| for project in Project.objects.filter(id__in=project_ids).iterator(): |
There was a problem hiding this comment.
I sense there's a bit of unnecessary stretch here in the data structure:
- In filtering
organisationon the join [withproject] level - In using a server-side cursor for a likely small queryset
| project_ids = Segment.live_objects.filter( | |
| project__organisation=organisation | |
| ).values_list("project_id", flat=True) | |
| for project in Project.objects.filter(id__in=project_ids).iterator(): | |
| projects_with_live_segments = Project.objects.filter( | |
| organisation=organisation, | |
| ).filter(Exists(Segment.live_objects.filter(project=OuterRef("pk")))) | |
| for project in projects_with_live_segments: |
| project__id=project.id, | ||
| environment__id=env.id, | ||
| ) | ||
| continue |
There was a problem hiding this comment.
I appreciate this may be out of scope, but I'd be interested in learning what went wrong here when seeding fails.
| run_every=timedelta(hours=1), | ||
| timeout=timedelta(minutes=5), | ||
| ) | ||
| def reconcile_segment_membership_seeds() -> None: |
There was a problem hiding this comment.
I understand why this exists, but I think more and more the organisations catered for here will be fewer, until we EOL the feature flag and it reaches zero.
Please update this with a TODO for future cleanup if you agree.
There was a problem hiding this comment.
After reading on and learning about refresh_all_segment_counts, I wonder if that task could also include opted_in - seeded organisations?
I think I'd prefer a temporary branch in a permanent task, rather than this temporary task, unless we want to collect any specific data?
| """Refresh counts for every project with a live segment on a slow cadence so | ||
| cached counts track identities ingested via CDC between segment edits. | ||
| `enqueue_membership_refresh` is the single flag + debounce gate. | ||
| """ |
There was a problem hiding this comment.
| """Refresh counts for every project with a live segment on a slow cadence so | |
| cached counts track identities ingested via CDC between segment edits. | |
| `enqueue_membership_refresh` is the single flag + debounce gate. | |
| """ | |
| """Refresh counts for every project with a live segment""" |
Thanks for submitting a PR! Please check the boxes below:
docs/if required so people know about the feature.Changes
Closes #7471.
In this PR, we convert
backfill_identities_to_clickhouseto a seed backfill expected to run once per org.seed_organisation_identities(org_id)— on-demand one-shot seed for one org. Stampsinserted_atat scan start so a CDC write landing mid-scan wins ReplacingMergeTree dedup.reconcile_segment_membership_seeds(hourly) — fires the seed once per allowed org, tracked bySegmentMembershipSeedmodel.refresh_all_segment_countsevery 6 hours.SegmentMembershipSeedfor a given org).All tasks that run the expensive ClickHouse queries are debounced.
Review complexity: 2/5.
How did you test this code?
Added new tests and modified existing where needed; will test extensively in staging.