-
-
Notifications
You must be signed in to change notification settings - Fork 1.8k
feat(browser): Add mode option for the browser session integration #18997
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
base: develop
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,8 @@ | ||
| import * as Sentry from '@sentry/browser'; | ||
|
|
||
| window.Sentry = Sentry; | ||
|
|
||
| Sentry.init({ | ||
| dsn: 'https://public@dsn.ingest.sentry.io/1337', | ||
| release: '0.1', | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,6 @@ | ||
| let clickCount = 0; | ||
|
|
||
| document.getElementById('navigate').addEventListener('click', () => { | ||
| clickCount++; | ||
| history.pushState({}, '', `/page-${clickCount}`); | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,9 @@ | ||
| <!DOCTYPE html> | ||
| <html> | ||
| <head> | ||
| <meta charset="utf-8" /> | ||
| </head> | ||
| <body> | ||
| <button id="navigate">Navigate via pushState</button> | ||
| </body> | ||
| </html> |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,24 @@ | ||
| import { expect } from '@playwright/test'; | ||
| import type { SessionContext } from '@sentry/core'; | ||
| import { sentryTest } from '../../../utils/fixtures'; | ||
| import { getMultipleSentryEnvelopeRequests } from '../../../utils/helpers'; | ||
|
|
||
| sentryTest('should start new sessions on pushState navigation in default mode.', async ({ getLocalTestUrl, page }) => { | ||
| const url = await getLocalTestUrl({ testDir: __dirname }); | ||
|
|
||
| const sessionsPromise = getMultipleSentryEnvelopeRequests<SessionContext>(page, 10, { | ||
| url, | ||
| envelopeType: 'session', | ||
| timeout: 4000, | ||
| }); | ||
|
|
||
| await page.waitForSelector('#navigate'); | ||
|
|
||
| await page.locator('#navigate').click(); | ||
| await page.locator('#navigate').click(); | ||
| await page.locator('#navigate').click(); | ||
|
|
||
| const sessions = (await sessionsPromise).filter(session => session.init); | ||
|
|
||
| expect(sessions.length).toBe(3); | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,9 @@ | ||
| import * as Sentry from '@sentry/browser'; | ||
|
|
||
| window.Sentry = Sentry; | ||
|
|
||
| Sentry.init({ | ||
| dsn: 'https://public@dsn.ingest.sentry.io/1337', | ||
| release: '0.1', | ||
| integrations: [Sentry.browserSessionIntegration({ mode: 'single' })], | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| let clickCount = 0; | ||
|
|
||
| document.getElementById('navigate').addEventListener('click', () => { | ||
| clickCount++; | ||
| // Each click navigates to a different page | ||
| history.pushState({}, '', `/page-${clickCount}`); | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,9 @@ | ||
| <!DOCTYPE html> | ||
| <html> | ||
| <head> | ||
| <meta charset="utf-8" /> | ||
| </head> | ||
| <body> | ||
| <button id="navigate">Navigate via pushState</button> | ||
| </body> | ||
| </html> |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,45 @@ | ||
| import { expect } from '@playwright/test'; | ||
| import type { SessionContext } from '@sentry/core'; | ||
| import { sentryTest } from '../../../utils/fixtures'; | ||
| import { getMultipleSentryEnvelopeRequests } from '../../../utils/helpers'; | ||
|
|
||
| sentryTest('should start a session on pageload in single mode.', async ({ getLocalTestUrl, page }) => { | ||
| const url = await getLocalTestUrl({ testDir: __dirname }); | ||
|
|
||
| const sessions = await getMultipleSentryEnvelopeRequests<SessionContext>(page, 1, { | ||
| url, | ||
| envelopeType: 'session', | ||
| timeout: 2000, | ||
| }); | ||
|
|
||
| expect(sessions.length).toBeGreaterThanOrEqual(1); | ||
| const session = sessions[0]; | ||
| expect(session).toBeDefined(); | ||
| expect(session.init).toBe(true); | ||
| expect(session.errors).toBe(0); | ||
| expect(session.status).toBe('ok'); | ||
| }); | ||
|
|
||
| sentryTest( | ||
| 'should NOT start a new session on pushState navigation in single mode.', | ||
| async ({ getLocalTestUrl, page }) => { | ||
| const url = await getLocalTestUrl({ testDir: __dirname }); | ||
|
|
||
| const sessionsPromise = getMultipleSentryEnvelopeRequests<SessionContext>(page, 10, { | ||
| url, | ||
| envelopeType: 'session', | ||
| timeout: 4000, | ||
| }); | ||
|
|
||
| await page.waitForSelector('#navigate'); | ||
|
|
||
| await page.locator('#navigate').click(); | ||
| await page.locator('#navigate').click(); | ||
| await page.locator('#navigate').click(); | ||
|
|
||
| const sessions = (await sessionsPromise).filter(session => session.init); | ||
|
|
||
| expect(sessions.length).toBe(1); | ||
| expect(sessions[0].init).toBe(true); | ||
| }, | ||
| ); | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,13 +3,30 @@ import { addHistoryInstrumentationHandler } from '@sentry-internal/browser-utils | |
| import { DEBUG_BUILD } from '../debug-build'; | ||
| import { WINDOW } from '../helpers'; | ||
|
|
||
| export interface BrowserSessionOptions { | ||
| /** | ||
| * Controls when sessions are created. | ||
| * | ||
| * - `'single'`: A session is created once when the page is loaded. Session is not | ||
| * updated on navigation. This is useful for webviews or single-page apps where | ||
| * URL changes should not trigger new sessions. | ||
| * - `'navigation'`: A session is created on page load and on every navigation. | ||
| * This is the default behavior. | ||
| * | ||
| * @default 'navigation' | ||
| */ | ||
| mode?: 'single' | 'navigation'; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
sessionMode: 'navigation' | 'only-pageload'Hm, now that I think more about it, it's probably fine because this is an option in the
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. uuuuuuh, I actually like @Lms24 wdyt? To get a third opinion here :)
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. hmm yeah, "only-pageload" to me also implies something like "we only track sessions for the pageload and stop afterwards". How about:
1 has the advantage that it's very descriptive, but confusion could come from "page". I think this is minimal and also kinda works anyway because it's primarily relevant for Single Page applications". 2 is a bit more technical but correlates well with established Sentry terms. I think with the proper JSDoc (which is already there xD), both would work well. Especially if we pair these with fwiw, Gemini clearly prefers 1 😅 |
||
| } | ||
|
|
||
| /** | ||
| * When added, automatically creates sessions which allow you to track adoption and crashes (crash free rate) in your Releases in Sentry. | ||
| * More information: https://docs.sentry.io/product/releases/health/ | ||
| * | ||
| * Note: In order for session tracking to work, you need to set up Releases: https://docs.sentry.io/product/releases/ | ||
| */ | ||
| export const browserSessionIntegration = defineIntegration(() => { | ||
| export const browserSessionIntegration = defineIntegration((options: BrowserSessionOptions = {}) => { | ||
| const mode = options.mode ?? 'navigation'; | ||
|
|
||
| return { | ||
| name: 'BrowserSession', | ||
| setupOnce() { | ||
|
|
@@ -26,14 +43,16 @@ export const browserSessionIntegration = defineIntegration(() => { | |
| startSession({ ignoreDuration: true }); | ||
| captureSession(); | ||
|
|
||
| // We want to create a session for every navigation as well | ||
| addHistoryInstrumentationHandler(({ from, to }) => { | ||
| // Don't create an additional session for the initial route or if the location did not change | ||
| if (from !== undefined && from !== to) { | ||
| startSession({ ignoreDuration: true }); | ||
| captureSession(); | ||
| } | ||
| }); | ||
| if (mode === 'navigation') { | ||
| // We want to create a session for every navigation as well | ||
| addHistoryInstrumentationHandler(({ from, to }) => { | ||
| // Don't create an additional session for the initial route or if the location did not change | ||
| if (from !== undefined && from !== to) { | ||
| startSession({ ignoreDuration: true }); | ||
| captureSession(); | ||
| } | ||
| }); | ||
| } | ||
| }, | ||
| }; | ||
| }); | ||
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
l: What I like to do when I test that something is not sent, is to await an event after navigating so that we have some time before ending the tests. I guess in this case, we could wait for an error or a transaction event. There's still an assumption here that the session would have been sent before that event but I think that's reasonable and better than not waiting at all or waiting for a a specific time.
We could even make it more deterministic by: