Merge branch 'develop' into feat-replay-change-sampling-logic

Author: billyvg

.github/workflows/label-last-commenter.yml

@@ -16,7 +16,7 @@ jobs:
           github.event.comment.author_association != 'COLLABORATOR'
           && github.event.comment.author_association != 'MEMBER'
           && github.event.comment.author_association != 'OWNER'
-          && !github.event.issue.closed
+          && github.event.issue.state == 'open'
         uses: actions-ecosystem/action-add-labels@v1
         with:
           labels: 'Waiting for: Team'

packages/browser-integration-tests/suites/replay/sessionExpiry/init.js

@@ -17,6 +17,7 @@ Sentry.init({
 });
 
 window.Replay._replay.timeouts = {
-  sessionIdle: 2000, // this is usually 5min, but we want to test this with shorter times
+  sessionIdlePause: 1000, // this is usually 5min, but we want to test this with shorter times
+  sessionIdleExpire: 2000, // this is usually 15min, but we want to test this with shorter times
   maxSessionLife: 3600000, // default: 60min
 };

packages/browser-integration-tests/suites/replay/sessionInactive/init.js

@@ -17,6 +17,7 @@ Sentry.init({
 });
 
 window.Replay._replay.timeouts = {
-  sessionIdle: 1000, // default: 5min
-  maxSessionLife: 2000, // this is usually 60min, but we want to test this with shorter times
+  sessionIdlePause: 1000, // this is usually 5min, but we want to test this with shorter times
+  sessionIdleExpire: 900000, // defayult: 15min
+  maxSessionLife: 3600000, // default: 60min
 };

packages/browser-integration-tests/suites/replay/sessionInactive/test.ts

@@ -11,8 +11,8 @@ import {
   waitForReplayRequest,
 } from '../../../utils/replayHelpers';
 
