feat(rum): add sessionOnErrorSampleRate

packages/rum-core/src/boot/startRum.ts

@@ -102,6 +102,11 @@ export function startRum(
   }
 
   const pageMayExitObservable = createPageMayExitObservable(configuration)
+  // Subscribed before the batch below, and it has to stay that way. The batch flushes on this same
+  // observable, and observers run in the order they subscribed - so the withheld event buffer, which
+  // releases on the lifecycle notification raised here, has to get its events into the batch before
+  // the flush that is the page's last chance to send them. The same holds for the session expiry
+  // relay in `startRumSessionManager`, which the session manager registers just below.
   const pageMayExitSubscription = pageMayExitObservable.subscribe((event) => {
     lifeCycle.notify(LifeCycleEventType.PAGE_MAY_EXIT, event)
   })
@@ -111,6 +116,9 @@ export function startRum(
     ? startRumSessionManager(configuration, lifeCycle, trackingConsentState)
     : startRumSessionManagerStub()
 
+  // Subscribed before the batch below, and it has to stay that way: the withheld event buffer runs
+  // on the same event, and only sees a session as released if this has already marked it. Reorder
+  // them and the release waits for whatever event happens to come next.
   const sessionErrorTracking = startSessionErrorTracking(lifeCycle, session)
   cleanupTasks.push(() => sessionErrorTracking.stop())
 
@@ -121,7 +129,7 @@ export function startRum(
       telemetry.observable,
       reportError,
       pageMayExitObservable,
-      session.expireObservable,
+      session,
       createEncoder
     )
     cleanupTasks.push(() => batch.stop())

packages/rum-core/src/domain/configuration/configuration.spec.ts

@@ -141,6 +141,85 @@ describe('validateAndBuildRumConfiguration', () => {
     })
   })
 
