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: Refactor loading of snapshots #20632

Merged
merged 32 commits into from
Apr 16, 2024
Merged

feat: Refactor loading of snapshots #20632

merged 32 commits into from
Apr 16, 2024

Conversation

benjackwhite
Copy link
Contributor

Problem

This is a beefier refactor than I initially intended... but i think its worth it.

Primarily I needed to be able to control the parsing better so that v3 realtime snapshots would be loaded just like blob ones.
This led me down a bit of a rabbit hole in the logic where I realised it is pretty fragile and we could make much better use of separating out loaders and selectors

Changes

  • Moves the initial list of sources to its own loader
  • Separate loader for loading specific sources which reduces to a cached obejct (no more in flight merging of data)
  • Selector to merge all the snapshots from the various sources (much easier to reason on and should perform better
  • Polling simplified as well
  • Changed the Player logic to react to changes in the state rather than listening to specific events (again should be easier to reason on later)
  • Fixed v3 realtime loading so that it parses it like blob data as it is streamed the same way
  • Added Etag management (so cool) which means once the browser has a cache of the realtime data, we will only transfer it again if the actual data changes

TODO

  • Confirm that there isn't any unneeded re computation going on for big recordings

👉 Stay up-to-date with PostHog coding conventions for a smoother review.

How did you test this code?

Still need to update the tests

@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 2)
  • 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

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

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

Triggered by this commit.

👉 Review this PR's diff of snapshots.

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.

long overdue improvement...

(doesn't actually playback for me at the moment 🙈 but it's still draft so 🚀)

@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 2)
  • 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

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

  • chromium: 0 added, 1 modified, 0 deleted (diff for shard 2)
  • 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

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

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

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, 0 modified, 0 deleted
  • webkit: 0 added, 1 modified, 0 deleted (diff for shard 2)

Triggered by this commit.

👉 Review this PR's diff of snapshots.

@benjackwhite benjackwhite removed the request for review from daibhin February 29, 2024 12:01
Copy link
Contributor

github-actions bot commented Feb 29, 2024

Size Change: 0 B

Total Size: 999 kB

ℹ️ View Unchanged
Filename Size
frontend/dist/toolbar.js 999 kB

compressed-size-action

@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 2)
  • 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

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

  • chromium: 0 added, 1 modified, 0 deleted (diff for shard 2)
  • 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

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

Triggered by this commit.

👉 Review this PR's diff of snapshots.

@pauldambra pauldambra mentioned this pull request Mar 7, 2024
@pauldambra
Copy link
Member

@daibhin I suggested we should pick this up to get it over the line... I'll chop out a few pieces while I get my head around it to get started

e.g. #20767

@daibhin
Copy link
Contributor

daibhin commented Mar 7, 2024

@pauldambra great idea! I was actually holding off some work on our performance timing until this went through to see if it fixed the weird P99 issues. Happy to help out

@posthog-bot
Copy link
Contributor

This PR hasn't seen activity in a week! Should it be merged, closed, or further worked on? If you want to keep it open, post a comment or remove the stale label – otherwise this will be closed in another week.

@pauldambra pauldambra added waiting Prevents stale-bot from marking the PR as stale. and removed stale labels Mar 15, 2024
Copy link
Contributor

@daibhin daibhin left a comment

Choose a reason for hiding this comment

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

Did a thorough review of this and

Copy link
Contributor

Choose a reason for hiding this comment

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

@PostHog/team-product-analytics this snapshot seems to be flapping on quite a few PRs this week

Comment on lines +707 to 708
// TODO: Do a proper check for all sources having been loaded
return (
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we check that the number of sources and the snapshots by source counts are the same e.g.

Suggested change
// TODO: Do a proper check for all sources having been loaded
return (
// TODO: Do a proper check for all sources having been loaded
return (
snapshotSources.length === Object.keys(snapshotsBySource).length

Copy link
Contributor

Choose a reason for hiding this comment

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

I've shipped some smaller parts of this change in previous PRs and did a thorough review today. It's pretty difficult to review 100% but I'm fairly confident it does what it's intended to. My plan is to test it locally but think it's probably worth shipping at this point and following up with any fixes forward we need.

@benjackwhite is there anything you'd like to do before we ship? There's still a few TODOs knocking about

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just don't have any spare capacity for it so its fully in your hands

@daibhin daibhin mentioned this pull request Apr 2, 2024
@daibhin daibhin requested a review from pauldambra April 12, 2024 10:19
])
})

it('can start polling for snapshots', async () => {
it('polls up to a max threshold', async () => {
Copy link
Member

Choose a reason for hiding this comment

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

🤯 I've just realised that the polling shouldn't trigger loading state in the logic - we don't want the loading affordances in the UI when we poll.

Totally separate to this, reading the code back made me realise

const params = {
source: source.source,
blob_key: source.blob_key,
version: values.featureFlags[FEATURE_FLAGS.SESSION_REPLAY_V3_INGESTION_PLAYBACK] ? '3' : '2',
Copy link
Member

Choose a reason for hiding this comment

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

and I think we can delete all the v3 stuff since that experiment is over

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.

ran locally and I can load blob and realtime sources

@daibhin daibhin marked this pull request as ready for review April 16, 2024 14:29
@daibhin daibhin merged commit 4fd7a76 into master Apr 16, 2024
105 checks passed
@daibhin daibhin deleted the fix/v3-snapshots branch April 16, 2024 15:01
@daibhin
Copy link
Contributor

daibhin commented Apr 17, 2024

Haven't seen any reports of this causing issues 🥳 Thanks for taking it on @benjackwhite

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
waiting Prevents stale-bot from marking the PR as stale.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants