Repository navigation
fix!: group_identify and alias never guess a distinct id - #1053
Conversation
396dfe8 to
7ed86bf
Compare
51b2482 to
ff2fed1
Compare
7ed86bf to
4dbd65a
Compare
ff2fed1 to
d205df8
Compare
4dbd65a to
3e65c58
Compare
dustinbyrne
left a comment
There was a problem hiding this comment.
AI-assisted review.
One correctness issue remains: the new synthetic group distinct ID can exceed capture v1’s byte limit and reject otherwise valid group updates. I reproduced the boundary through both clients against the immediate base, using the pinned Rust validator in an isolated harness. There is also an optional migration-guide contradiction.
| """ | ||
| distinct_id, personless = get_identity_state(passed) | ||
| if personless: | ||
| return f"${group_type}_{group_key}" |
There was a problem hiding this comment.
[P1] Bound the synthetic group identity without rejecting valid group keys
With no explicit or contextual distinct ID, this supported call now generates a 202-byte root distinct_id:
with posthog.new_context(fresh=True):
client.group_identify("company", "k" * 193, {"plan": "pro"})$company_ contributes nine bytes. Capture v1 rejects distinct IDs over 200 UTF-8 bytes: at backend 172974f3fe4bedbf3a1c2d6c43761a2a78e33ff7, rust/capture/src/v1/analytics/constants.rs:96 defines the limit and process.rs:680–682 returns DistinctIdTooLarge. Validation failures become per-event drops before publication (process.rs:545–554). The group key itself fits the 400-character field in posthog/models/group/group.py:9; group processing/storage passes it through to the upsert.
This is introduced by this PR: at immediate base d205df87685fd792a4762e48875662a9ad3dd9a6, the omitted-ID path generates a 36-character UUID independent of the key length. Both clients now use this shared helper. The optional distinct_id parameter, group-identify specification’s identity-resolution flow, and migration guide’s stable-fallback promise support this omitted-ID operation. Supplying a short explicit/context ID avoids the problem, but neither is required. #1058 does not change this helper.
Runtime validation: I ran real public SDK calls, queue/flush, v1 serialization, and error handling from the exact head and immediate base. Only HTTP POST was replaced: the adapter fed the actual JSON payload into the compiled, verbatim pinned Rust validate_event function, then returned its decision as a v1-shaped response. This was an isolated validator test, not a running ingestion service, storage test, or production observation.
Key with group type company |
Base, both clients | PR ID bytes | PR native validation, both clients |
|---|---|---|---|
| 191 ASCII characters | Accepted | 200 | Accepted |
| 192 ASCII characters | Accepted | 201 | DistinctIdTooLarge |
| 193 ASCII characters | Accepted | 202 | DistinctIdTooLarge |
96 é characters |
Accepted | 201 | DistinctIdTooLarge |
The complete runtime suite produced 16 passed at base, versus 6 failed/10 passed at head; all six failures were the deliverability assertions above. With max_retries=2, the returned drop invoked on_error after one attempt, with reason distinct_id_too_large and no retry exhaustion. Explicit short IDs passed while preserving the full key. This is a terminal per-event rejection, not whole-batch loss.
Consider retaining the readable fallback when it fits the 200-byte UTF-8 limit. For oversized values, a fixed prefix such as $group_ plus SHA-256 of an unambiguous encoding of the (group_type, group_key) pair would provide a bounded deterministic identity, without changing $group_key or $group_set. The exact representation is an identity-format choice: truncation can collide, while random IDs lose the intended stability. Peer SDKs using the same readable convention do not make an oversized v1 identity deliverable.
Suggested repository regression (not added or run as written; the equivalent runtime harness above was executed): alongside the existing wire helpers in posthog/test/test_event_options.py:
@pytest.mark.asyncio
@pytest.mark.parametrize("key", ["k" * 191, "k" * 192, "k" * 193, "é" * 96])
async def test_group_fallback_fits_capture_limit(key):
def call(client):
with new_context(fresh=True):
return client.group_identify("company", key, {"plan": "pro"})
for events in [_sync_wire_events(call), await _async_wire_events(call)]:
assert len(events) == 1
event = events[0]
assert event["event"] == "$groupidentify"
assert event["properties"]["$group_type"] == "company"
assert event["properties"]["$group_key"] == key
assert event["properties"]["$group_set"] == {"plan": "pro"}
assert len(event["distinct_id"].encode("utf-8")) <= 200The byte-bound assertion passes at the immediate base and fails on the current head for the last three keys through each client. After correction, retain the existing ordinary-key stability test and assert repeatability for oversized fallbacks too. Suggested command: uv run --no-sync pytest posthog/test/test_event_options.py -k group_fallback_fits_capture_limit --timeout=30.
There was a problem hiding this comment.
hmm I think enforcing this is reasonable as long it's clearly documented for v1, but I'm open to alternatives.
We've had trouble with this before that we've had customers address, and there are storage-side limits already. I didn't see any events in yesterday's event traffic that would hit this issue, and only a few hundred instances across US and EU for the last 30 days in legacy capture (all scanners, not legit group events) EDIT: did more digging, the legacy counter is by batch so undercounts. pondering this...
Is the feedback loop acceptable for a v1 response from backend to issue a clearly marked drop result for this event only, and an ingestion warning to the team, and leave this as-is?
We could also truncate oversized generated distinct_ids on the SDK side, but this could group unrelated persons at times. Given instances of this issue are rare and generally not from legit event traffic
WDYT?
There was a problem hiding this comment.
only to land if we stick with current behavior: drafted update to an SDK migration doc that wasn't explicit about this yet. public doc release draft PRs call this out too
There was a problem hiding this comment.
PR'd new capture v1 metric for this as well to track as legacy does. Also - legacy silently truncates long distinct IDs which feels more dangerous than calling it out to users prior to upgrade?
Anyway, worth discussing before we lock it in - might be easier to truncate and warn on capture v1 backend if you have concerns. A still saw very few long distinct IDs approaching this limit in prod data that were legit (not scanners/probes etc.)
d205df8 to
cc37ff4
Compare
a1ddf21 to
d8d308c
Compare
5d5c13a to
c9b2f6c
Compare
d8d308c to
a1ddf21
Compare
…ext by one rule group_identify uses the caller's distinct id, then the context id, then $<group_type>_<group_key>, never a random id, and no longer adds the context session id. alias() no longer takes its target from the context distinct id. Context tags and options now reach alias and group_identify like capture. An empty or whitespace-only distinct id counts as missing, and generated personless distinct ids are UUIDv7. Co-Authored-By: Claude Opus 5.5 <[email protected]>
…d limit Co-Authored-By: Claude Opus 5.5 <[email protected]>
…perties Co-Authored-By: Claude Opus 5.5 <[email protected]>
a1ddf21 to
36f39d2
Compare
💡 Motivation and Context
Stacked on #1052.
group_identifywithout a distinct ID used a random one, so each call created a new person, and it attached the request's session to that event.alias()could also take its target from the request context, which may come from a client-controlled header. This PR removes both guesses.Changes
group_identifyuses the caller's distinct ID, then the context's, then$<group_type>_<group_key>, never a random ID.group_identifyno longer adds the context session ID.alias()needs an explicit target and no longer uses the context distinct ID.aliasandgroup_identify, below the call's own values.posthog-python 7.x keeps the random
group_identifyfallback.aliasandgroup_identify, and that a distinct ID over 200 bytes is dropped (passdistinct_idwhen$<group_type>_<group_key>is longer).💚 How did you test it?
🤖 Agent context
Autonomy: Human-driven (agent-assisted)
Written with Claude Code under the direction of the assignee.
🤖 Generated with Claude Code