+  describe('sessionOnErrorSampleRate', () => {
+    it('is carried into the built configuration', () => {
+      expect(
+        validateAndBuildRumConfiguration({ ...DEFAULT_INIT_CONFIGURATION, sessionOnErrorSampleRate: 40 })!
+          .sessionOnErrorSampleRate
+      ).toBe(40)
+    })
+
+    it('defaults to collecting no error-only session at all', () => {
+      expect(validateAndBuildRumConfiguration(DEFAULT_INIT_CONFIGURATION)!.sessionOnErrorSampleRate).toBe(0)
+    })
+
+    it('is rejected when it is not a sample rate', () => {
+      expect(
+        validateAndBuildRumConfiguration({
+          ...DEFAULT_INIT_CONFIGURATION,
+          sessionOnErrorSampleRate: 'foo' as unknown as number,
+        })
+      ).toBeUndefined()
+      expect(displayErrorSpy).toHaveBeenCalledTimes(1)
+    })
+
+    it('warns when the replay it would withhold is never recorded', () => {
+      validateAndBuildRumConfiguration({
+        ...DEFAULT_INIT_CONFIGURATION,
+        sessionSampleRate: 20,
+        sessionOnErrorSampleRate: 50,
+        sessionReplaySampleRate: 50,
+        startSessionReplayRecordingManually: true,
+      })
+
+      expect(displayWarnSpy).toHaveBeenCalledTimes(1)
+      expect(displayWarnSpy.calls.argsFor(0)[0]).toContain('startSessionReplayRecordingManually')
+    })
+
+    it('says nothing about a replay it could never withhold anyway', () => {
+      validateAndBuildRumConfiguration({
+        ...DEFAULT_INIT_CONFIGURATION,
+        sessionOnErrorSampleRate: 50,
+        sessionReplaySampleRate: 30,
+        startSessionReplayRecordingManually: true,
+      })
+
+      // the on-error rate cannot draw at all here, which is the one thing worth saying
+      expect(displayWarnSpy).toHaveBeenCalledTimes(1)
+      expect(displayWarnSpy.calls.argsFor(0)[0]).toContain('sessionSampleRate did not draw')
+    })
+
+    it('warns when the default session rate leaves it nothing to draw from', () => {
+      validateAndBuildRumConfiguration({
+        ...DEFAULT_INIT_CONFIGURATION,
+        sessionOnErrorSampleRate: 50,
+      })
+
+      expect(displayWarnSpy).toHaveBeenCalledTimes(1)
+    })
+
+    it('says nothing once the plain session rate leaves room for it', () => {
+      validateAndBuildRumConfiguration({
+        ...DEFAULT_INIT_CONFIGURATION,
+        sessionSampleRate: 20,
+        sessionOnErrorSampleRate: 50,
+      })
+
+      expect(displayWarnSpy).not.toHaveBeenCalled()
+    })
+
+    it('makes a replay-on-error rate meaningful even with no plainly sampled session', () => {
+      validateAndBuildRumConfiguration({
+        ...DEFAULT_INIT_CONFIGURATION,
+        sessionSampleRate: 0,
+        sessionOnErrorSampleRate: 100,
+        sessionReplayOnErrorSampleRate: 50,
+      })
+
+      expect(displayWarnSpy).not.toHaveBeenCalled()
+    })
+  })
+
   describe('traceSampleRate', () => {
     it('defaults to 100 if the option is not provided', () => {
       expect(validateAndBuildRumConfiguration(DEFAULT_INIT_CONFIGURATION)!.traceSampleRate).toBe(100)
@@ -609,6 +688,7 @@ describe('serializeRumConfiguration', () => {
       subdomain: 'foo',
       sessionReplaySampleRate: 60,
       sessionReplayOnErrorSampleRate: 40,
+      sessionOnErrorSampleRate: 30,
       startSessionReplayRecordingManually: true,
       trackUserInteractions: true,
       actionNameAttribute: 'test-id',
@@ -636,6 +716,7 @@ describe('serializeRumConfiguration', () => {
                 | 'propagateTraceBaggage'
                 // not reported yet: needs a rum-events-format schema change first
                 | 'sessionReplayOnErrorSampleRate'
+                | 'sessionOnErrorSampleRate'
             ? never
             : CamelToSnakeCase<Key>
     // By specifying the type here, we can ensure that serializeConfiguration is returning an

packages/rum-core/src/domain/configuration/configuration.ts

@@ -111,6 +111,21 @@ export interface RumInitConfiguration extends InitConfiguration {
    * the withheld minute is uploaded and recording continues normally for the rest of the session.
    */
   sessionReplayOnErrorSampleRate?: number | undefined
+  /**
+   * Of the sessions that `sessionSampleRate` did not draw, the percentage that collect events but
+   * only upload them if the session reports an error: 100 for all of them, 0 for none. The base is
+   * what the plain rate missed - so with the default `sessionSampleRate` of 100 there is nothing
+   * left to draw from and this does nothing - and the share of all sessions it covers is
+   * `(100 - sessionSampleRate) * this / 100`.
+   *
+   * Such a session collects from the start and keeps at most the last minute of it in memory. If it
+   * never reports an error, nothing is uploaded and the session is not stored. On the first error,
+   * the withheld minute is uploaded and collection continues normally.
+   *
+   * A session sampled this way never uploads its replay ahead of its events: until the events are
+   * released the session does not exist yet, and a replay sent then would have nothing to attach to.
+   */
+  sessionOnErrorSampleRate?: number | undefined
   /**
    * If the session is sampled for Session Replay, only start the recording when `startSessionReplayRecording()` is called, instead of at the beginning of the session. Default: if startSessionReplayRecording is 0, true; otherwise, false.
    * See [Session Replay Usage](https://docs.datadoghq.com/real_user_monitoring/session_replay/browser/#usage) for further information.
@@ -187,6 +202,7 @@ export interface RumConfiguration extends Configuration {
   enablePrivacyForActionName: boolean
   sessionReplaySampleRate: number
   sessionReplayOnErrorSampleRate: number
+  sessionOnErrorSampleRate: number
   startSessionReplayRecordingManually: boolean
   trackUserInteractions: boolean
   trackViewsManually: boolean
@@ -220,6 +236,7 @@ export function validateAndBuildRumConfiguration(
   if (
     !isSampleRate(initConfiguration.sessionReplaySampleRate, 'Session Replay') ||
     !isSampleRate(initConfiguration.sessionReplayOnErrorSampleRate, 'Session Replay on Error') ||
+    !isSampleRate(initConfiguration.sessionOnErrorSampleRate, 'Session on Error') ||
     !isSampleRate(initConfiguration.traceSampleRate, 'Trace')
   ) {
     return
@@ -244,32 +261,51 @@ export function validateAndBuildRumConfiguration(
 
   const sessionReplaySampleRate = initConfiguration.sessionReplaySampleRate ?? 0
   const sessionReplayOnErrorSampleRate = initConfiguration.sessionReplayOnErrorSampleRate ?? 0
+  const sessionOnErrorSampleRate = initConfiguration.sessionOnErrorSampleRate ?? 0
 
-  // Each of these is a rate the customer set that cannot draw a single session. They are valid
-  // numbers, so validation lets them through - but silence would leave them waiting for data that
-  // is never coming.
+  // Each of the cases below is a rate the customer set that cannot draw a single session. They are
+  // valid numbers, so validation lets them through - but silence would leave someone waiting for
+  // data that is never coming.
+  if (sessionOnErrorSampleRate > 0 && (initConfiguration.sessionSampleRate ?? 100) === 100) {
+    display.warn(
+      'sessionOnErrorSampleRate is drawn only for sessions sessionSampleRate did not draw, and that rate is 100: it will never apply.'
+    )
+  }
   if (sessionReplayOnErrorSampleRate > 0) {
     if (sessionReplaySampleRate === 100) {
       display.warn(
         'sessionReplayOnErrorSampleRate is drawn only for sessions sessionReplaySampleRate did not draw, and that rate is 100: it will never apply.'
       )
     }
-    if ((initConfiguration.sessionSampleRate ?? 100) === 0) {
-      display.warn('sessionReplayOnErrorSampleRate has no effect while sessionSampleRate is 0: no session is tracked.')
-    }
-    if (initConfiguration.startSessionReplayRecordingManually) {
+    if ((initConfiguration.sessionSampleRate ?? 100) === 0 && sessionOnErrorSampleRate === 0) {
       display.warn(
-        'sessionReplayOnErrorSampleRate needs the recording to already be running when the error happens, and startSessionReplayRecordingManually keeps it stopped until you start it: there would be nothing to release.'
+        'sessionReplayOnErrorSampleRate has no effect while sessionSampleRate is 0 and sessionOnErrorSampleRate is unset: no session is tracked.'
       )
     }
   }
 
+  // A session drawn on error withholds whichever replay it draws, so the same trap is reachable
+  // through the plain replay rate as well - and there it is worse than silence, since the released
+  // views would report a replay for a recording that never ran.
+  if (
+    initConfiguration.startSessionReplayRecordingManually &&
+    (sessionReplayOnErrorSampleRate > 0 ||
+      (sessionOnErrorSampleRate > 0 &&
+        sessionReplaySampleRate > 0 &&
+        (initConfiguration.sessionSampleRate ?? 100) < 100))
+  ) {
+    display.warn(
+      'A replay kept until the session errors has to be recording before that error, and startSessionReplayRecordingManually keeps it stopped until you start it: there would be nothing to release.'
+    )
+  }
+
   return {
     applicationId: initConfiguration.applicationId,
     version: initConfiguration.version || undefined,
     actionNameAttribute: initConfiguration.actionNameAttribute,
     sessionReplaySampleRate,
     sessionReplayOnErrorSampleRate,
+    sessionOnErrorSampleRate,
     startSessionReplayRecordingManually:
       initConfiguration.startSessionReplayRecordingManually !== undefined
         ? !!initConfiguration.startSessionReplayRecordingManually

packages/rum-core/src/domain/contexts/sessionContext.spec.ts

@@ -170,6 +170,70 @@ describe('session context', () => {
     expect(eventSampledOutForReplay.session!.sampled_for_replay).toBe(false)
   })
 
+  it('should set sampled_for_replay on a session whose events are withheld alongside its replay', () => {
+    // these events only ever leave together with that replay, so reporting the state as it stands
+    // while they are held would mark the whole released burst as having none
+    sessionManager.setTrackedOnErrorWithSessionReplay()
+
+    const event = hooks.triggerHook(HookNames.Assemble, {
+      eventType: 'view',
+      startTime: 0 as RelativeTime,
+    }) as DefaultRumEventAttributes
+
+    expect(event.session!.sampled_for_replay).toBe(true)
+  })
+
+  it('should not claim a replay while one is withheld, whichever way it turns out', () => {
+    // the segment covering this event is dropped on the next view change and sent only if the error
+    // comes first; the event is assembled before either, so it claims nothing
+    sessionManager.setTrackedOnErrorWithSessionReplay()
+    isRecordingSpy.and.returnValue(true)
+    getReplayStatsSpy.and.returnValue(fakeStats)
+
+    const errorEvent = hooks.triggerHook(HookNames.Assemble, {
+      eventType: 'error',
+      startTime: 0 as RelativeTime,
+    }) as DefaultRumEventAttributes
+    const viewEvent = hooks.triggerHook(HookNames.Assemble, {
+      eventType: 'view',
+      startTime: 0 as RelativeTime,
+    }) as DefaultRumEventAttributes
+
+    expect(errorEvent.session!.has_replay).toBeUndefined()
+    expect(viewEvent.session!.has_replay).toBeUndefined()
+    // but the session was sampled for one, and that is answerable without knowing any segment's fate
+    expect(viewEvent.session!.sampled_for_replay).toBe(true)
+  })
+
+  it('should not claim a replay for a session that withholds its events and has none', () => {
+    sessionManager.setTrackedOnError()
+
+    const event = hooks.triggerHook(HookNames.Assemble, {
+      eventType: 'view',
+      startTime: 0 as RelativeTime,
+    }) as DefaultRumEventAttributes
+
+    expect(event.session!.sampled_for_replay).toBe(false)
+  })
+
+  it('should tell the backend a session was stored only because it errored', () => {
+    sessionManager.setTrackedOnError()
+    const onErrorEvent = hooks.triggerHook(HookNames.Assemble, {
+      eventType: 'view',
+      startTime: 0 as RelativeTime,
+    }) as DefaultRumEventAttributes
+
+    sessionManager.setTrackedWithSessionReplay()
+    const plainEvent = hooks.triggerHook(HookNames.Assemble, {
+      eventType: 'view',
+      startTime: 0 as RelativeTime,
+    }) as DefaultRumEventAttributes
+
+    expect(onErrorEvent.session!.sampled_for_error).toBeTrue()
+    // absent rather than false, so it costs nothing on every ordinary session
+    expect(plainEvent.session!.sampled_for_error).toBeUndefined()
+  })
+
   it('should discard the event if no session', () => {
     sessionManager.setNotTracked()
     const defaultRumEventAttributes = hooks.triggerHook(HookNames.Assemble, {

packages/rum-core/src/domain/contexts/sessionContext.ts

@@ -21,11 +21,16 @@ export function startSessionContext(
     }
 
     // A session withholding its replay is recording, but nothing has been uploaded and nothing may
-    // ever be. Reporting `has_replay` here would offer a replay that does not exist.
+    // ever be. An event assembled now cannot know which of the two it will turn out to be: the
+    // segment covering it is dropped on the next view change and sent only if the error comes first,
+    // and it is assembled before either happens - the final update of a view is emitted before the
+    // view change that drops that view's segment. So it does not claim a replay. Whether the session
+    // was *sampled* for one is a different question, answerable here, and answered below.
     const isReplayWithheld = session.sessionReplay === SessionReplayState.BUFFERED_ON_ERROR
 
     let hasReplay
     let sampledForReplay
+    let sampledForError
     let sampledForErrorReplay
     let isActive
     if (eventType === RumEventType.VIEW) {
@@ -35,7 +40,14 @@ export function startSessionContext(
       // because a host bridge takes the records itself and no segment is ever built for them.
       const replayStats = recorderApi.getReplayStats(view.id)
       hasReplay = !isReplayWithheld && replayStats && replayStats.records_count > 0 ? true : undefined
-      sampledForReplay = session.sessionReplay === SessionReplayState.SAMPLED
+      // A session that withholds its events withholds its replay alongside them, so if these events
+      // are ever uploaded that replay is on its way with them. Reporting the state as it stands at
+      // assembly time would mark the whole released burst as a session that has no replay.
+      sampledForReplay =
+        session.sessionReplay === SessionReplayState.SAMPLED || (isReplayWithheld && session.eventsWithheld)
+      // Tells the backend that this session's detail only starts where the buffer reached, so the
+      // gap before it reads as "not collected" rather than as missing data.
+      sampledForError = session.sampledOnError || undefined
       // Tells a replay collected only because the session errored apart from one collected
       // unconditionally - the two cost differently and are answered by different questions.
       sampledForErrorReplay = session.sampledOnErrorReplay || undefined
@@ -51,6 +63,7 @@ export function startSessionContext(
         type: SessionType.USER,
         has_replay: hasReplay,
         sampled_for_replay: sampledForReplay,
+        sampled_for_error: sampledForError,
         sampled_for_error_replay: sampledForErrorReplay,
         is_active: isActive,
       },