-// Session should expire after 2s - keep in sync with init.js
-const SESSION_TIMEOUT = 2000;
+// Session should be paused after 2s - keep in sync with init.js
+const SESSION_PAUSED = 2000;
 
 sentryTest('handles an inactive session', async ({ getLocalTestPath, page }) => {
   if (shouldSkipReplayTest()) {
@@ -44,11 +44,8 @@ sentryTest('handles an inactive session', async ({ getLocalTestPath, page }) =>
 
   await page.click('#button1');
 
-  // We wait for another segment 0
-  const reqPromise1 = waitForReplayRequest(page, 0);
-
   // Now we wait for the session timeout, nothing should be sent in the meanwhile
-  await new Promise(resolve => setTimeout(resolve, SESSION_TIMEOUT));
+  await new Promise(resolve => setTimeout(resolve, SESSION_PAUSED));
 
   // nothing happened because no activity/inactivity was detected
   const replay = await getReplaySnapshot(page);
@@ -64,17 +61,17 @@ sentryTest('handles an inactive session', async ({ getLocalTestPath, page }) =>
   expect(replay2._isEnabled).toEqual(true);
   expect(replay2._isPaused).toEqual(true);
 
-  // Trigger an action, should re-start the recording
+  // We wait for next segment to be sent once we resume the session
+  const reqPromise1 = waitForReplayRequest(page);
+
+  // Trigger an action, should resume the recording
   await page.click('#button2');
   const req1 = await reqPromise1;
 
   const replay3 = await getReplaySnapshot(page);
   expect(replay3._isEnabled).toEqual(true);
   expect(replay3._isPaused).toEqual(false);
 
-  const replayEvent1 = getReplayEvent(req1);
-  expect(replayEvent1).toEqual(getExpectedReplayEvent({}));
-
   const fullSnapshots1 = getFullRecordingSnapshots(req1);
   expect(fullSnapshots1.length).toEqual(1);
   const stringifiedSnapshot1 = normalize(fullSnapshots1[0]);

packages/browser-integration-tests/suites/replay/sessionMaxAge/init.js

@@ -17,6 +17,7 @@ Sentry.init({
 });
 
 window.Replay._replay.timeouts = {
-  sessionIdle: 300000, // default: 5min
+  sessionIdlePause: 300000, // default: 5min
+  sessionIdleExpire: 900000, // default: 15min
   maxSessionLife: 4000, // this is usually 60min, but we want to test this with shorter times
 };

packages/core/src/transports/multiplexed.ts

@@ -17,8 +17,9 @@ interface MatchParam {
   /**
    * A function that returns an event from the envelope if one exists. You can optionally pass an array of envelope item
    * types to filter by - only envelopes matching the given types will be multiplexed.
+   * Allowed values are: 'event', 'transaction', 'profile', 'replay_event'
    *
-   * @param types Defaults to ['event', 'transaction', 'profile', 'replay_event']
+   * @param types Defaults to ['event']
    */
   getEvent(types?: EnvelopeItemType[]): Event | undefined;
 }
@@ -61,8 +62,7 @@ export function makeMultiplexedTransport<TO extends BaseTransportOptions>(
 
     async function send(envelope: Envelope): Promise<void | TransportMakeRequestResponse> {
       function getEvent(types?: EnvelopeItemType[]): Event | undefined {
-        const eventTypes: EnvelopeItemType[] =
-          types && types.length ? types : ['event', 'transaction', 'profile', 'replay_event'];
+        const eventTypes: EnvelopeItemType[] = types && types.length ? types : ['event'];
         return eventFromEnvelope(envelope, eventTypes);
       }
 

packages/core/test/lib/transports/multiplexed.test.ts

@@ -1,4 +1,11 @@
-import type { BaseTransportOptions, ClientReport, EventEnvelope, EventItem, Transport } from '@sentry/types';
+import type {
+  BaseTransportOptions,
+  ClientReport,
+  EventEnvelope,
+  EventItem,
+  TransactionEvent,
+  Transport,
+} from '@sentry/types';
 import { createClientReportEnvelope, createEnvelope, dsnFromString } from '@sentry/utils';
 import { TextEncoder } from 'util';
 
@@ -15,9 +22,10 @@ const ERROR_ENVELOPE = createEnvelope<EventEnvelope>({ event_id: 'aa3ff046696b4b
   [{ type: 'event' }, ERROR_EVENT] as EventItem,
 ]);
 
+const TRANSACTION_EVENT: TransactionEvent = { type: 'transaction', event_id: 'aa3ff046696b4bc6b609ce6d28fde9e2' };
 const TRANSACTION_ENVELOPE = createEnvelope<EventEnvelope>(
   { event_id: 'aa3ff046696b4bc6b609ce6d28fde9e2', sent_at: '123' },
-  [[{ type: 'transaction' }, { event_id: 'aa3ff046696b4bc6b609ce6d28fde9e2' }] as EventItem],
+  [[{ type: 'transaction' }, TRANSACTION_EVENT] as EventItem],
 );
 
 const DEFAULT_DISCARDED_EVENTS: ClientReport['discarded_events'] = [
@@ -143,15 +151,32 @@ describe('makeMultiplexedTransport', () => {
     await transport.send(CLIENT_REPORT_ENVELOPE);
   });
 
-  it('callback getEvent can ignore transactions', async () => {
+  it('callback getEvent ignores transactions by default', async () => {
     expect.assertions(2);
 
     const makeTransport = makeMultiplexedTransport(
       createTestTransport(url => {
         expect(url).toBe(DSN2_URL);
       }),
       ({ getEvent }) => {
-        expect(getEvent(['event'])).toBeUndefined();
+        expect(getEvent()).toBeUndefined();
+        return [DSN2];
+      },
+    );
+
+    const transport = makeTransport({ url: DSN1_URL, ...transportOptions });
+    await transport.send(TRANSACTION_ENVELOPE);
+  });
+
+  it('callback getEvent can define envelope types', async () => {
+    expect.assertions(2);
+
+    const makeTransport = makeMultiplexedTransport(
+      createTestTransport(url => {
+        expect(url).toBe(DSN2_URL);
+      }),
+      ({ getEvent }) => {
+        expect(getEvent(['event', 'transaction'])).toBe(TRANSACTION_EVENT);
         return [DSN2];
       },
     );

packages/e2e-tests/package.json

@@ -27,7 +27,7 @@
     "glob": "8.0.3",
     "ts-node": "10.9.1",
     "typescript": "3.8.3",
-    "yaml": "2.1.1"
+    "yaml": "2.2.2"
   },
   "volta": {
     "extends": "../../package.json"

packages/replay/src/constants.ts

@@ -11,11 +11,14 @@ export const REPLAY_EVENT_NAME = 'replay_event';
 export const RECORDING_EVENT_NAME = 'replay_recording';
 export const UNABLE_TO_SEND_REPLAY = 'Unable to send Replay';
 
-// The idle limit for a session
-export const SESSION_IDLE_DURATION = 300_000; // 5 minutes in ms
+// The idle limit for a session after which recording is paused.
+export const SESSION_IDLE_PAUSE_DURATION = 300_000; // 5 minutes in ms
+
+// The idle limit for a session after which the session expires.
+export const SESSION_IDLE_EXPIRE_DURATION = 900_000; // 15 minutes in ms
 
 // The maximum length of a session
-export const MAX_SESSION_LIFE = 3_600_000; // 60 minutes
+export const MAX_SESSION_LIFE = 3_600_000; // 60 minutes in ms
 
 /** Default flush delays */
 export const DEFAULT_FLUSH_MIN_DELAY = 5_000;

packages/replay/src/replay.ts

@@ -4,7 +4,13 @@ import { captureException, getCurrentHub } from '@sentry/core';
 import type { Breadcrumb, ReplayRecordingMode } from '@sentry/types';
 import { logger } from '@sentry/utils';
 
-import { BUFFER_CHECKOUT_TIME, MAX_SESSION_LIFE, SESSION_IDLE_DURATION, WINDOW } from './constants';
+import {
+  BUFFER_CHECKOUT_TIME,
+  MAX_SESSION_LIFE,
+  SESSION_IDLE_EXPIRE_DURATION,
+  SESSION_IDLE_PAUSE_DURATION,
+  WINDOW,
+} from './constants';
 import { setupPerformanceObserver } from './coreHandlers/performanceObserver';
 import { createEventBuffer } from './eventBuffer';
 import { clearSession } from './session/clearSession';
@@ -63,7 +69,8 @@ export class ReplayContainer implements ReplayContainerInterface {
    * @hidden
    */
   public readonly timeouts: Timeouts = {
-    sessionIdle: SESSION_IDLE_DURATION,
+    sessionIdlePause: SESSION_IDLE_PAUSE_DURATION,
+    sessionIdleExpire: SESSION_IDLE_EXPIRE_DURATION,
     maxSessionLife: MAX_SESSION_LIFE,
   } as const;
 
@@ -484,12 +491,12 @@ export class ReplayContainer implements ReplayContainerInterface {
     const oldSessionId = this.getSessionId();
 
     // Prevent starting a new session if the last user activity is older than
-    // SESSION_IDLE_DURATION. Otherwise non-user activity can trigger a new
+    // SESSION_IDLE_PAUSE_DURATION. Otherwise non-user activity can trigger a new
     // session+recording. This creates noisy replays that do not have much
     // content in them.
     if (
       this._lastActivity &&
-      isExpired(this._lastActivity, this.timeouts.sessionIdle) &&
+      isExpired(this._lastActivity, this.timeouts.sessionIdlePause) &&
       this.session &&
       this.session.sampled === 'session'
     ) {
@@ -723,7 +730,7 @@ export class ReplayContainer implements ReplayContainerInterface {
     const isSessionActive = this.checkAndHandleExpiredSession();
 
     if (!isSessionActive) {
-      // If the user has come back to the page within SESSION_IDLE_DURATION
+      // If the user has come back to the page within SESSION_IDLE_PAUSE_DURATION
       // ms, we will re-use the existing session, otherwise create a new
       // session
       __DEBUG_BUILD__ && logger.log('[Replay] Document has become active, but session has expired');

packages/replay/src/types.ts

@@ -25,7 +25,8 @@ export interface SendReplayData {
 }
 
 export interface Timeouts {
-  sessionIdle: number;
+  sessionIdlePause: number;
+  sessionIdleExpire: number;
   maxSessionLife: number;
 }
 
@@ -476,10 +477,7 @@ export interface ReplayContainer {
   performanceEvents: AllPerformanceEntry[];
   session: Session | undefined;
   recordingMode: ReplayRecordingMode;
-  timeouts: {
-    sessionIdle: number;
-    maxSessionLife: number;
-  };
+  timeouts: Timeouts;
   isEnabled(): boolean;
   isPaused(): boolean;
   getContext(): InternalEventContext;

packages/replay/src/util/addEvent.ts

@@ -31,7 +31,7 @@ export async function addEvent(
   // page has been left open and idle for a long period of time and user
   // comes back to trigger a new session. The performance entries rely on
   // `performance.timeOrigin`, which is when the page first opened.
-  if (timestampInMs + replay.timeouts.sessionIdle < Date.now()) {
+  if (timestampInMs + replay.timeouts.sessionIdlePause < Date.now()) {
     return null;
   }
 

packages/replay/src/util/isSessionExpired.ts

@@ -9,7 +9,7 @@ export function isSessionExpired(session: Session, timeouts: Timeouts, targetTim
     // First, check that maximum session length has not been exceeded
     isExpired(session.started, timeouts.maxSessionLife, targetTime) ||
     // check that the idle timeout has not been exceeded (i.e. user has
-    // performed an action within the last `idleTimeout` ms)
-    isExpired(session.lastActivity, timeouts.sessionIdle, targetTime)
+    // performed an action within the last `sessionIdleExpire` ms)
+    isExpired(session.lastActivity, timeouts.sessionIdleExpire, targetTime)
   );
 }

packages/replay/test/integration/errorSampleRate.test.ts

@@ -5,7 +5,7 @@ import {
   BUFFER_CHECKOUT_TIME,
   MAX_SESSION_LIFE,
   REPLAY_SESSION_KEY,
-  SESSION_IDLE_DURATION,
+  SESSION_IDLE_EXPIRE_DURATION,
   WINDOW,
 } from '../../src/constants';
 import type { ReplayContainer } from '../../src/replay';
@@ -252,15 +252,15 @@ describe('Integration | errorSampleRate', () => {
     });
   });
 
-  it('does not send a replay when triggering a full dom snapshot when document becomes visible after [SESSION_IDLE_DURATION]ms', async () => {
+  it('does not send a replay when triggering a full dom snapshot when document becomes visible after [SESSION_IDLE_EXPIRE_DURATION]ms', async () => {
     Object.defineProperty(document, 'visibilityState', {
       configurable: true,
       get: function () {
         return 'visible';
       },
     });
 
-    jest.advanceTimersByTime(SESSION_IDLE_DURATION + 1);
+    jest.advanceTimersByTime(SESSION_IDLE_EXPIRE_DURATION + 1);
 
     document.dispatchEvent(new Event('visibilitychange'));
 
@@ -284,8 +284,8 @@ describe('Integration | errorSampleRate', () => {
 
     expect(replay).not.toHaveLastSentReplay();
 
-    // User comes back before `SESSION_IDLE_DURATION` elapses
-    jest.advanceTimersByTime(SESSION_IDLE_DURATION - 100);
+    // User comes back before `SESSION_IDLE_EXPIRE_DURATION` elapses
+    jest.advanceTimersByTime(SESSION_IDLE_EXPIRE_DURATION - 100);
     Object.defineProperty(document, 'visibilityState', {
       configurable: true,
       get: function () {
@@ -466,6 +466,27 @@ describe('Integration | errorSampleRate', () => {
     jest.advanceTimersByTime(DEFAULT_FLUSH_MIN_DELAY);
     await new Promise(process.nextTick);
 
+    // Remains disabled!
+    expect(replay.isEnabled()).toBe(false);
+  });
+
+  // Should behave the same as above test
+  it('stops replay if user has been idle for more than SESSION_IDLE_EXPIRE_DURATION and does not start a new session thereafter', async () => {
+    // Idle for 15 minutes
+    jest.advanceTimersByTime(SESSION_IDLE_EXPIRE_DURATION + 1);
+
+    const TEST_EVENT = {
+      data: { name: 'lost event' },
+      timestamp: BASE_TIMESTAMP,
+      type: 3,
+    };
+    mockRecord._emitter(TEST_EVENT);
+    expect(replay).not.toHaveLastSentReplay();
+
+    jest.runAllTimers();
+    await new Promise(process.nextTick);
+
+    // We stop recording after SESSION_IDLE_EXPIRE_DURATION of inactivity in error mode
     expect(replay).not.toHaveLastSentReplay();
     expect(replay.isEnabled()).toBe(true);
     expect(replay.isPaused()).toBe(false);
@@ -605,7 +626,7 @@ describe('Integration | errorSampleRate', () => {
     expect(replay).not.toHaveLastSentReplay();
 
     // Go idle
-    jest.advanceTimersByTime(SESSION_IDLE_DURATION + 1);
+    jest.advanceTimersByTime(SESSION_IDLE_EXPIRE_DURATION + 1);
     await new Promise(process.nextTick);
 
     mockRecord._emitter(TEST_EVENT);