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 stop() to flush and remove current session #7741

Merged
merged 15 commits into from
Apr 26, 2023

Conversation

billyvg
Copy link
Member

@billyvg billyvg commented Apr 4, 2023

stop() will now flush the eventBuffer before clearing it, as well as removing the session from Session Storage. Due to the flushing, stop() is now async.

Closes: #7738

`stop()` will now flush the eventBuffer before clearing it, as well as removing the session from Session Storage. Due to the flushing, `stop()` is now async.

Ref: #7738
@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%)
@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%)
@sentry/browser + @sentry/tracing - ES6 CDN Bundle (gzipped + minified) 26.82 KB (0%)
@sentry/replay ES6 CDN Bundle (gzipped + minified) 46.34 KB (+0.27% 🔺)
@sentry/replay - Webpack (gzipped + minified) 40.22 KB (+0.32% 🔺)
@sentry/browser + @sentry/tracing + @sentry/replay - ES6 CDN Bundle (gzipped + minified) 65.19 KB (+0.18% 🔺)
@sentry/browser + @sentry/replay - ES6 CDN Bundle (gzipped + minified) 58.17 KB (+0.21% 🔺)

@billyvg billyvg marked this pull request as ready for review April 4, 2023 20:50
@@ -207,12 +207,12 @@ Sentry.init({ replaysOnErrorSampleRate: ${errorSampleRate} })`,
* Currently, this needs to be manually called (e.g. for tests). Sentry SDK
* does not support a teardown
*/
public stop(): void {
public async stop(): Promise<void> {
Copy link
Member

Choose a reason for hiding this comment

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

No need to make this async, as this will just create a new promise!

@billyvg billyvg requested a review from mydea April 14, 2023 21:22
// Set this property so that it ignores `_isEnabled` = false
// We can't move `_isEnabled` after awaiting a flush, otherwise we can
// enter into an infinite loop when `stop()` is called while flushing.
this._shouldFinalFlush = true;
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, I don't really like this flag - I would prefer we find a way to solve this by e.g. passing an argument to flushImmediate() or making a separate function for this type of flushing to ensure this. Seems prone to run out of sync, be accidentally true/false when we don't mean it to, etc.

One option I could see is to just do:

this._isEnabled = false;
this._removeListeners();
this._flushAfterStop();

function _flushAfterStop() {
  this._debouncedFlush().cancel();
  this._flush({ force: true });
}

Or something along these lines? IMHO as we are stopping here anyhow, we don't necessarily need to run the debounced flush here at all, allowing us to separate this a bit better?

Copy link
Member Author

Choose a reason for hiding this comment

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

updated, instead of creating a private method, I just kept the flush calls inline as it isn't used elsewhere.

@billyvg billyvg requested a review from mydea April 24, 2023 21:16
@@ -474,6 +474,15 @@ export interface ReplayContainer {
setInitialState(): void;
}

export interface ReplayFlushOptions {
Copy link
Member

Choose a reason for hiding this comment

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

l: Do we need to export this here, or can we just inline this into the one place where it is used? Maybe actually gives more context seeing it there?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done here: fd448a6

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.

Very nice! 🚢

@@ -807,7 +820,7 @@ export class ReplayContainer implements ReplayContainerInterface {
// This means we retried 3 times and all of them failed,
// or we ran into a problem we don't want to retry, like rate limiting.
// In this case, we want to completely stop the replay - otherwise, we may get inconsistent segments
this.stop('sendReplay');
void this.stop('sendReplay');
Copy link
Member Author

Choose a reason for hiding this comment

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

Just noticed this @mydea -- I think this is ok since stop() will attempt the flush, fail, and when it retries, this._isEnabled is false, so will fail.

Copy link
Member

Choose a reason for hiding this comment

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

I think it should be fine since this._isEnabled = false should be set "synchronously" in the async stop function. So I would expect this to be false already in the next code line, basically.

@billyvg billyvg merged commit 300b220 into develop Apr 26, 2023
59 checks passed
@billyvg billyvg deleted the feat-replay-change-stop-flush-clear-session branch April 26, 2023 08:54
billyvg added a commit that referenced this pull request May 19, 2023
Introduced in #7741. This happens when session replay rate is > 0 and the session is unsampled, it calls `stop()` which ends up flushing without checking the recording mode (session vs buffer).

Closes #8054
billyvg added a commit that referenced this pull request May 23, 2023
…8168)

Introduced in #7741. This happens when session replay rate is > 0 and
the session is unsampled, it calls `stop()` which ends up flushing
without checking the recording mode (session vs buffer).

Closes #8054
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.

Change stop() API
2 participants