From 84bb410db94c66a2f8e4b18f04b7fac9310eace6 Mon Sep 17 00:00:00 2001 From: Billy Date: Wed, 7 Feb 2024 14:59:12 -0800 Subject: [PATCH 1/3] chore: consolidate sampling logic in SessionManager --- src/event-cache/EventCache.ts | 17 +-- src/event-cache/__tests__/EventCache.test.ts | 129 +++--------------- src/sessions/SessionManager.ts | 24 ++-- src/sessions/__tests__/SessionManager.test.ts | 49 ++++++- 4 files changed, 83 insertions(+), 136 deletions(-) diff --git a/src/event-cache/EventCache.ts b/src/event-cache/EventCache.ts index 6228bce9..b222e25d 100644 --- a/src/event-cache/EventCache.ts +++ b/src/event-cache/EventCache.ts @@ -99,12 +99,10 @@ export class EventCache { if (!this.enabled) { return; } - + this.sessionManager.getSession(); // refresh if (this.isCurrentUrlAllowed()) { - const session: Session = this.sessionManager.getSession(); this.sessionManager.incrementSessionEventCount(); - - if (this.canRecord(session)) { + if (this.sessionManager.shouldSample()) { this.addRecordToCache(type, eventData); } } @@ -118,7 +116,6 @@ export class EventCache { if (this.isCurrentUrlAllowed()) { return this.sessionManager.getSession(); } - return undefined; }; /** @@ -192,19 +189,11 @@ export class EventCache { this.sessionManager.incrementSessionEventCount(); - if (this.canRecord(session)) { + if (this.sessionManager.shouldSample()) { this.addRecordToCache(type, eventData); } }; - private canRecord = (session: Session): boolean => { - return ( - session.record && - (session.eventCount <= this.config.sessionEventLimit || - this.config.sessionEventLimit <= 0) - ); - }; - /** * Add an event to the cache. * diff --git a/src/event-cache/__tests__/EventCache.test.ts b/src/event-cache/__tests__/EventCache.test.ts index bc80b845..754e1384 100644 --- a/src/event-cache/__tests__/EventCache.test.ts +++ b/src/event-cache/__tests__/EventCache.test.ts @@ -1,7 +1,6 @@ import { EventCache } from '../EventCache'; import { advanceTo } from 'jest-date-mock'; import * as Utils from '../../test-utils/test-utils'; -import { SessionManager } from '../../sessions/SessionManager'; import { RumEvent } from '../../dispatch/dataplane'; import { DEFAULT_CONFIG, mockFetch } from '../../test-utils/test-utils'; import { INSTALL_MODULE, INSTALL_SCRIPT } from '../../utils/constants'; @@ -19,6 +18,7 @@ const getAttributes = jest.fn(); const incrementSessionEventCount = jest.fn(); const addSessionAttributes = jest.fn(); let samplingDecision = true; +let shouldSample = true; const isSampled = jest.fn().mockImplementation(() => samplingDecision); jest.mock('../../sessions/SessionManager', () => ({ SessionManager: jest.fn().mockImplementation(() => ({ @@ -27,7 +27,8 @@ jest.mock('../../sessions/SessionManager', () => ({ getAttributes, incrementSessionEventCount, addSessionAttributes, - isSampled + isSampled, + shouldSample: jest.fn().mockImplementation(() => shouldSample) })) })); @@ -446,92 +447,6 @@ describe('EventCache tests', () => { expect(eventCache.isSessionSampled()).toBeTruthy(); }); - test('when session.record is false then event is not recorded', async () => { - // Init - const getSession = jest.fn(() => ({ sessionId: 'a', record: true })); - const getUserId = jest.fn(() => 'b'); - const incrementSessionEventCount = jest.fn(); - (SessionManager as any).mockImplementation(() => ({ - getSession, - getUserId, - incrementSessionEventCount - })); - - const EVENT1_SCHEMA = 'com.amazon.rum.event1'; - const eventCache: EventCache = Utils.createDefaultEventCache(); - - // Run - eventCache.getEventBatch(); - eventCache.recordEvent(EVENT1_SCHEMA, {}); - - // Assert - expect(eventCache.hasEvents()).toBeFalsy(); - }); - - test('when session.record is true then event is recorded', async () => { - // Init - const getSession = jest.fn(() => ({ - sessionId: 'a', - record: true, - eventCount: 1 - })); - const getUserId = jest.fn(() => 'b'); - const incrementSessionEventCount = jest.fn(); - (SessionManager as any).mockImplementation(() => ({ - getSession, - getUserId, - getAttributes, - incrementSessionEventCount - })); - - const EVENT1_SCHEMA = 'com.amazon.rum.event1'; - const eventCache: EventCache = Utils.createDefaultEventCache(); - - // Run - eventCache.getEventBatch(); - eventCache.recordEvent(EVENT1_SCHEMA, {}); - - // Assert - expect(eventCache.hasEvents()).toBeTruthy(); - }); - - test('when event limit is reached then recordEvent does not record events', async () => { - // Init - let eventCount = 0; - const getSession = jest.fn().mockImplementation(() => { - eventCount++; - return { - sessionId: 'a', - record: true, - eventCount - }; - }); - const getUserId = jest.fn(() => 'b'); - const incrementSessionEventCount = jest.fn(); - (SessionManager as any).mockImplementation(() => ({ - getSession, - getUserId, - getAttributes, - incrementSessionEventCount - })); - const EVENT1_SCHEMA = 'com.amazon.rum.event1'; - const config = { - ...DEFAULT_CONFIG, - ...{ - sessionEventLimit: 1 - } - }; - const eventCache: EventCache = Utils.createEventCache(config); - - // Run - eventCache.recordEvent(EVENT1_SCHEMA, {}); - eventCache.recordEvent(EVENT1_SCHEMA, {}); - eventCache.recordEvent(EVENT1_SCHEMA, {}); - - // Assert - expect(eventCache.getEventBatch().length).toEqual(1); - }); - test('when event is recorded then events subscribers are notified with parsed rum event', async () => { // Init const EVENT1_SCHEMA = 'com.amazon.rum.event1'; @@ -586,31 +501,29 @@ describe('EventCache tests', () => { expect(bus.dispatch).not.toHaveBeenCalled(); // eslint-disable-line }); - test('when event limit is zero then recordEvent records all events', async () => { + test('when session should not sample, then no events are recorded', async () => { // Init - const eventCount = 0; - const getSession = jest.fn(() => ({ sessionId: 'a', record: true })); - const getUserId = jest.fn(() => 'b'); - const incrementSessionEventCount = jest.fn(); - (SessionManager as any).mockImplementation(() => ({ - getSession, - getUserId, - getAttributes, - incrementSessionEventCount - })); + shouldSample = false; const EVENT1_SCHEMA = 'com.amazon.rum.event1'; - const config = { - ...DEFAULT_CONFIG, - ...{ - sessionEventLimit: 0 - } - }; - const eventCache: EventCache = Utils.createEventCache(config); + const eventCache: EventCache = Utils.createEventCache({ + ...DEFAULT_CONFIG + }); // Run eventCache.recordEvent(EVENT1_SCHEMA, {}); + expect(eventCache.getEventBatch()).toEqual([]); - // Assert - expect(eventCache.getEventBatch().length).toEqual(1); + shouldSample = true; + eventCache.recordEvent(EVENT1_SCHEMA, {}); + eventCache.recordEvent(EVENT1_SCHEMA, {}); + expect(eventCache.getEventBatch()).toHaveLength(2); + + shouldSample = false; + eventCache.recordEvent(EVENT1_SCHEMA, {}); + eventCache.recordEvent(EVENT1_SCHEMA, {}); + expect(eventCache.getEventBatch()).toEqual([]); + + // Restore + shouldSample = true; }); }); diff --git a/src/sessions/SessionManager.ts b/src/sessions/SessionManager.ts index ac33d712..e0011ded 100644 --- a/src/sessions/SessionManager.ts +++ b/src/sessions/SessionManager.ts @@ -112,17 +112,14 @@ export class SessionManager { * Returns the session ID. If no session ID exists, one will be created. */ public getSession(): Session { - if (this.session.sessionId === NIL_UUID) { - // The session does not exist. Create a new one. - // If it is created before the page view is recorded, the session start event metadata will - // not have page attributes as the page does not exist yet. - this.createSession(); - } else if ( - this.session.sessionId !== NIL_UUID && + if ( + this.session.sessionId === NIL_UUID || new Date() > this.sessionExpiry ) { - // The session has expired. Create a new one. this.createSession(); + // The session does not exist or has expired.. Create a new one. + // If it is created before the page view is recorded, the session start event metadata will + // not have page attributes as the page does not exist yet. } return this.session; } @@ -154,6 +151,17 @@ export class SessionManager { this.renewSession(); } + public shouldSample(): boolean { + if (!this.isSampled()) { + return false; + } + + return ( + this.session.eventCount <= this.config.sessionEventLimit || + this.config.sessionEventLimit <= 0 + ); + } + private initializeUser() { let userId = ''; this.userExpiry = new Date(); diff --git a/src/sessions/__tests__/SessionManager.test.ts b/src/sessions/__tests__/SessionManager.test.ts index d502816f..238b87d7 100644 --- a/src/sessions/__tests__/SessionManager.test.ts +++ b/src/sessions/__tests__/SessionManager.test.ts @@ -22,7 +22,6 @@ import { DEFAULT_CONFIG, mockFetch } from '../../test-utils/test-utils'; -import { advanceTo } from 'jest-date-mock'; global.fetch = mockFetch; const NAVIGATION = 'navigation'; @@ -379,7 +378,7 @@ describe('SessionManager tests', () => { expect(userIdFromTracker).toEqual(NIL_UUID); }); - test('when the sessionId cookie expires then a new sessionId is created', async () => { + test('when the sessionId cookie expires then a new session is created', async () => { // Init const sessionManager = defaultSessionManager({ ...DEFAULT_CONFIG, @@ -387,11 +386,16 @@ describe('SessionManager tests', () => { }); const sessionOne = sessionManager.getSession(); + sessionManager.incrementSessionEventCount(); + sessionManager.incrementSessionEventCount(); + sessionManager.incrementSessionEventCount(); + await new Promise((resolve) => setTimeout(resolve, 10)); const sessionTwo = sessionManager.getSession(); // Assert expect(sessionOne.sessionId).not.toEqual(sessionTwo.sessionId); + expect(sessionTwo.eventCount).toEqual(0); }); test('When the sessionId cookie does not expire, sessionId remains the same', async () => { @@ -459,7 +463,7 @@ describe('SessionManager tests', () => { expect(mockRecord).toHaveBeenCalledTimes(0); }); - test('when sessionSampleRate is one then session.record is true', async () => { + test('when sessionSampleRate is one then should sample', async () => { // Init const sessionManager = defaultSessionManager({ ...DEFAULT_CONFIG, @@ -470,10 +474,10 @@ describe('SessionManager tests', () => { // Assert expect(session.record).toBeTruthy(); - expect(sessionManager.isSampled()).toBeTruthy(); + expect(sessionManager.isSampled()).toBe(true); }); - test('when sessionSampleRate is zero then session.record is false', async () => { + test('when sessionSampleRate is zero then should not sample', async () => { // Init const sessionManager = defaultSessionManager({ ...DEFAULT_CONFIG, @@ -484,7 +488,40 @@ describe('SessionManager tests', () => { // Assert expect(session.record).toBeFalsy(); - expect(sessionManager.isSampled()).toBeFalsy(); + expect(sessionManager.isSampled()).toBe(false); + expect(sessionManager.shouldSample()).toBe(false); + }); + + test('when sessionEventLimit is reached then should not sample ', async () => { + const sessionManager = defaultSessionManager({ + ...DEFAULT_CONFIG, + sessionSampleRate: 1, + allowCookies: true, + sessionEventLimit: 2 + }); + expect(sessionManager.shouldSample()).toBe(true); + sessionManager.incrementSessionEventCount(); + expect(sessionManager.shouldSample()).toBe(true); + sessionManager.incrementSessionEventCount(); + expect(sessionManager.shouldSample()).toBe(true); + sessionManager.incrementSessionEventCount(); + expect(sessionManager.shouldSample()).toBe(false); + }); + + test('when sessionEventLimit is zero then always should sample ', async () => { + const sessionManager = defaultSessionManager({ + ...DEFAULT_CONFIG, + sessionSampleRate: 1, + allowCookies: true, + sessionEventLimit: 0 + }); + expect(sessionManager.shouldSample()).toBe(true); + sessionManager.incrementSessionEventCount(); + expect(sessionManager.shouldSample()).toBe(true); + sessionManager.incrementSessionEventCount(); + expect(sessionManager.shouldSample()).toBe(true); + sessionManager.incrementSessionEventCount(); + expect(sessionManager.shouldSample()).toBe(true); }); test('when sessionId is nil then create new session with same sampling decision', async () => { From 9567a932cee4389c37e344a0e5c6aae561336774 Mon Sep 17 00:00:00 2001 From: Billy Date: Wed, 7 Feb 2024 17:20:44 -0800 Subject: [PATCH 2/3] rename verbose method --- src/event-cache/EventCache.ts | 4 +-- src/event-cache/__tests__/EventCache.test.ts | 6 ++-- src/sessions/SessionManager.ts | 2 +- src/sessions/__tests__/SessionManager.test.ts | 28 +++++++++---------- 4 files changed, 20 insertions(+), 20 deletions(-) diff --git a/src/event-cache/EventCache.ts b/src/event-cache/EventCache.ts index b222e25d..03c892fb 100644 --- a/src/event-cache/EventCache.ts +++ b/src/event-cache/EventCache.ts @@ -101,7 +101,7 @@ export class EventCache { } this.sessionManager.getSession(); // refresh if (this.isCurrentUrlAllowed()) { - this.sessionManager.incrementSessionEventCount(); + this.sessionManager.countEvent(); if (this.sessionManager.shouldSample()) { this.addRecordToCache(type, eventData); } @@ -187,7 +187,7 @@ export class EventCache { return; } - this.sessionManager.incrementSessionEventCount(); + this.sessionManager.countEvent(); if (this.sessionManager.shouldSample()) { this.addRecordToCache(type, eventData); diff --git a/src/event-cache/__tests__/EventCache.test.ts b/src/event-cache/__tests__/EventCache.test.ts index 754e1384..75fdad03 100644 --- a/src/event-cache/__tests__/EventCache.test.ts +++ b/src/event-cache/__tests__/EventCache.test.ts @@ -15,7 +15,7 @@ const getSession = jest.fn(() => ({ })); const getUserId = jest.fn(() => 'b'); const getAttributes = jest.fn(); -const incrementSessionEventCount = jest.fn(); +const countEvent = jest.fn(); const addSessionAttributes = jest.fn(); let samplingDecision = true; let shouldSample = true; @@ -25,7 +25,7 @@ jest.mock('../../sessions/SessionManager', () => ({ getSession, getUserId, getAttributes, - incrementSessionEventCount, + countEvent, addSessionAttributes, isSampled, shouldSample: jest.fn().mockImplementation(() => shouldSample) @@ -42,7 +42,7 @@ describe('EventCache tests', () => { beforeEach(() => { getSession.mockClear(); getUserId.mockClear(); - incrementSessionEventCount.mockClear(); + countEvent.mockClear(); }); test('record does nothing when cache is disabled', async () => { diff --git a/src/sessions/SessionManager.ts b/src/sessions/SessionManager.ts index e0011ded..06ea0c70 100644 --- a/src/sessions/SessionManager.ts +++ b/src/sessions/SessionManager.ts @@ -146,7 +146,7 @@ export class SessionManager { return NIL_UUID; } - public incrementSessionEventCount() { + public countEvent() { this.session.eventCount++; this.renewSession(); } diff --git a/src/sessions/__tests__/SessionManager.test.ts b/src/sessions/__tests__/SessionManager.test.ts index 238b87d7..77017de6 100644 --- a/src/sessions/__tests__/SessionManager.test.ts +++ b/src/sessions/__tests__/SessionManager.test.ts @@ -386,9 +386,9 @@ describe('SessionManager tests', () => { }); const sessionOne = sessionManager.getSession(); - sessionManager.incrementSessionEventCount(); - sessionManager.incrementSessionEventCount(); - sessionManager.incrementSessionEventCount(); + sessionManager.countEvent(); + sessionManager.countEvent(); + sessionManager.countEvent(); await new Promise((resolve) => setTimeout(resolve, 10)); const sessionTwo = sessionManager.getSession(); @@ -500,11 +500,11 @@ describe('SessionManager tests', () => { sessionEventLimit: 2 }); expect(sessionManager.shouldSample()).toBe(true); - sessionManager.incrementSessionEventCount(); + sessionManager.countEvent(); expect(sessionManager.shouldSample()).toBe(true); - sessionManager.incrementSessionEventCount(); + sessionManager.countEvent(); expect(sessionManager.shouldSample()).toBe(true); - sessionManager.incrementSessionEventCount(); + sessionManager.countEvent(); expect(sessionManager.shouldSample()).toBe(false); }); @@ -516,11 +516,11 @@ describe('SessionManager tests', () => { sessionEventLimit: 0 }); expect(sessionManager.shouldSample()).toBe(true); - sessionManager.incrementSessionEventCount(); + sessionManager.countEvent(); expect(sessionManager.shouldSample()).toBe(true); - sessionManager.incrementSessionEventCount(); + sessionManager.countEvent(); expect(sessionManager.shouldSample()).toBe(true); - sessionManager.incrementSessionEventCount(); + sessionManager.countEvent(); expect(sessionManager.shouldSample()).toBe(true); }); @@ -625,7 +625,7 @@ describe('SessionManager tests', () => { expect(session.eventCount).toEqual(0); }); - test('when cookies are allowed then incrementSessionEventCount increments session.eventCount in cookie', async () => { + test('when cookies are allowed then countEvent increments session.eventCount in cookie', async () => { // Init const config = { ...DEFAULT_CONFIG, @@ -641,14 +641,14 @@ describe('SessionManager tests', () => { const sessionManager = defaultSessionManager(config); sessionManager.getSession(); - sessionManager.incrementSessionEventCount(); + sessionManager.countEvent(); const session = JSON.parse(atob(getCookie(SESSION_COOKIE_NAME))); // Assert expect(session.eventCount).toEqual(2); }); - test('when cookies are not allowed then incrementSessionEventCount increments session.eventCount in member', async () => { + test('when cookies are not allowed then countEvent increments session.eventCount in member', async () => { // Init const sessionManager = defaultSessionManager({ ...DEFAULT_CONFIG, @@ -656,7 +656,7 @@ describe('SessionManager tests', () => { }); sessionManager.getSession(); - sessionManager.incrementSessionEventCount(); + sessionManager.countEvent(); const session = sessionManager.getSession(); // Assert @@ -764,7 +764,7 @@ describe('SessionManager tests', () => { sessionManager.getSession(); const userIdFromCookie1 = getCookie(USER_COOKIE_NAME); config.allowCookies = true; - sessionManager.incrementSessionEventCount(); + sessionManager.countEvent(); const userIdFromCookie2 = getCookie(USER_COOKIE_NAME); // Assert From f54ee2e598f394340a6f685d90f08517fc0b4525 Mon Sep 17 00:00:00 2001 From: Billy Date: Wed, 7 Feb 2024 17:50:53 -0800 Subject: [PATCH 3/3] fix off by one error --- src/event-cache/EventCache.ts | 4 ++-- src/sessions/SessionManager.ts | 2 +- src/sessions/__tests__/SessionManager.test.ts | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/event-cache/EventCache.ts b/src/event-cache/EventCache.ts index 03c892fb..fef776e9 100644 --- a/src/event-cache/EventCache.ts +++ b/src/event-cache/EventCache.ts @@ -99,11 +99,11 @@ export class EventCache { if (!this.enabled) { return; } - this.sessionManager.getSession(); // refresh + this.sessionManager.getSession(); // refresh session if needed if (this.isCurrentUrlAllowed()) { - this.sessionManager.countEvent(); if (this.sessionManager.shouldSample()) { this.addRecordToCache(type, eventData); + this.sessionManager.countEvent(); } } }; diff --git a/src/sessions/SessionManager.ts b/src/sessions/SessionManager.ts index 06ea0c70..82041d51 100644 --- a/src/sessions/SessionManager.ts +++ b/src/sessions/SessionManager.ts @@ -157,7 +157,7 @@ export class SessionManager { } return ( - this.session.eventCount <= this.config.sessionEventLimit || + this.session.eventCount < this.config.sessionEventLimit || this.config.sessionEventLimit <= 0 ); } diff --git a/src/sessions/__tests__/SessionManager.test.ts b/src/sessions/__tests__/SessionManager.test.ts index 77017de6..c78b2a22 100644 --- a/src/sessions/__tests__/SessionManager.test.ts +++ b/src/sessions/__tests__/SessionManager.test.ts @@ -503,7 +503,7 @@ describe('SessionManager tests', () => { sessionManager.countEvent(); expect(sessionManager.shouldSample()).toBe(true); sessionManager.countEvent(); - expect(sessionManager.shouldSample()).toBe(true); + expect(sessionManager.shouldSample()).toBe(false); sessionManager.countEvent(); expect(sessionManager.shouldSample()).toBe(false); });