Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

👷‍♀️ [RUM-8295] Change anonymous id format to uuid #3306

Merged
merged 3 commits into from
Feb 6, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions packages/core/src/domain/session/sessionState.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import { isEmptyObject } from '../../tools/utils/objectUtils'
import { objectEntries } from '../../tools/utils/polyfills'
import { dateNow } from '../../tools/utils/timeUtils'
import { generateAnonymousId } from '../user'
import { generateUUID } from '../../tools/utils/stringUtils'
import type { Configuration } from '../configuration'
import { SESSION_EXPIRATION_DELAY, SESSION_TIME_OUT_DELAY } from './sessionConstants'
import { isValidSessionString, SESSION_ENTRY_REGEXP, SESSION_ENTRY_SEPARATOR } from './sessionStateValidation'
Expand All @@ -27,7 +27,7 @@ export function getExpiredSessionState(
if (previousSessionState?.anonymousId) {
expiredSessionState.anonymousId = previousSessionState?.anonymousId
} else {
expiredSessionState.anonymousId = generateAnonymousId()
expiredSessionState.anonymousId = generateUUID()
}
}
return expiredSessionState
Expand Down
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import { resetExperimentalFeatures } from '../../../tools/experimentalFeatures'
import { mockClock } from '../../../../test'
import { mockClock, getSessionState } from '../../../../test'
import { setCookie, deleteCookie, getCookie, getCurrentSite } from '../../../browser/cookie'
import { type SessionState } from '../sessionState'
import type { SessionState } from '../sessionState'
import type { Configuration } from '../../configuration'
import { SESSION_TIME_OUT_DELAY } from '../sessionConstants'
import { buildCookieOptions, selectCookieStrategy, initCookieStrategy } from './sessionInCookie'
Expand Down Expand Up @@ -33,8 +33,8 @@ describe('session in cookie strategy', () => {
cookieStorageStrategy.persistSession(sessionState)
cookieStorageStrategy.expireSession(sessionState)
const session = cookieStorageStrategy.retrieveSession()
expect(session).toEqual({ isExpired: '1', anonymousId: '0000000000' })
expect(getCookie(SESSION_STORE_KEY)).toBe('isExpired=1&aid=0000000000')
expect(session).toEqual({ isExpired: '1', anonymousId: jasmine.any(String) })
expect(getSessionState(SESSION_STORE_KEY)).toEqual({ isExpired: '1', anonymousId: jasmine.any(String) })
})

