Skip to content
This repository was archived by the owner on Aug 30, 2023. It is now read-only.

feat: Upload replay after receiving event from core SDK#83

Merged
billyvg merged 1 commit intomainfrom
feat/finish-replays-after-core-sdk-events
Jun 3, 2022
Merged

feat: Upload replay after receiving event from core SDK#83
billyvg merged 1 commit intomainfrom
feat/finish-replays-after-core-sdk-events

Conversation

@billyvg
Copy link
Copy Markdown
Member

@billyvg billyvg commented Jun 3, 2022

Previously we did not create replay events after receiving new updates from core SDK (e.g. breadcrumbs) -- they only get flushed after an rrweb event.

This causes breadcrumbs to be attached to a new session instead of the previous session.

e.g. old session -> breadcrumbs -> (no upload happens)
--> new session -> rrweb checkout -> upload (with old breadcrumbs)

Fixes #79

@billyvg billyvg requested a review from JoshFerge June 3, 2022 14:55
@billyvg billyvg force-pushed the feat/finish-replays-after-core-sdk-events branch 2 times, most recently from b878746 to 3e51fa8 Compare June 3, 2022 14:57
data: {
// Not sure why this errors, Node should be correct (Argument of type 'Node' is not assignable to parameter of type 'INode')
nodeId: targetNode ? record.mirror.getId(targetNode as any) : undefined,
...(targetNode ? { nodeId: record.mirror.getId(targetNode as any) } : {}),
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

So we don't send {nodeId: undefined}

* should flush events immediately
*/
addReplayEvent(cb: AddReplayEventCallback) {
addUpdate(cb?: AddUpdateCallback) {
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Renamed ReplayEvent to Update

if (cb() === true) {
this.finishReplayEvent();
if (cb?.() === true) {
this.flushUpdate();
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Renamed finishReplayEvent to flushUpdate

Previously we did not create replay events after receiving new updates from core SDK (e.g. breadcrumbs) -- they only get flushed after an rrweb event.

This causes breadcrumbs to be attached to a new session instead of the previous session.

e.g. old session -> breadcrumbs -> <idle> (no upload happens)
 --> new session -> rrweb checkout -> upload (with old breadcrumbs)

 Fixes #79
@billyvg billyvg force-pushed the feat/finish-replays-after-core-sdk-events branch from 3e51fa8 to 88fc354 Compare June 3, 2022 15:02
@billyvg billyvg marked this pull request as ready for review June 3, 2022 15:04
@billyvg
Copy link
Copy Markdown
Member Author

billyvg commented Jun 3, 2022

Gonna merge this as it fixes an annoying bug.

@billyvg billyvg merged commit 25f3007 into main Jun 3, 2022
@billyvg billyvg deleted the feat/finish-replays-after-core-sdk-events branch June 3, 2022 15:27
mydea pushed a commit to getsentry/sentry-javascript that referenced this pull request Nov 23, 2022
…ntry-replay#83)

Previously we did not create replay events after receiving new updates from core SDK (e.g. breadcrumbs) -- they only get flushed after an rrweb event.

This causes breadcrumbs to be attached to a new session instead of the previous session.

e.g. old session -> breadcrumbs -> <idle> (no upload happens)
 --> new session -> rrweb checkout -> upload (with old breadcrumbs)

 Fixes getsentry/sentry-replay#79
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 this pull request may close these issues.

SDK instrumentation handlers do not flush events

1 participant