Repository navigation
fix!: group_identify and alias never guess a distinct id #1053
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
43fd11d
12273e4
36f39d2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| --- | ||
| pypi/posthog: major | ||
| --- | ||
|
|
||
| `group_identify` falls back to the context distinct ID and then to `$<group_type>_<group_key>` instead of 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 apply to `alias` and `group_identify`. An empty or whitespace-only `distinct_id` counts as missing, and generated personless distinct IDs are UUIDv7. `AsyncPosthog` follows the same rules. |
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -132,6 +132,7 @@ | |||||||||||||||||||||
| to_values, | ||||||||||||||||||||||
| ) | ||||||||||||||||||||||
| from posthog.utils import ( | ||||||||||||||||||||||
| _uuid7, | ||||||||||||||||||||||
| FlagCache, | ||||||||||||||||||||||
| RedisFlagCache, | ||||||||||||||||||||||
| SizeLimitedDict, | ||||||||||||||||||||||
|
|
@@ -263,17 +264,37 @@ def _positive_config_value( | |||||||||||||||||||||
| return value | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
|
|
||||||||||||||||||||||
| def _explicit_distinct_id(passed) -> Optional[str]: | ||||||||||||||||||||||
| """The caller's distinct id, or None when it is missing, empty or whitespace.""" | ||||||||||||||||||||||
| stringified = stringify_id(passed) | ||||||||||||||||||||||
| if stringified and stringified.strip(): | ||||||||||||||||||||||
| return stringified | ||||||||||||||||||||||
| return None | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
|
|
||||||||||||||||||||||
| def get_identity_state(passed) -> tuple[str, bool]: | ||||||||||||||||||||||
| """Returns the distinct id to use, and whether this is a personless event or not""" | ||||||||||||||||||||||
| stringified = stringify_id(passed) | ||||||||||||||||||||||
| if stringified and len(stringified): | ||||||||||||||||||||||
| return (stringified, False) | ||||||||||||||||||||||
| explicit = _explicit_distinct_id(passed) | ||||||||||||||||||||||
| if explicit: | ||||||||||||||||||||||
| return (explicit, False) | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| context_id = get_context_distinct_id() | ||||||||||||||||||||||
| if context_id: | ||||||||||||||||||||||
| return (context_id, False) | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| return (str(uuid4()), True) | ||||||||||||||||||||||
| return (str(_uuid7()), True) | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
|
|
||||||||||||||||||||||
| def _group_identify_distinct_id(passed, group_type, group_key) -> str: | ||||||||||||||||||||||
| """The caller's id, else the context id, else one stable id per group. | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| Ingestion creates a person for each new distinct id, so a random id | ||||||||||||||||||||||
| would add a person on every call. | ||||||||||||||||||||||
| """ | ||||||||||||||||||||||
| distinct_id, personless = get_identity_state(passed) | ||||||||||||||||||||||
| if personless: | ||||||||||||||||||||||
| return f"${group_type}_{group_key}" | ||||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [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 with posthog.new_context(fresh=True):
client.group_identify("company", "k" * 193, {"plan": "pro"})
This is introduced by this PR: at immediate base 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
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 Consider retaining the readable fallback when it fits the 200-byte UTF-8 limit. For oversized values, a fixed prefix such as Suggested repository regression (not added or run as written; the equivalent runtime harness above was executed): alongside the existing wire helpers in @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:
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 WDYT?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.) |
||||||||||||||||||||||
| return distinct_id | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
|
|
||||||||||||||||||||||
| def _personless_options(personless: bool) -> dict[str, Any]: | ||||||||||||||||||||||
|
|
@@ -2077,6 +2098,8 @@ def group_identify( | |||||||||||||||||||||
| ignored and replaced with a newly generated UUID. | ||||||||||||||||||||||
| disable_geoip: Whether to disable GeoIP for this event. | ||||||||||||||||||||||
| distinct_id: The distinct ID of the user performing the action. | ||||||||||||||||||||||
| Falls back to the context distinct ID, then to | ||||||||||||||||||||||
| ``$<group_type>_<group_key>``. | ||||||||||||||||||||||
| options: Capture options for this event, sent as given. | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| Examples: | ||||||||||||||||||||||
|
|
@@ -2106,8 +2129,7 @@ def group_identify( | |||||||||||||||||||||
|
|
||||||||||||||||||||||
| properties = properties or {} | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| # group_identify is purposefully always personful | ||||||||||||||||||||||
| distinct_id = get_identity_state(distinct_id)[0] | ||||||||||||||||||||||
| distinct_id = _group_identify_distinct_id(distinct_id, group_type, group_key) | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| msg: Dict[str, Any] = { | ||||||||||||||||||||||
| "event": "$groupidentify", | ||||||||||||||||||||||
|
|
@@ -2122,11 +2144,14 @@ def group_identify( | |||||||||||||||||||||
| "options": _event_options(options), | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| # NOTE - group_identify doesn't generally use context properties - should it? | ||||||||||||||||||||||
| if get_context_session_id(): | ||||||||||||||||||||||
| msg["properties"]["$session_id"] = str(get_context_session_id()) | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| return self._enqueue(msg, disable_geoip) | ||||||||||||||||||||||
| # No context session: this event's distinct id is not the session's | ||||||||||||||||||||||
| # user, and a session takes the person of its latest event. | ||||||||||||||||||||||
| return self._enqueue( | ||||||||||||||||||||||
| msg, | ||||||||||||||||||||||
| disable_geoip, | ||||||||||||||||||||||
| context_properties=_context_tag_defaults(), | ||||||||||||||||||||||
| context_options=_context_get_context_options(), | ||||||||||||||||||||||
| ) | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| @no_throw() | ||||||||||||||||||||||
| def alias( | ||||||||||||||||||||||
|
|
@@ -2144,9 +2169,9 @@ def alias( | |||||||||||||||||||||
| Args: | ||||||||||||||||||||||
| previous_id: The previous distinct ID. Required - the call is dropped | ||||||||||||||||||||||
| with a warning if it is missing or empty. | ||||||||||||||||||||||
| distinct_id: The new distinct ID to alias to. Falls back to the | ||||||||||||||||||||||
| context distinct ID; the call is dropped with a warning if | ||||||||||||||||||||||
| neither is available. | ||||||||||||||||||||||
| distinct_id: The new distinct ID to alias to. Required - the call is | ||||||||||||||||||||||
| dropped with a warning if it is missing or empty. The context | ||||||||||||||||||||||
| distinct ID is never used here. | ||||||||||||||||||||||
| timestamp: The timestamp of the event. UTC is preferred; non-UTC | ||||||||||||||||||||||
| datetimes and parseable ISO timestamp strings are converted to UTC. | ||||||||||||||||||||||
| uuid: A unique identifier for the event. If provided, it must be a | ||||||||||||||||||||||
|
|
@@ -2172,10 +2197,10 @@ def alias( | |||||||||||||||||||||
| ) | ||||||||||||||||||||||
| return None | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| (distinct_id, personless) = get_identity_state(distinct_id) | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| if personless: | ||||||||||||||||||||||
| # No alias target was passed and none is available from context. | ||||||||||||||||||||||
| # The alias target always comes from the caller: a context id can come | ||||||||||||||||||||||
| # from a client-controlled request header. | ||||||||||||||||||||||
| distinct_id = _explicit_distinct_id(distinct_id) | ||||||||||||||||||||||
| if not distinct_id: | ||||||||||||||||||||||
| self.log.warning( | ||||||||||||||||||||||
| "alias() called without a distinct_id, dropping the $create_alias event" | ||||||||||||||||||||||
| ) | ||||||||||||||||||||||
|
|
@@ -2196,7 +2221,12 @@ def alias( | |||||||||||||||||||||
| if get_context_session_id(): | ||||||||||||||||||||||
| msg["properties"]["$session_id"] = str(get_context_session_id()) | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| return self._enqueue(msg, disable_geoip) | ||||||||||||||||||||||
| return self._enqueue( | ||||||||||||||||||||||
| msg, | ||||||||||||||||||||||
| disable_geoip, | ||||||||||||||||||||||
| context_properties=_context_tag_defaults(), | ||||||||||||||||||||||
| context_options=_context_get_context_options(), | ||||||||||||||||||||||
| ) | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| def capture_exception( | ||||||||||||||||||||||
| self, | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
Uh oh!
There was an error while loading. Please reload this page.