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.

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

feat: recordings ordering #24794

Merged
merged 58 commits into from
Sep 10, 2024
Merged

feat: recordings ordering #24794

merged 58 commits into from
Sep 10, 2024

Conversation

daibhin
Copy link
Contributor

@daibhin daibhin commented Sep 4, 2024

Problem

Order replays!

Randomly choose replays! (closes #8182)

Changes

  • Add ordering back to replay filtering
  • Add more options to filter by (activity mainly)
  • Remove ability to customise "duration shown" (it uses the ordering duration or the total duration now)
  • Rewrite query such that it doesn't need to be parsed
    • Curious if this has any performance impact
  • Removed some legacy stuff about changing the search period (we should always respect the date range filter first and foremost)

ordering

Does this work well for both Cloud and self-hosted?

Yes

How did you test this code?

Added a test

@daibhin daibhin changed the title include ordering clause feat: recordings ordering Sep 4, 2024
@daibhin daibhin requested a review from a team September 4, 2024 17:38
Copy link
Collaborator

@neilkakkar neilkakkar left a comment

Choose a reason for hiding this comment

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

nice, some failing tests w.r.t assigning start_time instead of latest it looks like

@daibhin
Copy link
Contributor Author

daibhin commented Sep 5, 2024

@neilkakkar requested a re-review because I added in the random sample stuff and felt like things had changed enough. Will comment where things have changed

@@ -54,7 +73,7 @@ describe('sessionRecordingsPlaylistLogic', () => {
return [
200,
{
results: [`List of recordings offset by ${listOfSessionRecordings.length}`],
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The sortRecordings function did not like that this was not an object


def _order_by_clause(self) -> ast.OrderExpr:
expr = (
ast.Call(name="sipHash64", args=[ast.Field(chain=["session_id"])])
Copy link
Contributor Author

@daibhin daibhin Sep 5, 2024

Choose a reason for hiding this comment

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

Main addition here @neilkakkar. Idea is that it generates a hash based on the session id and uses that to pick a random but (mostly deterministic) ordering of recordings

Copy link
Collaborator

Choose a reason for hiding this comment

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

this seems unnecessary 🤔 , unless the worry is around pagination here? You can do order by rand() if you just want a random sampling.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, was trying to keep things consistent around pagination, link sharing and repeated runs

Copy link
Collaborator

Choose a reason for hiding this comment

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

since the hash is consistent, does this mean the first page after you select random will always be the same? 😅 Again not sure if this is more confusing than it changing every time (and then being able to sort that result list).

I think we can relax the product constraint of pagination working appropriately when random sampling is chosen perhaps.

Copy link
Collaborator

Choose a reason for hiding this comment

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

as a user I'd potentially like to be like a gacha, every time I click the toggle I get different recordings

Copy link
Member

Choose a reason for hiding this comment

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

i commented elsewhere but i'll add it here so there's one thread :)

i don't think I get random... why?

(it also feels wrong that it's consistently random, if i toggle it on and off i get the same random list each time)

Copy link
Member

Choose a reason for hiding this comment

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

ah, ok, i read the linked issue... fair enough :)

Copy link
Member

Choose a reason for hiding this comment

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

let's make sure we get metrics on what people are choosing / loading ??

Copy link
Contributor

github-actions bot commented Sep 5, 2024

Size Change: 0 B

Total Size: 1.1 MB

ℹ️ View Unchanged
Filename Size
frontend/dist/toolbar.js 1.1 MB

compressed-size-action

},
]}
size="small"
value={orderBy as string}
Copy link
Member

Choose a reason for hiding this comment

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

could probably avoid this cast by properly typing the options but they're nested and i'm lazy 🤣

@@ -417,7 +429,14 @@ export const sessionRecordingsPlaylistLogic = kea<sessionRecordingsPlaylistLogic
'start_time' as RecordingsQuery['order'],
{ persist: true },
{
setOrderBy: (_, { orderBy }) => orderBy,
setOrderBy: (_, { orderBy }) => orderBy as RecordingsQuery['order'],
Copy link
Member

Choose a reason for hiding this comment

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

casting here to make other typing easier. it's still protected by the typing on the setOrderBy param

setOrderBy: (_, { orderBy }) => orderBy as RecordingsQuery['order'],
},
],
randomSample: [
Copy link
Member

Choose a reason for hiding this comment

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

I don't think I get the use for random sample (especially since it appears to be a persistent randomness when toggling the sampling on and off)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Original thinking was in #8182

Tbh I might just remove it for now because I can foresee it causing customer confusion if there's misunderstanding about it and it hasn't made it past the PR 😅

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, the 8182 is "matching examples from a wider date range"

i.e. not each consecutive recording, not random recordings, just one from each day or whatever

considering it's not come up again, i'd strip it out of here, yep

then we can test if selecting something other then latest for people affects behaviour - which is what i'm really excited about

  1. do people choose this themselves
  2. what happens if we choose it for them

that's why i'd suggest making it front and center so we can see if they change away as well

Copy link
Member

Choose a reason for hiding this comment

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

well, top and right not front and center 🤣

Copy link
Member

@pauldambra pauldambra left a comment

Choose a reason for hiding this comment

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

feel free to change my typing, er, typing

Copy link
Member

Choose a reason for hiding this comment

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

commenting here so we can thread if need be

high quality photoshop warning

image

should it be here (but not beige)

Copy link
Member

Choose a reason for hiding this comment

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

oh... and we have a random sample toggle, and a random ordering in the menu... so i can choose random random 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hesitant to put something on that level in case it ends up getting muddled with a large amount of filters. Going to go with something like this instead:

Screenshot 2024-09-06 at 11 35 47

Experimented with an even more natural language based approach but I don't think it quite works because of our button paddings

natural_language

Copy link
Member

Choose a reason for hiding this comment

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

🙌

@posthog-bot
Copy link
Contributor

📸 UI snapshots have been updated

4 snapshot changes in total. 0 added, 4 modified, 0 deleted:

Triggered by this commit.

👉 Review this PR's diff of snapshots.

@posthog-bot
Copy link
Contributor

📸 UI snapshots have been updated

1 snapshot changes in total. 0 added, 1 modified, 0 deleted:

  • chromium: 0 added, 1 modified, 0 deleted (diff for shard 1)
  • webkit: 0 added, 0 modified, 0 deleted

Triggered by this commit.

👉 Review this PR's diff of snapshots.

@posthog-bot
Copy link
Contributor

📸 UI snapshots have been updated

3 snapshot changes in total. 0 added, 3 modified, 0 deleted:

Triggered by this commit.

👉 Review this PR's diff of snapshots.

@posthog-bot
Copy link
Contributor

📸 UI snapshots have been updated

2 snapshot changes in total. 0 added, 2 modified, 0 deleted:

  • chromium: 0 added, 2 modified, 0 deleted (diff for shard 1)
  • webkit: 0 added, 0 modified, 0 deleted

Triggered by this commit.

👉 Review this PR's diff of snapshots.

@daibhin daibhin merged commit f855d23 into master Sep 10, 2024
93 checks passed
@daibhin daibhin deleted the dn-feat/replay-filtering branch September 10, 2024 14:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Stratified Recordings
4 participants