feat(browser): Add browserSessionIntegration
#14551
Conversation
❌ 1 Tests Failed:
View the top 1 failed tests by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
size-limit report 📦
|
| // concept that can be used as a metric. | ||
| // Automatically captured sessions are akin to page views, and thus we | ||
| // discard their duration. | ||
| startSession({ ignoreDuration: true }); |
There was a problem hiding this comment.
Should we adjust the startSession syntax to allow to optionally pass a client directly, and run all of this on setup(client) instead of setupOnce()? 🤔
Then again all of this is kind of global, so maybe this does not help anyhow 🤔
There was a problem hiding this comment.
I had the same thoughts. I am actually extremely unsure on how to proceed. It all depends on how we would want sessions to work. If we want there to be multiple at once, it would make sense, however I would err towards the exact opposite.
I think always only having one session active per page makes the most sense. Even in MFEs. I would even go so far to lock down the API down even more in v9, simply because it makes wrapping your head around the concept easier.
All of this is just thinking out loud though. In general I didn't want this PR to be behaviorally significant. That's why I chose this route.
There was a problem hiding this comment.
I agree, let's leave it at one "global" session per page.
| @@ -34,7 +34,7 @@ sentryTest('should capture replays (@sentry/browser export)', async ({ getLocalT | |||
| event_id: expect.stringMatching(/\w{32}/), | |||
| environment: 'production', | |||
| sdk: { | |||
| integrations: [ | |||
| integrations: expect.arrayContaining([ | |||
There was a problem hiding this comment.
l: any reason to loosen the assertions?
There was a problem hiding this comment.
I personally found it to be very annoying that the ordering needed to be exact. Lmk if you want me to change it back.
There was a problem hiding this comment.
ah I see, the ordering is annoying, agreed. Though these tests wouldn't fail additionally added integrations, unless I'm completely missing something. So that opens the test up to more behaviour change than previously. I don't think this is the end of the world but it'd be good to have at least one test that fails if we add a new integration by default. We should be aware of stuff like this.
Ref: #14550
Adds a
browserSessionIntegrationthat is exported from all browser SDKs which is added whenautoSessionTracking: true, and entails the exact same functionality asautoSessionTrackingdoes today.In v9 this integration will become more fleshed out and completely replace
autoSessionTrackingin the browser SDKs.