Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions api/segment_membership/tasks.py
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,9 @@ def seed_organisation_identities(organisation_id: int) -> None:

Rows are versioned at scan start via `inserted_at`
so writes arriving mid-scan win ReplacingMergeTree dedup over the seeded row.

Identities carrying no traits at all are skipped, keeping the mirror off the
large populations of empty identities some environments accumulate.
Comment on lines +65 to +66

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
Identities carrying no traits at all are skipped, keeping the mirror off the
large populations of empty identities some environments accumulate.
Identities carrying no traits are skipped as they carry little to no value for segment membership.

Deixis. If we need to mention a temporary context, consider adding a TODO pointing to an issue in which we discuss stop persisting ephemeral identities.

"""
log = logger.bind(organisation__id=organisation_id)
if not settings.CLICKHOUSE_ENABLED:
Expand Down Expand Up @@ -104,7 +107,13 @@ def seed_organisation_identities(organisation_id: int) -> None:
scan_started_at,
)
for doc in batch
# An identity with nothing on it can only match
# a segment by percentage split or `is not set`.
Comment on lines +110 to +111

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Sorry I don't see how this comment warrants skipping identities. As I see, both percentage split and is not set are perfectly valid segment use case; we're skipping because we need to avoid ephemeral identities and checking for empty traits happens to match how a certain customer adds them.

if doc.get("identity_traits")
or doc.get("system_traits")
]
if not rows:
continue
# Django's CursorWrapper stub forbids dicts in
# the params sequence; clickhouse-driver accepts
# them as JSON-column payloads.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -151,6 +151,53 @@ def test_seed_organisation_identities__insert_fails__logs_and_continues(
]


def test_seed_organisation_identities__traitless_identities__are_not_mirrored(
mocker: MockerFixture,
settings: SettingsWrapper,
project: Project,
environment: Environment,
segment: Segment,
flagsmith_identities_table: Table,
dynamo_identities: None,
enable_features: EnableFeaturesFixture,
) -> None:
# Given
enable_features("segment_membership_inspection")
settings.CLICKHOUSE_ENABLED = True
# One identity per batch, so `dave` forms a batch with nothing left to write.
mocker.patch.object(tasks, "_INSERT_BATCH_SIZE", 1)
for identifier, extra in (
("dave", {}),
("erin", {"system_traits": {"flagsmith_cohort_e2b1": True}}),
):
Comment thread
coderabbitai[bot] marked this conversation as resolved.
flagsmith_identities_table.put_item(
Item={
"composite_key": f"{environment.api_key}_{identifier}",
"environment_api_key": environment.api_key,
"identifier": identifier,
"identity_uuid": f"f47ac10b-58cc-4372-a567-0e02b2c3d4{identifier[:2]}",
"identity_traits": [],
**extra,
}
)
cursor = MagicMock()
open_cursor = mocker.patch.object(tasks, "open_clickhouse_cursor")
open_cursor.return_value.__enter__.return_value = cursor
mocker.patch.object(tasks, "enqueue_membership_refresh")

# When
seed_organisation_identities(project.organisation_id)

# Then
# `dave` has nothing on him at all; `erin`'s cohort membership lives in
# `system_traits`, so she stays in the mirror.
payloads = [call.args[1] for call in cursor.executemany.call_args_list]
mirrored_identifiers = sorted(row[1] for payload in payloads for row in payload)
assert mirrored_identifiers == ["alice", "carol", "erin"]
# `dave`'s batch is skipped outright rather than written as an empty INSERT.
assert all(payloads)


@pytest.mark.clickhouse
def test_seed_organisation_identities__matching_identities__inserts_rows_versioned_at_scan_start(
mocker: MockerFixture,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -618,7 +618,7 @@ Attributes:
### `segment_membership.refresh.project.completed`

Logged at `info` from:
- `api/segment_membership/tasks.py:266`
- `api/segment_membership/tasks.py:275`

Attributes:
- `membership_counts.count`
Expand All @@ -628,16 +628,16 @@ Attributes:
### `segment_membership.refresh.project.failed`

Logged at `exception` from:
- `api/segment_membership/tasks.py:239`
- `api/segment_membership/tasks.py:248`

Attributes:
- `project.id`

### `segment_membership.refresh.project.skipped`

Logged at `info` from:
- `api/segment_membership/tasks.py:206`
- `api/segment_membership/tasks.py:218`
- `api/segment_membership/tasks.py:215`
- `api/segment_membership/tasks.py:227`

Attributes:
- `project.id`
Expand All @@ -647,7 +647,7 @@ Attributes:
### `segment_membership.seed.environment.completed`

Logged at `info` from:
- `api/segment_membership/tasks.py:121`
- `api/segment_membership/tasks.py:130`

Attributes:
- `environment.id`
Expand All @@ -658,7 +658,7 @@ Attributes:
### `segment_membership.seed.environment.failed`

Logged at `exception` from:
- `api/segment_membership/tasks.py:114`
- `api/segment_membership/tasks.py:123`

Attributes:
- `environment.id`
Expand All @@ -668,9 +668,9 @@ Attributes:
### `segment_membership.seed.skipped`

Logged at `warning` from:
- `api/segment_membership/tasks.py:67`
- `api/segment_membership/tasks.py:72`
- `api/segment_membership/tasks.py:77`
- `api/segment_membership/tasks.py:70`
- `api/segment_membership/tasks.py:75`
- `api/segment_membership/tasks.py:80`

Attributes:
- `organisation.id`
Expand Down
Loading