fix(release-health): Prevent sending terminal status session updates (#3701)

* 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

Author: ahmedetefy

packages/core/src/baseclient.ts

@@ -228,7 +228,6 @@ export abstract class BaseClient<B extends Backend, O extends Options> 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) {
@@ -243,24 +242,19 @@ export abstract class BaseClient<B extends Backend, O extends Options> 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;
-        }
-      }
-    }
+    // 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),
-    });
-    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 */

packages/hub/src/hub.ts

@@ -424,10 +424,16 @@ 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<{ navigator?: { userAgent?: string } }>();
+    const { userAgent } = global.navigator || {};
+
     const session = new Session({
       release,
       environment,
       ...(scope && { user: scope.getUser() }),
+      ...(userAgent && { userAgent }),
       ...context,
     });
 

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') {

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 }],

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 & node test/manual/release-health/single-session/terminal-state-sessions-sent-once.js",
     "pack": "npm pack",
     "circularDepCheck": "madge --circular src/index.ts"
   },

packages/node/test/manual/release-health/single-session/errors-in-session-capped-to-one.js

@@ -0,0 +1,50 @@
+const Sentry = require('../../../../dist');
+const {
+  assertSessions,
+  constructStrippedSessionObject,
+  BaseDummyTransport,
+  validateSessionCountFunction,
+} = require('../test-utils');
+
+const sessionCounts = {
+  sessionCounter: 0,
+  expectedSessions: 2,
+};
+
+validateSessionCountFunction(sessionCounts);
+
+class DummyTransport extends BaseDummyTransport {
+  sendSession(session) {
+    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: 'exited',
+        errors: 1,
+        release: '1.1',
+      });
+    }
+    return super.sendSession(session);
+  }
+}
+
+Sentry.init({
+  dsn: 'http://[email protected]/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'));
+}

packages/node/test/manual/release-health/single-session/terminal-state-sessions-sent-once.js

@@ -0,0 +1,55 @@
+const Sentry = require('../../../../dist');
+const {
+  assertSessions,
+  constructStrippedSessionObject,
+  BaseDummyTransport,
+  validateSessionCountFunction,
+} = require('../test-utils');
+
+const sessionCounts = {
+  sessionCounter: 0,
+  expectedSessions: 1,
+};
+
+validateSessionCountFunction(sessionCounts);
+
+class DummyTransport extends BaseDummyTransport {
+  sendSession(session) {
+    sessionCounts.sessionCounter++;
+
+    if (sessionCounts.sessionCounter === 1) {
+      assertSessions(constructStrippedSessionObject(session), {
+        init: true,
+        status: 'crashed',
+        errors: 1,
+        release: '1.1',
+      });
+    }
+    return super.sendSession(session);
+  }
+}
+
+Sentry.init({
+  dsn: 'http://[email protected]/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');
+});

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 };