From 1534e1e1edb53ef069e890791ed1b5a85f0f0c8d Mon Sep 17 00:00:00 2001 From: Ahmed Etefy Date: Thu, 17 Jun 2021 20:59:53 +0200 Subject: [PATCH 1/9] fix(release-health): Prevent sending terminal status session updates Drops sending session updates for sessions that are already in terminal states and caps the number of errors for session at 1 --- packages/core/src/baseclient.ts | 9 ++- packages/node/package.json | 2 +- .../errors-in-session-capped-to-one.js | 65 +++++++++++++++++++ 3 files changed, 73 insertions(+), 3 deletions(-) create mode 100644 packages/node/test/manual/release-health/single-session/errors-in-session-capped-to-one.js diff --git a/packages/core/src/baseclient.ts b/packages/core/src/baseclient.ts index 7ae94741b16c..a4b9f708bca8 100644 --- a/packages/core/src/baseclient.ts +++ b/packages/core/src/baseclient.ts @@ -253,13 +253,18 @@ export abstract class BaseClient implement } } + const initialSessionStatus = session.status; + const terminalStates = [SessionStatus.Crashed, SessionStatus.Abnormal]; + session.update({ ...(crashed && { status: SessionStatus.Crashed }), user, userAgent, - errors: session.errors + Number(errored || crashed), + errors: Math.max(session.errors, Number(errored || crashed)), }); - this.captureSession(session); + + // Only send a session update if session was not already in a terminal + if (!terminalStates.includes(initialSessionStatus)) this.captureSession(session); } /** Deliver captured session to Sentry */ diff --git a/packages/node/package.json b/packages/node/package.json index e4aa83b0fb8a..f50fe6d5932e 100644 --- a/packages/node/package.json +++ b/packages/node/package.json @@ -60,7 +60,7 @@ "test:watch": "jest --watch", "test:express": "node test/manual/express-scope-separation/start.js", "test:webpack": "cd test/manual/webpack-domain/ && yarn && node npm-build.js", - "test:release-health": "node test/manual/release-health/single-session/healthy-session.js && node test/manual/release-health/single-session/caught-exception-errored-session.js && node test/manual/release-health/single-session/uncaught-exception-crashed-session.js && node test/manual/release-health/single-session/unhandled-rejection-crashed-session.js && node test/manual/release-health/session-aggregates/aggregates-disable-single-session.js", + "test:release-health": "node test/manual/release-health/single-session/healthy-session.js && node test/manual/release-health/single-session/caught-exception-errored-session.js && node test/manual/release-health/single-session/uncaught-exception-crashed-session.js && node test/manual/release-health/single-session/unhandled-rejection-crashed-session.js && node test/manual/release-health/session-aggregates/aggregates-disable-single-session.js && node test/manual/release-health/single-session/errors-in-session-capped-to-one.js", "pack": "npm pack", "circularDepCheck": "madge --circular src/index.ts" }, diff --git a/packages/node/test/manual/release-health/single-session/errors-in-session-capped-to-one.js b/packages/node/test/manual/release-health/single-session/errors-in-session-capped-to-one.js new file mode 100644 index 000000000000..781b22758f7c --- /dev/null +++ b/packages/node/test/manual/release-health/single-session/errors-in-session-capped-to-one.js @@ -0,0 +1,65 @@ +const Sentry = require('../../../../dist'); +const { assertSessions, constructStrippedSessionObject, BaseDummyTransport } = require('../test-utils'); +let sessionCounter = 0; +process.on('exit', ()=> { + if (process.exitCode !== 1) { + console.log('SUCCESS: All application mode sessions were sent to node transport as expected'); + } +}) + +class DummyTransport extends BaseDummyTransport { + sendSession(session) { + sessionCounter++; + if (sessionCounter === 1) { + assertSessions(constructStrippedSessionObject(session), + { + init: true, + status: 'ok', + errors: 1, + release: '1.1' + } + ) + } + else if (sessionCounter === 2) { + assertSessions(constructStrippedSessionObject(session), + { + init: false, + status: 'ok', + errors: 1, + release: '1.1' + } + ) + } + else if (sessionCounter === 3) { + assertSessions(constructStrippedSessionObject(session), + { + init: false, + status: 'exited', + errors: 1, + release: '1.1' + } + ) + } + else { + console.log('FAIL: Received way too many Sessions!'); + process.exit(1); + } + return super.sendSession(session); + } +} + +Sentry.init({ + dsn: 'http://test@example.com/1337', + release: '1.1', + transport: DummyTransport, + autoSessionTracking: true +}); +/** + * The following code snippet will throw multiple errors, and thereby send session updates everytime an error is + * captured. However, the number of errors in the session should be capped at 1, regardless of how many errors there are + */ +for (let i = 0; i < 2; i++) { + Sentry.captureException(new Error('hello world')); +} + + From bc0fd0c535403ff5cf42c93577f99c2f104bb20c Mon Sep 17 00:00:00 2001 From: Ahmed Etefy Date: Fri, 18 Jun 2021 10:03:21 +0200 Subject: [PATCH 2/9] Added test that ensures terminal state sessions are sent only once --- packages/node/package.json | 2 +- .../terminal-state-sessions-sent-once.js | 57 +++++++++++++++++++ 2 files changed, 58 insertions(+), 1 deletion(-) create mode 100644 packages/node/test/manual/release-health/single-session/terminal-state-sessions-sent-once.js diff --git a/packages/node/package.json b/packages/node/package.json index f50fe6d5932e..331e25052b9c 100644 --- a/packages/node/package.json +++ b/packages/node/package.json @@ -60,7 +60,7 @@ "test:watch": "jest --watch", "test:express": "node test/manual/express-scope-separation/start.js", "test:webpack": "cd test/manual/webpack-domain/ && yarn && node npm-build.js", - "test:release-health": "node test/manual/release-health/single-session/healthy-session.js && node test/manual/release-health/single-session/caught-exception-errored-session.js && node test/manual/release-health/single-session/uncaught-exception-crashed-session.js && node test/manual/release-health/single-session/unhandled-rejection-crashed-session.js && node test/manual/release-health/session-aggregates/aggregates-disable-single-session.js && node test/manual/release-health/single-session/errors-in-session-capped-to-one.js", + "test:release-health": "node test/manual/release-health/single-session/healthy-session.js && node test/manual/release-health/single-session/caught-exception-errored-session.js && node test/manual/release-health/single-session/uncaught-exception-crashed-session.js && node test/manual/release-health/single-session/unhandled-rejection-crashed-session.js && node test/manual/release-health/session-aggregates/aggregates-disable-single-session.js && node test/manual/release-health/single-session/errors-in-session-capped-to-one.js & node test/manual/release-health/single-session/terminal-state-sessions-sent-once.js", "pack": "npm pack", "circularDepCheck": "madge --circular src/index.ts" }, diff --git a/packages/node/test/manual/release-health/single-session/terminal-state-sessions-sent-once.js b/packages/node/test/manual/release-health/single-session/terminal-state-sessions-sent-once.js new file mode 100644 index 000000000000..4c3afc231445 --- /dev/null +++ b/packages/node/test/manual/release-health/single-session/terminal-state-sessions-sent-once.js @@ -0,0 +1,57 @@ +const Sentry = require('../../../../dist'); +const { assertSessions, constructStrippedSessionObject, BaseDummyTransport } = require('../test-utils'); + +let sessionCounter = 0; +process.on('exit', () => { + if (process.exitCode !== 1) { + console.log('SUCCESS: All application mode sessions were sent to node transport as expected'); + } +}) + +class DummyTransport extends BaseDummyTransport { + sendSession(session) { + sessionCounter++; + + if (sessionCounter === 1) { + assertSessions(constructStrippedSessionObject(session), + { + init: true, + status: 'crashed', + errors: 1, + release: '1.1' + } + ) + } + else { + console.log('FAIL: Received way too many Sessions!'); + process.exit(1); + } + + return super.sendSession(session); + } +} + +Sentry.init({ + dsn: 'http://test@example.com/1337', + release: '1.1', + transport: DummyTransport, + autoSessionTracking: true +}); + +/** + * The following code snippet will throw an exception of `mechanism.handled` equal to `false`, and so this session + * is treated as a Crashed Session. + * However we want to ensure that once a crashed terminal state is achieved, no more session updates are sent regardless + * of whether more crashes happen or not + */ +new Promise(function(resolve, reject) { + reject(); +}).then(function() { + console.log('Promise Resolved'); +}); + +new Promise(function(resolve, reject) { + reject(); +}).then(function() { + console.log('Promise Resolved'); +}); From 7b694678e3b03e17ce5c57066a54d06b16f7c023 Mon Sep 17 00:00:00 2001 From: Ahmed Etefy Date: Fri, 18 Jun 2021 10:44:11 +0200 Subject: [PATCH 3/9] Update user data on session only if it is not set before --- packages/hub/src/session.ts | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/packages/hub/src/session.ts b/packages/hub/src/session.ts index 6e2755793cdb..654754d2aee7 100644 --- a/packages/hub/src/session.ts +++ b/packages/hub/src/session.ts @@ -33,11 +33,11 @@ export class Session implements SessionInterface { // eslint-disable-next-line complexity public update(context: SessionContext = {}): void { if (context.user) { - if (context.user.ip_address) { + if (!this.ipAddress && context.user.ip_address) { this.ipAddress = context.user.ip_address; } - if (!context.did) { + if (!this.did && !context.did) { this.did = context.user.id || context.user.email || context.user.username; } } @@ -53,7 +53,7 @@ export class Session implements SessionInterface { if (context.init !== undefined) { this.init = context.init; } - if (context.did) { + if (!this.did && context.did) { this.did = `${context.did}`; } if (typeof context.started === 'number') { @@ -73,10 +73,10 @@ export class Session implements SessionInterface { if (context.environment) { this.environment = context.environment; } - if (context.ipAddress) { + if (!this.ipAddress && context.ipAddress) { this.ipAddress = context.ipAddress; } - if (context.userAgent) { + if (!this.userAgent && context.userAgent) { this.userAgent = context.userAgent; } if (typeof context.errors === 'number') { From 9840b69129764f6cdcaf45a9a772b7a7307413ee Mon Sep 17 00:00:00 2001 From: Ahmed Etefy Date: Fri, 18 Jun 2021 10:52:45 +0200 Subject: [PATCH 4/9] Adds changes that doesn't send updates once a session is errored --- packages/core/src/baseclient.ts | 9 +++++---- .../single-session/errors-in-session-capped-to-one.js | 10 ---------- 2 files changed, 5 insertions(+), 14 deletions(-) diff --git a/packages/core/src/baseclient.ts b/packages/core/src/baseclient.ts index a4b9f708bca8..fb955a664528 100644 --- a/packages/core/src/baseclient.ts +++ b/packages/core/src/baseclient.ts @@ -253,8 +253,8 @@ export abstract class BaseClient implement } } - const initialSessionStatus = session.status; - const terminalStates = [SessionStatus.Crashed, SessionStatus.Abnormal]; + const terminalStates = [SessionStatus.Crashed, SessionStatus.Abnormal, SessionStatus.Exited]; + const shouldSendUpdate = !terminalStates.includes(session.status) && session.errors !== 1; session.update({ ...(crashed && { status: SessionStatus.Crashed }), @@ -263,8 +263,9 @@ export abstract class BaseClient implement errors: Math.max(session.errors, Number(errored || crashed)), }); - // Only send a session update if session was not already in a terminal - if (!terminalStates.includes(initialSessionStatus)) this.captureSession(session); + // Only send a session update if session was not already in a terminal state or if it didn't already have an errored + // state i.e. errors: 1 + if (shouldSendUpdate) this.captureSession(session); } /** Deliver captured session to Sentry */ diff --git a/packages/node/test/manual/release-health/single-session/errors-in-session-capped-to-one.js b/packages/node/test/manual/release-health/single-session/errors-in-session-capped-to-one.js index 781b22758f7c..ce5ae0ce1289 100644 --- a/packages/node/test/manual/release-health/single-session/errors-in-session-capped-to-one.js +++ b/packages/node/test/manual/release-health/single-session/errors-in-session-capped-to-one.js @@ -21,16 +21,6 @@ class DummyTransport extends BaseDummyTransport { ) } else if (sessionCounter === 2) { - assertSessions(constructStrippedSessionObject(session), - { - init: false, - status: 'ok', - errors: 1, - release: '1.1' - } - ) - } - else if (sessionCounter === 3) { assertSessions(constructStrippedSessionObject(session), { init: false, From 1c3d0e4574888471f2d253df4115e88970bd91c3 Mon Sep 17 00:00:00 2001 From: Ahmed Etefy Date: Fri, 18 Jun 2021 11:46:21 +0200 Subject: [PATCH 5/9] Fixing PR comments --- packages/core/src/baseclient.ts | 8 +++--- .../errors-in-session-capped-to-one.js | 27 +++++++++++++------ .../terminal-state-sessions-sent-once.js | 15 ++++++----- 3 files changed, 30 insertions(+), 20 deletions(-) diff --git a/packages/core/src/baseclient.ts b/packages/core/src/baseclient.ts index fb955a664528..ad8147cc55e2 100644 --- a/packages/core/src/baseclient.ts +++ b/packages/core/src/baseclient.ts @@ -253,18 +253,16 @@ export abstract class BaseClient implement } } - const terminalStates = [SessionStatus.Crashed, SessionStatus.Abnormal, SessionStatus.Exited]; - const shouldSendUpdate = !terminalStates.includes(session.status) && session.errors !== 1; + const shouldSendUpdate = session.status === SessionStatus.Ok; session.update({ ...(crashed && { status: SessionStatus.Crashed }), user, userAgent, - errors: Math.max(session.errors, Number(errored || crashed)), + errors: session.errors || Number(errored || crashed), }); - // Only send a session update if session was not already in a terminal state or if it didn't already have an errored - // state i.e. errors: 1 + // Only send a session update if session was not already in a terminal state if (shouldSendUpdate) this.captureSession(session); } diff --git a/packages/node/test/manual/release-health/single-session/errors-in-session-capped-to-one.js b/packages/node/test/manual/release-health/single-session/errors-in-session-capped-to-one.js index ce5ae0ce1289..3d958f3f4f44 100644 --- a/packages/node/test/manual/release-health/single-session/errors-in-session-capped-to-one.js +++ b/packages/node/test/manual/release-health/single-session/errors-in-session-capped-to-one.js @@ -1,8 +1,15 @@ const Sentry = require('../../../../dist'); const { assertSessions, constructStrippedSessionObject, BaseDummyTransport } = require('../test-utils'); + let sessionCounter = 0; -process.on('exit', ()=> { - if (process.exitCode !== 1) { +const expectedSessions = 3; + +process.on('exit', (exitCode) => { + if (sessionCounter !== expectedSessions) { + console.log(`FAIL: Expected ${expectedSessions} Sessions, Received ${sessionCounter}.`); + process.exitCode = 1 + } + if (exitCode === 0) { console.log('SUCCESS: All application mode sessions were sent to node transport as expected'); } }) @@ -24,15 +31,21 @@ class DummyTransport extends BaseDummyTransport { assertSessions(constructStrippedSessionObject(session), { init: false, - status: 'exited', + status: 'ok', errors: 1, release: '1.1' } ) } - else { - console.log('FAIL: Received way too many Sessions!'); - process.exit(1); + else if (sessionCounter === 3) { + assertSessions(constructStrippedSessionObject(session), + { + init: false, + status: 'exited', + errors: 1, + release: '1.1' + } + ) } return super.sendSession(session); } @@ -51,5 +64,3 @@ Sentry.init({ for (let i = 0; i < 2; i++) { Sentry.captureException(new Error('hello world')); } - - diff --git a/packages/node/test/manual/release-health/single-session/terminal-state-sessions-sent-once.js b/packages/node/test/manual/release-health/single-session/terminal-state-sessions-sent-once.js index 4c3afc231445..d9f933aba15f 100644 --- a/packages/node/test/manual/release-health/single-session/terminal-state-sessions-sent-once.js +++ b/packages/node/test/manual/release-health/single-session/terminal-state-sessions-sent-once.js @@ -2,8 +2,14 @@ const Sentry = require('../../../../dist'); const { assertSessions, constructStrippedSessionObject, BaseDummyTransport } = require('../test-utils'); let sessionCounter = 0; -process.on('exit', () => { - if (process.exitCode !== 1) { +let expectedSessions = 1; + +process.on('exit', (exitCode) => { + if (sessionCounter !== expectedSessions) { + console.log(`FAIL: Expected ${expectedSessions} Sessions, Received ${sessionCounter}.`); + process.exitCode = 1 + } + if (exitCode === 0) { console.log('SUCCESS: All application mode sessions were sent to node transport as expected'); } }) @@ -22,11 +28,6 @@ class DummyTransport extends BaseDummyTransport { } ) } - else { - console.log('FAIL: Received way too many Sessions!'); - process.exit(1); - } - return super.sendSession(session); } } From a2b6f28749f66b58566fb70e32a34e6ec5ada039 Mon Sep 17 00:00:00 2001 From: Ahmed Etefy Date: Fri, 18 Jun 2021 11:52:22 +0200 Subject: [PATCH 6/9] Fixed failing test that overwrites user ip address in session once it is set --- packages/hub/test/session.test.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/hub/test/session.test.ts b/packages/hub/test/session.test.ts index 83dbeae3fc6e..d334becd4821 100644 --- a/packages/hub/test/session.test.ts +++ b/packages/hub/test/session.test.ts @@ -68,9 +68,9 @@ describe('Session', () => { ['sets an environment', { environment: 'staging' }, { attrs: { environment: 'staging' } }], ['sets an ipAddress', { ipAddress: '0.0.0.0' }, { attrs: { ip_address: '0.0.0.0' } }], [ - 'overwrites user ip_address did with custom ipAddress', + 'should not overwrite user ip_address did with custom ipAddress', { ipAddress: '0.0.0.0', user: { ip_address: '1.1.1.1' } }, - { attrs: { ip_address: '0.0.0.0' } }, + { attrs: { ip_address: '1.1.1.1' } }, ], ['sets an userAgent', { userAgent: 'Mozilla/5.0' }, { attrs: { user_agent: 'Mozilla/5.0' } }], ['sets errors', { errors: 3 }, { errors: 3 }], From 838567ce020be1095dc452930654632fa197ecb0 Mon Sep 17 00:00:00 2001 From: Ahmed Etefy Date: Fri, 18 Jun 2021 12:27:06 +0200 Subject: [PATCH 7/9] Refactored tests --- .../errors-in-session-capped-to-one.js | 77 ++++++++----------- .../terminal-state-sessions-sent-once.js | 43 +++++------ .../test/manual/release-health/test-utils.js | 15 +++- 3 files changed, 68 insertions(+), 67 deletions(-) diff --git a/packages/node/test/manual/release-health/single-session/errors-in-session-capped-to-one.js b/packages/node/test/manual/release-health/single-session/errors-in-session-capped-to-one.js index 3d958f3f4f44..c0a7e3b1f67d 100644 --- a/packages/node/test/manual/release-health/single-session/errors-in-session-capped-to-one.js +++ b/packages/node/test/manual/release-health/single-session/errors-in-session-capped-to-one.js @@ -1,51 +1,42 @@ const Sentry = require('../../../../dist'); -const { assertSessions, constructStrippedSessionObject, BaseDummyTransport } = require('../test-utils'); +const { + assertSessions, + constructStrippedSessionObject, + BaseDummyTransport, + validateSessionCountFunction, +} = require('../test-utils'); -let sessionCounter = 0; -const expectedSessions = 3; +const sessionCounts = { + sessionCounter: 0, + expectedSessions: 3, +}; -process.on('exit', (exitCode) => { - if (sessionCounter !== expectedSessions) { - console.log(`FAIL: Expected ${expectedSessions} Sessions, Received ${sessionCounter}.`); - process.exitCode = 1 - } - if (exitCode === 0) { - console.log('SUCCESS: All application mode sessions were sent to node transport as expected'); - } -}) +validateSessionCountFunction(sessionCounts); class DummyTransport extends BaseDummyTransport { sendSession(session) { - sessionCounter++; - if (sessionCounter === 1) { - assertSessions(constructStrippedSessionObject(session), - { - init: true, - status: 'ok', - errors: 1, - release: '1.1' - } - ) - } - else if (sessionCounter === 2) { - assertSessions(constructStrippedSessionObject(session), - { - init: false, - status: 'ok', - errors: 1, - release: '1.1' - } - ) - } - else if (sessionCounter === 3) { - assertSessions(constructStrippedSessionObject(session), - { - init: false, - status: 'exited', - errors: 1, - release: '1.1' - } - ) + sessionCounts.sessionCounter++; + if (sessionCounts.sessionCounter === 1) { + assertSessions(constructStrippedSessionObject(session), { + init: true, + status: 'ok', + errors: 1, + release: '1.1', + }); + } else if (sessionCounts.sessionCounter === 2) { + assertSessions(constructStrippedSessionObject(session), { + init: false, + status: 'ok', + errors: 1, + release: '1.1', + }); + } else if (sessionCounts.sessionCounter === 3) { + assertSessions(constructStrippedSessionObject(session), { + init: false, + status: 'exited', + errors: 1, + release: '1.1', + }); } return super.sendSession(session); } @@ -55,7 +46,7 @@ Sentry.init({ dsn: 'http://test@example.com/1337', release: '1.1', transport: DummyTransport, - autoSessionTracking: true + autoSessionTracking: true, }); /** * The following code snippet will throw multiple errors, and thereby send session updates everytime an error is diff --git a/packages/node/test/manual/release-health/single-session/terminal-state-sessions-sent-once.js b/packages/node/test/manual/release-health/single-session/terminal-state-sessions-sent-once.js index d9f933aba15f..4336e94ef7c9 100644 --- a/packages/node/test/manual/release-health/single-session/terminal-state-sessions-sent-once.js +++ b/packages/node/test/manual/release-health/single-session/terminal-state-sessions-sent-once.js @@ -1,32 +1,29 @@ const Sentry = require('../../../../dist'); -const { assertSessions, constructStrippedSessionObject, BaseDummyTransport } = require('../test-utils'); +const { + assertSessions, + constructStrippedSessionObject, + BaseDummyTransport, + validateSessionCountFunction, +} = require('../test-utils'); -let sessionCounter = 0; -let expectedSessions = 1; +const sessionCounts = { + sessionCounter: 0, + expectedSessions: 1, +}; -process.on('exit', (exitCode) => { - if (sessionCounter !== expectedSessions) { - console.log(`FAIL: Expected ${expectedSessions} Sessions, Received ${sessionCounter}.`); - process.exitCode = 1 - } - if (exitCode === 0) { - console.log('SUCCESS: All application mode sessions were sent to node transport as expected'); - } -}) +validateSessionCountFunction(sessionCounts); class DummyTransport extends BaseDummyTransport { sendSession(session) { - sessionCounter++; + sessionCounts.sessionCounter++; - if (sessionCounter === 1) { - assertSessions(constructStrippedSessionObject(session), - { - init: true, - status: 'crashed', - errors: 1, - release: '1.1' - } - ) + if (sessionCounts.sessionCounter === 1) { + assertSessions(constructStrippedSessionObject(session), { + init: true, + status: 'crashed', + errors: 1, + release: '1.1', + }); } return super.sendSession(session); } @@ -36,7 +33,7 @@ Sentry.init({ dsn: 'http://test@example.com/1337', release: '1.1', transport: DummyTransport, - autoSessionTracking: true + autoSessionTracking: true, }); /** diff --git a/packages/node/test/manual/release-health/test-utils.js b/packages/node/test/manual/release-health/test-utils.js index 7b0b93b59825..f38ff1dac57c 100644 --- a/packages/node/test/manual/release-health/test-utils.js +++ b/packages/node/test/manual/release-health/test-utils.js @@ -26,4 +26,17 @@ class BaseDummyTransport { } } -module.exports = { assertSessions, constructStrippedSessionObject, BaseDummyTransport }; +function validateSessionCountFunction(sessionCounts) { + process.on('exit', exitCode => { + const { sessionCounter, expectedSessions } = sessionCounts; + if (sessionCounter !== expectedSessions) { + console.log(`FAIL: Expected ${expectedSessions} Sessions, Received ${sessionCounter}.`); + process.exitCode = 1; + } + if ((exitCode === 0) & !process.exitCode) { + console.log('SUCCESS: All application mode sessions were sent to node transport as expected'); + } + }); +} + +module.exports = { assertSessions, constructStrippedSessionObject, BaseDummyTransport, validateSessionCountFunction }; From ca87b1f070e6d9d2bebced0b8dc55992a287936b Mon Sep 17 00:00:00 2001 From: Ahmed Etefy Date: Fri, 18 Jun 2021 15:28:23 +0200 Subject: [PATCH 8/9] Removed update of user attrs on session + send only session update for status changes --- packages/core/src/baseclient.ts | 34 +++++++------------ packages/hub/src/hub.ts | 7 ++++ .../errors-in-session-capped-to-one.js | 9 +---- 3 files changed, 20 insertions(+), 30 deletions(-) diff --git a/packages/core/src/baseclient.ts b/packages/core/src/baseclient.ts index ad8147cc55e2..f725ff98f091 100644 --- a/packages/core/src/baseclient.ts +++ b/packages/core/src/baseclient.ts @@ -227,7 +227,6 @@ export abstract class BaseClient implement protected _updateSessionFromEvent(session: Session, event: Event): void { let crashed = false; let errored = false; - let userAgent; const exceptions = event.exception && event.exception.values; if (exceptions) { @@ -242,28 +241,19 @@ export abstract class BaseClient implement } } - const user = event.user; - if (!session.userAgent) { - const headers = event.request ? event.request.headers : {}; - for (const key in headers) { - if (key.toLowerCase() === 'user-agent') { - userAgent = headers[key]; - break; - } - } - } - - const shouldSendUpdate = session.status === SessionStatus.Ok; + // A session is updated and that session update is sent in only one of the two following scenarios: + // 1. Session with non terminal status and 0 errors + an error occurred -> Will set error count to 1 and send update + // 2. Session with non terminal status and 1 error + a crash occurred -> Will set status crashed and send update + const sessionNonTerminal = session.status === SessionStatus.Ok; + const shouldUpdateAndSend = (sessionNonTerminal && session.errors === 0) || (sessionNonTerminal && crashed); - session.update({ - ...(crashed && { status: SessionStatus.Crashed }), - user, - userAgent, - errors: session.errors || Number(errored || crashed), - }); - - // Only send a session update if session was not already in a terminal state - if (shouldSendUpdate) this.captureSession(session); + if (shouldUpdateAndSend) { + session.update({ + ...(crashed && { status: SessionStatus.Crashed }), + errors: session.errors || Number(errored || crashed), + }); + this.captureSession(session); + } } /** Deliver captured session to Sentry */ diff --git a/packages/hub/src/hub.ts b/packages/hub/src/hub.ts index 160a73e8b29f..7e79fff0dd40 100644 --- a/packages/hub/src/hub.ts +++ b/packages/hub/src/hub.ts @@ -424,10 +424,17 @@ export class Hub implements HubInterface { public startSession(context?: SessionContext): Session { const { scope, client } = this.getStackTop(); const { release, environment } = (client && client.getOptions()) || {}; + + // Will fetch userAgent if called from browser sdk + const global = getGlobalObject(); + // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access + const { userAgent } = (global as any).navigator || {}; + const session = new Session({ release, environment, ...(scope && { user: scope.getUser() }), + ...(userAgent && { userAgent: userAgent }), ...context, }); diff --git a/packages/node/test/manual/release-health/single-session/errors-in-session-capped-to-one.js b/packages/node/test/manual/release-health/single-session/errors-in-session-capped-to-one.js index c0a7e3b1f67d..d5224d467391 100644 --- a/packages/node/test/manual/release-health/single-session/errors-in-session-capped-to-one.js +++ b/packages/node/test/manual/release-health/single-session/errors-in-session-capped-to-one.js @@ -8,7 +8,7 @@ const { const sessionCounts = { sessionCounter: 0, - expectedSessions: 3, + expectedSessions: 2, }; validateSessionCountFunction(sessionCounts); @@ -24,13 +24,6 @@ class DummyTransport extends BaseDummyTransport { release: '1.1', }); } else if (sessionCounts.sessionCounter === 2) { - assertSessions(constructStrippedSessionObject(session), { - init: false, - status: 'ok', - errors: 1, - release: '1.1', - }); - } else if (sessionCounts.sessionCounter === 3) { assertSessions(constructStrippedSessionObject(session), { init: false, status: 'exited', From 88d9634cf2f89fa21e5e88bcf719eed58fb14d57 Mon Sep 17 00:00:00 2001 From: Ahmed Etefy Date: Fri, 18 Jun 2021 15:32:19 +0200 Subject: [PATCH 9/9] Fixed type --- packages/hub/src/hub.ts | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/packages/hub/src/hub.ts b/packages/hub/src/hub.ts index 7e79fff0dd40..1b14f6f5b899 100644 --- a/packages/hub/src/hub.ts +++ b/packages/hub/src/hub.ts @@ -426,15 +426,14 @@ export class Hub implements HubInterface { const { release, environment } = (client && client.getOptions()) || {}; // Will fetch userAgent if called from browser sdk - const global = getGlobalObject(); - // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access - const { userAgent } = (global as any).navigator || {}; + const global = getGlobalObject<{ navigator?: { userAgent?: string } }>(); + const { userAgent } = global.navigator || {}; const session = new Session({ release, environment, ...(scope && { user: scope.getUser() }), - ...(userAgent && { userAgent: userAgent }), + ...(userAgent && { userAgent }), ...context, });