From 0956c95192f1b34cc0e9f4e9afa12b4e2bf6d135 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Mon, 5 Dec 2022 17:07:07 +0100 Subject: [PATCH 1/7] ref(replay): Extract `session.toJSON()` into test util As this is only used in tests, we don't need to include this in the user-facing bundle. --- packages/replay/src/session/Session.ts | 15 ++++----------- .../replay/test/unit/session/fetchSession.test.ts | 3 ++- .../replay/test/unit/session/getSession.test.ts | 9 +++++---- packages/replay/test/utils/sessionToJson.ts | 11 +++++++++++ 4 files changed, 22 insertions(+), 16 deletions(-) create mode 100644 packages/replay/test/utils/sessionToJson.ts diff --git a/packages/replay/src/session/Session.ts b/packages/replay/src/session/Session.ts index 4eb20c057519..2b1436fe14d8 100644 --- a/packages/replay/src/session/Session.ts +++ b/packages/replay/src/session/Session.ts @@ -5,7 +5,7 @@ import { isSampled } from '../util/isSampled'; type Sampled = false | 'session' | 'error'; -interface SessionObject { +export interface SessionObject { id: string; /** @@ -29,6 +29,9 @@ interface SessionObject { sampled: Sampled; } +/** + * A wrapper for the session object that handles sampling. + */ export class Session { /** * Session ID @@ -69,14 +72,4 @@ export class Session { this.sampled = session.sampled ?? (isSampled(sessionSampleRate) ? 'session' : isSampled(errorSampleRate) ? 'error' : false); } - - toJSON(): SessionObject { - return { - id: this.id, - started: this.started, - lastActivity: this.lastActivity, - segmentId: this.segmentId, - sampled: this.sampled, - } as SessionObject; - } } diff --git a/packages/replay/test/unit/session/fetchSession.test.ts b/packages/replay/test/unit/session/fetchSession.test.ts index 89cddfb30599..b029c1cc7d30 100644 --- a/packages/replay/test/unit/session/fetchSession.test.ts +++ b/packages/replay/test/unit/session/fetchSession.test.ts @@ -1,5 +1,6 @@ import { REPLAY_SESSION_KEY, WINDOW } from '../../../src/constants'; import { fetchSession } from '../../../src/session/fetchSession'; +import { sessionToJSON } from '../../utils/sessionToJson'; const oldSessionStorage = WINDOW.sessionStorage; @@ -26,7 +27,7 @@ it('fetches a valid and sampled session', function () { '{"id":"fd09adfc4117477abc8de643e5a5798a","sampled": true,"started":1648827162630,"lastActivity":1648827162658}', ); - expect(fetchSession(SAMPLE_RATES)?.toJSON()).toEqual({ + expect(sessionToJSON(fetchSession(SAMPLE_RATES)!)).toEqual({ id: 'fd09adfc4117477abc8de643e5a5798a', lastActivity: 1648827162658, segmentId: 0, diff --git a/packages/replay/test/unit/session/getSession.test.ts b/packages/replay/test/unit/session/getSession.test.ts index ddc5e4a6e515..b0cea5d9b3d8 100644 --- a/packages/replay/test/unit/session/getSession.test.ts +++ b/packages/replay/test/unit/session/getSession.test.ts @@ -4,6 +4,7 @@ import * as FetchSession from '../../../src/session/fetchSession'; import { getSession } from '../../../src/session/getSession'; import { saveSession } from '../../../src/session/saveSession'; import { Session } from '../../../src/session/Session'; +import { sessionToJSON } from '../../utils/sessionToJson'; jest.mock('@sentry/utils', () => { return { @@ -52,7 +53,7 @@ it('creates a non-sticky session when one does not exist', function () { expect(FetchSession.fetchSession).not.toHaveBeenCalled(); expect(CreateSession.createSession).toHaveBeenCalled(); - expect(session.toJSON()).toEqual({ + expect(sessionToJSON(session)).toEqual({ id: 'test_session_id', segmentId: 0, lastActivity: expect.any(Number), @@ -115,7 +116,7 @@ it('creates a sticky session when one does not exist', function () { expect(FetchSession.fetchSession).toHaveBeenCalled(); expect(CreateSession.createSession).toHaveBeenCalled(); - expect(session.toJSON()).toEqual({ + expect(sessionToJSON(session)).toEqual({ id: 'test_session_id', segmentId: 0, lastActivity: expect.any(Number), @@ -124,7 +125,7 @@ it('creates a sticky session when one does not exist', function () { }); // Should not have anything in storage - expect(FetchSession.fetchSession(SAMPLE_RATES)?.toJSON()).toEqual({ + expect(sessionToJSON(FetchSession.fetchSession(SAMPLE_RATES)!)).toEqual({ id: 'test_session_id', segmentId: 0, lastActivity: expect.any(Number), @@ -147,7 +148,7 @@ it('fetches an existing sticky session', function () { expect(FetchSession.fetchSession).toHaveBeenCalled(); expect(CreateSession.createSession).not.toHaveBeenCalled(); - expect(session.toJSON()).toEqual({ + expect(sessionToJSON(session)).toEqual({ id: 'test_session_id', segmentId: 0, lastActivity: now, diff --git a/packages/replay/test/utils/sessionToJson.ts b/packages/replay/test/utils/sessionToJson.ts new file mode 100644 index 000000000000..3a121955a096 --- /dev/null +++ b/packages/replay/test/utils/sessionToJson.ts @@ -0,0 +1,11 @@ +import type { Session, SessionObject } from '../../src/session/Session'; + +export function sessionToJSON(session: Session): SessionObject { + return { + id: session.id, + started: session.started, + lastActivity: session.lastActivity, + segmentId: session.segmentId, + sampled: session.sampled, + }; +} From d5852615eb764208a63e3624e3461d30dae93dc8 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Mon, 5 Dec 2022 17:19:04 +0100 Subject: [PATCH 2/7] ref(replay): Make session a POJO To trim down bundle size a bit. --- packages/replay/src/session/Session.ts | 68 +++++++------------ packages/replay/src/session/createSession.ts | 13 ++-- packages/replay/src/session/fetchSession.ts | 4 +- .../replay/test/unit/session/Session.test.ts | 37 ++++++---- .../test/unit/session/fetchSession.test.ts | 3 +- .../test/unit/session/getSession.test.ts | 17 +++-- .../test/unit/session/saveSession.test.ts | 4 +- .../test/unit/util/isSessionExpired.test.ts | 4 +- packages/replay/test/utils/sessionToJson.ts | 11 --- 9 files changed, 71 insertions(+), 90 deletions(-) delete mode 100644 packages/replay/test/utils/sessionToJson.ts diff --git a/packages/replay/src/session/Session.ts b/packages/replay/src/session/Session.ts index 2b1436fe14d8..699f96fdbb17 100644 --- a/packages/replay/src/session/Session.ts +++ b/packages/replay/src/session/Session.ts @@ -5,7 +5,7 @@ import { isSampled } from '../util/isSampled'; type Sampled = false | 'session' | 'error'; -export interface SessionObject { +export interface Session { id: string; /** @@ -24,52 +24,34 @@ export interface SessionObject { segmentId: number; /** - * Is the session sampled? `null` if the sampled, otherwise, `session` or `error` + * The ID of the previous session. + * If this is empty, there was no previous session. + */ + previousSessionId?: string; + + /** + * Is the session sampled? `false` if not sampled, otherwise, `session` or `error` */ sampled: Sampled; } /** - * A wrapper for the session object that handles sampling. + * Get a session with defaults & applied sampling. */ -export class Session { - /** - * Session ID - */ - public readonly id: string; - - /** - * Start time of current session - */ - public started: number; - - /** - * Last known activity of the session - */ - public lastActivity: number; - - /** - * Sequence ID specific to replay updates - */ - public segmentId: number; - - /** - * Previous session ID - */ - public previousSessionId: string | undefined; - - /** - * Is the Session sampled? - */ - public readonly sampled: Sampled; - - public constructor(session: Partial = {}, { sessionSampleRate, errorSampleRate }: SampleRates) { - const now = new Date().getTime(); - this.id = session.id || uuid4(); - this.started = session.started ?? now; - this.lastActivity = session.lastActivity ?? now; - this.segmentId = session.segmentId ?? 0; - this.sampled = - session.sampled ?? (isSampled(sessionSampleRate) ? 'session' : isSampled(errorSampleRate) ? 'error' : false); - } +export function makeSession(session: Partial, { sessionSampleRate, errorSampleRate }: SampleRates): Session { + const now = new Date().getTime(); + const id = session.id || uuid4(); + const started = session.started ?? now; + const lastActivity = session.lastActivity ?? now; + const segmentId = session.segmentId ?? 0; + const sampled = + session.sampled ?? (isSampled(sessionSampleRate) ? 'session' : isSampled(errorSampleRate) ? 'error' : false); + + return { + id, + started, + lastActivity, + segmentId, + sampled, + }; } diff --git a/packages/replay/src/session/createSession.ts b/packages/replay/src/session/createSession.ts index d5d2e1ee98e6..d15dda4018b8 100644 --- a/packages/replay/src/session/createSession.ts +++ b/packages/replay/src/session/createSession.ts @@ -2,7 +2,7 @@ import { logger } from '@sentry/utils'; import { SessionOptions } from '../types'; import { saveSession } from './saveSession'; -import { Session } from './Session'; +import { makeSession, Session } from './Session'; /** * Create a new session, which in its current implementation is a Sentry event @@ -10,10 +10,13 @@ import { Session } from './Session'; * one of these Sentry events per "replay session". */ export function createSession({ sessionSampleRate, errorSampleRate, stickySession = false }: SessionOptions): Session { - const session = new Session(undefined, { - errorSampleRate, - sessionSampleRate, - }); + const session = makeSession( + {}, + { + errorSampleRate, + sessionSampleRate, + }, + ); __DEBUG_BUILD__ && logger.log(`[Replay] Creating new session: ${session.id}`); diff --git a/packages/replay/src/session/fetchSession.ts b/packages/replay/src/session/fetchSession.ts index 1585c4037644..16be86f13206 100644 --- a/packages/replay/src/session/fetchSession.ts +++ b/packages/replay/src/session/fetchSession.ts @@ -1,6 +1,6 @@ import { REPLAY_SESSION_KEY, WINDOW } from '../constants'; import { SampleRates } from '../types'; -import { Session } from './Session'; +import { makeSession, Session } from './Session'; /** * Fetches a session from storage @@ -22,7 +22,7 @@ export function fetchSession({ sessionSampleRate, errorSampleRate }: SampleRates const sessionObj = JSON.parse(sessionStringFromStorage); - return new Session(sessionObj, { sessionSampleRate, errorSampleRate }); + return makeSession(sessionObj, { sessionSampleRate, errorSampleRate }); } catch { return null; } diff --git a/packages/replay/test/unit/session/Session.test.ts b/packages/replay/test/unit/session/Session.test.ts index b82459c96610..d8fc592c95d1 100644 --- a/packages/replay/test/unit/session/Session.test.ts +++ b/packages/replay/test/unit/session/Session.test.ts @@ -26,7 +26,7 @@ jest.mock('@sentry/utils', () => { import * as Sentry from '@sentry/browser'; import { WINDOW } from '../../../src/constants'; -import { Session } from '../../../src/session/Session'; +import { makeSession } from '../../../src/session/Session'; type CaptureEventMockType = jest.MockedFunction; @@ -39,34 +39,43 @@ afterEach(() => { }); it('does not sample', function () { - const newSession = new Session(undefined, { - sessionSampleRate: 0.0, - errorSampleRate: 0.0, - }); + const newSession = makeSession( + {}, + { + sessionSampleRate: 0.0, + errorSampleRate: 0.0, + }, + ); expect(newSession.sampled).toBe(false); }); it('samples using `sessionSampleRate`', function () { - const newSession = new Session(undefined, { - sessionSampleRate: 1.0, - errorSampleRate: 0.0, - }); + const newSession = makeSession( + {}, + { + sessionSampleRate: 1.0, + errorSampleRate: 0.0, + }, + ); expect(newSession.sampled).toBe('session'); }); it('samples using `errorSampleRate`', function () { - const newSession = new Session(undefined, { - sessionSampleRate: 0, - errorSampleRate: 1.0, - }); + const newSession = makeSession( + {}, + { + sessionSampleRate: 0, + errorSampleRate: 1.0, + }, + ); expect(newSession.sampled).toBe('error'); }); it('does not run sampling function if existing session was sampled', function () { - const newSession = new Session( + const newSession = makeSession( { sampled: 'session', }, diff --git a/packages/replay/test/unit/session/fetchSession.test.ts b/packages/replay/test/unit/session/fetchSession.test.ts index b029c1cc7d30..68fe2cb0435b 100644 --- a/packages/replay/test/unit/session/fetchSession.test.ts +++ b/packages/replay/test/unit/session/fetchSession.test.ts @@ -1,6 +1,5 @@ import { REPLAY_SESSION_KEY, WINDOW } from '../../../src/constants'; import { fetchSession } from '../../../src/session/fetchSession'; -import { sessionToJSON } from '../../utils/sessionToJson'; const oldSessionStorage = WINDOW.sessionStorage; @@ -27,7 +26,7 @@ it('fetches a valid and sampled session', function () { '{"id":"fd09adfc4117477abc8de643e5a5798a","sampled": true,"started":1648827162630,"lastActivity":1648827162658}', ); - expect(sessionToJSON(fetchSession(SAMPLE_RATES)!)).toEqual({ + expect(fetchSession(SAMPLE_RATES)).toEqual({ id: 'fd09adfc4117477abc8de643e5a5798a', lastActivity: 1648827162658, segmentId: 0, diff --git a/packages/replay/test/unit/session/getSession.test.ts b/packages/replay/test/unit/session/getSession.test.ts index b0cea5d9b3d8..a3d7631371a3 100644 --- a/packages/replay/test/unit/session/getSession.test.ts +++ b/packages/replay/test/unit/session/getSession.test.ts @@ -3,8 +3,7 @@ import * as CreateSession from '../../../src/session/createSession'; import * as FetchSession from '../../../src/session/fetchSession'; import { getSession } from '../../../src/session/getSession'; import { saveSession } from '../../../src/session/saveSession'; -import { Session } from '../../../src/session/Session'; -import { sessionToJSON } from '../../utils/sessionToJson'; +import { makeSession } from '../../../src/session/Session'; jest.mock('@sentry/utils', () => { return { @@ -19,7 +18,7 @@ const SAMPLE_RATES = { }; function createMockSession(when: number = new Date().getTime()) { - return new Session( + return makeSession( { id: 'test_session_id', segmentId: 0, @@ -53,7 +52,7 @@ it('creates a non-sticky session when one does not exist', function () { expect(FetchSession.fetchSession).not.toHaveBeenCalled(); expect(CreateSession.createSession).toHaveBeenCalled(); - expect(sessionToJSON(session)).toEqual({ + expect(session).toEqual({ id: 'test_session_id', segmentId: 0, lastActivity: expect.any(Number), @@ -85,7 +84,7 @@ it('creates a non-sticky session, when one is expired', function () { expiry: 1000, stickySession: false, ...SAMPLE_RATES, - currentSession: new Session( + currentSession: makeSession( { id: 'old_session_id', lastActivity: new Date().getTime() - 1001, @@ -116,7 +115,7 @@ it('creates a sticky session when one does not exist', function () { expect(FetchSession.fetchSession).toHaveBeenCalled(); expect(CreateSession.createSession).toHaveBeenCalled(); - expect(sessionToJSON(session)).toEqual({ + expect(session).toEqual({ id: 'test_session_id', segmentId: 0, lastActivity: expect.any(Number), @@ -125,7 +124,7 @@ it('creates a sticky session when one does not exist', function () { }); // Should not have anything in storage - expect(sessionToJSON(FetchSession.fetchSession(SAMPLE_RATES)!)).toEqual({ + expect(FetchSession.fetchSession(SAMPLE_RATES)).toEqual({ id: 'test_session_id', segmentId: 0, lastActivity: expect.any(Number), @@ -148,7 +147,7 @@ it('fetches an existing sticky session', function () { expect(FetchSession.fetchSession).toHaveBeenCalled(); expect(CreateSession.createSession).not.toHaveBeenCalled(); - expect(sessionToJSON(session)).toEqual({ + expect(session).toEqual({ id: 'test_session_id', segmentId: 0, lastActivity: now, @@ -181,7 +180,7 @@ it('fetches a non-expired non-sticky session', function () { expiry: 1000, stickySession: false, ...SAMPLE_RATES, - currentSession: new Session( + currentSession: makeSession( { id: 'test_session_id_2', lastActivity: +new Date() - 500, diff --git a/packages/replay/test/unit/session/saveSession.test.ts b/packages/replay/test/unit/session/saveSession.test.ts index 804725f220bd..c6c75bc6affc 100644 --- a/packages/replay/test/unit/session/saveSession.test.ts +++ b/packages/replay/test/unit/session/saveSession.test.ts @@ -1,6 +1,6 @@ import { REPLAY_SESSION_KEY, WINDOW } from '../../../src/constants'; import { saveSession } from '../../../src/session/saveSession'; -import { Session } from '../../../src/session/Session'; +import { makeSession } from '../../../src/session/Session'; beforeAll(() => { WINDOW.sessionStorage.clear(); @@ -11,7 +11,7 @@ afterEach(() => { }); it('saves a valid session', function () { - const session = new Session( + const session = makeSession( { id: 'fd09adfc4117477abc8de643e5a5798a', segmentId: 0, diff --git a/packages/replay/test/unit/util/isSessionExpired.test.ts b/packages/replay/test/unit/util/isSessionExpired.test.ts index 6e8c01c939b8..41323cdf5f3b 100644 --- a/packages/replay/test/unit/util/isSessionExpired.test.ts +++ b/packages/replay/test/unit/util/isSessionExpired.test.ts @@ -1,8 +1,8 @@ -import { Session } from '../../../src/session/Session'; +import { makeSession } from '../../../src/session/Session'; import { isSessionExpired } from '../../../src/util/isSessionExpired'; function createSession(extra?: Record) { - return new Session( + return makeSession( { started: 0, lastActivity: 0, diff --git a/packages/replay/test/utils/sessionToJson.ts b/packages/replay/test/utils/sessionToJson.ts deleted file mode 100644 index 3a121955a096..000000000000 --- a/packages/replay/test/utils/sessionToJson.ts +++ /dev/null @@ -1,11 +0,0 @@ -import type { Session, SessionObject } from '../../src/session/Session'; - -export function sessionToJSON(session: Session): SessionObject { - return { - id: session.id, - started: session.started, - lastActivity: session.lastActivity, - segmentId: session.segmentId, - sampled: session.sampled, - }; -} From 079a1bf67506b9ed41b30dcc2ac72466d291fd05 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Mon, 5 Dec 2022 17:25:06 +0100 Subject: [PATCH 3/7] docs(replay): Add migration docs --- packages/replay/MIGRATION.md | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/packages/replay/MIGRATION.md b/packages/replay/MIGRATION.md index 2d9360bafc0a..0d300e462c43 100644 --- a/packages/replay/MIGRATION.md +++ b/packages/replay/MIGRATION.md @@ -57,3 +57,8 @@ If you only imported from `@sentry/replay`, this will not affect you. It is highly unlikely to affect anybody, but the type `IEventBuffer` was renamed to `EventBuffer` for consistency. Unless you manually imported this and used it somewhere in your codebase, this will not affect you. + +## Session object is now a POJO (https://github.com/getsentry/sentry-javascript/pull/6417) + +The `Session` object exported from Replay is now a POJO, instead of a class. +This should not affect you unless you specifically accessed this class & did custom things with it. From d2a06b10aedc4cdc0b62ec7463990dfb71237da1 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Tue, 6 Dec 2022 12:02:29 +0100 Subject: [PATCH 4/7] ref(replay): Avoid nullish coalescing operator --- packages/replay/src/session/Session.ts | 18 +++++++++++++----- .../test/unit/util/isSessionExpired.test.ts | 7 ++++--- 2 files changed, 17 insertions(+), 8 deletions(-) diff --git a/packages/replay/src/session/Session.ts b/packages/replay/src/session/Session.ts index 699f96fdbb17..6d910d4cc06d 100644 --- a/packages/replay/src/session/Session.ts +++ b/packages/replay/src/session/Session.ts @@ -41,11 +41,11 @@ export interface Session { export function makeSession(session: Partial, { sessionSampleRate, errorSampleRate }: SampleRates): Session { const now = new Date().getTime(); const id = session.id || uuid4(); - const started = session.started ?? now; - const lastActivity = session.lastActivity ?? now; - const segmentId = session.segmentId ?? 0; - const sampled = - session.sampled ?? (isSampled(sessionSampleRate) ? 'session' : isSampled(errorSampleRate) ? 'error' : false); + // Note that this means we cannot set a started/lastActivity of `0`, but this should not be relevant outside of tests. + const started = session.started || now; + const lastActivity = session.lastActivity || now; + const segmentId = session.segmentId || 0; + const sampled = sampleSession(session.sampled, { sessionSampleRate, errorSampleRate }); return { id, @@ -55,3 +55,11 @@ export function makeSession(session: Partial, { sessionSampleRate, erro sampled, }; } + +function sampleSession(sampled: Sampled | undefined, { sessionSampleRate, errorSampleRate }: SampleRates): Sampled { + if (typeof sampled !== 'undefined') { + return sampled; + } + + return isSampled(sessionSampleRate) ? 'session' : isSampled(errorSampleRate) ? 'error' : false; +} diff --git a/packages/replay/test/unit/util/isSessionExpired.test.ts b/packages/replay/test/unit/util/isSessionExpired.test.ts index 41323cdf5f3b..e891a5927349 100644 --- a/packages/replay/test/unit/util/isSessionExpired.test.ts +++ b/packages/replay/test/unit/util/isSessionExpired.test.ts @@ -4,8 +4,9 @@ import { isSessionExpired } from '../../../src/util/isSessionExpired'; function createSession(extra?: Record) { return makeSession( { - started: 0, - lastActivity: 0, + // Setting started/lastActivity to 0 makes it use the default, which is `Date.now()` + started: 1, + lastActivity: 1, segmentId: 0, ...extra, }, @@ -26,5 +27,5 @@ it('session age is not older than max session life', function () { }); it('session age is older than max session life', function () { - expect(isSessionExpired(createSession(), 1_800_000, 1_800_000)).toBe(true); // Session expires at ts >= 1_800_000 + expect(isSessionExpired(createSession(), 1_800_000, 1_800_001)).toBe(true); // Session expires at ts >= 1_800_000 }); From 9b19dd4b8f45717e948eb8399886c3c5e62d58ac Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Tue, 6 Dec 2022 10:10:37 +0100 Subject: [PATCH 5/7] ref(replay): Extract sample decision for session --- packages/replay/src/session/Session.ts | 14 ++++-- packages/replay/src/session/createSession.ts | 13 ++--- packages/replay/src/session/fetchSession.ts | 7 +-- .../replay/test/unit/session/Session.test.ts | 44 +++++------------ .../test/unit/session/fetchSession.test.ts | 34 ++++++++++++- .../test/unit/session/getSession.test.ts | 49 ++++++++----------- .../test/unit/session/saveSession.test.ts | 17 +++---- .../test/unit/util/isSessionExpired.test.ts | 18 +++---- 8 files changed, 100 insertions(+), 96 deletions(-) diff --git a/packages/replay/src/session/Session.ts b/packages/replay/src/session/Session.ts index 6d910d4cc06d..ec83b4b73821 100644 --- a/packages/replay/src/session/Session.ts +++ b/packages/replay/src/session/Session.ts @@ -1,6 +1,5 @@ import { uuid4 } from '@sentry/utils'; -import { SampleRates } from '../types'; import { isSampled } from '../util/isSampled'; type Sampled = false | 'session' | 'error'; @@ -38,14 +37,14 @@ export interface Session { /** * Get a session with defaults & applied sampling. */ -export function makeSession(session: Partial, { sessionSampleRate, errorSampleRate }: SampleRates): Session { +export function makeSession(session: Partial & { sampled: Sampled }): Session { const now = new Date().getTime(); const id = session.id || uuid4(); // Note that this means we cannot set a started/lastActivity of `0`, but this should not be relevant outside of tests. const started = session.started || now; const lastActivity = session.lastActivity || now; const segmentId = session.segmentId || 0; - const sampled = sampleSession(session.sampled, { sessionSampleRate, errorSampleRate }); + const sampled = session.sampled; return { id, @@ -56,7 +55,14 @@ export function makeSession(session: Partial, { sessionSampleRate, erro }; } -function sampleSession(sampled: Sampled | undefined, { sessionSampleRate, errorSampleRate }: SampleRates): Sampled { +/** + * Get the sampled status for a session based on sample rates & current sampled status. + */ +export function sampleSession( + sampled: Sampled | undefined, + sessionSampleRate: number, + errorSampleRate: number, +): Sampled { if (typeof sampled !== 'undefined') { return sampled; } diff --git a/packages/replay/src/session/createSession.ts b/packages/replay/src/session/createSession.ts index d15dda4018b8..3cb38b64a423 100644 --- a/packages/replay/src/session/createSession.ts +++ b/packages/replay/src/session/createSession.ts @@ -2,7 +2,7 @@ import { logger } from '@sentry/utils'; import { SessionOptions } from '../types'; import { saveSession } from './saveSession'; -import { makeSession, Session } from './Session'; +import { makeSession, sampleSession, Session } from './Session'; /** * Create a new session, which in its current implementation is a Sentry event @@ -10,13 +10,10 @@ import { makeSession, Session } from './Session'; * one of these Sentry events per "replay session". */ export function createSession({ sessionSampleRate, errorSampleRate, stickySession = false }: SessionOptions): Session { - const session = makeSession( - {}, - { - errorSampleRate, - sessionSampleRate, - }, - ); + const sampled = sampleSession(undefined, sessionSampleRate, errorSampleRate); + const session = makeSession({ + sampled, + }); __DEBUG_BUILD__ && logger.log(`[Replay] Creating new session: ${session.id}`); diff --git a/packages/replay/src/session/fetchSession.ts b/packages/replay/src/session/fetchSession.ts index 16be86f13206..5143e8580891 100644 --- a/packages/replay/src/session/fetchSession.ts +++ b/packages/replay/src/session/fetchSession.ts @@ -1,6 +1,6 @@ import { REPLAY_SESSION_KEY, WINDOW } from '../constants'; import { SampleRates } from '../types'; -import { makeSession, Session } from './Session'; +import { makeSession, sampleSession, Session } from './Session'; /** * Fetches a session from storage @@ -20,9 +20,10 @@ export function fetchSession({ sessionSampleRate, errorSampleRate }: SampleRates return null; } - const sessionObj = JSON.parse(sessionStringFromStorage); + const sessionObj = JSON.parse(sessionStringFromStorage) as Partial; + const sampled = sampleSession(sessionObj.sampled, sessionSampleRate, errorSampleRate); - return makeSession(sessionObj, { sessionSampleRate, errorSampleRate }); + return makeSession({ ...sessionObj, sampled }); } catch { return null; } diff --git a/packages/replay/test/unit/session/Session.test.ts b/packages/replay/test/unit/session/Session.test.ts index d8fc592c95d1..7ab0083a3ab8 100644 --- a/packages/replay/test/unit/session/Session.test.ts +++ b/packages/replay/test/unit/session/Session.test.ts @@ -26,7 +26,7 @@ jest.mock('@sentry/utils', () => { import * as Sentry from '@sentry/browser'; import { WINDOW } from '../../../src/constants'; -import { makeSession } from '../../../src/session/Session'; +import { makeSession, sampleSession } from '../../../src/session/Session'; type CaptureEventMockType = jest.MockedFunction; @@ -39,51 +39,33 @@ afterEach(() => { }); it('does not sample', function () { - const newSession = makeSession( - {}, - { - sessionSampleRate: 0.0, - errorSampleRate: 0.0, - }, - ); + const newSession = makeSession({ + sampled: sampleSession(undefined, 0, 0), + }); expect(newSession.sampled).toBe(false); }); it('samples using `sessionSampleRate`', function () { - const newSession = makeSession( - {}, - { - sessionSampleRate: 1.0, - errorSampleRate: 0.0, - }, - ); + const newSession = makeSession({ + sampled: sampleSession(undefined, 1.0, 0), + }); expect(newSession.sampled).toBe('session'); }); it('samples using `errorSampleRate`', function () { - const newSession = makeSession( - {}, - { - sessionSampleRate: 0, - errorSampleRate: 1.0, - }, - ); + const newSession = makeSession({ + sampled: sampleSession(undefined, 0, 1), + }); expect(newSession.sampled).toBe('error'); }); it('does not run sampling function if existing session was sampled', function () { - const newSession = makeSession( - { - sampled: 'session', - }, - { - sessionSampleRate: 0, - errorSampleRate: 0, - }, - ); + const newSession = makeSession({ + sampled: 'session', + }); expect(newSession.sampled).toBe('session'); }); diff --git a/packages/replay/test/unit/session/fetchSession.test.ts b/packages/replay/test/unit/session/fetchSession.test.ts index 68fe2cb0435b..f8df61086f42 100644 --- a/packages/replay/test/unit/session/fetchSession.test.ts +++ b/packages/replay/test/unit/session/fetchSession.test.ts @@ -23,14 +23,44 @@ const SAMPLE_RATES = { it('fetches a valid and sampled session', function () { WINDOW.sessionStorage.setItem( REPLAY_SESSION_KEY, - '{"id":"fd09adfc4117477abc8de643e5a5798a","sampled": true,"started":1648827162630,"lastActivity":1648827162658}', + '{"id":"fd09adfc4117477abc8de643e5a5798a","sampled": "session","started":1648827162630,"lastActivity":1648827162658}', ); expect(fetchSession(SAMPLE_RATES)).toEqual({ id: 'fd09adfc4117477abc8de643e5a5798a', lastActivity: 1648827162658, segmentId: 0, - sampled: true, + sampled: 'session', + started: 1648827162630, + }); +}); + +it('fetches an unsampled session', function () { + WINDOW.sessionStorage.setItem( + REPLAY_SESSION_KEY, + '{"id":"fd09adfc4117477abc8de643e5a5798a","sampled": false,"started":1648827162630,"lastActivity":1648827162658}', + ); + + expect(fetchSession(SAMPLE_RATES)).toEqual({ + id: 'fd09adfc4117477abc8de643e5a5798a', + lastActivity: 1648827162658, + segmentId: 0, + sampled: false, + started: 1648827162630, + }); +}); + +it('auto-fixes a session without sampled', function () { + WINDOW.sessionStorage.setItem( + REPLAY_SESSION_KEY, + '{"id":"fd09adfc4117477abc8de643e5a5798a","started":1648827162630,"lastActivity":1648827162658}', + ); + + expect(fetchSession(SAMPLE_RATES)).toEqual({ + id: 'fd09adfc4117477abc8de643e5a5798a', + lastActivity: 1648827162658, + segmentId: 0, + sampled: 'session', started: 1648827162630, }); }); diff --git a/packages/replay/test/unit/session/getSession.test.ts b/packages/replay/test/unit/session/getSession.test.ts index a3d7631371a3..022521c83abe 100644 --- a/packages/replay/test/unit/session/getSession.test.ts +++ b/packages/replay/test/unit/session/getSession.test.ts @@ -18,16 +18,13 @@ const SAMPLE_RATES = { }; function createMockSession(when: number = new Date().getTime()) { - return makeSession( - { - id: 'test_session_id', - segmentId: 0, - lastActivity: when, - started: when, - sampled: 'session', - }, - { ...SAMPLE_RATES }, - ); + return makeSession({ + id: 'test_session_id', + segmentId: 0, + lastActivity: when, + started: when, + sampled: 'session', + }); } beforeAll(() => { @@ -84,15 +81,13 @@ it('creates a non-sticky session, when one is expired', function () { expiry: 1000, stickySession: false, ...SAMPLE_RATES, - currentSession: makeSession( - { - id: 'old_session_id', - lastActivity: new Date().getTime() - 1001, - started: new Date().getTime() - 1001, - segmentId: 0, - }, - { ...SAMPLE_RATES }, - ), + currentSession: makeSession({ + id: 'old_session_id', + lastActivity: new Date().getTime() - 1001, + started: new Date().getTime() - 1001, + segmentId: 0, + sampled: 'session', + }), }); expect(FetchSession.fetchSession).not.toHaveBeenCalled(); @@ -180,15 +175,13 @@ it('fetches a non-expired non-sticky session', function () { expiry: 1000, stickySession: false, ...SAMPLE_RATES, - currentSession: makeSession( - { - id: 'test_session_id_2', - lastActivity: +new Date() - 500, - started: +new Date() - 500, - segmentId: 0, - }, - { ...SAMPLE_RATES }, - ), + currentSession: makeSession({ + id: 'test_session_id_2', + lastActivity: +new Date() - 500, + started: +new Date() - 500, + segmentId: 0, + sampled: 'session', + }), }); expect(FetchSession.fetchSession).not.toHaveBeenCalled(); diff --git a/packages/replay/test/unit/session/saveSession.test.ts b/packages/replay/test/unit/session/saveSession.test.ts index c6c75bc6affc..c1a884bca84f 100644 --- a/packages/replay/test/unit/session/saveSession.test.ts +++ b/packages/replay/test/unit/session/saveSession.test.ts @@ -11,16 +11,13 @@ afterEach(() => { }); it('saves a valid session', function () { - const session = makeSession( - { - id: 'fd09adfc4117477abc8de643e5a5798a', - segmentId: 0, - started: 1648827162630, - lastActivity: 1648827162658, - sampled: 'session', - }, - { sessionSampleRate: 1.0, errorSampleRate: 0 }, - ); + const session = makeSession({ + id: 'fd09adfc4117477abc8de643e5a5798a', + segmentId: 0, + started: 1648827162630, + lastActivity: 1648827162658, + sampled: 'session', + }); saveSession(session); expect(WINDOW.sessionStorage.getItem(REPLAY_SESSION_KEY)).toEqual(JSON.stringify(session)); diff --git a/packages/replay/test/unit/util/isSessionExpired.test.ts b/packages/replay/test/unit/util/isSessionExpired.test.ts index e891a5927349..0db371e8e4ef 100644 --- a/packages/replay/test/unit/util/isSessionExpired.test.ts +++ b/packages/replay/test/unit/util/isSessionExpired.test.ts @@ -2,16 +2,14 @@ import { makeSession } from '../../../src/session/Session'; import { isSessionExpired } from '../../../src/util/isSessionExpired'; function createSession(extra?: Record) { - return makeSession( - { - // Setting started/lastActivity to 0 makes it use the default, which is `Date.now()` - started: 1, - lastActivity: 1, - segmentId: 0, - ...extra, - }, - { sessionSampleRate: 1.0, errorSampleRate: 0 }, - ); + return makeSession({ + // Setting started/lastActivity to 0 makes it use the default, which is `Date.now()` + started: 1, + lastActivity: 1, + segmentId: 0, + sampled: 'session', + ...extra, + }); } it('session last activity is older than expiry time', function () { From a0c0afdda0da1844cc2fd279d71634d872887ada Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Tue, 6 Dec 2022 10:39:31 +0100 Subject: [PATCH 6/7] ref(replay): Sessions don't need to be sampled when fetched --- packages/replay/src/session/fetchSession.ts | 10 +++---- packages/replay/src/session/getSession.ts | 2 +- .../test/unit/session/fetchSession.test.ts | 30 ++++--------------- .../test/unit/session/getSession.test.ts | 6 ++-- 4 files changed, 13 insertions(+), 35 deletions(-) diff --git a/packages/replay/src/session/fetchSession.ts b/packages/replay/src/session/fetchSession.ts index 5143e8580891..c2dc95c9454c 100644 --- a/packages/replay/src/session/fetchSession.ts +++ b/packages/replay/src/session/fetchSession.ts @@ -1,11 +1,10 @@ import { REPLAY_SESSION_KEY, WINDOW } from '../constants'; -import { SampleRates } from '../types'; -import { makeSession, sampleSession, Session } from './Session'; +import { makeSession, Session } from './Session'; /** * Fetches a session from storage */ -export function fetchSession({ sessionSampleRate, errorSampleRate }: SampleRates): Session | null { +export function fetchSession(): Session | null { const hasSessionStorage = 'sessionStorage' in WINDOW; if (!hasSessionStorage) { @@ -20,10 +19,9 @@ export function fetchSession({ sessionSampleRate, errorSampleRate }: SampleRates return null; } - const sessionObj = JSON.parse(sessionStringFromStorage) as Partial; - const sampled = sampleSession(sessionObj.sampled, sessionSampleRate, errorSampleRate); + const sessionObj = JSON.parse(sessionStringFromStorage) as Session; - return makeSession({ ...sessionObj, sampled }); + return makeSession(sessionObj); } catch { return null; } diff --git a/packages/replay/src/session/getSession.ts b/packages/replay/src/session/getSession.ts index 18689e599317..103008061ad0 100644 --- a/packages/replay/src/session/getSession.ts +++ b/packages/replay/src/session/getSession.ts @@ -29,7 +29,7 @@ export function getSession({ errorSampleRate, }: GetSessionParams): { type: 'new' | 'saved'; session: Session } { // If session exists and is passed, use it instead of always hitting session storage - const session = currentSession || (stickySession && fetchSession({ sessionSampleRate, errorSampleRate })); + const session = currentSession || (stickySession && fetchSession()); if (session) { // If there is a session, check if it is valid (e.g. "last activity" time diff --git a/packages/replay/test/unit/session/fetchSession.test.ts b/packages/replay/test/unit/session/fetchSession.test.ts index f8df61086f42..9f7e7694a5e0 100644 --- a/packages/replay/test/unit/session/fetchSession.test.ts +++ b/packages/replay/test/unit/session/fetchSession.test.ts @@ -15,18 +15,13 @@ afterEach(() => { WINDOW.sessionStorage.clear(); }); -const SAMPLE_RATES = { - sessionSampleRate: 1.0, - errorSampleRate: 0.0, -}; - it('fetches a valid and sampled session', function () { WINDOW.sessionStorage.setItem( REPLAY_SESSION_KEY, '{"id":"fd09adfc4117477abc8de643e5a5798a","sampled": "session","started":1648827162630,"lastActivity":1648827162658}', ); - expect(fetchSession(SAMPLE_RATES)).toEqual({ + expect(fetchSession()).toEqual({ id: 'fd09adfc4117477abc8de643e5a5798a', lastActivity: 1648827162658, segmentId: 0, @@ -41,7 +36,7 @@ it('fetches an unsampled session', function () { '{"id":"fd09adfc4117477abc8de643e5a5798a","sampled": false,"started":1648827162630,"lastActivity":1648827162658}', ); - expect(fetchSession(SAMPLE_RATES)).toEqual({ + expect(fetchSession()).toEqual({ id: 'fd09adfc4117477abc8de643e5a5798a', lastActivity: 1648827162658, segmentId: 0, @@ -50,29 +45,14 @@ it('fetches an unsampled session', function () { }); }); -it('auto-fixes a session without sampled', function () { - WINDOW.sessionStorage.setItem( - REPLAY_SESSION_KEY, - '{"id":"fd09adfc4117477abc8de643e5a5798a","started":1648827162630,"lastActivity":1648827162658}', - ); - - expect(fetchSession(SAMPLE_RATES)).toEqual({ - id: 'fd09adfc4117477abc8de643e5a5798a', - lastActivity: 1648827162658, - segmentId: 0, - sampled: 'session', - started: 1648827162630, - }); -}); - it('fetches a session that does not exist', function () { - expect(fetchSession(SAMPLE_RATES)).toBe(null); + expect(fetchSession()).toBe(null); }); it('fetches an invalid session', function () { WINDOW.sessionStorage.setItem(REPLAY_SESSION_KEY, '{"id":"fd09adfc4117477abc8de643e5a5798a",'); - expect(fetchSession(SAMPLE_RATES)).toBe(null); + expect(fetchSession()).toBe(null); }); it('safely attempts to fetch session when Session Storage is disabled', function () { @@ -85,5 +65,5 @@ it('safely attempts to fetch session when Session Storage is disabled', function }, }); - expect(fetchSession(SAMPLE_RATES)).toEqual(null); + expect(fetchSession()).toEqual(null); }); diff --git a/packages/replay/test/unit/session/getSession.test.ts b/packages/replay/test/unit/session/getSession.test.ts index 022521c83abe..fd1606820c99 100644 --- a/packages/replay/test/unit/session/getSession.test.ts +++ b/packages/replay/test/unit/session/getSession.test.ts @@ -58,7 +58,7 @@ it('creates a non-sticky session when one does not exist', function () { }); // Should not have anything in storage - expect(FetchSession.fetchSession(SAMPLE_RATES)).toBe(null); + expect(FetchSession.fetchSession()).toBe(null); }); it('creates a non-sticky session, regardless of session existing in sessionStorage', function () { @@ -98,7 +98,7 @@ it('creates a non-sticky session, when one is expired', function () { }); it('creates a sticky session when one does not exist', function () { - expect(FetchSession.fetchSession(SAMPLE_RATES)).toBe(null); + expect(FetchSession.fetchSession()).toBe(null); const { session } = getSession({ expiry: 900000, @@ -119,7 +119,7 @@ it('creates a sticky session when one does not exist', function () { }); // Should not have anything in storage - expect(FetchSession.fetchSession(SAMPLE_RATES)).toEqual({ + expect(FetchSession.fetchSession()).toEqual({ id: 'test_session_id', segmentId: 0, lastActivity: expect.any(Number), From 9f1a8344bbf5ccb555fe9ff63fd2970839c8cde7 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Tue, 6 Dec 2022 12:44:26 +0100 Subject: [PATCH 7/7] ref(replay): Apply PR feedback --- packages/replay/MIGRATION.md | 4 ++-- packages/replay/src/session/Session.ts | 10 +--------- packages/replay/src/session/createSession.ts | 4 ++-- packages/replay/test/unit/session/Session.test.ts | 8 ++++---- 4 files changed, 9 insertions(+), 17 deletions(-) diff --git a/packages/replay/MIGRATION.md b/packages/replay/MIGRATION.md index 0d300e462c43..8bceac5d669e 100644 --- a/packages/replay/MIGRATION.md +++ b/packages/replay/MIGRATION.md @@ -58,7 +58,7 @@ If you only imported from `@sentry/replay`, this will not affect you. It is highly unlikely to affect anybody, but the type `IEventBuffer` was renamed to `EventBuffer` for consistency. Unless you manually imported this and used it somewhere in your codebase, this will not affect you. -## Session object is now a POJO (https://github.com/getsentry/sentry-javascript/pull/6417) +## Session object is now a plain object (https://github.com/getsentry/sentry-javascript/pull/6417) -The `Session` object exported from Replay is now a POJO, instead of a class. +The `Session` object exported from Replay is now a plain object, instead of a class. This should not affect you unless you specifically accessed this class & did custom things with it. diff --git a/packages/replay/src/session/Session.ts b/packages/replay/src/session/Session.ts index ec83b4b73821..2936a8c19dda 100644 --- a/packages/replay/src/session/Session.ts +++ b/packages/replay/src/session/Session.ts @@ -58,14 +58,6 @@ export function makeSession(session: Partial & { sampled: Sampled }): S /** * Get the sampled status for a session based on sample rates & current sampled status. */ -export function sampleSession( - sampled: Sampled | undefined, - sessionSampleRate: number, - errorSampleRate: number, -): Sampled { - if (typeof sampled !== 'undefined') { - return sampled; - } - +export function getSessionSampleType(sessionSampleRate: number, errorSampleRate: number): Sampled { return isSampled(sessionSampleRate) ? 'session' : isSampled(errorSampleRate) ? 'error' : false; } diff --git a/packages/replay/src/session/createSession.ts b/packages/replay/src/session/createSession.ts index 3cb38b64a423..5dcb015fe615 100644 --- a/packages/replay/src/session/createSession.ts +++ b/packages/replay/src/session/createSession.ts @@ -2,7 +2,7 @@ import { logger } from '@sentry/utils'; import { SessionOptions } from '../types'; import { saveSession } from './saveSession'; -import { makeSession, sampleSession, Session } from './Session'; +import { getSessionSampleType, makeSession, Session } from './Session'; /** * Create a new session, which in its current implementation is a Sentry event @@ -10,7 +10,7 @@ import { makeSession, sampleSession, Session } from './Session'; * one of these Sentry events per "replay session". */ export function createSession({ sessionSampleRate, errorSampleRate, stickySession = false }: SessionOptions): Session { - const sampled = sampleSession(undefined, sessionSampleRate, errorSampleRate); + const sampled = getSessionSampleType(sessionSampleRate, errorSampleRate); const session = makeSession({ sampled, }); diff --git a/packages/replay/test/unit/session/Session.test.ts b/packages/replay/test/unit/session/Session.test.ts index 7ab0083a3ab8..ca34e72b4172 100644 --- a/packages/replay/test/unit/session/Session.test.ts +++ b/packages/replay/test/unit/session/Session.test.ts @@ -26,7 +26,7 @@ jest.mock('@sentry/utils', () => { import * as Sentry from '@sentry/browser'; import { WINDOW } from '../../../src/constants'; -import { makeSession, sampleSession } from '../../../src/session/Session'; +import { getSessionSampleType, makeSession } from '../../../src/session/Session'; type CaptureEventMockType = jest.MockedFunction; @@ -40,7 +40,7 @@ afterEach(() => { it('does not sample', function () { const newSession = makeSession({ - sampled: sampleSession(undefined, 0, 0), + sampled: getSessionSampleType(0, 0), }); expect(newSession.sampled).toBe(false); @@ -48,7 +48,7 @@ it('does not sample', function () { it('samples using `sessionSampleRate`', function () { const newSession = makeSession({ - sampled: sampleSession(undefined, 1.0, 0), + sampled: getSessionSampleType(1.0, 0), }); expect(newSession.sampled).toBe('session'); @@ -56,7 +56,7 @@ it('samples using `sessionSampleRate`', function () { it('samples using `errorSampleRate`', function () { const newSession = makeSession({ - sampled: sampleSession(undefined, 0, 1), + sampled: getSessionSampleType(0, 1), }); expect(newSession.sampled).toBe('error');