Skip to content

Commit 7ff79f6

Browse files
Lms24JPeer264
Lms24
and
JPeer264
authored
fix(browser): Ensure user id is consistently added to sessions (#19341)
This patch fixes multiple bugs and problems around browser sessions, mostly related to user id assignment: 1. When calling `Sentry.setUser()` on static pages (i.e. no soft navigations), the user id would never be added to sessions. This is because in static pages, we don't send an `"exited"` session update. **The fix**: We send a session update whenever the user is set on the isolationScope (see comment about limitations) 2. When calling `Sentry.setUser()` in a single page application (i.e. with soft navigations), we would update the initial session with the user data when starting a new session for a new navigation. However, we did not include the user id on the new session, because the `getCurrentScope().getUser() || getIsolationScope().getUser()` check was flawed. **The fix**: we use our `getCombinedScopeData` helper to get the "correct" (read, consistently like in other telemetry items) user. 3. It seems like we had an incorrect check that would skip creating a new sessions for the first soft navigation after the pageload (in the default `'route'`) session lifecycle. **The fix**: We no longer check for `from` being undefined. --------- Co-authored-by: Jan Peer Stöcklmair <jan.peer@sentry.io>
1 parent e74975c commit 7ff79f6

File tree

18 files changed

+276
-74
lines changed

18 files changed

+276
-74
lines changed

‎.size-limit.js‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,7 @@ module.exports = [
1515
path: 'packages/browser/build/npm/esm/prod/index.js',
1616
import: createImport('init'),
1717
gzip: true,
18-
limit: '24.1 KB',
18+
limit: '24.5 KB',
1919
modifyWebpackConfig: function (config) {
2020
const webpack = require('webpack');
2121

@@ -190,7 +190,7 @@ module.exports = [
190190
name: 'CDN Bundle (incl. Logs, Metrics)',
191191
path: createCDNPath('bundle.logs.metrics.min.js'),
192192
gzip: true,
193-
limit: '29 KB',
193+
limit: '30 KB',
194194
},
195195
{
196196
name: 'CDN Bundle (incl. Tracing, Logs, Metrics)',
@@ -214,7 +214,7 @@ module.exports = [
214214
name: 'CDN Bundle (incl. Tracing, Replay, Logs, Metrics)',
215215
path: createCDNPath('bundle.tracing.replay.logs.metrics.min.js'),
216216
gzip: true,
217-
limit: '81 KB',
217+
limit: '82 KB',
218218
},
219219
{
220220
name: 'CDN Bundle (incl. Tracing, Replay, Feedback)',
@@ -241,7 +241,7 @@ module.exports = [
241241
path: createCDNPath('bundle.tracing.min.js'),
242242
gzip: false,
243243
brotli: false,
244-
limit: '128 KB',
244+
limit: '129 KB',
245245
},
246246
{
247247
name: 'CDN Bundle (incl. Logs, Metrics) - uncompressed',

‎CHANGELOG.md‎

Lines changed: 16 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@
44

55
- "You miss 100 percent of the chances you don't take. — Wayne Gretzky" — Michael Scott
66

7-
Work in this release was contributed by @LudvigHz. Thank you for your contribution!
7+
### Important Changes
88

99
- **feat(tanstackstart-react): Add global sentry exception middlewares ([#19330](https://github.com/getsentry/sentry-javascript/pull/19330))**
1010

@@ -20,11 +20,23 @@ Work in this release was contributed by @LudvigHz. Thank you for your contributi
2020
});
2121
```
2222

23-
### Important Changes
23+
- **fix(node-core): Reduce bundle size by removing apm-js-collab and requiring pino >= 9.10 ([#18631](https://github.com/getsentry/sentry-javascript/pull/18631))**
24+
25+
In order to keep receiving pino logs, you need to update your pino version to >= 9.10, the reason for the support bump is to reduce the bundle size of the node-core SDK in frameworks that cannot tree-shake the apm-js-collab dependency.
2426

25-
- fix(node-core): Reduce bundle size by removing apm-js-collab and requiring pino >= 9.10 ([#18631](https://github.com/getsentry/sentry-javascript/pull/18631))
27+
- **fix(browser): Ensure user id is consistently added to sessions ([#19341](https://github.com/getsentry/sentry-javascript/pull/19341))**
2628

27-
In order to keep receiving pino logs, you need to update your pino version to >= 9.10, the reason for the support bump is to reduce the bundle size of the node-core SDK in frameworks that cannot tree-shake the apm-js-collab dependency.
29+
Previously, the SDK inconsistently set the user id on sessions, meaning sessions were often lacking proper coupling to the user set for example via `Sentry.setUser()`.
30+
Additionally, the SDK incorrectly skipped starting a new session for the first soft navigation after the pageload.
31+
This patch fixes these issues. As a result, metrics around sessions, like "Crash Free Sessions" or "Crash Free Users" might change.
32+
This could also trigger alerts, depending on your set thresholds and conditions.
33+
We apologize for any inconvenience caused!
34+
35+
While we're at it, if you're using Sentry in a Single Page App or meta framework, you might want to give the new `'page'` session lifecycle a try!
36+
This new mode no longer creates a session per soft navigation but continues the initial session until the next hard page refresh.
37+
Check out the [docs](https://docs.sentry.io/platforms/javascript/guides/nextjs/configuration/integrations/browsersession/) to learn more!
38+
39+
Work in this release was contributed by @LudvigHz. Thank you for your contribution!
2840

2941
## 10.39.0
3042

‎dev-packages/browser-integration-tests/suites/sessions/initial-scope/template.html‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,6 @@
44
<meta charset="utf-8" />
55
</head>
66
<body>
7-
<a id="navigate" href="foo">Navigate</a>
7+
<a id="navigate" href="#foo">Navigate</a>
88
</body>
99
</html>

‎dev-packages/browser-integration-tests/suites/sessions/initial-scope/test.ts‎

Lines changed: 26 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -1,31 +1,42 @@
1-
import type { Route } from '@playwright/test';
21
import { expect } from '@playwright/test';
3-
import type { SessionContext } from '@sentry/core';
42
import { sentryTest } from '../../../utils/fixtures';
5-
import { getFirstSentryEnvelopeRequest } from '../../../utils/helpers';
3+
import { waitForSession } from '../../../utils/helpers';
64

7-
sentryTest('should start a new session on pageload.', async ({ getLocalTestUrl, page }) => {
5+
sentryTest('starts a new session on pageload.', async ({ getLocalTestUrl, page }) => {
86
const url = await getLocalTestUrl({ testDir: __dirname });
9-
const session = await getFirstSentryEnvelopeRequest<SessionContext>(page, url);
7+
const sessionPromise = waitForSession(page, s => !!s.init && s.status === 'ok');
8+
9+
await page.goto(url);
10+
const session = await sessionPromise;
1011

1112
expect(session).toBeDefined();
12-
expect(session.init).toBe(true);
13-
expect(session.errors).toBe(0);
14-
expect(session.status).toBe('ok');
15-
expect(session.did).toBe('1337');
13+
expect(session).toEqual({
14+
attrs: {
15+
environment: 'production',
16+
release: '0.1',
17+
user_agent: expect.any(String),
18+
},
19+
did: '1337',
20+
errors: 0,
21+
init: true,
22+
sid: expect.any(String),
23+
started: expect.any(String),
24+
status: 'ok',
25+
timestamp: expect.any(String),
26+
});
1627
});
1728

18-
sentryTest('should start a new session with navigation.', async ({ getLocalTestUrl, page }) => {
29+
sentryTest('starts a new session with navigation.', async ({ getLocalTestUrl, page }) => {
1930
const url = await getLocalTestUrl({ testDir: __dirname });
31+
const initSessionPromise = waitForSession(page, s => !!s.init && s.status === 'ok');
2032

21-
// Route must be set up before any navigation to avoid race conditions
22-
await page.route('**/foo', (route: Route) => route.continue({ url }));
23-
24-
const initSession = await getFirstSentryEnvelopeRequest<SessionContext>(page, url);
33+
await page.goto(url);
34+
const initSession = await initSessionPromise;
2535

36+
const newSessionPromise = waitForSession(page, s => !!s.init && s.status === 'ok');
2637
await page.locator('#navigate').click();
2738

28-
const newSession = await getFirstSentryEnvelopeRequest<SessionContext>(page, url);
39+
const newSession = await newSessionPromise;
2940

3041
expect(newSession).toBeDefined();
3142
expect(newSession.init).toBe(true);

‎dev-packages/browser-integration-tests/suites/sessions/page-lifecycle/subject.js‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,3 +10,7 @@ document.getElementById('manual-session').addEventListener('click', () => {
1010
Sentry.startSession();
1111
Sentry.captureException('Test error from manual session');
1212
});
13+
14+
document.getElementById('error').addEventListener('click', () => {
15+
throw new Error('Test error from error button');
16+
});

‎dev-packages/browser-integration-tests/suites/sessions/page-lifecycle/template.html‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,5 +6,6 @@
66
<body>
77
<button id="navigate">Navigate via pushState</button>
88
<button id="manual-session">Manual session</button>
9+
<button id="error">Throw error</button>
910
</body>
1011
</html>

‎dev-packages/browser-integration-tests/suites/sessions/page-lifecycle/test.ts‎

Lines changed: 56 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -1,45 +1,53 @@
11
import { expect } from '@playwright/test';
2-
import type { SessionContext } from '@sentry/core';
2+
import type { SerializedSession } from '@sentry/core/src';
33
import { sentryTest } from '../../../utils/fixtures';
4-
import { getMultipleSentryEnvelopeRequests } from '../../../utils/helpers';
4+
import {
5+
envelopeRequestParser,
6+
getMultipleSentryEnvelopeRequests,
7+
waitForErrorRequest,
8+
waitForSession,
9+
} from '../../../utils/helpers';
510

6-
sentryTest('should start a session on pageload with page lifecycle.', async ({ getLocalTestUrl, page }) => {
11+
sentryTest('starts a session on pageload with page lifecycle.', async ({ getLocalTestUrl, page }) => {
712
const url = await getLocalTestUrl({ testDir: __dirname });
813

9-
const sessions = await getMultipleSentryEnvelopeRequests<SessionContext>(page, 1, {
10-
url,
11-
envelopeType: 'session',
12-
timeout: 2000,
13-
});
14+
const sessionPromise = waitForSession(page, s => !!s.init && s.status === 'ok');
15+
await page.goto(url);
16+
const session = await sessionPromise;
1417

15-
expect(sessions.length).toBeGreaterThanOrEqual(1);
16-
const session = sessions[0];
1718
expect(session).toBeDefined();
18-
expect(session.init).toBe(true);
19-
expect(session.errors).toBe(0);
20-
expect(session.status).toBe('ok');
19+
expect(session).toEqual({
20+
attrs: {
21+
environment: 'production',
22+
release: '0.1',
23+
user_agent: expect.any(String),
24+
},
25+
errors: 0,
26+
init: true,
27+
sid: expect.any(String),
28+
started: expect.any(String),
29+
status: 'ok',
30+
timestamp: expect.any(String),
31+
});
2132
});
2233

2334
sentryTest(
24-
'should NOT start a new session on pushState navigation with page lifecycle.',
35+
"doesn't start a new session on pushState navigation with page lifecycle.",
2536
async ({ getLocalTestUrl, page }) => {
2637
const url = await getLocalTestUrl({ testDir: __dirname });
2738

28-
const sessionsPromise = getMultipleSentryEnvelopeRequests<SessionContext>(page, 10, {
39+
const sessionsPromise = getMultipleSentryEnvelopeRequests<SerializedSession>(page, 10, {
2940
url,
3041
envelopeType: 'session',
3142
timeout: 4000,
3243
});
3344

34-
const manualSessionsPromise = getMultipleSentryEnvelopeRequests<SessionContext>(page, 10, {
45+
const manualSessionsPromise = getMultipleSentryEnvelopeRequests<SerializedSession>(page, 10, {
3546
envelopeType: 'session',
3647
timeout: 4000,
3748
});
3849

39-
const eventsPromise = getMultipleSentryEnvelopeRequests<SessionContext>(page, 10, {
40-
envelopeType: 'event',
41-
timeout: 4000,
42-
});
50+
const eventsPromise = waitForErrorRequest(page, e => e.message === 'Test error from manual session');
4351

4452
await page.waitForSelector('#navigate');
4553

@@ -56,17 +64,42 @@ sentryTest(
5664
await page.locator('#manual-session').click();
5765

5866
const newSessions = (await manualSessionsPromise).filter(session => session.init);
59-
const events = await eventsPromise;
67+
const event = envelopeRequestParser(await eventsPromise);
6068

6169
expect(newSessions.length).toBe(2);
6270
expect(newSessions[0].init).toBe(true);
6371
expect(newSessions[1].init).toBe(true);
6472
expect(newSessions[1].sid).not.toBe(newSessions[0].sid);
65-
expect(events).toEqual([
73+
expect(event).toEqual(
6674
expect.objectContaining({
6775
level: 'error',
6876
message: 'Test error from manual session',
6977
}),
70-
]);
78+
);
7179
},
7280
);
81+
82+
sentryTest('Updates the session when an error is thrown', async ({ getLocalTestUrl, page }) => {
83+
const url = await getLocalTestUrl({ testDir: __dirname });
84+
85+
const initialSessionPromise = waitForSession(page, s => !!s.init && s.status === 'ok');
86+
await page.goto(url);
87+
const initialSession = await initialSessionPromise;
88+
89+
// for good measure, throw in a few navigations
90+
await page.locator('#navigate').click();
91+
await page.locator('#navigate').click();
92+
await page.locator('#navigate').click();
93+
94+
const updatedSessionPromise = waitForSession(page, s => !s.init && s.status !== 'ok');
95+
await page.locator('#error').click();
96+
const updatedSession = await updatedSessionPromise;
97+
98+
expect(updatedSession).toEqual({
99+
...initialSession,
100+
errors: 1,
101+
init: false,
102+
status: 'crashed',
103+
timestamp: expect.any(String),
104+
});
105+
});

‎dev-packages/browser-integration-tests/suites/sessions/route-lifecycle/test.ts‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
import { expect } from '@playwright/test';
2-
import type { SessionContext } from '@sentry/core';
2+
import type { SerializedSession } from '@sentry/core/src';
33
import { sentryTest } from '../../../utils/fixtures';
44
import { getMultipleSentryEnvelopeRequests } from '../../../utils/helpers';
55

@@ -8,7 +8,7 @@ sentryTest(
88
async ({ getLocalTestUrl, page }) => {
99
const url = await getLocalTestUrl({ testDir: __dirname });
1010

11-
const sessionsPromise = getMultipleSentryEnvelopeRequests<SessionContext>(page, 10, {
11+
const sessionsPromise = getMultipleSentryEnvelopeRequests<SerializedSession>(page, 10, {
1212
url,
1313
envelopeType: 'session',
1414
timeout: 4000,
@@ -20,8 +20,8 @@ sentryTest(
2020
await page.locator('#navigate').click();
2121
await page.locator('#navigate').click();
2222

23-
const sessions = (await sessionsPromise).filter(session => session.init);
23+
const startedSessions = (await sessionsPromise).filter(session => session.init);
2424

25-
expect(sessions.length).toBe(3);
25+
expect(startedSessions.length).toBe(4);
2626
},
2727
);

‎dev-packages/browser-integration-tests/suites/sessions/start-session/template.html‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,6 @@
44
<meta charset="utf-8" />
55
</head>
66
<body>
7-
<a id="navigate" href="foo">Navigate</a>
7+
<a id="navigate" href="#foo">Navigate</a>
88
</body>
99
</html>

‎dev-packages/browser-integration-tests/suites/sessions/start-session/test.ts‎

Lines changed: 10 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1,12 +1,12 @@
1-
import type { Route } from '@playwright/test';
21
import { expect } from '@playwright/test';
3-
import type { SessionContext } from '@sentry/core';
42
import { sentryTest } from '../../../utils/fixtures';
5-
import { getFirstSentryEnvelopeRequest } from '../../../utils/helpers';
3+
import { waitForSession } from '../../../utils/helpers';
64

75
sentryTest('should start a new session on pageload.', async ({ getLocalTestUrl, page }) => {
86
const url = await getLocalTestUrl({ testDir: __dirname });
9-
const session = await getFirstSentryEnvelopeRequest<SessionContext>(page, url);
7+
const sessionPromise = waitForSession(page, s => !!s.init && s.status === 'ok');
8+
await page.goto(url);
9+
const session = await sessionPromise;
1010

1111
expect(session).toBeDefined();
1212
expect(session.init).toBe(true);
@@ -16,18 +16,20 @@ sentryTest('should start a new session on pageload.', async ({ getLocalTestUrl,
1616

1717
sentryTest('should start a new session with navigation.', async ({ getLocalTestUrl, page }) => {
1818
const url = await getLocalTestUrl({ testDir: __dirname });
19-
await page.route('**/foo', (route: Route) => route.continue({ url }));
2019

21-
const initSession = await getFirstSentryEnvelopeRequest<SessionContext>(page, url);
20+
const initSessionPromise = waitForSession(page, s => !!s.init && s.status === 'ok');
21+
await page.goto(url);
22+
const initSession = await initSessionPromise;
2223

24+
const newSessionPromise = waitForSession(page, s => !!s.init && s.status === 'ok');
2325
await page.locator('#navigate').click();
24-
25-
const newSession = await getFirstSentryEnvelopeRequest<SessionContext>(page, url);
26+
const newSession = await newSessionPromise;
2627

2728
expect(newSession).toBeDefined();
2829
expect(newSession.init).toBe(true);
2930
expect(newSession.errors).toBe(0);
3031
expect(newSession.status).toBe('ok');
3132
expect(newSession.sid).toBeDefined();
33+
3234
expect(initSession.sid).not.toBe(newSession.sid);
3335
});

0 commit comments

Comments
 (0)