fix(sessions): Correctly compute session duration (#3616)

Closes #3615
* ref: Update the session sent in `@sentry/browser` to ignore duration. The duration is
   not relevant at the moment as it doesn't represent anything users can relate to.
* ref: Leverage timestampInSeconds from @sentry/utils instead of Date.now()
   to make sure that durations are correctly reported to Sentry.
* test: Add tests around creating, updating and closing sessions

packages/browser/src/sdk.ts

@@ -199,7 +199,11 @@ function startSessionTracking(): void {
     return;
   }
 
-  hub.startSession();
+  // The session duration for browser sessions does not track a meaningful
+  // concept that can be used as a metric.
+  // Automatically captured sessions are akin to page views, and thus we
+  // discard their duration.
+  hub.startSession({ ignoreDuration: true });
   hub.captureSession();
 
   // We want to create a session for every navigation as well
@@ -209,7 +213,7 @@ function startSessionTracking(): void {
       if (from === undefined || from === to) {
         return;
       }
-      hub.startSession();
+      hub.startSession({ ignoreDuration: true });
       hub.captureSession();
     },
     type: 'history',

packages/hub/src/session.ts

@@ -8,7 +8,7 @@ import {
   SessionStatus,
   Transport,
 } from '@sentry/types';
-import { dropUndefinedKeys, logger, uuid4 } from '@sentry/utils';
+import { dropUndefinedKeys, logger, timestampInSeconds, uuid4 } from '@sentry/utils';
 
 import { getCurrentHub } from './hub';
 
@@ -21,15 +21,20 @@ export class Session implements SessionInterface {
   public release?: string;
   public sid: string = uuid4();
   public did?: string;
-  public timestamp: number = Date.now();
-  public started: number = Date.now();
-  public duration: number = 0;
+  public timestamp: number;
+  public started: number;
+  public duration?: number = 0;
   public status: SessionStatus = SessionStatus.Ok;
   public environment?: string;
   public ipAddress?: string;
   public init: boolean = true;
+  public ignoreDuration: boolean = false;
 
   public constructor(context?: Omit<SessionContext, 'started' | 'status'>) {
+    // Both timestamp and started are in seconds since the UNIX epoch.
+    const startingTime = timestampInSeconds();
+    this.timestamp = startingTime;
+    this.started = startingTime;
     if (context) {
       this.update(context);
     }
@@ -48,8 +53,10 @@ export class Session implements SessionInterface {
       }
     }
 
-    this.timestamp = context.timestamp || Date.now();
-
+    this.timestamp = context.timestamp || timestampInSeconds();
+    if (context.ignoreDuration) {
+      this.ignoreDuration = context.ignoreDuration;
+    }
     if (context.sid) {
       // Good enough uuid validation. — Kamil
       this.sid = context.sid.length === 32 ? context.sid : uuid4();
@@ -63,10 +70,13 @@ export class Session implements SessionInterface {
     if (typeof context.started === 'number') {
       this.started = context.started;
     }
-    if (typeof context.duration === 'number') {
+    if (this.ignoreDuration) {
+      this.duration = undefined;
+    } else if (typeof context.duration === 'number') {
       this.duration = context.duration;
     } else {
-      this.duration = this.timestamp - this.started;
+      const duration = this.timestamp - this.started;
+      this.duration = duration >= 0 ? duration : 0;
     }
     if (context.release) {
       this.release = context.release;
@@ -106,7 +116,7 @@ export class Session implements SessionInterface {
     did?: string;
     timestamp: string;
     started: string;
-    duration: number;
+    duration?: number;
     status: SessionStatus;
     errors: number;
     attrs?: {
@@ -119,8 +129,9 @@ export class Session implements SessionInterface {
     return dropUndefinedKeys({
       sid: `${this.sid}`,
       init: this.init,
-      started: new Date(this.started).toISOString(),
-      timestamp: new Date(this.timestamp).toISOString(),
+      // Make sure that sec is converted to ms for date constructor
+      started: new Date(this.started * 1000).toISOString(),
+      timestamp: new Date(this.timestamp * 1000).toISOString(),
       status: this.status,
       errors: this.errors,
       did: typeof this.did === 'number' || typeof this.did === 'string' ? `${this.did}` : undefined,

packages/hub/test/session.test.ts

@@ -0,0 +1,115 @@
+import { SessionContext, SessionStatus } from '@sentry/types';
+import { timestampInSeconds } from '@sentry/utils';
+
+import { Session } from '../src/session';
+
+describe('Session', () => {
+  it('initializes with the proper defaults', () => {
+    const session = new Session().toJSON();
+
+    // Grab current year to check if we are converting from sec -> ms correctly
+    const currentYear = new Date(timestampInSeconds() * 1000).toISOString().slice(0, 4);
+    expect(session).toEqual({
+      attrs: {},
+      duration: 0,
+      errors: 0,
+      init: true,
+      sid: expect.any(String),
+      started: expect.stringMatching(currentYear),
+      status: SessionStatus.Ok,
+      timestamp: expect.stringMatching(currentYear),
+    });
+
+    expect(session.sid).toHaveLength(32);
+
+    // started and timestamp should be the same on creation
+    expect(session.started).toEqual(session.timestamp);
+  });
+
+  describe('update', () => {
+    const time = timestampInSeconds();
+    // [ name, in, out ]
+    const table: Array<[string, SessionContext, Record<string, any>]> = [
+      ['sets an ip address', { user: { ip_address: '0.0.0.0' } }, { attrs: { ip_address: '0.0.0.0' } }],
+      ['sets a did', { user: { id: 'specialID123' } }, { did: 'specialID123' }],
+      ['sets a timestamp', { timestamp: time }, { timestamp: new Date(time * 1000).toISOString() }],
+      ['sets a sid', { sid: '99705f22a3f1468e95ba7386e84691aa' }, { sid: '99705f22a3f1468e95ba7386e84691aa' }],
+      ['requires custom sid to be of certain length', { sid: '123' }, { sid: expect.not.stringMatching('123') }],
+      ['requires custom sid to be of certain length', { sid: '123' }, { sid: expect.not.stringMatching('123') }],
+      ['sets an init', { init: false }, { init: false }],
+      ['sets an did', { did: 'specialID123' }, { did: 'specialID123' }],
+      ['overwrites user did with custom did', { did: 'custom-did', user: { id: 'user-id' } }, { did: 'custom-did' }],
+      ['sets a started time', { started: time }, { started: new Date(time * 1000).toISOString() }],
+      ['does not set a duration for browser env', { ignoreDuration: true }, { duration: undefined }],
+      ['sets a duration', { duration: 12000 }, { duration: 12000 }],
+      [
+        'does not use custom duration for browser env',
+        { duration: 12000, ignoreDuration: true },
+        { duration: undefined },
+      ],
+      [
+        'does not set a negative duration',
+        { timestamp: 10, started: 100 },
+        { duration: 0, timestamp: expect.any(String), started: expect.any(String) },
+      ],
+      [
+        'sets duration based on timestamp and started',
+        { timestamp: 100, started: 10 },
+        { duration: 90, timestamp: expect.any(String), started: expect.any(String) },
+      ],
+      [
+        'sets a release',
+        { release: 'f1557994979ecd969963f53c27ca946379d721f3' },
+        { attrs: { release: 'f1557994979ecd969963f53c27ca946379d721f3' } },
+      ],
+      ['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',
+        { ipAddress: '0.0.0.0', user: { ip_address: '1.1.1.1' } },
+        { attrs: { ip_address: '0.0.0.0' } },
+      ],
+      ['sets an userAgent', { userAgent: 'Mozilla/5.0' }, { attrs: { user_agent: 'Mozilla/5.0' } }],
+      ['sets errors', { errors: 3 }, { errors: 3 }],
+      ['sets status', { status: SessionStatus.Crashed }, { status: SessionStatus.Crashed }],
+    ];
+
+    test.each(table)('%s', (...test) => {
+      // duration and timestamp can vary after session update, so let's expect anything unless
+      // the out variable in a test explicitly refers to it.
+      const DEFAULT_OUT = { duration: expect.any(Number), timestamp: expect.any(String) };
+
+      const session = new Session();
+      const initSessionProps = session.toJSON();
+
+      session.update(test[1]);
+      expect(session.toJSON()).toEqual({ ...initSessionProps, ...DEFAULT_OUT, ...test[2] });
+    });
+  });
+
+  describe('close', () => {
+    it('exits a normal session', () => {
+      const session = new Session();
+      expect(session.status).toEqual(SessionStatus.Ok);
+      session.close();
+      expect(session.status).toEqual(SessionStatus.Exited);
+    });
+
+    it('updates session status when give status', () => {
+      const session = new Session();
+      expect(session.status).toEqual(SessionStatus.Ok);
+
+      session.close(SessionStatus.Abnormal);
+      expect(session.status).toEqual(SessionStatus.Abnormal);
+    });
+
+    it('only changes status ok to exited', () => {
+      const session = new Session();
+      session.update({ status: SessionStatus.Crashed });
+      expect(session.status).toEqual(SessionStatus.Crashed);
+
+      session.close();
+      expect(session.status).toEqual(SessionStatus.Crashed);
+    });
+  });
+});

packages/types/src/session.ts

@@ -17,7 +17,7 @@ export interface Session extends SessionContext {
     did?: string;
     timestamp: string;
     started: string;
-    duration: number;
+    duration?: number;
     status: SessionStatus;
     errors: number;
     attrs?: {
@@ -40,7 +40,9 @@ export interface SessionContext {
   sid?: string;
   did?: string;
   init?: boolean;
+  // seconds since the UNIX epoch
   timestamp?: number;
+  // seconds since the UNIX epoch
   started?: number;
   duration?: number;
   status?: SessionStatus;
@@ -50,6 +52,7 @@ export interface SessionContext {
   ipAddress?: string;
   errors?: number;
   user?: User | null;
+  ignoreDuration?: boolean;
 }
 
 /**

packages/utils/src/time.ts

@@ -103,7 +103,7 @@ const timestampSource: TimestampSource =
 /**
  * Returns a timestamp in seconds since the UNIX epoch using the Date API.
  */
-export const dateTimestampInSeconds = dateTimestampSource.nowSeconds.bind(dateTimestampSource);
+export const dateTimestampInSeconds: () => number = dateTimestampSource.nowSeconds.bind(dateTimestampSource);
 
 /**
  * Returns a timestamp in seconds since the UNIX epoch using either the Performance or Date APIs, depending on the
@@ -116,7 +116,7 @@ export const dateTimestampInSeconds = dateTimestampSource.nowSeconds.bind(dateTi
  * skew can grow to arbitrary amounts like days, weeks or months.
  * See https://github.com/getsentry/sentry-javascript/issues/2590.
  */
-export const timestampInSeconds = timestampSource.nowSeconds.bind(timestampSource);
+export const timestampInSeconds: () => number = timestampSource.nowSeconds.bind(timestampSource);
 
 // Re-exported with an old name for backwards-compatibility.
 export const timestampWithMs = timestampInSeconds;