Skip to content
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.

Sign up for GitHub

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

fix(hogql): fix recodings in actors response #20953

Merged
merged 5 commits into from
Mar 18, 2024
Merged

fix(hogql): fix recodings in actors response #20953

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
a1e810b
fix expanded recordings keys
thmsobrmlr Mar 15, 2024
fa5b0e2
prepare recordings also for actor column
thmsobrmlr Mar 15, 2024
ab3a9f5
return each recoding only once
thmsobrmlr Mar 15, 2024
9b60a8c
Use set instead
webjunkie Mar 18, 2024
75be489
Merge branch 'master' into fix-hogql-actors-recordings
thmsobrmlr Mar 18, 2024
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: 4 additions & 5 deletions frontend/src/scenes/trends/persons-modal/PersonsModal.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ import { ProfilePicture } from 'lib/lemon-ui/ProfilePicture'
import { Spinner } from 'lib/lemon-ui/Spinner/Spinner'
import { Tooltip } from 'lib/lemon-ui/Tooltip'
import { capitalizeFirstLetter, isGroupType, midEllipsis, pluralize } from 'lib/utils'
import { useCallback, useState } from 'react'
import React, { useCallback, useState } from 'react'
import { createRoot } from 'react-dom/client'
import { InsightErrorState, InsightValidationError } from 'scenes/insights/EmptyStates'
import { isOtherBreakdown } from 'scenes/insights/utils'
Expand Down Expand Up @@ -406,11 +406,10 @@ export function ActorRow({ actor, onOpenRecording, propertiesTimelineFilter }: A
<ul className="space-y-px">
{matchedRecordings?.length
? matchedRecordings.map((recording, i) => (
<>
<React.Fragment key={i}>
<LemonDivider className="my-0" />
<li key={i}>
<li>
<LemonButton
key={i}
fullWidth
onClick={() => {
recording.session_id &&
Expand All @@ -431,7 +430,7 @@ export function ActorRow({ actor, onOpenRecording, propertiesTimelineFilter }: A
</div>
</LemonButton>
</li>
</>
</React.Fragment>
))
: null}
</ul>
Expand Down
11 changes: 4 additions & 7 deletions posthog/hogql_queries/actors_query_runner.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,11 +39,10 @@ def determine_strategy(self) -> ActorStrategy:
return GroupStrategy(self.group_type_index, team=self.team, query=self.query, paginator=self.paginator)
return PersonStrategy(team=self.team, query=self.query, paginator=self.paginator)

def get_recordings(self, event_results, recordings_lookup) -> Generator[dict, None, None]:
def serialize_recordings(self, recordings_lookup) -> Generator[dict, None, None]:
return (
{"session_id": session_id, "events": recordings_lookup[session_id]}
for session_id in (event[2] for event in event_results)
if session_id in recordings_lookup
for session_id in recordings_lookup.keys()
Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn't this simply return everything in lookup for each row in results? 🤔

I don't remember it exactly, but I think going via event we should only pick the matching recordings for each event row.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm in case of funnels this has a list of "matched_recordings" that include person ids with matched session ids. Doing the event lookup results in 10 recordings, where in reality there are only 3 for this example:

[
        (datetime.datetime(2024, 3, 14, 22, 33, 13, 749000, tzinfo=<UTC>), UUID('018e3f19-1b02-76bd-8d6f-4fd5f5ef5826'), '018e3f0e-0031-781d-a8d4-5cf3487503f3', '018e3f18-fae2-7c29-8520-f5a57c19e267'), 
        (datetime.datetime(2024, 3, 14, 15, 25, 40, 59000, tzinfo=<UTC>), UUID('018e3d91-a94b-76eb-a322-b73324a07a8f'), '018e3d69-1372-78c8-adae-e1ac913a2dba', '018e3d91-a85f-7a4c-af5d-692adbaca3cf'), 
        (datetime.datetime(2024, 3, 15, 11, 27, 51, 141000, tzinfo=<UTC>), UUID('018e41de-4b54-7c70-9d24-98e343ab91ab'), '018e4193-7a80-74a4-9df3-c8806ee08cba', '018e41de-4a0a-7229-9695-90e4a52d571b'), 
        (datetime.datetime(2024, 3, 15, 11, 27, 50, 837000, tzinfo=<UTC>), UUID('018e41de-4a2c-741d-b6b9-dab1c3b1540f'), '018e4193-7a80-74a4-9df3-c8806ee08cba', '018e41de-4a0a-7229-9695-90e4a52d571b'), 
        (datetime.datetime(2024, 3, 15, 11, 41, 41, 817000, tzinfo=<UTC>), UUID('018e41ea-f81e-7b31-b2c6-a507306f988a'), '018e4193-7a80-74a4-9df3-c8806ee08cba', '018e41dd-c212-7eee-9780-f21aac398718'), 
        (datetime.datetime(2024, 3, 15, 11, 40, 20, 300000, tzinfo=<UTC>), UUID('018e41e9-b9bf-700d-80f3-fb95ef7346a6'), '018e4193-7a80-74a4-9df3-c8806ee08cba', '018e41de-620f-7b69-8162-aae854f2d949'), 
        (datetime.datetime(2024, 3, 15, 11, 40, 20, 11000, tzinfo=<UTC>), UUID('018e41e9-b89c-7720-9c77-187700096c22'), '018e4193-7a80-74a4-9df3-c8806ee08cba', '018e41de-620f-7b69-8162-aae854f2d949'), 
        (datetime.datetime(2024, 3, 15, 11, 39, 49, 859000, tzinfo=<UTC>), UUID('018e41e9-42c9-7b8d-8969-46732fbcdcb2'), '018e4193-7a80-74a4-9df3-c8806ee08cba', '018e41de-620f-7b69-8162-aae854f2d949'), 
        (datetime.datetime(2024, 3, 15, 11, 39, 49, 696000, tzinfo=<UTC>), UUID('018e41e9-4234-7c97-81ec-631045381b4e'), '018e4193-7a80-74a4-9df3-c8806ee08cba', '018e41de-620f-7b69-8162-aae854f2d949'), 
        (datetime.datetime(2024, 3, 15, 11, 39, 47, 494000, tzinfo=<UTC>), UUID('018e41e9-3997-762f-9f4f-04131349e240'), '018e4193-7a80-74a4-9df3-c8806ee08cba', '018e41de-620f-7b69-8162-aae854f2d949')
]

These are the valid session ids here:

{'018e3f0e-0031-781d-a8d4-5cf3487503f3', '018e4193-7a80-74a4-9df3-c8806ee08cba', '018e3d69-1372-78c8-adae-e1ac913a2dba'}

But maybe I don't understand how this should work in general.

Copy link
Contributor

Choose a reason for hiding this comment

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

Tried locally and I see what you mean. I tried to debug what the difference is... I think the funnels gives events with same session id, maybe I didn't have that case in paths (and locally I see a single session there).

I just pushed a commit that simply uses a set to only return a single recording in your example. This way it can be left as is otherwise.

)

def enrich_with_actors(
Expand All @@ -60,13 +59,11 @@ def enrich_with_actors(
actor = actors_lookup.get(actor_id)
new_row[actor_column_index] = actor if actor else {"id": actor_id}
if recordings_column_index is not None and recordings_lookup is not None:
new_row[recordings_column_index] = (
self.get_recordings(result[recordings_column_index], recordings_lookup) or None
)
new_row[recordings_column_index] = self.serialize_recordings(recordings_lookup) or None
yield new_row

def prepare_recordings(self, column_name, input_columns):
if column_name != "person" or "matched_recordings" not in input_columns:
if (column_name != "person" and column_name != "actor") or "matched_recordings" not in input_columns:
thmsobrmlr marked this conversation as resolved.
Show resolved Hide resolved
return None, None

column_index_events = input_columns.index("matched_recordings")
Expand Down
Loading