it('should return an empty object if session string is invalid', () => {
Expand Down
Original file line number Diff line number Diff line change
@@ -1,10 +1,14 @@
import type { Configuration } from '../../configuration'
import { SessionPersistence } from '../sessionConstants'
import { type SessionState } from '../sessionState'
import { toSessionState } from '../sessionState'
import type { SessionState } from '../sessionState'
import { selectLocalStorageStrategy, initLocalStorageStrategy } from './sessionInLocalStorage'
import { SESSION_STORE_KEY } from './sessionStoreStrategy'
const DEFAULT_INIT_CONFIGURATION = { trackAnonymousUser: true } as Configuration

function getSessionStateFromLocalStorage(SESSION_STORE_KEY: string): SessionState {
return toSessionState(window.localStorage.getItem(SESSION_STORE_KEY))
}
describe('session in local storage strategy', () => {
const sessionState: SessionState = { id: '123', created: '0' }
beforeEach(() => {
Expand All @@ -31,16 +35,26 @@ describe('session in local storage strategy', () => {
localStorageStrategy.persistSession(sessionState)
const session = localStorageStrategy.retrieveSession()
expect(session).toEqual({ ...sessionState })
expect(window.localStorage.getItem(SESSION_STORE_KEY)).toMatch(/.*id=.*created/)
expect(getSessionStateFromLocalStorage(SESSION_STORE_KEY)).toEqual(sessionState)
})

it('should set `isExpired=1` to the local storage item holding the session', () => {
const localStorageStrategy = initLocalStorageStrategy(DEFAULT_INIT_CONFIGURATION)
localStorageStrategy.persistSession(sessionState)
localStorageStrategy.expireSession(sessionState)
const session = localStorageStrategy?.retrieveSession()
expect(session).toEqual({ isExpired: '1', anonymousId: '0000000000' })
expect(window.localStorage.getItem(SESSION_STORE_KEY)).toBe('isExpired=1&aid=0000000000')
expect(session).toEqual({ isExpired: '1', anonymousId: jasmine.any(String) })
expect(getSessionStateFromLocalStorage(SESSION_STORE_KEY)).toEqual({
isExpired: '1',
anonymousId: jasmine.any(String),
})
})

it('should return an empty object if session string is invalid', () => {
const localStorageStrategy = initLocalStorageStrategy(DEFAULT_INIT_CONFIGURATION)
window.localStorage.setItem(SESSION_STORE_KEY, '{test:42}')
const session = localStorageStrategy.retrieveSession()
expect(session).toEqual({})
})

it('should not interfere with other keys present in local storage', () => {
Expand Down
8 changes: 1 addition & 7 deletions packages/core/src/domain/user/user.spec.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { display } from '../../tools/display'
import { checkUser, generateAnonymousId } from './user'
import { checkUser } from './user'
import type { User } from './user.types'

describe('check user function', () => {
Expand All @@ -20,9 +20,3 @@ describe('check user function', () => {
expect(display.error).toHaveBeenCalledTimes(3)
})
})

describe('check anonymous id storage functions', () => {
it('should generate a random anonymous id', () => {
expect(generateAnonymousId()).toMatch(/^[a-z0-9]+$/)
})
})
6 changes: 0 additions & 6 deletions packages/core/src/domain/user/user.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,9 +12,3 @@ export function checkUser(newUser: User): boolean {
}
return isValid
}

export function generateAnonymousId() {
return Math.floor(Math.random() * Math.pow(36, 10))
.toString(36)
.padStart(10, '0')
}
1 change: 0 additions & 1 deletion packages/core/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,6 @@ export {
deleteCookie,
resetInitCookies,
} from './browser/cookie'
export { generateAnonymousId } from './domain/user'
export type { CookieStore, WeakRef, WeakRefConstructor } from './browser/browser.types'
export type { XhrCompleteContext, XhrStartContext } from './browser/xhrObservable'
export { initXhrObservable } from './browser/xhrObservable'
Expand Down
9 changes: 8 additions & 1 deletion test/e2e/lib/helpers/session.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,10 +10,17 @@ export async function renewSession() {
expect((await findSessionCookie())?.isExpired).not.toEqual('1')
}

/**
* TODO:
* After Playwright migration, we should replace this with
* `await page.clock.runFor(SESSION_TIME_OUT_DELAY)`, so we can test
* the expiration logic fully and avoid manually setting the cookie
* to expire status.
*/
export async function expireSession() {
// mock expire session with anonymous id
const cookies = await browser.getCookies(SESSION_STORE_KEY)
const anonymousId = cookies[0]?.value.match(/aid=[a-z0-9]+/)
const anonymousId = cookies[0]?.value.match(/aid=[a-z0-9-]+/)
const expireCookie = `isExpired=1&${anonymousId && anonymousId[0]}`

await setCookie(SESSION_STORE_KEY, expireCookie, SESSION_TIME_OUT_DELAY)
Expand Down
17 changes: 6 additions & 11 deletions test/e2e/scenario/rum/sessions.scenario.ts
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ describe('rum sessions', () => {
// prevent recording start to generate late events
.withRum({ startSessionReplayRecordingManually: true })
.run(async ({ intakeRegistry }) => {
// TODO: use `page.clock.runFor` with Playwright
await expireSession()
intakeRegistry.empty()
await sendXhr('/ok')
Expand All @@ -56,7 +57,10 @@ describe('rum sessions', () => {
.run(async () => {
const anonymousId = (await findSessionCookie())?.aid

await expireSession()
await browser.execute(() => {
// TODO: use `page.clock.runFor` with Playwright
window.DD_RUM!.stopSession()
})
await flushEvents()

expect((await findSessionCookie())?.aid).toEqual(anonymousId)
Expand All @@ -69,6 +73,7 @@ describe('rum sessions', () => {
expect(anonymousId).not.toBeNull()

await browser.execute(() => {
// TODO: use `page.clock.runFor` with Playwright
window.DD_RUM!.stopSession()
})
await (await $('html')).click()
Expand All @@ -78,16 +83,6 @@ describe('rum sessions', () => {

expect((await findSessionCookie())?.aid).toEqual(anonymousId)
})

createTest('generated when cookie is cleared')
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is removed as renewSession is basically putting the id back manually. So the test is not much meaningful. Plus, we should not restart a session when the cookie cleared by third party.

.withRum()
.run(async () => {
await deleteAllCookies()
await renewSession()
await flushEvents()

expect((await findSessionCookie())?.aid).toBeDefined()
})
})

describe('manual session expiration', () => {
Expand Down