fix: do not extend session from discarded session replay spans (#939)

Author: Joozty

packages/session-recorder/src/index.ts

@@ -194,9 +194,7 @@ const SplunkRumRecorder = {
 					return
 				}
 
-				if (SplunkRum._internalOnExternalSpanCreated) {
-					SplunkRum._internalOnExternalSpanCreated()
-				}
+				let isExtended = false
 
 				// Safeguards from our ingest getting DDOSed:
 				// 1. A session can send up to 4 hours of data
@@ -206,6 +204,13 @@ const SplunkRumRecorder = {
 						return
 					}
 
+					// We need to call it here before another getSessionID call. This will create a new session
+					// if the previous one was expired
+					if (SplunkRum._internalOnExternalSpanCreated) {
+						SplunkRum._internalOnExternalSpanCreated()
+						isExtended = true
+					}
+
 					lastKnownSession = SplunkRum.getSessionId()
 					sessionStartTime = Date.now()
 					// reset counters
@@ -218,6 +223,12 @@ const SplunkRumRecorder = {
 					return
 				}
 
+				if (!isExtended) {
+					if (SplunkRum._internalOnExternalSpanCreated) {
+						SplunkRum._internalOnExternalSpanCreated()
+					}
+				}
+
 				const time = event.timestamp
 				const eventI = eventCounter
 				eventCounter += 1

packages/web/src/SessionBasedSampler.ts

@@ -52,11 +52,14 @@ export class SessionBasedSampler implements Sampler {
 
 	protected _upperBound: number
 
-	constructor({
-		ratio = 1,
-		sampled = new AlwaysOnSampler(),
-		notSampled = new AlwaysOffSampler(),
-	}: SessionBasedSamplerConfig = {}) {
+	constructor(
+		{
+			ratio = 1,
+			sampled = new AlwaysOnSampler(),
+			notSampled = new AlwaysOffSampler(),
+		}: SessionBasedSamplerConfig = {},
+		private readonly useLocalStorageForSessionMetadata = false,
+	) {
 		this._ratio = this._normalize(ratio)
 		this._upperBound = Math.floor(this._ratio * 0xffffffff)
 
@@ -75,7 +78,7 @@ export class SessionBasedSampler implements Sampler {
 		// Implementation based on @opentelemetry/core TraceIdRatioBasedSampler
 		// but replacing deciding based on traceId with sessionId
 		// (not extended from due to private methods)
-		const currentSession = getRumSessionId()
+		const currentSession = getRumSessionId({ useLocalStorage: this.useLocalStorageForSessionMetadata })
 		if (this._currentSession !== currentSession) {
 			this._currentSessionSampled = this._accumulate(currentSession) < this._upperBound
 			this._currentSession = currentSession

packages/web/src/SplunkSpanAttributesProcessor.ts

@@ -23,7 +23,10 @@ import { getRumSessionId } from './session'
 export class SplunkSpanAttributesProcessor implements SpanProcessor {
 	private readonly _globalAttributes: Attributes
 
-	constructor(globalAttributes: Attributes) {
+	constructor(
+		globalAttributes: Attributes,
+		private useLocalStorageForSessionMetadata: boolean,
+	) {
 		this._globalAttributes = globalAttributes ?? {}
 	}
 
@@ -42,7 +45,10 @@ export class SplunkSpanAttributesProcessor implements SpanProcessor {
 	onStart(span: Span): void {
 		span.setAttribute('location.href', location.href)
 		span.setAttributes(this._globalAttributes)
-		span.setAttribute('splunk.rumSessionId', getRumSessionId())
+		span.setAttribute(
+			'splunk.rumSessionId',
+			getRumSessionId({ useLocalStorage: this.useLocalStorageForSessionMetadata }),
+		)
 		span.setAttribute('browser.instance.visibility_state', document.visibilityState)
 	}
 

packages/web/src/index.ts

@@ -392,13 +392,16 @@ export const SplunkRum: SplunkOtelWebType = {
 			return null
 		}).filter((a): a is Exclude<typeof a, null> => Boolean(a))
 
-		this.attributesProcessor = new SplunkSpanAttributesProcessor({
-			...(deploymentEnvironment
-				? { 'environment': deploymentEnvironment, 'deployment.environment': deploymentEnvironment }
-				: {}),
-			...(version ? { 'app.version': version } : {}),
-			...(processedOptions.globalAttributes || {}),
-		})
+		this.attributesProcessor = new SplunkSpanAttributesProcessor(
+			{
+				...(deploymentEnvironment
+					? { 'environment': deploymentEnvironment, 'deployment.environment': deploymentEnvironment }
+					: {}),
+				...(version ? { 'app.version': version } : {}),
+				...(processedOptions.globalAttributes || {}),
+			},
+			this._processedOptions.persistence === 'localStorage',
+		)
 		provider.addSpanProcessor(this.attributesProcessor)
 
 		if (processedOptions.beaconEndpoint) {
@@ -517,7 +520,11 @@ export const SplunkRum: SplunkOtelWebType = {
 	},
 
 	getSessionId() {
-		return getRumSessionId()
+		if (!inited) {
+			return undefined
+		}
+
+		return getRumSessionId({ useLocalStorage: this._processedOptions.persistence === 'localStorage' })
 	},
 	_experimental_getSessionId() {
 		return this.getSessionId()

packages/web/src/session/session.ts

@@ -45,7 +45,6 @@ import { context } from '@opentelemetry/api'
     with setting cookies, checking for inactivity, etc.
 */
 
-let rumSessionId: SessionId | undefined
 let recentActivity = false
 let cookieDomain: string
 let eventTarget: InternalEventTarget | undefined
@@ -93,8 +92,7 @@ export function updateSessionStatus({
 		}
 	}
 
-	rumSessionId = sessionState.id
-	eventTarget?.emit('session-changed', { sessionId: rumSessionId })
+	eventTarget?.emit('session-changed', { sessionId: sessionState.id })
 
 	if (recentActivity || forceActivity) {
 		sessionState.expiresAt = Date.now() + SESSION_INACTIVITY_TIMEOUT_MS
@@ -157,17 +155,14 @@ export function initSessionTracking(
 	if (hasNativeSessionId()) {
 		// short-circuit and bail out - don't create cookie, watch for inactivity, or anything
 		return {
-			deinit: () => {
-				rumSessionId = undefined
-			},
+			deinit: () => {},
 		}
 	}
 
 	if (domain) {
 		cookieDomain = domain
 	}
 
-	rumSessionId = instanceId
 	recentActivity = true // document loaded implies activity
 	eventTarget = newEventTarget
 
@@ -179,16 +174,15 @@ export function initSessionTracking(
 	return {
 		deinit: () => {
 			ACTIVITY_EVENTS.forEach((type) => document.removeEventListener(type, markActivity))
-			rumSessionId = undefined
 			eventTarget = undefined
 		},
 	}
 }
 
-export function getRumSessionId(): SessionId | undefined {
+export function getRumSessionId({ useLocalStorage }: { useLocalStorage: boolean }): SessionId | undefined {
 	if (hasNativeSessionId()) {
 		return window['SplunkRumNative'].getNativeSessionId()
 	}
 
-	return rumSessionId
+	return getCurrentSessionState({ useLocalStorage, forceStoreRead: true })?.id
 }

packages/web/test/SplunkSpanAttributesProcessor.test.ts

@@ -22,20 +22,26 @@ import { SplunkSpanAttributesProcessor } from '../src/SplunkSpanAttributesProces
 describe('SplunkSpanAttributesProcessor', () => {
 	describe('setting global attribute', () => {
 		it('should set attributes via constructor', () => {
-			const processor = new SplunkSpanAttributesProcessor({
-				key1: 'value1',
-			})
+			const processor = new SplunkSpanAttributesProcessor(
+				{
+					key1: 'value1',
+				},
+				false,
+			)
 
 			expect(processor.getGlobalAttributes()).to.deep.eq({
 				key1: 'value1',
 			})
 		})
 
 		it('should patch attributes via .setGlobalAttributes()', () => {
-			const processor = new SplunkSpanAttributesProcessor({
-				key1: 'value1',
-				key2: 'value2',
-			})
+			const processor = new SplunkSpanAttributesProcessor(
+				{
+					key1: 'value1',
+					key2: 'value2',
+				},
+				false,
+			)
 
 			processor.setGlobalAttributes({
 				key2: 'value2-updaged',

packages/web/test/session.test.ts

@@ -38,11 +38,11 @@ describe('Session tracking', () => {
 		// the init tests have possibly already started the setInterval for updateSessionStatus.  Try to accomodate this.
 		const provider = new SplunkWebTracerProvider()
 		const trackingHandle = initSessionTracking(provider, '1234', new InternalEventTarget())
-		const firstSessionId = getRumSessionId()
+		const firstSessionId = getRumSessionId({ useLocalStorage: false })
 		assert.strictEqual(firstSessionId.length, 32)
 		// no marked activity, should keep same state
 		updateSessionStatus({ forceStore: false, useLocalStorage: false })
-		assert.strictEqual(firstSessionId, getRumSessionId())
+		assert.strictEqual(firstSessionId, getRumSessionId({ useLocalStorage: false }))
 		// set cookie to expire in 2 seconds, mark activity, and then updateSessionStatus.
 		// Wait 4 seconds and cookie should still be there (having been renewed)
 		const cookieValue = encodeURIComponent(
@@ -58,7 +58,7 @@ describe('Session tracking', () => {
 		setTimeout(() => {
 			// because of activity, same session should be there
 			assert.ok(document.cookie.includes(SESSION_STORAGE_KEY))
-			assert.strictEqual(firstSessionId, getRumSessionId())
+			assert.strictEqual(firstSessionId, getRumSessionId({ useLocalStorage: false }))
 
 			// Finally, set a fake cookie with startTime 5 hours ago, update status, and find a new cookie with a new session ID
 			// after max age code does its thing
@@ -74,7 +74,7 @@ describe('Session tracking', () => {
 
 			updateSessionStatus({ forceStore: true, useLocalStorage: false })
 			assert.ok(document.cookie.includes(SESSION_STORAGE_KEY))
-			const newSessionId = getRumSessionId()
+			const newSessionId = getRumSessionId({ useLocalStorage: false })
 			assert.strictEqual(newSessionId.length, 32)
 			assert.ok(firstSessionId !== newSessionId)
 
@@ -90,7 +90,7 @@ describe('Session tracking', () => {
 
 		function subject(allSpansAreActivity = false) {
 			const provider = new SplunkWebTracerProvider()
-			const firstSessionId = getRumSessionId()
+			const firstSessionId = getRumSessionId({ useLocalStorage: false })
 			initSessionTracking(provider, firstSessionId, new InternalEventTarget(), undefined, allSpansAreActivity)
 
 			provider.getTracer('tracer').startSpan('any-span').end()
@@ -138,9 +138,9 @@ describe('Session tracking - localStorage', () => {
 			useLocalStorage,
 		)
 
-		const firstSessionId = getRumSessionId()
+		const firstSessionId = getRumSessionId({ useLocalStorage })
 		updateSessionStatus({ forceStore: true, useLocalStorage })
-		assert.strictEqual(firstSessionId, getRumSessionId())
+		assert.strictEqual(firstSessionId, getRumSessionId({ useLocalStorage }))
 
 		trackingHandle.deinit()
 	})