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.

Sign up for GitHub

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

Jump to bottom

✨[RUMF-430] new session strategy #343

Merged
merged 15 commits into from
Apr 8, 2020
Merged

✨[RUMF-430] new session strategy #343

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
6dbed1c
🐛 monitor activity listener
bcaudan Apr 7, 2020
b5db9cb
✨[RUMF-430] add session timeout
bcaudan Apr 7, 2020
34e43da
✨[RUMF-430] enforce session expiration
bcaudan Apr 7, 2020
742d842
✨[RUMF-430] expand session on visibility
bcaudan Apr 7, 2020
3ac7581
🚩 add 'new-session' flag
bcaudan Apr 7, 2020
c07170f
👌 extract DOM_EVENT enum
bcaudan Apr 7, 2020
d6546d7
Merge branch 'master' into bcaudan/new-session
bcaudan Apr 7, 2020
88260f9
👌 make clear session more explicit
bcaudan Apr 8, 2020
a0969e7
👌 merge remove listener and stop interval callbacks
bcaudan Apr 8, 2020
a1c550a
Merge branch 'master' into bcaudan/new-session
bcaudan Apr 8, 2020
f96e1cd
♻️ use DOM_EVENT for user action
bcaudan Apr 8, 2020
b6c783e
🏷 fix spec type
bcaudan Apr 8, 2020
3b732ab
👌 extract clearSession
bcaudan Apr 8, 2020
b3e79c8
👌 mock page visibility
bcaudan Apr 8, 2020
c5bb1e7
👌 use clear session
bcaudan Apr 8, 2020
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
95 changes: 77 additions & 18 deletions packages/core/src/sessionManagement.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,22 +1,24 @@
import { cacheCookieAccess, COOKIE_ACCESS_DELAY, CookieCache } from './cookie'
import { monitor } from './internalMonitoring'
import { Observable } from './observable'
import { tryOldCookiesMigration } from './oldCookiesMigration'
import * as utils from './utils'

export const SESSION_COOKIE_NAME = '_dd_s'
export const EXPIRATION_DELAY = 15 * utils.ONE_MINUTE
export const SESSION_EXPIRATION_DELAY = 15 * utils.ONE_MINUTE
glorieux marked this conversation as resolved.
Show resolved Hide resolved
export const SESSION_TIME_OUT_DELAY = 4 * utils.ONE_HOUR
export const VISIBILITY_CHECK_DELAY = utils.ONE_MINUTE

export interface Session<T> {
renewObservable: Observable<void>

getId(): string | undefined

getType(): T | undefined
}

export interface SessionState {
id?: string

created?: string
expire?: string
[key: string]: string | undefined
}

