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

fix(replay): Use session.started for min/max duration check #8617

Merged
merged 3 commits into from
Jul 25, 2023

Conversation

mydea
Copy link
Member

@mydea mydea commented Jul 24, 2023

We've been using context.initialTimestamp to check for min/max session duration, but actually this is always updated when a tab is refreshed, for example.

Instead, we now use session.started, which should also always be updated for a buffer session.

note that this also means, when an error happens, we flush immediately, but then wait for 5s until we flush again (which is fine, I'd say) - because at the time of conversion of buffer->session, we update the started of the session. But that's OK I'd say?

I also updated the logging to be based on this. Overall having a log for the buffer->session change makes sense to me anyhow, plus I also added one for the only other case where session.started is updated.

Note: I added a test to kind of cover this, but this test also passed with the previous version, so 🤷 not entirely sure how to test this better 😬

@mydea mydea added the Package: replay Issues related to the Sentry Replay SDK label Jul 24, 2023
@mydea mydea requested a review from billyvg July 24, 2023 16:01
@mydea mydea self-assigned this Jul 24, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Jul 24, 2023

size-limit report 📦

Path Size
@sentry/browser - ES5 CDN Bundle (gzipped + minified) 21.99 KB (-0.01% 🔽)
@sentry/browser - ES5 CDN Bundle (minified) 69.25 KB (0%)
@sentry/browser - ES6 CDN Bundle (gzipped + minified) 20.33 KB (-0.01% 🔽)
@sentry/browser - ES6 CDN Bundle (minified) 60.48 KB (0%)
@sentry/browser - Webpack (gzipped + minified) 21.95 KB (0%)
@sentry/browser - Webpack (minified) 71.63 KB (0%)
@sentry/react - Webpack (gzipped + minified) 21.96 KB (0%)
@sentry/nextjs Client - Webpack (gzipped + minified) 50.83 KB (0%)
@sentry/browser + @sentry/tracing - ES5 CDN Bundle (gzipped + minified) 30.37 KB (-0.01% 🔽)
@sentry/browser + @sentry/tracing - ES6 CDN Bundle (gzipped + minified) 28.24 KB (-0.01% 🔽)
@sentry/replay ES6 CDN Bundle (gzipped + minified) 49.58 KB (+0.05% 🔺)
@sentry/replay - Webpack (gzipped + minified) 43.37 KB (+0.12% 🔺)
@sentry/browser + @sentry/tracing + @sentry/replay - ES6 CDN Bundle (gzipped + minified) 69.76 KB (+0.05% 🔺)
@sentry/browser + @sentry/replay - ES6 CDN Bundle (gzipped + minified) 62.04 KB (+0.05% 🔺)

Copy link
Member

Choose a reason for hiding this comment

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

Does this test fail without the new changes?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, sadly it does not 😬 I think it is because it still triggers the session refresh, as it should, this is kind of the root cause somewhere that this is under some (?) circumstances not happening. This change should be good/better anyhow, I'd say. And with the improved logging, we maybe get closer to the root of this!

@mydea mydea merged commit 893330a into develop Jul 25, 2023
65 checks passed
@mydea mydea deleted the fn/replay-long-session branch July 25, 2023 10:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Package: replay Issues related to the Sentry Replay SDK
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants