Skip to content
This repository has been archived by the owner on Nov 4, 2021. It is now read-only.

Don't process $snapshot via plugins #280

Closed
macobo opened this issue Mar 24, 2021 · 3 comments · Fixed by #359
Closed

Don't process $snapshot via plugins #280

macobo opened this issue Mar 24, 2021 · 3 comments · Fixed by #359
Assignees

Comments

@macobo
Copy link
Contributor

macobo commented Mar 24, 2021

Provocative title, I know.

Currently $snapshot events are treated like any other and processed by plugins. However:

  1. Modifying the payload data could easily break the session recording feature or display misleading information on the screen.
  2. The data payload is chunked and compressed - making it hard to make anything out of it.
  3. The payload data contains pretty large data that we need to serialize to send back-and-forth from consumer to worker to VM and back again - this hurts performance.
  4. Plugin authors might not know about treating $snapshot events in a special way.

The only usecase for plugins is syncing it to other data sources and playing back via rrweb there.

I'd propose ignoring snapshot events for now and in the future perhaps add a processSnapshotBatch or similar function to plugins if the syncing scenario comes up. This function would not be allowed to modify the data though.

@mariusandra
Copy link
Collaborator

Import and export plugins are the only ones that legitimately need to access this event. Handling them in a separate processSnapshot function makes sense and shouldn't be too hard to implement. 👍 for skipping them in processEvent.

Regarding processSnapshotBatch, this is a side quest and better suited for #242, but we might benefit from deprecating processEventBatch altogether. In reality, in a well performing system, 99% time when processEventBatch gets called, it gets just one event. The original motivation behind this function was that you for example get 100 events at once and then upload them to S3 or wherever. In reality, this won't really work. The bigquery export for example uses some internal buffering queue to batch its uploads... and we'll need something similar for S3. (Plus a onShutdown function to flush it... but that's another story)

@yakkomajuri
Copy link
Contributor

Interestingly, I had a call with a user recently about this. He would want to actually do some processing on session recording events.

We should probably make a determination otherwise people could actually start writing plugins that target $snapshot events

@mariusandra
Copy link
Collaborator

@yakkomajuri what kind of processing was this user looking at?

If nothing substantial, I'd propose we:

  • remove $snapshots from processEvent.
  • implement the async onEvent as discussed here: Add onEvent #328
  • implement a async onSnapshot to complement onEvent that just gets session recording events. Plugin authors can choose what to target, or both (export const onSnapshot = onEvent)

@mariusandra mariusandra self-assigned this May 6, 2021
@mariusandra mariusandra mentioned this issue May 6, 2021
2 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants