♻️ [RUMF-1508] reorganise error handling (#2181)

* ✅ add missing tests for computeRawError * ✅ add integration tests * ♻️ cleanup instrumentOnError * ♻️ reorganise computeRawError * 👌use optional chaining Co-authored-by: Aymeric <aymeric.mortemousque@datadoghq.com> * 👌develop condition * 👌stack can be "unusable" not stackTrace * 👌use collectAsync * 👌remove unnecessary check --------- Co-authored-by: Aymeric <aymeric.mortemousque@datadoghq.com>
DataDog · Apr 25, 2023 · 0854beb · 0854beb
1 parent 4bf5a36
commit 0854beb
Show file tree

Hide file tree

Showing 5 changed files with 276 additions and 167 deletions.
diff --git a/packages/core/src/domain/error/error.spec.ts b/packages/core/src/domain/error/error.spec.ts
@@ -5,98 +5,151 @@ import type { RawErrorCause, ErrorWithCause } from './error.types'
 import { ErrorHandling, ErrorSource, NonErrorPrefix } from './error.types'
 
 describe('computeRawError', () => {
+  const ERROR_INSTANCE = new TypeError('oh snap!')
+  const DEFAULT_STACK = [
+    {
+      args: ['1', 'bar'],
+      column: 15,
+      func: 'foo',
+      line: 52,
+      url: 'http://path/to/file.js',
+    },
+    {
+      args: [],
+      column: undefined,
+      func: '?',
+      line: 12,
+      url: 'http://path/to/file.js',
+    },
+    {
+      args: ['baz'],
+      column: undefined,
+      func: '?',
+      line: undefined,
+      url: 'http://path/to/file.js',
+    },
+  ]
   const NOT_COMPUTED_STACK_TRACE: StackTrace = { name: undefined, message: undefined, stack: [] } as any
   const DEFAULT_RAW_ERROR_PARAMS = {
     startClocks: clocksNow(),
     nonErrorPrefix: NonErrorPrefix.UNCAUGHT,
     source: ErrorSource.CUSTOM,
   }
 
-  it('should format an error', () => {
-    const stackTrace: StackTrace = {
-      message: 'oh snap!',
-      name: 'TypeError',
-      stack: [
-        {
-          args: ['1', 'bar'],
-          column: 15,
-          func: 'foo',
-          line: 52,
-          url: 'http://path/to/file.js',
-        },
-        {
-          args: [],
-          column: undefined,
-          func: '?',
-          line: 12,
-          url: 'http://path/to/file.js',
-        },
-        {
-          args: ['baz'],
-          column: undefined,
-          func: '?',
-          line: undefined,
-          url: 'http://path/to/file.js',
-        },
-      ],
-    }
-
-    const formatted = computeRawError({
-      ...DEFAULT_RAW_ERROR_PARAMS,
-      stackTrace,
-      originalError: undefined,
-      handling: ErrorHandling.HANDLED,
-    })
-
-    expect(formatted.message).toEqual('oh snap!')
-    expect(formatted.type).toEqual('TypeError')
-    expect(formatted.stack).toEqual(`TypeError: oh snap!
+  describe('when stackTrace', () => {
+    describe('from an error instance', () => {
+      it('should format', () => {
+        const stackTrace: StackTrace = {
+          message: 'oh snap!',
+          name: 'TypeError',
+          stack: DEFAULT_STACK,
+        }
+
+        const formatted = computeRawError({
+          ...DEFAULT_RAW_ERROR_PARAMS,
+          stackTrace,
+          originalError: ERROR_INSTANCE,
+          handling: ErrorHandling.HANDLED,
+        })
+
+        expect(formatted.message).toEqual('oh snap!')
+        expect(formatted.type).toEqual('TypeError')
+        expect(formatted.stack).toEqual(`TypeError: oh snap!
   at foo(1, bar) @ http://path/to/file.js:52:15
   at <anonymous> @ http://path/to/file.js:12
   at <anonymous>(baz) @ http://path/to/file.js`)
-  })
-
-  it('should format an error with an empty message', () => {
-    const stackTrace: StackTrace = {
-      message: '',
-      name: 'TypeError',
-      stack: [],
-    }
+      })
+
+      it('should format with an empty message', () => {
+        const stackTrace: StackTrace = {
+          message: '',
+          name: 'TypeError',
+          stack: DEFAULT_STACK,
+        }
+
+        const formatted = computeRawError({
+          ...DEFAULT_RAW_ERROR_PARAMS,
+          stackTrace,
+          originalError: ERROR_INSTANCE,
+          handling: ErrorHandling.HANDLED,
+        })
+
+        expect(formatted.message).toEqual('Empty message')
+      })
+    })
 
-    const formatted = computeRawError({
-      ...DEFAULT_RAW_ERROR_PARAMS,
-      stackTrace,
-      originalError: undefined,
-      handling: ErrorHandling.HANDLED,
+    describe('from a string', () => {
+      it('should format with stack message', () => {
+        const error = 'Uncaught ReferenceError: foo is undefined'
+        const stackTrace = {
+          name: 'ReferenceError',
+          message: 'foo is undefined',
+          stack: [{ url: undefined, line: undefined, column: undefined }],
+        }
+
+        const formatted = computeRawError({
+          ...DEFAULT_RAW_ERROR_PARAMS,
+          stackTrace,
+          originalError: error,
+          handling: ErrorHandling.HANDLED,
+        })
+
+        expect(formatted.type).toEqual('ReferenceError')
+        expect(formatted.message).toEqual('foo is undefined')
+      })
+
+      it('should format without stack message', () => {
+        const error = 'oh snap!'
+
+        const formatted = computeRawError({
+          ...DEFAULT_RAW_ERROR_PARAMS,
+          stackTrace: NOT_COMPUTED_STACK_TRACE,
+          originalError: error,
+          handling: ErrorHandling.HANDLED,
+        })
+
+        expect(formatted.message).toEqual('Uncaught "oh snap!"')
+      })
     })
 
-    expect(formatted.message).toEqual('Empty message')
-  })
+    it('should format an object error', () => {
+      const error = { foo: 'bar' }
 
-  it('should format a string error', () => {
-    const error = 'oh snap!'
+      const formatted = computeRawError({
+        ...DEFAULT_RAW_ERROR_PARAMS,
+        stackTrace: NOT_COMPUTED_STACK_TRACE,
+        originalError: error,
+        handling: ErrorHandling.HANDLED,
+      })
 
-    const formatted = computeRawError({
-      ...DEFAULT_RAW_ERROR_PARAMS,
-      stackTrace: NOT_COMPUTED_STACK_TRACE,
-      originalError: error,
-      handling: ErrorHandling.HANDLED,
+      expect(formatted.message).toEqual('Uncaught {"foo":"bar"}')
     })
-
-    expect(formatted.message).toEqual('Uncaught "oh snap!"')
   })
 
-  it('should format an object error', () => {
-    const error = { foo: 'bar' }
+  describe('when no stackTrace', () => {
+    it('should format a string', () => {
+      const error = 'foo is undefined'
 
-    const formatted = computeRawError({
-      ...DEFAULT_RAW_ERROR_PARAMS,
-      stackTrace: NOT_COMPUTED_STACK_TRACE,
-      originalError: error,
-      handling: ErrorHandling.HANDLED,
+      const formatted = computeRawError({
+        ...DEFAULT_RAW_ERROR_PARAMS,
+        originalError: error,
+        handling: ErrorHandling.HANDLED,
+      })
+
+      expect(formatted.message).toEqual('Uncaught "foo is undefined"')
     })
 
-    expect(formatted.message).toEqual('Uncaught {"foo":"bar"}')
+    it('should format an object', () => {
+      const error = { foo: 'bar' }
+
+      const formatted = computeRawError({
+        ...DEFAULT_RAW_ERROR_PARAMS,
+        originalError: error,
+        handling: ErrorHandling.HANDLED,
+      })
+
+      expect(formatted.message).toEqual('Uncaught {"foo":"bar"}')
+    })
   })
 
   it('should set handling according to given parameter', () => {
@@ -125,7 +178,13 @@ describe('computeRawError', () => {
     const stackTrace: StackTrace = {
       message: 'some typeError message',
       name: 'TypeError',
-      stack: [],
+      stack: [
+        {
+          url: '<fake url>',
+          line: 1,
+          column: 2,
+        },
+      ],
     }
 
     const error = new Error('foo: bar') as ErrorWithCause

diff --git a/packages/core/src/domain/error/error.ts b/packages/core/src/domain/error/error.ts
@@ -29,30 +29,40 @@ export function computeRawError({
   source,
   handling,
 }: RawErrorParams): RawError {
-  if (!stackTrace || (stackTrace.message === undefined && !(originalError instanceof Error))) {
-    return {
-      startClocks,
-      source,
-      handling,
-      originalError,
-      message: `${nonErrorPrefix} ${jsonStringify(sanitize(originalError))!}`,
-      stack: NO_ERROR_STACK_PRESENT_MESSAGE,
-      handlingStack,
-      type: stackTrace && stackTrace.name,
-    }
-  }
+  const isErrorInstance = originalError instanceof Error
+
+  const message = stackTrace?.message
+    ? stackTrace.message
+    : !isErrorInstance
+    ? `${nonErrorPrefix} ${jsonStringify(sanitize(originalError))!}`
+    : 'Empty message'
+  const stack = hasUsableStack(isErrorInstance, stackTrace)
+    ? toStackTraceString(stackTrace)
+    : NO_ERROR_STACK_PRESENT_MESSAGE
+  const causes = isErrorInstance ? flattenErrorCauses(originalError as ErrorWithCause, source) : undefined
+  const type = stackTrace?.name
 
   return {
     startClocks,
     source,
     handling,
-    originalError,
-    message: stackTrace.message || 'Empty message',
-    stack: toStackTraceString(stackTrace),
     handlingStack,
-    type: stackTrace.name,
-    causes: flattenErrorCauses(originalError as ErrorWithCause, source),
+    originalError,
+    type,
+    message,
+    stack,
+    causes,
+  }
+}
+
+function hasUsableStack(isErrorInstance: boolean, stackTrace?: StackTrace): stackTrace is StackTrace {
+  if (stackTrace === undefined) {
+    return false
+  }
+  if (isErrorInstance) {
+    return true
   }
+  return stackTrace.stack.length > 0 && (stackTrace.stack.length > 1 || stackTrace.stack[0].url !== undefined)
 }
 
 export function toStackTraceString(stack: StackTrace) {

diff --git a/packages/core/src/domain/error/trackRuntimeError.spec.ts b/packages/core/src/domain/error/trackRuntimeError.spec.ts
@@ -1,9 +1,10 @@
 import { Observable } from '../../tools/observable'
+import { collectAsyncCalls } from '../../../test'
 import { NO_ERROR_STACK_PRESENT_MESSAGE } from './error'
 import { trackRuntimeError } from './trackRuntimeError'
 import type { RawError } from './error.types'
 
-describe('runtime error tracker', () => {
+describe('trackRuntimeError', () => {
   const ERROR_MESSAGE = 'foo'
 
   let originalOnErrorHandler: OnErrorEventHandler
@@ -39,12 +40,11 @@ describe('runtime error tracker', () => {
   it('should call original error handler', (done) => {
     setTimeout(() => {
       throw new Error(ERROR_MESSAGE)
-    }, 10)
-
-    setTimeout(() => {
+    })
+    collectAsyncCalls(onErrorSpy, 1, () => {
       expect(onErrorSpy.calls.mostRecent().args[0]).toMatch(ERROR_MESSAGE)
       done()
-    }, 100)
+    })
   })
 
   it('should call original unhandled rejection handler', () => {
@@ -55,27 +55,53 @@ describe('runtime error tracker', () => {
     expect(onUnhandledrejectionSpy.calls.mostRecent().args[0].reason).toMatch(ERROR_MESSAGE)
   })
 
-  it('should notify error', (done) => {
+  it('should notify unhandled error instance', (done) => {
     setTimeout(() => {
       throw new Error(ERROR_MESSAGE)
-    }, 10)
+    })
+    collectAsyncCalls(onErrorSpy, 1, () => {
+      const collectedError = notifyError.calls.mostRecent().args[0] as RawError
+      expect(collectedError.message).toEqual(ERROR_MESSAGE)
+      expect(collectedError.stack).not.toEqual(NO_ERROR_STACK_PRESENT_MESSAGE)
+      done()
+    })
+  })
 
+  it('should notify unhandled string', (done) => {
     setTimeout(() => {
-      expect((notifyError.calls.mostRecent().args[0] as RawError).message).toEqual(ERROR_MESSAGE)
+      // eslint-disable-next-line no-throw-literal
+      throw 'foo'
+    })
+    collectAsyncCalls(onErrorSpy, 1, () => {
+      const collectedError = notifyError.calls.mostRecent().args[0] as RawError
+      expect(collectedError.message).toEqual('Uncaught "foo"')
+      expect(collectedError.stack).toEqual(NO_ERROR_STACK_PRESENT_MESSAGE)
       done()
-    }, 100)
+    })
   })
 
-  it('should handle direct onerror calls with objects', (done) => {
+  it('should notify unhandled object', (done) => {
     setTimeout(() => {
-      window.onerror!({ foo: 'bar' } as any)
-    }, 10)
+      // eslint-disable-next-line no-throw-literal
+      throw { a: 'foo' }
+    })
+    collectAsyncCalls(onErrorSpy, 1, () => {
+      const collectedError = notifyError.calls.mostRecent().args[0] as RawError
+      expect(collectedError.message).toEqual('Uncaught {"a":"foo"}')
+      expect(collectedError.stack).toEqual(NO_ERROR_STACK_PRESENT_MESSAGE)
+      done()
+    })
+  })
 
+  it('should handle direct onerror calls with objects', (done) => {
     setTimeout(() => {
+      window.onerror!({ foo: 'bar' } as any)
+    })
+    collectAsyncCalls(onErrorSpy, 1, () => {
       const collectedError = notifyError.calls.mostRecent().args[0] as RawError
       expect(collectedError.message).toEqual('Uncaught {"foo":"bar"}')
       expect(collectedError.stack).toEqual(NO_ERROR_STACK_PRESENT_MESSAGE)
       done()
-    }, 100)
+    })
   })
 })