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(replay): Change flush() API to record current event buffer #7743

Merged
merged 8 commits into from
Apr 25, 2023

Conversation

billyvg
Copy link
Member

@billyvg billyvg commented Apr 4, 2023

This changes the public API flush to:

  • in non-session mode, record the current event buffer and by default, convert the replay type to "session" and continue recording.
  • in session-mode it remains unchanged (flush immediately
  • can call with flush({continueRecording: false}) to stop recording immediately after flushing

Essentially, we have extracted the same logic that was used for "onError" capturing and made it a public API.

Closes #7737
Partially addresses #7770

This adds a public API: `capture` that will record the current event buffer and by default, convert the replay type to "session" and continue recording. We have extracted the logic that was used for "onError" capturing and made it a public API.
@github-actions
Copy link
Contributor

github-actions bot commented Apr 4, 2023

size-limit report 📦

Path Size
@sentry/browser - ES5 CDN Bundle (gzipped + minified) 21.02 KB (-0.01% 🔽)
@sentry/browser - ES5 CDN Bundle (minified) 65.66 KB (0%)
@sentry/browser - ES6 CDN Bundle (gzipped + minified) 19.56 KB (-0.01% 🔽)
@sentry/browser - ES6 CDN Bundle (minified) 58.12 KB (0%)
@sentry/browser - Webpack (gzipped + minified) 21.17 KB (0%)
@sentry/browser - Webpack (minified) 69.07 KB (0%)
@sentry/react - Webpack (gzipped + minified) 21.19 KB (0%)
@sentry/nextjs Client - Webpack (gzipped + minified) 49.03 KB (0%)
@sentry/browser + @sentry/tracing - ES5 CDN Bundle (gzipped + minified) 28.59 KB (-0.01% 🔽)
@sentry/browser + @sentry/tracing - ES6 CDN Bundle (gzipped + minified) 26.82 KB (-0.01% 🔽)
@sentry/replay ES6 CDN Bundle (gzipped + minified) 46.26 KB (+0.11% 🔺)
@sentry/replay - Webpack (gzipped + minified) 40.14 KB (+0.12% 🔺)
@sentry/browser + @sentry/tracing + @sentry/replay - ES6 CDN Bundle (gzipped + minified) 65.12 KB (+0.07% 🔺)
@sentry/browser + @sentry/replay - ES6 CDN Bundle (gzipped + minified) 58.1 KB (+0.09% 🔺)

@billyvg
Copy link
Member Author

billyvg commented Apr 4, 2023

Flaked:
image

@billyvg billyvg marked this pull request as ready for review April 4, 2023 20:52
packages/replay/src/types.ts Outdated Show resolved Hide resolved
@billyvg billyvg changed the title ref(replay): Add capture() API to record current event buffer ref(replay): Change flush() API to record current event buffer Apr 5, 2023
packages/replay/src/replay.ts Outdated Show resolved Hide resolved
packages/replay/src/replay.ts Outdated Show resolved Hide resolved
@mydea
Copy link
Member

mydea commented Apr 6, 2023

Also note that I would mark this as feat(replay) still, as it kind of changes the public API!

@billyvg billyvg changed the title ref(replay): Change flush() API to record current event buffer feat(replay): Change flush() API to record current event buffer Apr 11, 2023
@billyvg billyvg requested a review from mydea April 14, 2023 21:22
await this.flushImmediate();

if (!continueRecording) {
return;
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, do we need to still stop the recording here? 🤔

Copy link
Member

Choose a reason for hiding this comment

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

May be simplified to:

if (!continueRecording || !hasStoppedRecording) {
  return;
}

// Reset all ...

Copy link
Member

@mydea mydea left a comment

Choose a reason for hiding this comment

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

LGTM!
Maybe we should also add two tests in browser-integration-tests checking that the public API of flush works as expected with/without continueRecording? But we can also do that in a followup, I guess!

@billyvg
Copy link
Member Author

billyvg commented Apr 18, 2023

I think the jest tests are sufficient here, but I intend to add browser integration tests that should cover flush in #7768!

@billyvg billyvg merged commit 0161cdd into develop Apr 25, 2023
@billyvg billyvg deleted the feat-replay-add-capture branch April 25, 2023 09:26
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.

Add capture() API to capture replay from buffered events
2 participants