Expand All @@ -25,22 +27,27 @@ export interface SessionState {
*/
export function startSessionManagement<Type extends string>(
sessionTypeKey: string,
computeSessionState: (rawType?: string) => { type: Type; isTracked: boolean }
computeSessionState: (rawType?: string) => { type: Type; isTracked: boolean },
withNewSessionStrategy = false,
visibilityStateProvider = () => document.visibilityState
): Session<Type> {
const sessionCookie = cacheCookieAccess(SESSION_COOKIE_NAME)
tryOldCookiesMigration(sessionCookie)
const renewObservable = new Observable<void>()
let currentSessionId = retrieveSession(sessionCookie).id
let currentSessionId = retrieveActiveSession(sessionCookie, withNewSessionStrategy).id

const expandOrRenewSession = utils.throttle(() => {
const session = retrieveSession(sessionCookie)
const session = retrieveActiveSession(sessionCookie, withNewSessionStrategy)
const { type, isTracked } = computeSessionState(session[sessionTypeKey])
session[sessionTypeKey] = type
if (isTracked && !session.id) {
session.id = utils.generateUUID()
if (withNewSessionStrategy) {
session.created = String(Date.now())
}
}
// save changes and expand session duration
persistSession(session, sessionCookie)
persistSession(session, sessionCookie, withNewSessionStrategy)

// If the session id has changed, notify that the session has been renewed
if (isTracked && currentSessionId !== session.id) {
Expand All @@ -49,15 +56,23 @@ export function startSessionManagement<Type extends string>(
}
}, COOKIE_ACCESS_DELAY)

const expandSession = () => {
BenoitZugmeyer marked this conversation as resolved.
Show resolved Hide resolved
const session = retrieveActiveSession(sessionCookie, withNewSessionStrategy)
persistSession(session, sessionCookie, withNewSessionStrategy)
}

expandOrRenewSession()
trackActivity(expandOrRenewSession)
if (withNewSessionStrategy) {
trackVisibility(expandSession, visibilityStateProvider)
}

return {
getId() {
return retrieveSession(sessionCookie).id
return retrieveActiveSession(sessionCookie, withNewSessionStrategy).id
},
getType() {
return retrieveSession(sessionCookie)[sessionTypeKey] as Type | undefined
return retrieveActiveSession(sessionCookie, withNewSessionStrategy)[sessionTypeKey] as Type | undefined
},
renewObservable,
}
Expand All @@ -74,6 +89,25 @@ export function isValidSessionString(sessionString: string | undefined): session
)
}

function retrieveActiveSession(sessionCookie: CookieCache, withNewSessionStrategy: boolean): SessionState {
const session = retrieveSession(sessionCookie)
if (!withNewSessionStrategy || isActiveSession(session)) {
return session
}
const inactiveSession = {}
persistSession(inactiveSession, sessionCookie, withNewSessionStrategy)
bcaudan marked this conversation as resolved.
Show resolved Hide resolved
return inactiveSession
}

function isActiveSession(session: SessionState) {
// created and expire can be undefined for versions which was not storing them
// these checks could be removed when older versions will not be available/live anymore
return (
(session.created === undefined || Date.now() - Number(session.created) < SESSION_TIME_OUT_DELAY) &&
(session.expire === undefined || Date.now() < Number(session.expire))
)
}

function retrieveSession(sessionCookie: CookieCache): SessionState {
const sessionString = sessionCookie.get()
const session: SessionState = {}
Expand All @@ -89,25 +123,50 @@ function retrieveSession(sessionCookie: CookieCache): SessionState {
return session
}

export function persistSession(session: SessionState, cookie: CookieCache) {
export function persistSession(session: SessionState, cookie: CookieCache, withNewSessionStrategy = false) {
if (withNewSessionStrategy && !utils.isEmptyObject(session)) {
bcaudan marked this conversation as resolved.
Show resolved Hide resolved
session.expire = String(Date.now() + SESSION_EXPIRATION_DELAY)
}
const cookieString = utils
.objectEntries(session)
.map(([key, value]) => `${key}=${value}`)
.join(SESSION_ENTRY_SEPARATOR)
cookie.set(cookieString, EXPIRATION_DELAY)
cookie.set(cookieString, SESSION_EXPIRATION_DELAY)
}

export function stopSessionManagement() {
registeredActivityListeners.forEach((e) => e())
registeredActivityListeners = []
registeredListeners.forEach((e) => e())
registeredIntervals.forEach((interval) => clearInterval(interval))
registeredListeners = []
registeredIntervals = []
}

let registeredActivityListeners: Array<() => void> = []
let registeredListeners: Array<() => void> = []
let registeredIntervals: number[] = []

export function trackActivity(expandOrRenewSession: () => void) {
const doExpandOrRenewSession = monitor(expandOrRenewSession)
const options = { capture: true, passive: true }
;['click', 'touchstart', 'keydown', 'scroll'].forEach((event: string) => {
document.addEventListener(event, expandOrRenewSession, options)
registeredActivityListeners.push(() => document.removeEventListener(event, expandOrRenewSession, options))
;[utils.DOM_EVENT.CLICK, utils.DOM_EVENT.TOUCH_START, utils.DOM_EVENT.KEY_DOWN, utils.DOM_EVENT.SCROLL].forEach(
(event: string) => {
document.addEventListener(event, doExpandOrRenewSession, options)
registeredListeners.push(() => document.removeEventListener(event, doExpandOrRenewSession, options))
}
)
}

function trackVisibility(expandSession: () => void, visibilityStateProvider: () => VisibilityState) {
mquentin marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

IMO

  • trackVisibility should expose an Observable instead of a callback function
  • be self contained: no visibilityStateProvider, use document.visibilityState directly. We can mock document.visibilityState in tests

Copy link
Contributor Author

@bcaudan bcaudan Apr 8, 2020 •
edited by BenoitZugmeyer
Loading

Choose a reason for hiding this comment

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

trackVisibility should expose an Observable instead of a callback function

what advantage do you see with it?

We can mock document.visibilityState in tests

How would you do that?

Copy link
Member

Choose a reason for hiding this comment

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

what advantage do you see with it?

I think having a common strategy to pass data streams around would make things more future-proof. For now, our Observable implementation is quite limited, but in the future it could be extendend with a "destructor" when unused (example), and we would have a common way to remove listeners and stuff. This would make functions like this more self-contained and easier to test.

But if you feel that this is thinking too far ahead, I'm okay to keep a callback for now :)

How would you do that?

Object.defineProperty(document, 'visibilityState', { get() { return "zog" }, configurable: true })
// then
delete document.visibilityState

Copy link
Contributor Author

Choose a reason for hiding this comment

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

About observable instead of callbacks, I am not sure that observable pattern should be enforce in every situation.
I think it:

  • allow us to easily reuse a data source in another part of the code
  • can help us to decouple some of the browser API to the domain logic
    But it also adds an extra layer of abstraction.
    About future-proof arguments, let's see when we will have the need to do more complicated stuff with our observable and address it at this time.

In this case, I am not sure to see much benefit to switch to observable.

const expandSessionWhenVisible = monitor(() => {
if (visibilityStateProvider() === 'visible') {
expandSession()
}
})

const visibilityCheckInterval = window.setInterval(expandSessionWhenVisible, VISIBILITY_CHECK_DELAY)
document.addEventListener(utils.DOM_EVENT.VISIBILITY_CHANGE, expandSessionWhenVisible)

registeredIntervals.push(visibilityCheckInterval)
bcaudan marked this conversation as resolved.
Show resolved Hide resolved
registeredListeners.push(() =>
document.removeEventListener(utils.DOM_EVENT.VISIBILITY_CHANGE, expandSessionWhenVisible)
)
}
8 changes: 4 additions & 4 deletions packages/core/src/transport.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import lodashMerge from 'lodash.merge'

import { monitor } from './internalMonitoring'
import { Context, jsonStringify, objectValues } from './utils'
import { Context, DOM_EVENT, jsonStringify, objectValues } from './utils'

/**
* Use POST request without content type to:
Expand Down Expand Up @@ -161,7 +161,7 @@ export class Batch<T> {
* caveat: unload can still be canceled by another listener
*/
window.addEventListener(
'beforeunload',
DOM_EVENT.BEFORE_UNLOAD,
monitor(() => {
this.beforeFlushOnUnloadHandlers.forEach((handler) => handler())
})
Expand All @@ -172,7 +172,7 @@ export class Batch<T> {
* (e.g. when user switches to a different application, goes to homescreen, etc), or is being unloaded.
*/
document.addEventListener(
'visibilitychange',
DOM_EVENT.VISIBILITY_CHANGE,
monitor(() => {
if (document.visibilityState === 'hidden') {
this.flush()
Expand All @@ -184,7 +184,7 @@ export class Batch<T> {
* - a visibility change during doc unload (cf: https://bugs.webkit.org/show_bug.cgi?id=194897)
* - a page hide transition (cf: https://bugs.webkit.org/show_bug.cgi?id=188329)
*/
window.addEventListener('beforeunload', monitor(() => this.flush()))
window.addEventListener(DOM_EVENT.BEFORE_UNLOAD, monitor(() => this.flush()))
}
}
}
16 changes: 15 additions & 1 deletion packages/core/src/utils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,19 @@ export type Omit<T, K extends keyof T> = Pick<T, Exclude<keyof T, K>>
export const ONE_SECOND = 1000
export const ONE_MINUTE = 60 * ONE_SECOND
export const ONE_HOUR = 60 * ONE_MINUTE
export const ONE_DAY = 24 * ONE_HOUR
export const ONE_KILO_BYTE = 1024

export enum DOM_EVENT {
BEFORE_UNLOAD = 'beforeunload',
CLICK = 'click',
KEY_DOWN = 'keydown',
LOAD = 'load',
POP_STATE = 'popstate',
SCROLL = 'scroll',
TOUCH_START = 'touchstart',
VISIBILITY_CHANGE = 'visibilitychange',
}

export enum ResourceKind {
DOCUMENT = 'document',
XHR = 'xhr',
Expand Down Expand Up @@ -223,6 +233,10 @@ export function objectEntries(object: { [key: string]: unknown }) {
return Object.keys(object).map((key) => [key, object[key]])
}

export function isEmptyObject(object: object) {
return Object.keys(object).length === 0
}

export function getGlobalObject<T>(): T {
// tslint:disable-next-line: function-constructor no-function-constructor-with-string-args
return (typeof globalThis === 'object' ? globalThis : Function('return this')()) as T
Expand Down
12 changes: 6 additions & 6 deletions packages/core/test/oldCookiesMigration.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,21 +6,21 @@ import {
OLD_SESSION_COOKIE_NAME,
tryOldCookiesMigration,
} from '../src/oldCookiesMigration'
import { EXPIRATION_DELAY } from '../src/sessionManagement'
import { SESSION_EXPIRATION_DELAY } from '../src/sessionManagement'

describe('old cookies migration', () => {
it('should not touch current cookie', () => {
setCookie(SESSION_COOKIE_NAME, 'id=abcde&rum=0&logs=1', EXPIRATION_DELAY)
setCookie(SESSION_COOKIE_NAME, 'id=abcde&rum=0&logs=1', SESSION_EXPIRATION_DELAY)

tryOldCookiesMigration(cacheCookieAccess(SESSION_COOKIE_NAME))

expect(getCookie(SESSION_COOKIE_NAME)).toBe('id=abcde&rum=0&logs=1')
})

it('should create new cookie from old cookie values', () => {
setCookie(OLD_SESSION_COOKIE_NAME, 'abcde', EXPIRATION_DELAY)
setCookie(OLD_LOGS_COOKIE_NAME, '1', EXPIRATION_DELAY)
setCookie(OLD_RUM_COOKIE_NAME, '0', EXPIRATION_DELAY)
setCookie(OLD_SESSION_COOKIE_NAME, 'abcde', SESSION_EXPIRATION_DELAY)
setCookie(OLD_LOGS_COOKIE_NAME, '1', SESSION_EXPIRATION_DELAY)
setCookie(OLD_RUM_COOKIE_NAME, '0', SESSION_EXPIRATION_DELAY)

tryOldCookiesMigration(cacheCookieAccess(SESSION_COOKIE_NAME))

Expand All @@ -30,7 +30,7 @@ describe('old cookies migration', () => {
})

it('should create new cookie from a single old cookie', () => {
setCookie(OLD_RUM_COOKIE_NAME, '0', EXPIRATION_DELAY)
setCookie(OLD_RUM_COOKIE_NAME, '0', SESSION_EXPIRATION_DELAY)

tryOldCookiesMigration(cacheCookieAccess(SESSION_COOKIE_NAME))

Expand Down
Loading